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

Fix UploadedFile#new regression #215

Merged
merged 1 commit into from Feb 13, 2018
Merged

Conversation

perlun
Copy link
Contributor

@perlun perlun commented Dec 27, 2017

In #210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.

@aebirim, please try the following in your application's Gemfile to see if this now resolves the problem you were seeing.

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

@junaruga and/or @scepticulous, please review.

Closes #211

In #210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.
@perlun perlun added the bug label Dec 27, 2017
@perlun perlun self-assigned this Dec 27, 2017
@perlun
Copy link
Contributor Author

perlun commented Jan 8, 2018

@joemsak or others, could you check if this PR helps your problem?

@ollieh-m
Copy link

@perlun I just encountered the problem that (I think) this is intended to resolve. I've just tried with this branch and I don't think it fixes it.

The issue I believe is at this line in utils.rb:
if value.respond_to?(:original_filename)

Where value is #<Capybara::RackTest::Form::NilUploadedFile:0x007fe6ee86da80 @empty_file=#<Tempfile:/var/folders/9h/hrvm5snd38v55y2qlwn3mnjr0000gn/T/nil_uploaded_file20180116-1014-n8cdmw (closed)>>, this evaluates to true, because it does respond to :original_filename.
So we go to build_file_part, which evaluates this:
<<-EOF --#{MULTIPART_BOUNDARY}\r Content-Disposition: form-data; name="#{parameter_name}"; filename="#{escape(uploaded_file.original_filename)}"\r Content-Type: #{uploaded_file.content_type}\r Content-Length: #{uploaded_file.size}\r \r #{uploaded_file.read}\r EOF
And calling uploaded_file.size is where I get the undefined method size' for nil:NilClasserror. (Method_missing tries to sendsizetotempfile, but tempfile` is blank.)

Is the problem that value.respond_to?(:original_filename) is not an appropriate check where the value is a Capybara::RackTest::Form::NilUploadedFile?

(Disclaimer is that this is my first venture into rack-test so I may well be missing or misunderstanding something.)

@ollieh-m
Copy link

ollieh-m commented Jan 16, 2018

Apologies, I have just realised Capybara 2.17 resolves the problem I described above.

@perlun perlun merged commit 10042d3 into master Feb 13, 2018
@perlun perlun deleted the fix/uploaded-file-regression-v2 branch February 13, 2018 07:00
@arkhamRejek
Copy link

is a new version going to be released ? or should we just pull from master

@perlun
Copy link
Contributor Author

perlun commented Feb 22, 2018

@PaulBrunache Sorry for the delay, will try to get a new version published asap.

@arkhamRejek
Copy link

@perlun no rush man, thank you for all your hard work! :bowtie:

@perlun
Copy link
Contributor Author

perlun commented Feb 27, 2018

New release out: https://github.com/rack-test/rack-test/releases/tag/v0.8.3 🎉

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
In rack#210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

My FactoryBot error is reoccurring with rack-test 0.8.2
3 participants