-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 some internal errors in filters from invalid input. #1476
Conversation
nil | ||
end | ||
end | ||
(@filters.public_methods - Object.public_methods).each do |method| |
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 think you could use public_methods(false)
to only get the methods on the receiver:
(@filters.public_methods - Object.public_methods).each do |method| | |
@filters.public_methods(false).each do |method| |
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.
@filters.public_methods(false)
would return []
since @filters
is an instance of a class that includes the StandardFilters
module. However, we could use StandardFilters.public_instance_methods(false)
to get those methods directly. I tested (@filters.public_methods - Object.public_instance_methods) == StandardFilters.public_instance_methods(false)
locally and it returns true
These fixes came from improving the corresponding test, so these might not actually be causing problems in practice.
37850fc
to
0912f68
Compare
Ruby returns 0 (not nil) for nil <=> nil, i.e. nil and nil are judged as equal for comparison, and so returns nil_safe_compare . ref: Shopify#1476 To make the behavior of nil_safe_casecmp consistent with nil_safe_compare , change nil_safe_casecmp so that comparison between nil <=> nil return 0 (equal). Also change testsuite to reflect this change. Fixes Shopify#1759 .
Ruby returns 0 (not nil) for nil <=> nil, i.e. nil and nil are judged as equal for comparison, and so returns nil_safe_compare . ref: Shopify#1476 To make the behavior of nil_safe_casecmp consistent with nil_safe_compare , change nil_safe_casecmp so that comparison between nil <=> nil return 0 (equal). Also change testsuite to reflect this change. Fixes Shopify#1759 .
Problem
While reviewing #1422 I noticed that StandardFilters#test_all_filters_never_raise_non_liquid_exception was doing a lot of redundant work and started to change it to make it more scalable and general. In the process, I noticed that it also wasn't passing in a default argument, which prevented it from finding some code paths that could result in internal errors.
Solution
I used Array#repeated_permutation in StandardFilters#test_all_filters_never_raise_non_liquid_exception so it would test permutations in proportion to the arity of the filter. If the filter method takes optional arguments, then pass in an optional argument in addition to the required arguments.
This uncovered a few errors:
ary.first.respond_to?(:[])
checks which assumed the array had elements of a homogenous type, which could result in an internal error if the first argument responded to[]
but a following one didn't. I added arescue NoMethodError
to handle these errors.nil
to explicitly raise a liquid error.Note that I haven't actually checked if these internal errors are happening in practice.