Using Rack::Test::UploadedFile.new with StringIO causes an exception

pawlik · March 28, 2023, 8:28am

Hello there. I was wondering if someone could take a look at this PR

github.com/rails/rails

Using Rack::Test::UploadedFile.new with `StringIO` causes an exception

rails:main ← pawlik:possible-bug-in-activestorage-with-rack-test-uploaded-file

opened 06:41PM - 21 Mar 23 UTC

pawlik

+23 -2

### Motivation / Background This Pull Request has been created because I believe we're using the `Rack::Test::UplaodedFile` wrong. This object can be instantiated with an actual File object (this part is extensively tested throughout the suite), but it can also be instantiated with StringIO. Unfortunately, the latter does not have the `#open` method, and using it results in an exception. We need to branch out (with duck-typing, as proposed in this PR, but other suggestions are welcome) and use the `Rack::Test::UploadedFile` depending on what it has "under the hood." ### Detail This Pull Request changes `ActiveStorage::Attached::Changes::CreateOne#upload` (and private `#find_or_build_blob`) so it does not call `#open` on a `Rack::Test::UploadedFile` if it does not respond to this method. It uses the object itself (which can be read from just like opened File) ### Additional information This is my first PR to rails. I'm eager to learn and drive it all the way, but I have some doubts and can use some guidance: - [ ] is it better to use `attachable` or `attachable.reopen` ? - [ ] are there more tests needed to cover this? - [ ] I believe it's better to split the cases for `ActionDispatch::Http::UploadedFile` and `Rack::Test::UploadedFile`. It does introduce some duplication, but on the other hand I'm not sure the former needs this extra treatment. - [ ] it feels weird to test the testing aspect of the ActiveStorage, not ActiveStorage itself, maybe there's a better place for the test? I also confirmed with `rack-test`'s maintainer this gem's behavior is expected: https://github.com/rack/rack-test/issues/336 ### Checklist Before submitting the PR make sure the following are checked: * [ ] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs. * [ ] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: `[Fix #issue-number]` * [ ] Tests are added or updated if you fix a bug or add a feature. * [ ] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

I think I’ve found a genuine bug, and I’m excited to drive the PR all the way with your guidance.