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

fix(db) run select transformations before process auto fields #9049

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

bungle
Copy link
Member

@bungle bungle commented Jul 4, 2022

Summary

Run select transformations before process auto fields.

Background: there has been reports that for example EE keyring may not work well with vaults. It might be due to fact that for some reason the read transformations ran after process auto fields. I think the correct place to run transformations is to run them late as possible with writes and early as possible with reads. I will open this on EE too, so I can get an idea if this order change causes any problems there.

@bungle bungle marked this pull request as ready for review July 4, 2022 14:07
@bungle bungle requested a review from a team as a code owner July 4, 2022 14:07
@bungle bungle force-pushed the fix/select-transformations branch 2 times, most recently from f21a3d7 to 0bea438 Compare July 5, 2022 09:01
@jschmid1
Copy link
Contributor

jschmid1 commented Jul 5, 2022

To add some context:

When a keyring is present & configured, all fields that are marked with encrypted are being stored encrypted in the datastore. When this field needs to be read again, we have to make sure to decrypt it before dereferencing it.

If process_auto_fields - this is where the a potential reference is being _de_referenced, runs before Schema:transform() we're not able to _de_reference a field as we're still seeing encrypted data.

@bungle Since this is somewhat of a regression, I'd advocate for adding a small test to ensure we're not running into this issue again. wdyt?

@bungle
Copy link
Member Author

bungle commented Jul 5, 2022

@bungle Since this is somewhat of a regression, I'd advocate for adding a small test to ensure we're not running into this issue again. wdyt?

@jschmid1, I agree. Thank you for initial review.

@bungle bungle force-pushed the fix/select-transformations branch from 0bea438 to 1e2e584 Compare July 6, 2022 10:30
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 6, 2022
@bungle bungle force-pushed the fix/select-transformations branch from 1e2e584 to 62e89ed Compare July 6, 2022 10:32
@bungle bungle force-pushed the fix/select-transformations branch from 62e89ed to 1cff800 Compare July 6, 2022 10:33
@bungle
Copy link
Member Author

bungle commented Jul 6, 2022

@bungle Since this is somewhat of a regression, I'd advocate for adding a small test to ensure we're not running into this issue again. wdyt?

@jschmid1, I agree. Thank you for initial review.

The test is now added.

Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

🎉

@bungle bungle merged commit 2d80d97 into master Jul 7, 2022
@bungle bungle deleted the fix/select-transformations branch July 7, 2022 12:15
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

2 participants