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

Raise unless Ransack ignore_unknown_conditions? #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sienic
Copy link

@sienic sienic commented Aug 25, 2021

What is the current behavior?

#62

What is the new behavior?

It allows to use the Ransack configuration property ignore_unkown_conditions with the same semantics as in Ransack.
A disadvantage of this solution is that Ransack so far has been a dependency of jsonapi.rb, and not necessarily of the projects which use Jsonapi.rb. Somehow it could be considered a co-dependency of jsonapi.rb, but that is not up to me.

Another solution would be to add a configuration object to jsonapi.rb.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@sienic
Copy link
Author

sienic commented Aug 25, 2021

Unfortunately, this does not work with the sort parameter (which uses the same function I modified).
I will wait for further feedback on how you'd like to go about this, as there are multiple possibilities, and this issue actually might not be an issue from your point of view.

Copy link
Owner

@stas stas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, in the next version v1.8 Ransack will be a soft-dependency.

@@ -17,6 +17,9 @@ def self.extract_attributes_and_predicates(requested_field)
.detect_and_strip_from_string!(field_name)
predicates << Ransack::Predicate.named(predicate)
end
unless predicates.present? || Ransack.options[:ignore_unknown_conditions]
raise ArgumentError, "No valid predicates for #{requested_field}"
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this is handled already by Ransack:
https://github.com/stas/jsonapi.rb/blob/master/lib/jsonapi/patches.rb#L6

Just reset the option in your config and it should work, no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stas: It seems to be ignored, from my testing.

@CofenseLabs-Doug
Copy link

Thank you for this, @sienic. FWIW I decided to slightly slightly change your solution so that ActionController::BadRequest is raised instead of ArgumentError, resulting in a 400 response to the client instead of 500.

@sienic
Copy link
Author

sienic commented Apr 9, 2022

@CofenseLabs-Doug 👍
@stas how about the other idea of having a way to configure jsonapi, so as to set properties for Ransack at https://github.com/stas/jsonapi.rb/blob/master/lib/jsonapi/patches.rb#L3-L6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants