ENG-3270: Add target_system_steward_ids column to StagedResource#7912
ENG-3270: Add target_system_steward_ids column to StagedResource#7912dsill-ethyca merged 8 commits intomainfrom
Conversation
Add nullable ARRAY(String) column for per-resource steward assignment. None = use monitor fallback on promotion, [] = explicitly no stewards. Includes alembic migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
The steward_ids migration pointed to c5d6e7f8a9b0 which already had a child (1724da7ee981), creating multiple heads. Repoint to b3c8d5e7f2a1 which is the actual chain tip after merging main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7912 +/- ##
=======================================
Coverage 85.06% 85.06%
=======================================
Files 629 629
Lines 40855 40860 +5
Branches 4748 4748
=======================================
+ Hits 34753 34758 +5
Misses 5029 5029
Partials 1073 1073 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Required for fides scan dataset db check to pass with 100% coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: Add steward_ids column to StagedResource
Scope: SQLAlchemy model change + Alembic migration. Two files of substance; the changelog and db_dataset.yml additions are trivial.
Migration chain
I traced the full April migration chain to verify down_revision:
d4e5f6a7b8c9 → 22cf8ccaec40 → 6a42f48c23dd → b9c4d5e6f7a8
→ c5d6e7f8a9b0 → 1724da7ee981 → b3c8d5e7f2a1 ← (current head)
↑
a42ef09a3dfe (this PR)
b3c8d5e7f2a1 (add_google_workspace_connectiontype) is the true leaf — nothing else names it as a down_revision. The migration in this PR correctly extends from it. No multi-head issue.
Findings
Suggestion — server_default=None is a no-op (inline comment on line 687)
This is consistent with the existing user_assigned_data_uses pattern but doesn't add meaningful information. Low priority; noted for awareness.
Suggestion — missing test for None vs [] semantic (inline comment on line 681)
The comment documents a behavioural contract that fidesplus will depend on. An existing test covers this for user_assigned_data_uses; a parallel test for steward_ids would protect the contract from silent regressions.
Overall
The implementation is clean and minimal. The column type, nullability, and DDL are all correct. The comment explaining the None/[] distinction is appreciated. Both findings are non-blocking; the test gap is the more important of the two.
🔬 Codegraph: connected (46690 nodes)
💡 Write /code-review in a comment to re-run this review.
| steward_ids = Column( | ||
| ARRAY(String), | ||
| nullable=True, | ||
| server_default=None, |
There was a problem hiding this comment.
src/fides/api/models/detection_discovery/core.py:687
server_default=None is a no-op in SQLAlchemy — it means "emit no server-side default clause in DDL," which is already the behaviour when the parameter is omitted entirely. It doesn't communicate anything to the database that nullable=True doesn't already express.
This mirrors the identical pattern on user_assigned_data_uses, so it's consistent with local convention. If the intent is documentation-as-code (making the absence of a server default explicit), a brief inline comment would be clearer than the no-op keyword argument. Either way, not blocking — just worth being intentional.
| # frontend to show a sparkle icon only for system-generated descriptions. | ||
| user_assigned_description = Column(String, nullable=True) | ||
|
|
||
| # Nullable to distinguish "not set" (None, use monitor fallback on promotion) |
There was a problem hiding this comment.
src/fides/api/models/detection_discovery/core.py:681
The comment documents a three-way semantic (None = not set / use monitor fallback, [] = explicitly empty / skip fallback, [ids] = assigned stewards) that is a behavioural contract downstream code in fidesplus will rely on. There's already a test_empty_user_assigned_data_uses test in tests/ops/models/detection_discovery/test_core.py that verifies the equivalent contract for user_assigned_data_uses.
Consider adding a parallel test for steward_ids that asserts:
- A freshly created
StagedResourcehassteward_ids is None(not[]). - Explicitly setting
steward_ids = []round-trips as an empty list, notNULL.
This would guard against future migrations or ORM changes accidentally collapsing the None/[] distinction.
adamsachs
left a comment
There was a problem hiding this comment.
lgtm, i think the nullable approach is sound here 👍
i think the only thing to note is that at some point we'll want resource-level 'stewardship', i.e. assigning certain users to act on certain StagedResources. at that point, it may become a bit ambiguous whether steward_ids refers to those 'stewards' or the stewards created on promotion.
maybe we can just add in a comment here on the model generally to make it clear that these are stewards to be assigned on promotion? (not sure exactly the right way to phrase that...)
|
@adamsachs I think at the level of assigining permissions to specific users as stewards of the resource itself we may want to use a join table strategy similar to For the sake of clarity here we could prefer to name this column something like |
yup, generally aligned on this vision. i think it's a good instinct to try to clarify this in the column name - the column name itself isn't that hard to adjust, but these names have a tendency to permeate up through the API and into the FE codebase too, so getting some clarity in the name could help reduce the need for a large-scale 'migration' down the road... can't say i love |
|
After discussion, we've settled on Rationale: While Renaming commit incoming for both this PR and the dependent fidesplus PR (#3409). |
Clarifies that these are stewards for the target system the staged resource will become upon promotion, not stewards of the staged resource itself. Name chosen to reduce ambiguity as it permeates through the API and frontend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-3270
Description Of Changes
Adds a nullable
target_system_steward_idscolumn (ARRAY(String)) toStagedResourcefor per-resource steward assignment. This is the OSS data model foundation for the fidesplus per-resource steward feature.Semantics:
None(default) — steward assignment not set; fidesplus uses monitor-level fallback on promotion[](empty list) — explicitly no stewards; fidesplus skips monitor fallback["user_id_1", ...]— explicit steward assignmentThis follows the same nullable-vs-empty convention as
user_assigned_data_uses.Naming decision:
target_system_steward_idswas chosen oversteward_idsoruser_assigned_steward_idsto make it explicit that these are stewards for the target system this resource will become upon promotion, not stewards of the staged resource itself.Code Changes
src/fides/api/models/detection_discovery/core.py— Addedtarget_system_steward_idscolumn toStagedResourcesrc/fides/api/alembic/migrations/versions/xx_..._add_target_system_steward_ids_to_stagedresource.py— Alembic migration adding the columnSteps to Confirm
Run the migration:
Expected: Migration applies cleanly.
Verify column exists:
Expected:
target_system_steward_ids | ARRAYPre-Merge Checklist
CHANGELOG.md🤖 Generated with Claude Code