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

Do not revalidate encrypted attribute with current encrypted_type #48462

Merged
merged 2 commits into from Sep 25, 2023

Conversation

iamradioactive
Copy link
Contributor

@iamradioactive iamradioactive commented Jun 13, 2023

Motivation / Background

Fixes #48445
Fixes #48248

As per the current behaviour, Two queries are being fired to validate uniqueness of an encrypted attribute even in cases when encrypted_type.previous_types is nil. An additional query with the current encrypted/ciphered value should not be fired.

Detail

This PR updates the loop in ExtendedDeterministicUniquenessValidator to iterate over encrypted_type.previous_types only i.e. to exclude the current encrypted_type.

Additional information

For Additional context Please refer to #48445

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated 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.

Copy link
Contributor

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Great work tracking this one down @iamradioactive 🙏

@ghiculescu
Copy link
Member

@iamradioactive do you mind adding Fixes #48248 to your PR body? I confirmed this will also fix #48248

@nvasilevski
Copy link
Contributor

nvasilevski commented Sep 25, 2023

to iterate over encrypted_type.previous_types only i.e. to exclude the current encrypted_type.

Just for my own understanding, by dropping encrypted_type from the list, are we saying that we deliberately don't want to validate encrypted_type or it will still be validated because it will be included in encrypted_type? Thanks

The change itself looks clear!

Upd: I think I found the answer in one of the issues and we deliberately remove the encrypted_type from the list as it is redundant to validate it

@byroot byroot merged commit 99e2999 into rails:main Sep 25, 2023
9 checks passed
@iamradioactive
Copy link
Contributor Author

@iamradioactive do you mind adding Fixes #48248 to your PR body? I confirmed this will also fix #48248

@ghiculescu , Seems like it's already added. Thanks @byroot

@ghiculescu
Copy link
Member

Thanks for the fix @iamradioactive 👏

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