ENG-2799: Support many:one integration-to-system relationship#7555
ENG-2799: Support many:one integration-to-system relationship#7555
Conversation
- Remove lazy-load footgun in from_orm; pass system info explicitly via SQL joins - Return lightweight ref entities from resolve_connection_config/resolve_system - Add SystemLinkResponse.from_entity() to reduce route handler duplication - Move TooManyLinksError check after connection config lookup in set_links - Remove empty __table_args__ from SystemConnectionConfigLink model - Remove unnecessary cleanup_links autouse fixtures from tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ype annotation - Use outerjoin instead of inner join so links with deleted systems are still returned (with None system info) rather than silently dropped - Make _to_entity_with_system an instance method (called via self) - Remove unnecessary string forward reference on SystemRef type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change System.connection_configs from a scalar (uselist=False) to a list (uselist=True) so the ORM, schema, API endpoints, and frontend all correctly handle multiple integrations per system. The join table already supports this — this change propagates the list semantics through every layer. No new migrations required. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConverts System.connection_configs from a single object to a list throughout the codebase; updates DB model, API endpoints, type definitions, fixtures, frontend access patterns, and tests to use array semantics and to operate on the first connection config when needed. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant API
participant DB
Frontend->>API: Request system details
API->>DB: Query System with connection_configs (ordered by created_at)
DB-->>API: System + list(connection_configs)
API-->>Frontend: SystemResponse (connection_configs: [...])
Frontend->>Frontend: Use connection_configs?.[0]?.key for UI actions
Frontend->>API: PATCH /systems/:id/connection_secrets (uses first config key)
API->>DB: Update secrets for ConnectionConfig.key = connection_configs[0].key
DB-->>API: Success
API-->>Frontend: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Guard against undefined connection_config_key in CatalogSystemsTable and update test assertion to expect [] instead of None for the now list-valued connection_configs relationship. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
adamsachs
left a comment
adamsachs
left a comment
There was a problem hiding this comment.
some self-review notes, at least one thing i need to circle back to:
| type: Changed | ||
| description: Changed System.connection_configs from scalar to list for many:one support | ||
| pr: 7555 | ||
| labels: ["high-risk"] |
There was a problem hiding this comment.
eh, claude thought this was high-risk but i think i'll probably remove this label
| labels: ["high-risk"] | |
| labels: [] |
| </Box> | ||
| <ConnectionForm | ||
| connectionConfig={activeSystem.connection_configs} | ||
| connectionConfig={activeSystem.connection_configs?.[0] ?? null} |
There was a problem hiding this comment.
we keep this functionality the same on the FE for now - we simply choose the 'first' integration back from the API. this is to limit the scope of the change for now, and we have a followup effort to update this UX.
| return update_connection_secrets( | ||
| connection_service, | ||
| system.connection_configs.key, | ||
| system.connection_configs[0].key, |
There was a problem hiding this comment.
hmm. looking at this more closely, i think this could introduce a potential bug with the FE integration: if the 'first' integration coming back on the 'get_system_connections` endpoint response is not the same as this 'first' integration that gets chosen here on the secret update, someone using the UI to update secrets could actually update secrets of a different connection.
the source of the issue here is clear: we're continuing to try and support this particular UX that treats this as only a single connection/integration per system, but on the BE we can have many.
i'll think a bit about how to ensure this discrepancy doesn't occur for now, in a targeted way, until we change/improve that UX...
There was a problem hiding this comment.
Would it help if we made the ordering of the connection configs deterministic (e.g. always ordering by created_at)? That might ensure the "first" connection is consistent between the get_system_connections response and the secret update.
Maybe as a temporary mitigation until we improve the UX around multiple connections per system
adamsachs
requested review from
gilluminate and
thabofletcher
and removed request for
a team
Greptile SummaryThis PR propagates the Key changes and observations:
Confidence Score: 3/5
Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/fides/api/api/v1/endpoints/system.py (1)
202-209:⚠️ Potential issue | 🔴 CriticalBlock ambiguous first-item mutation/deletion when multiple integrations exist.
Line 209 and Line 241 implicitly target
connection_configs[0]. With multiple linked configs, this can patch/delete the wrong integration.🛠️ Proposed fix
def patch_connection_secrets( @@ if not system.connection_configs: raise HTTPException( status_code=HTTP_404_NOT_FOUND, detail="No integration found linked to this system", ) + if len(system.connection_configs) > 1: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + "Multiple integrations are linked to this system. " + "Use PATCH /connection/{connection_key}/secret." + ), + ) return update_connection_secrets( connection_service, system.connection_configs[0].key, @@ def delete_connection(fides_key: str, *, db: Session = Depends(deps.get_db)) -> None: @@ if not system.connection_configs: raise HTTPException( status_code=HTTP_404_NOT_FOUND, detail="No integration found linked to this system", ) + if len(system.connection_configs) > 1: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + "Multiple integrations are linked to this system. " + "Use DELETE /connection/{connection_key} or system-links endpoints." + ), + ) delete_connection_config(db, system.connection_configs[0].key)Also applies to: 235-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/api/v1/endpoints/system.py` around lines 202 - 209, The code currently assumes the first linked config (system.connection_configs[0]) when calling update_connection_secrets and when deleting, which can mutate/delete the wrong integration if multiple configs exist; update the endpoints (calls to update_connection_secrets and the delete logic) to require an unambiguous selection: if len(system.connection_configs) > 1, return an HTTP 400/422 asking for an explicit connection key or accept and validate a supplied connection_key parameter, otherwise proceed using the single config; validate the chosen key exists in system.connection_configs (match by .key) before calling update_connection_secrets or delete_connection_config to avoid accidentally operating on the wrong config.
🧹 Nitpick comments (1)
tests/ops/api/v1/endpoints/test_system.py (1)
557-559: Add a multi-link delete test to lock expected selection semantics.This assertion now depends on first-item selection. Please add one test with 2 linked configs to explicitly define which config should be deleted when calling the system-level delete endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/api/v1/endpoints/test_system.py` around lines 557 - 559, Add a new test (e.g., test_system_multi_link_delete_selection_semantics) that constructs a system with two linked connection configs by appending a second config to system_linked_with_oauth2_authorization_code_connection_config.connection_configs, capture both keys (connection_configs[0].key and connection_configs[1].key), invoke the system-level delete endpoint the same way the existing test does, and assert that the deleted config is the expected one (explicitly assert which key was removed and that the remaining connection_configs contains the other key) to lock the first-item selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/cypress/fixtures/systems/system_active_integration.json`:
- Around line 47-71: Replace internal/system-specific identifiers and
non-reserved domains in the connection_configs fixture: change the "key" value
("fidesctl_system_iterable_api") and "saas_config.fides_key" to a generic
placeholder (e.g., "example_system_iterable" or "example_saas_key"), and update
"secrets.domain" ("api.iterable.com") to an RFC 2606 reserved example domain
like "api.example.com" or "example.com"; keep the "secrets.api_key" masked
(**********) and leave other fields intact so tests still reflect a saas
connection with enabled erasure action.
In `@clients/admin-ui/cypress/fixtures/systems/system_disabled_integration.json`:
- Line 56: Replace the non‑RFC2606 hostname in the fixture: in the
system_disabled_integration.json fixture update the "host" JSON field (currently
"host.docker.internal") to an RFC2606 reserved example domain such as
"db.example.com" so the fixture uses an example domain instead of a real
hostname.
In `@src/fides/api/models/sql_models.py`:
- Around line 599-605: The connection_configs relationship on the model is
unordered so using connection_configs[0] can return an arbitrary row; update the
relationship declaration for connection_configs to include an explicit order_by
(e.g., order_by=ConnectionConfig.id or ConnectionConfig.created_at) so the list
is deterministic, ensuring callers like update_connection_secrets and
delete_connection_config in system.py operate on the intended config; modify the
relationship definition that references "ConnectionConfig" to add the order_by
argument and run tests to confirm index-based usages are stable.
---
Duplicate comments:
In `@src/fides/api/api/v1/endpoints/system.py`:
- Around line 202-209: The code currently assumes the first linked config
(system.connection_configs[0]) when calling update_connection_secrets and when
deleting, which can mutate/delete the wrong integration if multiple configs
exist; update the endpoints (calls to update_connection_secrets and the delete
logic) to require an unambiguous selection: if len(system.connection_configs) >
1, return an HTTP 400/422 asking for an explicit connection key or accept and
validate a supplied connection_key parameter, otherwise proceed using the single
config; validate the chosen key exists in system.connection_configs (match by
.key) before calling update_connection_secrets or delete_connection_config to
avoid accidentally operating on the wrong config.
---
Nitpick comments:
In `@tests/ops/api/v1/endpoints/test_system.py`:
- Around line 557-559: Add a new test (e.g.,
test_system_multi_link_delete_selection_semantics) that constructs a system with
two linked connection configs by appending a second config to
system_linked_with_oauth2_authorization_code_connection_config.connection_configs,
capture both keys (connection_configs[0].key and connection_configs[1].key),
invoke the system-level delete endpoint the same way the existing test does, and
assert that the deleted config is the expected one (explicitly assert which key
was removed and that the remaining connection_configs contains the other key) to
lock the first-item selection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c25a311-58dc-44c1-af44-65bfa9595a41
📒 Files selected for processing (16)
changelog/7555-many-one-integration-system.yamlclients/admin-ui/cypress/fixtures/data-catalog/catalog-systems.jsonclients/admin-ui/cypress/fixtures/systems/system_active_integration.jsonclients/admin-ui/cypress/fixtures/systems/system_disabled_integration.jsonclients/admin-ui/src/features/data-catalog/systems/CatalogSystemsTable.tsxclients/admin-ui/src/features/system/hooks/useSystemFormTabs.tsxclients/admin-ui/src/pages/systems/configure/[id]/test-datasets.tsxclients/admin-ui/src/types/api/models/SystemResponse.tsclients/admin-ui/src/types/api/models/SystemResponseExtended.tsclients/admin-ui/src/types/api/models/SystemWithMonitorKeys.tsclients/privacy-center/types/api/models/SystemResponse.tssrc/fides/api/api/v1/endpoints/system.pysrc/fides/api/models/sql_models.pysrc/fides/api/schemas/system.pytests/ctl/core/test_api.pytests/ops/api/v1/endpoints/test_system.py
| "connection_configs": [ | ||
| { | ||
| "name": null, | ||
| "key": "fidesctl_system_iterable_api", | ||
| "description": "", | ||
| "connection_type": "saas", | ||
| "access": "write", | ||
| "created_at": "2024-08-01T20:38:51.953543+00:00", | ||
| "updated_at": "2024-08-02T16:07:57.769608+00:00", | ||
| "disabled": false, | ||
| "last_test_timestamp": "2024-08-02T16:07:58.452157+00:00", | ||
| "last_test_succeeded": true, | ||
| "saas_config": { | ||
| "fides_key": "fidesctl_system_iterable_api", | ||
| "name": "Iterable", | ||
| "type": "iterable" | ||
| }, | ||
| "secrets": { | ||
| "api_key": "**********", | ||
| "domain": "api.iterable.com" | ||
| }, | ||
| "authorized": false, | ||
| "enabled_actions": ["erasure"] | ||
| } | ||
| ] |
There was a problem hiding this comment.
Sanitize fixture identifiers and domains to approved placeholders.
This changed block includes non-reserved/example domain data (api.iterable.com) and internal-like system identifiers (fidesctl_system_iterable_api). Please replace with generic placeholders.
🧹 Suggested fixture-safe replacement
- "key": "fidesctl_system_iterable_api",
+ "key": "example_saas_integration",
...
- "fides_key": "fidesctl_system_iterable_api",
- "name": "Iterable",
- "type": "iterable"
+ "fides_key": "example_saas_integration",
+ "name": "Example SaaS",
+ "type": "example_saas"
...
- "domain": "api.iterable.com"
+ "domain": "api.example.com"As per coding guidelines, "Use RFC 2606 reserved domains and subdomains in code examples" and "Do not include internal Ethyca URLs, server names, database names, or system references in code - use generic placeholders or environment variables instead".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "connection_configs": [ | |
| { | |
| "name": null, | |
| "key": "fidesctl_system_iterable_api", | |
| "description": "", | |
| "connection_type": "saas", | |
| "access": "write", | |
| "created_at": "2024-08-01T20:38:51.953543+00:00", | |
| "updated_at": "2024-08-02T16:07:57.769608+00:00", | |
| "disabled": false, | |
| "last_test_timestamp": "2024-08-02T16:07:58.452157+00:00", | |
| "last_test_succeeded": true, | |
| "saas_config": { | |
| "fides_key": "fidesctl_system_iterable_api", | |
| "name": "Iterable", | |
| "type": "iterable" | |
| }, | |
| "secrets": { | |
| "api_key": "**********", | |
| "domain": "api.iterable.com" | |
| }, | |
| "authorized": false, | |
| "enabled_actions": ["erasure"] | |
| } | |
| ] | |
| "connection_configs": [ | |
| { | |
| "name": null, | |
| "key": "example_saas_integration", | |
| "description": "", | |
| "connection_type": "saas", | |
| "access": "write", | |
| "created_at": "2024-08-01T20:38:51.953543+00:00", | |
| "updated_at": "2024-08-02T16:07:57.769608+00:00", | |
| "disabled": false, | |
| "last_test_timestamp": "2024-08-02T16:07:58.452157+00:00", | |
| "last_test_succeeded": true, | |
| "saas_config": { | |
| "fides_key": "example_saas_integration", | |
| "name": "Example SaaS", | |
| "type": "example_saas" | |
| }, | |
| "secrets": { | |
| "api_key": "**********", | |
| "domain": "api.example.com" | |
| }, | |
| "authorized": false, | |
| "enabled_actions": ["erasure"] | |
| } | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/cypress/fixtures/systems/system_active_integration.json`
around lines 47 - 71, Replace internal/system-specific identifiers and
non-reserved domains in the connection_configs fixture: change the "key" value
("fidesctl_system_iterable_api") and "saas_config.fides_key" to a generic
placeholder (e.g., "example_system_iterable" or "example_saas_key"), and update
"secrets.domain" ("api.iterable.com") to an RFC 2606 reserved example domain
like "api.example.com" or "example.com"; keep the "secrets.api_key" masked
(**********) and leave other fields intact so tests still reflect a saas
connection with enabled erasure action.
| "last_test_succeeded": null, | ||
| "saas_config": null, | ||
| "secrets": { | ||
| "host": "host.docker.internal", |
There was a problem hiding this comment.
Use an RFC2606-safe hostname in fixture secrets.
host.docker.internal should be replaced with a reserved example domain (for example, db.example.com) in this fixture.
🔧 Proposed fix
- "host": "host.docker.internal",
+ "host": "db.example.com",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "host": "host.docker.internal", | |
| "host": "db.example.com", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/cypress/fixtures/systems/system_disabled_integration.json`
at line 56, Replace the non‑RFC2606 hostname in the fixture: in the
system_disabled_integration.json fixture update the "host" JSON field (currently
"host.docker.internal") to an RFC2606 reserved example domain such as
"db.example.com" so the fixture uses an example domain instead of a real
hostname.
vcruces
left a comment
vcruces
left a comment
There was a problem hiding this comment.
Looks good to me! I left a comment that might help mitigate the potential issue you mentioned. Coderabbit suggested the same idea as well
vcruces
left a comment
vcruces
left a comment
There was a problem hiding this comment.
Looks good to me! I left a comment that might help mitigate the potential issue you mentioned. Coderabbit suggested the same idea as well
- Remove high-risk changelog label - Add deterministic order_by=created_at to connection_configs relationship so connection_configs[0] is always the oldest integration - Document deprecated endpoint behavior when multiple integrations exist - Add test for system with multiple connection configs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Ticket ENG-2799
Description Of Changes
Change
System.connection_configsfrom a scalar (uselist=False) to a list (uselist=True) so the ORM, schema, API endpoints, and frontend all correctly handle multiple integrations per system.The join table (
system_connection_config_link) already supports many:one — this change propagates list semantics through every layer. No new migrations required.This is a breaking API change:
connection_configsin the system response is now an array instead of a single object. External consumers expecting a dict will need to update.Note: our
System > Integrationstab for a given system will stay showing one integration, as it does today, for simplicity and to keep this changed scoped. the UI will simply use the 'first' connection config information, ordered by the connection config (integration)'s created date (first created shows first). we do have a followup effort planned to update that UX to show the potentially many integrations associated with a given system.#7557 is a followup to handle a change to consent tracking status.
Code Changes
src/fides/api/models/sql_models.py- Changeuselist=False→uselist=TrueonSystem.connection_configsrelationshipsrc/fides/api/schemas/system.py- Changeconnection_configstype fromOptional[ConnectionConfigurationResponse]→Optional[List[ConnectionConfigurationResponse]]src/fides/api/api/v1/endpoints/system.py- Update deprecatedpatch_connection_secretsanddelete_connectionendpoints to guard for empty list and use[0]indexingclients/admin-ui/src/features/data-catalog/systems/CatalogSystemsTable.tsx- Update 3 scalar access patterns to?.[0]?.keyclients/admin-ui/src/features/system/hooks/useSystemFormTabs.tsx- Passconnection_configs?.[0] ?? nullto ConnectionForm, update key accessclients/admin-ui/src/pages/systems/configure/[id]/test-datasets.tsx- Update scalar.keyaccess to?.[0]?.keyclients/admin-ui/src/types/api/models/SystemResponse.ts- Update TS type toArray<ConnectionConfigurationResponse> | nullclients/admin-ui/src/types/api/models/SystemResponseExtended.ts- Same type updateclients/admin-ui/src/types/api/models/SystemWithMonitorKeys.ts- Same type updateclients/privacy-center/types/api/models/SystemResponse.ts- Same type updateclients/admin-ui/cypress/fixtures/systems/system_active_integration.json- Wrap fixtureconnection_configsin arrayclients/admin-ui/cypress/fixtures/systems/system_disabled_integration.json- Wrap fixtureconnection_configsin arrayclients/admin-ui/cypress/fixtures/data-catalog/catalog-systems.json- Wrap fixtureconnection_configsin arraystests/ops/api/v1/endpoints/test_system.py- Update.connection_configs.key→.connection_configs[0].keySteps to Confirm
nox -s static_checkspasses (ruff check, ruff format, mypy — all clean)grep -rn "\.connection_configs\." src/ tests/shows no remaining scalar access patternsArray<ConnectionConfigurationResponse> | nullGET /system/{fides_key}returns both in a listPre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation