ENG-2806: Migrate IDP staged resources to unified IDP_APP type#7712
ENG-2806: Migrate IDP staged resources to unified IDP_APP type#7712dsill-ethyca merged 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Migrates existing Okta App/Entra App staged resources to unified IDP App resource type with provider stored in meta JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8ae7791 to
8500b2f
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit ea7c614.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds an Alembic data migration that consolidates two provider-specific staged resource types (
Confidence Score: 4/5
Important Files Changed
Reviews (1): Last reviewed commit: "Add changelog fragment for IDP staged re..." | Re-trigger Greptile |
| UPDATE stagedresource | ||
| SET resource_type = 'Okta App' | ||
| WHERE resource_type = 'IDP App' | ||
| AND COALESCE(meta->>'provider', 'okta') = 'okta' |
There was a problem hiding this comment.
Silent
Okta App default for provider-less rows
COALESCE(meta->>'provider', 'okta') = 'okta' means any IDP App row where meta is NULL or the provider key is absent will be silently restored as Okta App during downgrade. The upgrade always stamps a provider value for the two known types, but if any IDP App rows are inserted after the upgrade runs (by application code) without a provider in meta, they would be misclassified as Okta App on rollback rather than being skipped or surfaced as an error.
Consider being explicit about the NULL case, e.g.:
| AND COALESCE(meta->>'provider', 'okta') = 'okta' | |
| AND meta->>'provider' = 'okta' |
…and adding a separate safety check for the NULL provider case (e.g., logging or leaving those rows as-is) to make rollback behaviour fully predictable.
| op.execute( | ||
| sa.text( | ||
| """ | ||
| UPDATE stagedresource | ||
| SET meta = (meta - 'okta_app_id') || jsonb_build_object('app_id', meta->'okta_app_id') | ||
| WHERE jsonb_exists(meta, 'okta_app_id') | ||
| """ | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Rename not scoped to Okta App rows, asymmetric with downgrade
The okta_app_id → app_id rename runs on all rows in stagedresource that have okta_app_id in their meta (no resource_type filter). The corresponding downgrade step (line 89–98) only renames back for resource_type = 'Okta App' rows. If any non-Okta resource happened to have an okta_app_id key in its meta, the upgrade would rename it to app_id but the downgrade would not restore it, silently losing the original key.
Scoping the upgrade rename to match is safer:
UPDATE stagedresource
SET meta = (meta - 'okta_app_id') || jsonb_build_object('app_id', meta->'okta_app_id')
WHERE resource_type = 'Okta App'
AND jsonb_exists(meta, 'okta_app_id')There was a problem hiding this comment.
Code Review
This is a clean, focused migration. The SQL logic is sound and the overall approach — rename the key, add provider, update the type, with a symmetrical downgrade — is correct. The down_revision is properly set to 38071fffda39 (the current chain head). A few minor points below.
Suggestions
-
Rename scope in upgrade (line 22): The
okta_app_id → app_idrename is not restricted toresource_type = 'Okta App'. The downgrade's reverse step is restricted to Okta rows, which creates a subtle asymmetry if any other row type ever has that key. Low risk given the current data model, but tightening theWHEREclause would make the upgrade/downgrade pair perfectly symmetric. -
COALESCE default in downgrade (line 69): The
COALESCE(meta->>'provider', 'okta')default treats provider-lessIDP Approws as Okta on downgrade. This is safe for a clean upgrade→downgrade cycle but could silently misclassify rows inserted by application code without a provider key. An explicitmeta->>'provider' = 'okta'(or a comment explaining the intentional default) would be clearer.
Nice to Have
- Unknown providers in downgrade (line 80):
IDP Approws with a provider other than'okta'or'entra'would survive the downgrade unchanged (stillIDP App, still withproviderkey). Not a concern today, but worth a comment for future maintainers.
No critical issues. The migration is well-structured for its stated scope.
| """ | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Suggestion: Scope the okta_app_id rename to Okta App rows only
The upgrade renames okta_app_id → app_id for all rows that have the key in meta, regardless of resource_type. The downgrade (step 3) only reverses this for rows whose resource_type was restored to Okta App. If any Entra App row happened to have okta_app_id in its meta, the downgrade would leave it with app_id instead of restoring the original key.
Scoping the upgrade to match the downgrade would make the two more symmetric:
UPDATE stagedresource
SET meta = (meta - 'okta_app_id') || jsonb_build_object('app_id', meta->'okta_app_id')
WHERE resource_type = 'Okta App'
AND jsonb_exists(meta, 'okta_app_id')This is low-risk given okta_app_id is semantically Okta-specific, but it makes the intention clearer and the downgrade fully invertible.
| def downgrade(): | ||
| # Restore Okta App resource_type based on provider | ||
| op.execute( | ||
| sa.text( |
There was a problem hiding this comment.
Suggestion: Potential over-matching in downgrade for IDP App rows
AND COALESCE(meta->>'provider', 'okta') = 'okta'This treats any IDP App row that lacks a provider key as an Okta row. The upgrade only adds provider for rows that don't already have it, so any pre-existing IDP App rows without a provider key in meta would be (incorrectly) converted to Okta App during downgrade.
The PR description notes IDP monitor resources aren't in production yet, so the practical risk is low. But it's worth documenting this assumption with a comment, or restricting the downgrade to only rows that were explicitly migrated — for example by checking that meta->>'provider' is not null rather than using COALESCE:
WHERE resource_type = 'IDP App'
AND meta->>'provider' = 'okta'This would leave any IDP App rows with no provider key untouched (their resource_type would remain IDP App after downgrade), which is safer than incorrectly labelling them as Okta.
| AND jsonb_exists(meta, 'provider') | ||
| """ | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Nice to have: Minor round-trip data change for null-meta Okta App rows
An Okta App row with meta = NULL will end up with meta = '{}'::jsonb (empty JSON object) after a full upgrade → downgrade cycle. This happens because:
- Upgrade step 2 sets
meta = COALESCE(NULL, '{}') || '{"provider":"okta"}'→'{"provider":"okta"}' - Downgrade step 4 removes the
providerkey →'{}'::jsonb
The result is semantically equivalent and practically harmless, but worth being aware of. A comment noting this side-effect would help future maintainers.
There was a problem hiding this comment.
Suggestion: No migration tests
Other data migrations in this repo have corresponding tests (see tests/ops/migration_tests/test_custom_reports_migration.py and tests/ops/migration_tests/test_system_dictionary_data_migration.py for examples). Those tests exercise the upgrade and downgrade functions directly against a real DB session.
Given this migration makes four separate SQL updates (some of which are order-dependent), a test that:
- Seeds
stagedresourcerows for Okta App (withokta_app_id), Entra App, and a null-meta Okta App - Runs the upgrade statements
- Asserts
resource_type = 'IDP App',provideris set correctly,app_idpresent,okta_app_idabsent - Runs the downgrade statements
- Asserts original state is restored
...would give confidence that the order of operations is correct and would catch any future regressions from other migrations touching this table.
There was a problem hiding this comment.
Migration verified end-to-end against a live DB with 40 real Okta resources — upgrade, API verification, downgrade, and API verification again all passed. Skipping formal test for this data-only migration given no customers are using IDP resources yet.
| def upgrade(): | ||
| # Rename okta_app_id -> app_id in meta JSON | ||
| op.execute( | ||
| sa.text( |
There was a problem hiding this comment.
Suggestion: The okta_app_id → app_id rename applies to all rows where meta contains okta_app_id, regardless of resource_type. If any non-Okta row ever has that key, it will be renamed here. Consider tightening the filter:
WHERE jsonb_exists(meta, 'okta_app_id')
AND resource_type = 'Okta App'This also creates a downgrade asymmetry: the reverse step (app_id → okta_app_id) is scoped to WHERE resource_type = 'Okta App', so any non-Okta row affected by the upgrade rename would not be restored on downgrade.
There was a problem hiding this comment.
Scoped the rename to resource_type = 'Okta App' for symmetry with the downgrade.
| # Restore Okta App resource_type based on provider | ||
| op.execute( | ||
| sa.text( | ||
| """ |
There was a problem hiding this comment.
Suggestion: The COALESCE(meta->>'provider', 'okta') = 'okta' default silently classifies any IDP App row that has no provider key as an Okta resource. For a clean upgrade→downgrade cycle this is safe (the upgrade always writes a provider key), but if application code ever inserts an IDP App row without a provider, a subsequent downgrade would silently promote it to Okta App without warning. Consider being explicit and using meta->>'provider' = 'okta' — or at minimum add a comment explaining the intentional default.
There was a problem hiding this comment.
Updated to use explicit meta->>'provider' = 'okta' — rows without a provider key will be left as-is during downgrade.
| op.execute( | ||
| sa.text( | ||
| """ | ||
| UPDATE stagedresource |
There was a problem hiding this comment.
Nice to have: Any IDP App row whose provider is neither 'okta' nor 'entra' (e.g. a future provider added later) will not match either of the first two UPDATE statements and will remain as resource_type = 'IDP App' after the downgrade. The provider removal step below would also skip it (since it only targets 'Okta App' and 'Entra App'). This is not a concern today, but worth a short comment acknowledging the gap so it's not surprising if/when new providers are added.
There was a problem hiding this comment.
Acknowledged — only okta and entra providers exist today. Future providers would need this downgrade updated, but that's expected for any migration that predates them.
- Scope okta_app_id rename to Okta App rows for upgrade/downgrade symmetry - Use explicit provider check instead of COALESCE default in downgrade Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-2806
Description Of Changes
Add a data migration to unify IDP staged resource types. Existing
Okta AppandEntra Approws are consolidated to a singleIDP Appresource type, with the provider stored inmeta["provider"]. Also renames the legacyokta_app_idkey to the genericapp_idin the meta JSON.No customer data is affected — IDP monitor resources are not yet in production. This migration keeps continuity in dev/staging environments.
Code Changes
src/fides/api/alembic/migrations/versions/xx_2026_03_20_1745_ad1bb600715b_migrate_idp_staged_resources.py- Data migration: renameokta_app_id→app_idin meta, addproviderkey, updateresource_typefromOkta App/Entra ApptoIDP AppSteps to Confirm
alembic upgrade head— verify migration applies without errorsSELECT resource_type, meta->>'provider', meta->>'app_id' FROM stagedresource WHERE resource_type = 'IDP App'— verify all IDP resources have the new type and provideralembic downgrade 38071fffda39— verify downgrade restores originalOkta App/Entra Apptypes andokta_app_idkeyPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works