Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test::UploadedFile: unintended breakage of public API #207

Closed
perlun opened this issue Nov 20, 2017 · 10 comments
Closed

Test::UploadedFile: unintended breakage of public API #207

perlun opened this issue Nov 20, 2017 · 10 comments
Labels

Comments

@perlun
Copy link
Contributor

perlun commented Nov 20, 2017

This is a regression in #149, reported by @tagliala and @007lva. Copying from a comment there:


Hi,

This change is breaking an use case in specs with Paperclip + FactoryBot.

The suggested method documented here: https://github.com/thoughtbot/paperclip#testing

FactoryGirl.define do
  factory :user do
    avatar { File.new("#{Rails.root}/spec/support/fixtures/image.jpg") }
  end
end

does not work anymore:

     NoMethodError:
       undefined method `size' for nil:NilClass
     # ~/dev/rack-test/lib/rack/test/uploaded_file.rb:38:in `public_send'
     # ~/dev/rack-test/lib/rack/test/uploaded_file.rb:38:in `method_missing'
     # ~/dev/rack-test/lib/rack/test/utils.rb:136:in `build_file_part'
     # ~/dev/rack-test/lib/rack/test/utils.rb:101:in `block in get_parts'
     # ~/dev/rack-test/lib/rack/test/utils.rb:92:in `each'
     # ~/dev/rack-test/lib/rack/test/utils.rb:92:in `map'
     # ~/dev/rack-test/lib/rack/test/utils.rb:92:in `get_parts'
     # ~/dev/rack-test/lib/rack/test/utils.rb:87:in `build_parts'
     # ~/dev/rack-test/lib/rack/test/utils.rb:77:in `build_multipart'
     # ~/dev/rack-test/lib/rack/test.rb:231:in `env_for'
     # ~/dev/rack-test/lib/rack/test.rb:128:in `custom_request'
     # ~/dev/rack-test/lib/rack/test.rb:66:in `post'

Is there any suggested way of doing this with rack-test 0.7.1? Should I report this to Paperclip?

I've tried several things but I'm unable to make it work.

TIA

@perlun perlun added the bug label Nov 20, 2017
@aebirim
Copy link

aebirim commented Nov 20, 2017

Hi,

This is occurring with me as well using FactoryBot (4.8.2), rspec-rails (3.7.2), ruby (2.4.2), rack-test (0.8.0)

  FactoryBot.define do
        factory :document do
            file Rack::Test::UploadedFile.new(File.new("#{Rails.root}/spec/fixtures/files/test_doc.doc"))
            initialize_with { new({ file: file }) }
         end
  end

Stacktrace:

#gems/ruby-2.4.2/gems/rack-test-0.8.0/lib/rack/test/uploaded_file.rb:58:in `initialize_from_io': Missing `original_filename` for IO (ArgumentError)
#gems/.rvm/gems/ruby-2.4.2/gems/rack-test-0.8.0/lib/rack/test/uploaded_file.rb:23:in `initialize'
#spec/factories/document.rb:3:in `new'
#spec/factories/document.rb:3:in `block (2 levels) in <top (required)>'
#.rvm/gems/ruby-2.4.2/gems/factory_bot-4.8.2/lib/factory_bot/syntax/default.rb:18:in `instance_eval'

When I roll back to rack-test 0.7.0, everything works fine.

Is there a way to get this to work in rack-test 0.8.0 ?

@Petercopter
Copy link

Getting this same error when converting a FactoryBot factory to JSON.

perlun added a commit that referenced this issue Nov 20, 2017
Dynamic languages... Can't live with them, can't live without them.

In #149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message...

This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts.

Fixes #207.
@perlun
Copy link
Contributor Author

perlun commented Nov 20, 2017

I think I have a fix for this, thanks to you all for your input. See #210 - please try that branch in your project and see if it works:

gem 'rack-test', github: 'rack-test/rack-test', branch: 'fix/uploaded-file-regression'

(I haven't seen this error myself, so I am a bit "walking in the dark here" - your input is highly valued!)

@aebirim
Copy link

aebirim commented Nov 20, 2017

just tested your fix and it works.

much appreciated for quick turnaround.

@Petercopter
Copy link

Just tested, works for me 👍 🌮 🍺

@007lva
Copy link

007lva commented Nov 21, 2017

Update Capybara to 2.16.1 also fix the issue to me, due to this change:

teamcapybara/capybara@95a297e

perlun referenced this issue in teamcapybara/capybara Nov 21, 2017
perlun added a commit that referenced this issue Nov 21, 2017
Dynamic languages... Can't live with them, can't live without them.

In #149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message...

This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts.

Fixes #207.
perlun added a commit that referenced this issue Nov 21, 2017
* UploadedFile: Handle content being a Pathname

Dynamic languages... Can't live with them, can't live without them.

In #149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message...

This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts.

Fixes #207.

* Peer suggestion: Invert check to make the code safer.
@perlun
Copy link
Contributor Author

perlun commented Nov 21, 2017

Fix released in v0.8.2, published to Rubygems a minute ago. Enjoy!

@aebirim
Copy link

aebirim commented Nov 21, 2017

Sorry to be a pain guys but my error is reoccurring with rack-test 0.8.2.

Have also opened it as a new issue: #211

@tagliala
Copy link

Thanks!

@aebirim are you also using capybara?

@aebirim
Copy link

aebirim commented Nov 21, 2017

@tagliala not using Capybara. Using FactoryBot_Rails (4.8.2), rspec-rails (3.7.2), ruby (2.4.2)

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this issue Apr 5, 2021
* UploadedFile: Handle content being a Pathname

Dynamic languages... Can't live with them, can't live without them.

In rack#149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message...

This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts.

Fixes rack#207.

* Peer suggestion: Invert check to make the code safer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants