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

Update Filter Attributes to perform exact matches instead of Fuzzy matches #51488

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

Conversation

keshavbiswa
Copy link

@keshavbiswa keshavbiswa commented Apr 4, 2024

Motivation / Background

This Pull Request has been created because of a bug mentioned in #51254 wherein encrypting one attribute in a model filters out other similar matching attributes in inspect.

Detail

This PR makes the following changes:

  • Updates filter_attributes to exact regex matches instead of fuzzy matching

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@bdewater
Copy link
Contributor

bdewater commented Apr 4, 2024

I don't think an extra argument is needed when you already can pass a regex to precisely filter:

# # Replaces values for the exact key "pin" and for keys that begin with
# # "pin_". Does not match keys that otherwise include "pin" as a
# # substring, such as "shipping_id".
# ActiveSupport::ParameterFilter.new([/\Apin\z/, /\Apin_/])

IMO better would be for auto_filtered_parameters.rb to compile a more precise regex than ParameterFilter.precompile_filters does when given a string/symbol.

@keshavbiswa keshavbiswa force-pushed the keshav/add-exact-filtering-to-parameter-filter branch 2 times, most recently from 6d7820d to ffefaf3 Compare April 4, 2024 16:06
@keshavbiswa
Copy link
Author

I don't think an extra argument is needed when you already can pass a regex to precisely filter:

# # Replaces values for the exact key "pin" and for keys that begin with
# # "pin_". Does not match keys that otherwise include "pin" as a
# # substring, such as "shipping_id".
# ActiveSupport::ParameterFilter.new([/\Apin\z/, /\Apin_/])

IMO better would be for auto_filtered_parameters.rb to compile a more precise regex than ParameterFilter.precompile_filters does when given a string/symbol.

You're right. adding this line does fix the problem:

            klass.filter_attributes += [ /^#{attribute}$/ ]

My concern was if we ever need to do the same then it would make sense to have a seperate approach for it altogether.
But then again, I'd be willing to remove and replace just this line if we don't want to add any more methods as well.

@hahmed
Copy link
Contributor

hahmed commented Apr 4, 2024

I don't think an extra argument is needed when you already can pass a regex to precisely filter:

+1 to this

Maybe we can add a config value to switch between fuzzy and exact match with the default being what we have currently (fuzzy)? I don't think we should have one or the other, not sure that's a consistent experience with how Filtering is currently used.

@keshavbiswa keshavbiswa force-pushed the keshav/add-exact-filtering-to-parameter-filter branch from ffefaf3 to 1e021bb Compare April 5, 2024 07:36
@keshavbiswa
Copy link
Author

I don't think an extra argument is needed when you already can pass a regex to precisely filter:

+1 to this

Maybe we can add a config value to switch between fuzzy and exact match with the default being what we have currently (fuzzy)? I don't think we should have one or the other, not sure that's a consistent experience with how Filtering is currently used.

I don't think for encryption we need fuzzy matches. If i'm encrypting name I'd only expect name to be filtered and not first_name or name_entered.

@keshavbiswa keshavbiswa force-pushed the keshav/add-exact-filtering-to-parameter-filter branch from 1e021bb to e8fd465 Compare April 5, 2024 08:55
@rails-bot rails-bot bot added the railties label Apr 5, 2024
@bdewater
Copy link
Contributor

bdewater commented Apr 5, 2024

Agreed 👍

Can you update the pull request title and add a CHANGELOG entry? Hopefully it'll be merged soon :)

@keshavbiswa keshavbiswa force-pushed the keshav/add-exact-filtering-to-parameter-filter branch from e8fd465 to bdb003e Compare April 6, 2024 11:46
@keshavbiswa keshavbiswa requested a review from p8 April 6, 2024 11:46
@keshavbiswa keshavbiswa changed the title Add exact filtering to parameter filter Update Filter Attributes to perform exact matches instead of Fuzzy matches Apr 6, 2024
@keshavbiswa
Copy link
Author

Updated the CHANGELOG and the description.

@keshavbiswa keshavbiswa force-pushed the keshav/add-exact-filtering-to-parameter-filter branch from bdb003e to 6b0724f Compare April 6, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants