Skip to content

Commit d4e27f4

Browse files
adamsachsclaude
andcommitted
ENG-3324: Address review comments on the inheritance data model
* **Rename migration file** to ``xx_2026_05_13_0900_6ebb8e54e130_...`` — the previous filename was dated 2026-04-09 but the rebased ``down_revision`` (``e3f4a5b6c7d8``) is from 2026-04-23, leaving the migration filename apparently predating its predecessor. Fix the docstring ``Revises:`` to match the actual ``down_revision`` and bump ``Create Date`` to today. * **Fix ``server_default`` for the ``source`` column** — ``server_default`` treats plain strings as raw SQL expressions, so ``server_default="explicit"`` rendered ``DEFAULT explicit`` (unquoted identifier) rather than ``DEFAULT 'explicit'``. Use ``sa.text("'explicit'")`` in both the migration and the ORM model (matching the ``sa.text("true")`` pattern already used for the ``inherit_system_stewards`` column). * **Add DB-level CHECK constraints on ``monitorsteward.source``**: * ``ck_monitorsteward_source_values``: ``source IN ('explicit', 'inherited')`` — prevents an API-only sentinel (like ``"both"``) from being persisted even if application code misuses the enum. * ``ck_monitorsteward_inherited_has_system``: ``source != 'inherited' OR source_system_id IS NOT NULL`` — encodes the domain invariant directly at the DB level since inherited rows are populated by separate logic. Mirrored in the ORM ``__table_args__`` so the constraints are visible at the model level too. * **Remove ``StewardSource.both`` from the fides enum** — the value is API-only (computed when a user has rows of both sources on a monitor) and mixing it into a DB-backed ``StrEnum`` is a footgun. Keep the enum pure DB-side; fidesplus moves the response-layer attribution (including ``both``) to its own schema module that references these values rather than redefining them. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 2444eac commit d4e27f4

2 files changed

Lines changed: 50 additions & 11 deletions

File tree

src/fides/api/alembic/migrations/versions/xx_2026_04_09_1000_6ebb8e54e130_monitor_steward_inheritance.py renamed to src/fides/api/alembic/migrations/versions/xx_2026_05_13_0900_6ebb8e54e130_monitor_steward_inheritance.py

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88
current behavior is preserved. New monitors default to true.
99
1010
Revision ID: 6ebb8e54e130
11-
Revises: 6a42f48c23dd
12-
Create Date: 2026-04-09 10:00:00.000000
11+
Revises: e3f4a5b6c7d8
12+
Create Date: 2026-05-13 09:00:00.000000
1313
"""
1414

1515
import sqlalchemy as sa
@@ -42,7 +42,7 @@ def upgrade():
4242
"source",
4343
sa.String(),
4444
nullable=False,
45-
server_default="explicit",
45+
server_default=sa.text("'explicit'"),
4646
),
4747
)
4848

@@ -81,8 +81,35 @@ def upgrade():
8181
["user_id", "monitor_config_id", "source"],
8282
)
8383

84+
# --- monitorsteward: DB-level guards on source ---
85+
# Whitelist the only persistable values (matches StewardSource db-side members).
86+
op.create_check_constraint(
87+
"ck_monitorsteward_source_values",
88+
"monitorsteward",
89+
"source IN ('explicit', 'inherited')",
90+
)
91+
# Inherited rows MUST point at the system that contributed them; explicit
92+
# rows have no source system.
93+
op.create_check_constraint(
94+
"ck_monitorsteward_inherited_has_system",
95+
"monitorsteward",
96+
"source != 'inherited' OR source_system_id IS NOT NULL",
97+
)
98+
8499

85100
def downgrade():
101+
# --- monitorsteward: drop DB-level source guards ---
102+
op.drop_constraint(
103+
"ck_monitorsteward_inherited_has_system",
104+
"monitorsteward",
105+
type_="check",
106+
)
107+
op.drop_constraint(
108+
"ck_monitorsteward_source_values",
109+
"monitorsteward",
110+
type_="check",
111+
)
112+
86113
# --- monitorsteward: restore original unique constraint ---
87114
op.drop_constraint(
88115
"uq_monitorsteward_user_monitor_source",

src/fides/api/models/detection_discovery/monitor_steward.py

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,26 @@
11
from enum import StrEnum
22

3-
from sqlalchemy import Column, ForeignKey, String, UniqueConstraint
3+
from sqlalchemy import (
4+
CheckConstraint,
5+
Column,
6+
ForeignKey,
7+
String,
8+
UniqueConstraint,
9+
text,
10+
)
411

512
from fides.api.db.base_class import Base
613

714

815
class StewardSource(StrEnum):
9-
"""How a monitor steward row was assigned.
16+
"""Persisted values for ``MonitorSteward.source``.
1017
11-
``explicit`` and ``inherited`` are the values actually persisted to
12-
``MonitorSteward.source``. ``both`` is API-only — it's never stored;
13-
callers building response payloads return it when a user has both an
14-
explicit and an inherited row for the same monitor.
18+
DB-side enum only — response-layer attribution values are defined
19+
separately in the API schema.
1520
"""
1621

1722
explicit = "explicit"
1823
inherited = "inherited"
19-
both = "both"
2024

2125

2226
class MonitorSteward(Base):
@@ -45,7 +49,7 @@ class MonitorSteward(Base):
4549
String,
4650
nullable=False,
4751
default=StewardSource.explicit.value,
48-
server_default=StewardSource.explicit.value,
52+
server_default=text("'explicit'"),
4953
)
5054
source_system_id = Column(
5155
String,
@@ -61,4 +65,12 @@ class MonitorSteward(Base):
6165
"source",
6266
name="uq_monitorsteward_user_monitor_source",
6367
),
68+
CheckConstraint(
69+
"source IN ('explicit', 'inherited')",
70+
name="ck_monitorsteward_source_values",
71+
),
72+
CheckConstraint(
73+
"source != 'inherited' OR source_system_id IS NOT NULL",
74+
name="ck_monitorsteward_inherited_has_system",
75+
),
6476
)

0 commit comments

Comments
 (0)