ENG-2799: Track consent status per-connection instead of per-system#7557
ENG-2799: Track consent status per-connection instead of per-system#7557
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>
Add ConnectionConfig.consent_tracking_key property that uses the connection config's key (unique per integration) instead of system_key (shared across all integrations on a system). This prevents status overwrites when a system has multiple integrations (e.g. DSR + monitoring) processing consent for the same privacy request. affected_system_status dict keys change from system fides_key to connection config key. This is a data-level breaking change for consent reporting queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change all affected_system_status assertions from system_key/ system.fides_key/connection_config.name to consent_tracking_key (connection_config.key), matching the new per-connection keying. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Add missed callsites (seed.py, consent_util.py/graph_task.py as Category D addressed by PR #7557), expand fidesplus discovery monitor coverage, cite exact dead code evidence, and add verification notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request updates consent status tracking to use per-connection keys instead of system keys. Changes include adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Use saas_example_opt_out_only_connection_config (the actual fixture) instead of saas_example_connection_config which is not in the test's parameter list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a race condition in consent status tracking where Issues for follow-up:
Confidence Score: 4/5
Last reviewed commit: 5db70cf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @property | ||
| def consent_tracking_key(self) -> str: | ||
| """Unique key used for per-connection consent status tracking. | ||
|
|
||
| Uses the ConnectionConfig key so that each integration on a system | ||
| gets its own entry in ``affected_system_status``, avoiding the | ||
| overwrite bug when a system has multiple integrations. | ||
| """ | ||
| return self.key |
There was a problem hiding this comment.
Missing unit test for consent_tracking_key
The new consent_tracking_key property has no dedicated unit test in tests/ops/models/test_connectionconfig.py, whereas system_key has test_system_key right next to it (line 161). Since this is the new canonical property for all consent status tracking, a test that verifies it always returns self.key (regardless of whether a system is linked) would document the contract and guard against accidental regressions.
def test_consent_tracking_key(self, db, connection_config, system):
# Always returns the connection config's own key, not the system key
assert connection_config.consent_tracking_key == connection_config.key
SystemIntegrationLinkRepository().create_or_update_link(
system_id=system.id,
connection_config_id=connection_config.id,
session=db,
)
db.refresh(connection_config)
# Still returns connection key, not system fides_key
assert connection_config.consent_tracking_key == connection_config.key
assert connection_config.consent_tracking_key != system.fides_key| @property | ||
| def system_key(self) -> Optional[str]: | ||
| """Property for caching a system identifier for systems (or connector names as a fallback) for consent reporting""" | ||
| """Property for caching a system identifier for systems (or connector names as a fallback) for consent reporting | ||
|
|
||
| .. deprecated:: | ||
| Use :attr:`consent_tracking_key` instead. ``system_key`` returns | ||
| the *system* fides_key which conflates multiple integrations on the | ||
| same system. Kept only for backward-compatible log context. | ||
| """ | ||
| if self.system: | ||
| return self.system.fides_key | ||
| # TODO: Remove this fallback once all connection configs are linked to systems | ||
| # This will always be None in the future. `self.system` will always be set. | ||
| return self.name |
There was a problem hiding this comment.
Misleading deprecation comment on system_key
The deprecation docstring states the property is "Kept only for backward-compatible log context," but no code path in the source actually calls system_key for logging — saas_connector.py accesses self.configuration.system.fides_key directly (saas_connector.py:86). The only remaining callers of .system_key in the codebase are the existing test_system_key test and seed.py (which reads system_key from seed data YAML, not from this model).
The comment as written implies there is an active log consumer that depends on this property, which could mislead future maintainers into thinking they need to keep it around. Consider clarifying, e.g.:
@property
def system_key(self) -> Optional[str]:
"""Returns the system fides_key (or connector name as a fallback).
.. deprecated::
Use :attr:`consent_tracking_key` for consent status tracking.
``system_key`` returns the *system* fides_key, which conflates
multiple integrations on the same system. Retained for any
external callers and backward compatibility only.
"""| @@ -511,7 +511,7 @@ def test_cache_initial_status_and_identities_for_consent_reporting( | |||
|
|
|||
| # non-relevant systems | |||
| assert privacy_preference_history.affected_system_status == { | |||
| connection_config.name: "skipped" | |||
| connection_config.key: "skipped" | |||
There was a problem hiding this comment.
Inconsistent assertion style: .key vs .consent_tracking_key
This file now asserts against connection_config.key directly, while all other updated test files (test_saas_connector.py, test_email_batch_send.py, test_request_runner_service.py) assert against connection_config.consent_tracking_key. Using the property name makes the test's intent self-documenting and means tests will automatically reflect any future change to what consent_tracking_key returns.
Change instances at lines 505, 514, 554, 563, 603, and 612 to use connection_config.consent_tracking_key instead of connection_config.key for consistency with the other test files and clearer intent documentation.
Example:
| == {connection_config.consent_tracking_key: "pending"} |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/ops/service/privacy_request/test_request_runner_service.py (1)
2023-2027: Add a regression with two connectors on one system.This assertion only proves the renamed key for a single connector. The bug fixed by this PR is the overwrite case when two
ConnectionConfigrows share the sameSystem; please add a case that links two connectors to one system and asserts both keys survive inaffected_system_status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/service/privacy_request/test_request_runner_service.py` around lines 2023 - 2027, Add a regression test that covers the overwrite case where two ConnectionConfig rows share the same System: create a second ConnectionConfig (e.g., other_email_connection_config or another config object) that references the same System as sovrn_email_connection_config, ensure the privacy_request_with_consent_policy is set up so both connectors are exercised, and then assert privacy_request_with_consent_policy.privacy_preferences[0].affected_system_status contains both keys (sovrn_email_connection_config.consent_tracking_key and the second connector's consent_tracking_key) with their expected statuses (e.g., "skipped"), verifying neither key is overwritten; update related setup helpers or fixtures to attach both configs to the same System and include the second key in the expected dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changelog/7557-per-connection-consent-tracking.yaml`:
- Around line 2-4: Update the changelog entry to explicitly document the
reporting break: state that affected_system_status is changing its key shape
from system.fides_key to connection_config.key, that existing rows will not be
backfilled, and that any downstream reporting or queries aggregating by
system.fides_key must be updated to use connection_config.key (or migrated) to
avoid data gaps; reference the field names affected_system_status,
system.fides_key, and connection_config.key so consumers can locate and update
their logic.
In `@src/fides/api/util/consent_util.py`:
- Around line 181-190: The code writes status only under
connection_config.consent_tracking_key but preexisting entries may use the
legacy system_key; update the write paths that call pref.cache_system_status(db,
connection_config.consent_tracking_key, ...) to first check
pref.get_affected_system_status/db for an existing entry under
connection_config.system_key (legacy) and, if found, migrate/normalize it to
connection_config.consent_tracking_key (copy or rename the key and remove the
legacy entry) before performing the new write so both old and new keys aren’t
left mixed; apply this normalization logic in the same way for the other call
sites that use pref.cache_system_status and touch affected_system_status (the
blocks around where ExecutionLogStatus is set and the other occurrences
referenced).
---
Nitpick comments:
In `@tests/ops/service/privacy_request/test_request_runner_service.py`:
- Around line 2023-2027: Add a regression test that covers the overwrite case
where two ConnectionConfig rows share the same System: create a second
ConnectionConfig (e.g., other_email_connection_config or another config object)
that references the same System as sovrn_email_connection_config, ensure the
privacy_request_with_consent_policy is set up so both connectors are exercised,
and then assert
privacy_request_with_consent_policy.privacy_preferences[0].affected_system_status
contains both keys (sovrn_email_connection_config.consent_tracking_key and the
second connector's consent_tracking_key) with their expected statuses (e.g.,
"skipped"), verifying neither key is overwritten; update related setup helpers
or fixtures to attach both configs to the same System and include the second key
in the expected dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaf62424-c34e-4ffa-9fec-14037643615e
📒 Files selected for processing (9)
changelog/7557-per-connection-consent-tracking.yamlsrc/fides/api/models/connectionconfig.pysrc/fides/api/service/connectors/consent_email_connector.pysrc/fides/api/task/graph_task.pysrc/fides/api/util/consent_util.pytests/ops/service/connectors/test_saas_connector.pytests/ops/service/privacy_request/test_email_batch_send.pytests/ops/service/privacy_request/test_request_runner_service.pytests/ops/util/test_consent_util.py
| description: Fixed consent status tracking to key per-connection instead of per-system | ||
| pr: 7557 | ||
| labels: ["high-risk"] |
There was a problem hiding this comment.
Document the reporting break in the changelog.
This reads like a transparent bug fix, but affected_system_status is changing shape from system.fides_key to connection_config.key. Please call out that existing rows are not backfilled and any downstream reporting/query logic aggregating by system key must be updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@changelog/7557-per-connection-consent-tracking.yaml` around lines 2 - 4,
Update the changelog entry to explicitly document the reporting break: state
that affected_system_status is changing its key shape from system.fides_key to
connection_config.key, that existing rows will not be backfilled, and that any
downstream reporting or queries aggregating by system.fides_key must be updated
to use connection_config.key (or migrated) to avoid data gaps; reference the
field names affected_system_status, system.fides_key, and connection_config.key
so consumers can locate and update their logic.
| pref.cache_system_status( | ||
| db, connection_config.system_key, ExecutionLogStatus.pending | ||
| db, | ||
| connection_config.consent_tracking_key, | ||
| ExecutionLogStatus.pending, | ||
| ) | ||
| else: | ||
| pref.cache_system_status( | ||
| db, connection_config.system_key, ExecutionLogStatus.skipped | ||
| db, | ||
| connection_config.consent_tracking_key, | ||
| ExecutionLogStatus.skipped, |
There was a problem hiding this comment.
Preserve rollout compatibility for preexisting status entries.
These paths now read/write only connection_config.consent_tracking_key. For requests already in flight before deploy, affected_system_status may already contain the legacy system_key, so this can leave mixed entries behind for the same connector (for example old pending plus new complete/skipped/error). Please normalize or migrate the legacy key on first write during the rollout window.
Also applies to: 205-213, 249-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/util/consent_util.py` around lines 181 - 190, The code writes
status only under connection_config.consent_tracking_key but preexisting entries
may use the legacy system_key; update the write paths that call
pref.cache_system_status(db, connection_config.consent_tracking_key, ...) to
first check pref.get_affected_system_status/db for an existing entry under
connection_config.system_key (legacy) and, if found, migrate/normalize it to
connection_config.consent_tracking_key (copy or rename the key and remove the
legacy entry) before performing the new write so both old and new keys aren’t
left mixed; apply this normalization logic in the same way for the other call
sites that use pref.cache_system_status and touch affected_system_status (the
blocks around where ExecutionLogStatus is set and the other occurrences
referenced).
1 participant
Ticket ENG-2799
Description Of Changes
Fix the consent status tracking race condition where
affected_system_statuswas keyed bysystem.fides_key, causing status overwrites when a system has multiple integrations (e.g. DSR + monitoring) processing consent for the same privacy request.Add
ConnectionConfig.consent_tracking_keyproperty that returnsself.key(unique per integration) instead ofsystem_key(shared across all integrations on the same system). All consent tracking callsites now use this property.This is a data-level breaking change: new
affected_system_statusdict entries will be keyed by connection config key instead of system fides_key. Existing historical rows are unaffected (old keys remain readable, just won't be updated). Any reporting queries that aggregate by system key will need to be aware of the change.Code Changes
src/fides/api/models/connectionconfig.py- Addconsent_tracking_keyproperty, deprecatesystem_keyfor consent usesrc/fides/api/util/consent_util.py- All 6system_keyreferences →consent_tracking_keysrc/fides/api/task/graph_task.py-system_key→consent_tracking_keyincache_system_status_for_preferencessrc/fides/api/service/connectors/consent_email_connector.py-system_key→consent_tracking_keyin skip status cachingtests/ops/util/test_consent_util.py- Assertions updated fromconnection_config.nametoconnection_config.keytests/ops/service/connectors/test_saas_connector.py- Assertions updated fromsystem.fides_key/system_keytoconsent_tracking_keytests/ops/service/privacy_request/test_email_batch_send.py- Assertions updated fromsystem_keytoconsent_tracking_keytests/ops/service/privacy_request/test_request_runner_service.py- Assertion updated fromsystem.fides_keytoconsent_tracking_keySteps to Confirm
nox -s static_checkspasses (ruff + mypy clean)affected_system_statusinstead of overwritinggrep -rn "\.system_key" src/fides/api/util/consent_util.py src/fides/api/task/graph_task.py src/fides/api/service/connectors/consent_email_connector.pyreturns no matchesPre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit