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

Add ransack! method which raises error if passed unknown condition #1132

Merged
merged 2 commits into from
Dec 21, 2020

Conversation

alipman88
Copy link
Contributor

Use case: We use Ransack for both public-facing search pages and internal purposes such as targeting bulk email deliveries. In the context of public-facing search, we'd like to be lenient with unknown/invalid search params. However, in the context of targeting bulk emails, this seems dangerous - a broken query could potentially go out to many more recipients than intended.

Thus, we'd like to be able to trigger the ignore_unknown_conditions config option on a case-by-case basis. (Previously requested in #535)

I've drafted what feels like a clean implementation, although here are some other options I considered:

# add Ransack::Search#validate!
Person.ransack(unknown_attr_eq: "Ernie").validate!.result

# pass ignore_unknown_conditions directly as param
Person.ransack(unknown_attr_eq: "Ernie", ignore_unknown_conditions: false)

If folks like this idea but have a different preference for implementing it, let me know and I'll adjust this PR accordingly!

@scarroll32
Copy link
Member

@alipman88 thank you for this PR. 🎸

Can I suggest you rebase as there have been a lot of fixes to Ransack, so this might pass the CI now.

It is a nice feature, and I'm wondering if as part of this PR the ignore_unknown_conditions configuration option could also be documented in the README ? If there are other undocumented configuration options, perhaps also those?

@alipman88
Copy link
Contributor Author

alipman88 commented Dec 13, 2020

Getting back around to this - thanks for your patience, @seanfcarroll.

I'm wondering if as part of this PR the ignore_unknown_conditions configuration option could also be documented in the README ?

Good catch. I've updated the readme to include include both the ignore_unknown_conditions and the new ransack! method under a "Handling unknown predicates or attributes" heading, which I've also moved a little further down (under the whitelisting/blacklisting section)".

If there are other undocumented configuration options, perhaps also those?

I took a glance at the options in lib/ransack/configuration.rb. The remaining config options (search_key, ignore_unknown_conditions, custom_arrows, sanitize_custom_scope_booleans and hide_sort_order_indicators) are covered in the readme.

The slight exception is the search_key option, which is linked to from the the "If you're coming from MetaSearch" section of the readme. I'd say that section of the readme can be moved to the end of the readme now that MetaSearch is eight years old. I've added a second commit moving that section to the bottom, and documenting the search_key option elsewhere.

this might pass the CI now.

The mysql tests are failing. Don't think that's related, but taking a quick look.

@alipman88
Copy link
Contributor Author

alipman88 commented Dec 14, 2020

The mysql tests are failing. Don't think that's related, but taking a quick look.

nvm - I have no idea how, but I had commented out the mysql2 line from the Gemfile. Fixed!

It's been eight years since Ransack succeeded MetaSearch. As most
developers have likely migrated from MetaSearch to Ransack by now,
anyone reading the readme is likely starting fresh with Ransack - so
it's perhaps less useful to highlight differences from MetaSearch in
the readme's introduction.

Move the "Updating From MetaSearch" section to the bottom of the readme,
and make sure any important details (like setting the default search
param) are covered appropriately within the readme.
@scarroll32
Copy link
Member

This looks pretty useful, what do you think @deivid-rodriguez ?

@deivid-rodriguez
Copy link
Contributor

I'm sorry @seanfcarroll, I don't have time at the moment to evaluate this. If it looks good to you, please move it forward, otherwise I can give it a look when I find time 👍.

@scarroll32
Copy link
Member

nvm - I have no idea how, but I had commented out the mysql2 line from the Gemfile. Fixed!

@alipman88 😄 that will do it!

@scarroll32 scarroll32 merged commit a9e84e1 into activerecord-hackery:master Dec 21, 2020
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