-
Notifications
You must be signed in to change notification settings - Fork 113
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 CSV delegation to missing StringIO methods #80
Fix CSV delegation to missing StringIO methods #80
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Could you check my comments?
If you create a `CSV` from raw content like: csv = CSV.new("h1,h2") You'll get method missing when calling `csv.path` but still get true when you call `csv.respond_to?(:path)`. This tricks 3rd party libraries like `carrierwave` which try to call `#path` on their input if it responds to it. See https://github.com/carrierwaveuploader/carrierwave/blob/a91ab69fdd8052cdf5a5e48ef8baf40939e441fb/lib/carrierwave/sanitized_file.rb#L109-L123 This stops me from passing `CSV` objects as `StringIO` into `carrierwave` uploads, for example, but the problem can also be manifested in other 3rd party libraries, as responding to a method and returning a `NoMethodError` when calling it is still an unexpected behavior. I have went through the `CSV` delegation scheme and made sure that every method that `StringIO` doesn't respond to returns a meaningful zero value and does not raise a `NoMethodError` while used through a `CSV` Instance.
Thanks for taking a look, @kou! I have changed some of the operations to return |
Did you push? It seems that there is no new commit. |
837cb39
to
cb83cd7
Compare
Yes! I did forget... 🤦♂️ Can you check now? 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could check. :-)
Thank you! |
If you create a
CSV
from raw content like:You'll get a
NoMethodError
when callingcsv.path
but still get true whenyou call
csv.respond_to?(:path)
.This tricks 3rd party libraries like
carrierwave
which try to call pathon their input, if it responds to it.
See https://github.com/carrierwaveuploader/carrierwave/blob/a91ab69fdd8052cdf5a5e48ef8baf40939e441fb/lib/carrierwave/sanitized_file.rb#L109-L123
This stops me from passing
CSV
objects asStringIO
s intocarrierwave
uploads, for example, but the problem can also be manifested in other
3rd party libraries, as responding to a method and returning a
NoMethodError
when calling it is still an unexpected behavior.I have gone through the
CSV
delegation scheme and made sure that everymethod
StringIO
doesn't respond to returns a meaningful zero valueand does not raise a
NoMethodError
while used through aCSV
Instance.Ref. ruby/ruby#2094