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 appconfig migration to handle existing record #2780

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Mar 8, 2023

Closes #2779

Code Changes

  • include a server_default of encrypted {} value when creating the config_set column in migration, so as to handle cases where an existing appconfig record exists while allow the new column to be nullable.

Steps to Confirm

  • see issue description for repro steps

Pre-Merge Checklist

Description Of Changes

the proposed solution is not ideal - i'm not sure of a truly clean way to handle this. the column in question here is a new column in 2.8.x and it's an encrypted JSON column that ideally should be non-nullable. before this change, the problem with the migration was that it just added the non-nullable column to the table, but doesn't account for the fact that there may be an existing record in the db, even though that's quite a narrow edge case. but in that case, the new non-nullable column is added to the record, but the column is obviously null since it didn't exist, and that's the error we get.

the "obvious" fix would be to give that column a server_default when creating it in the migration, so that it's not null. but i don't think we can do effectively that from the context of the migration, since it's encrypted JSON, and it's not like the migration has access to the app encryption key. UPDATE - i was totally wrong about this, thanks to @pattisdr for showing me that we do indeed have access to the app encryption key in the migration. so this is the path i've decided on now!

it's worth noting that this is handling a pretty narrow edge case - but @RobertKeyser did run into it on a migration of an internally hosted app, so it's worth us fixing!

@cypress
Copy link

cypress bot commented Mar 8, 2023

Passing run #710 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 4a8d593 into 0602e8b...
Project: fides Commit: a5d59b6c96 ℹ️
Status: Passed Duration: 00:36 💡
Started: Mar 9, 2023 6:25 PM Ended: Mar 9, 2023 6:26 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (f5ae75a) 86.55% compared to head (4a8d593) 86.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2780   +/-   ##
=======================================
  Coverage   86.55%   86.55%           
=======================================
  Files         291      291           
  Lines       16488    16488           
  Branches     2117     2117           
=======================================
  Hits        14272    14272           
  Misses       1817     1817           
  Partials      399      399           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

this looks fine! I've provided some other optional paths, but this is good too.

@adamsachs adamsachs force-pushed the 2779-db-migration-fails-from-27x-28x-if-applicationconfig-record-is-present branch from f0b4be1 to aab7ea5 Compare March 9, 2023 16:25
@adamsachs adamsachs changed the title update appconfig migration to remove existing record update appconfig migration to handle existing record Mar 9, 2023
@adamsachs adamsachs marked this pull request as ready for review March 9, 2023 16:51
@adamsachs adamsachs force-pushed the 2779-db-migration-fails-from-27x-28x-if-applicationconfig-record-is-present branch from aab7ea5 to 4a8d593 Compare March 9, 2023 18:12
@adamsachs adamsachs merged commit 41a5540 into main Mar 9, 2023
@adamsachs adamsachs deleted the 2779-db-migration-fails-from-27x-28x-if-applicationconfig-record-is-present branch March 9, 2023 19:29
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.

DB migration fails from 2.7.x --> 2.8.x if applicationconfig record is present
2 participants