From 659b6005de5708e31cd855e7c543a6c7facc1381 Mon Sep 17 00:00:00 2001 From: Yian Shang Date: Tue, 8 Sep 2026 12:32:34 -0700 Subject: [PATCH 1/3] Exempt metrics from the declared-column existence check A metric's single output column is not named by its query. On deploy the first projection's alias is overwritten with the amenable form of the node name, so `shared.main.weekly_active_players` always persists a column named `shared_DOT_main_DOT_weekly_active_players` no matter what the query aliased it to, or whether it aliased it at all. The declared-column check added in #2522 compares declared names against the names the query itself produces, which for a metric is a name that never survives deploy. Any metric whose query alias differs from its declared column name, including the common case of no alias at all, failed validation with INVALID_COLUMN. This broke real deployments: a semantic repo with 481 metrics declaring column blocks had 11 of them fail as soon as a query edit sent them back through validation. There is nothing for the check to assert here. `MetricSpec.columns` is an internal field, excluded from serialization, sitting at ChangeTier.NONE, and a metric's unit is authored at the metric level precisely so authors never have to know the output column name. The declared metadata is reconciled onto the single output column regardless of what it is called. #2522's behavior is unchanged for transform, dimension, and source nodes, where the declared name really does have to match a query output. --- .../internal/deployment/validation.py | 5 +++ .../internal/deployment/validation_test.py | 31 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/datajunction-server/datajunction_server/internal/deployment/validation.py b/datajunction-server/datajunction_server/internal/deployment/validation.py index daaf3156f..3b32643f8 100644 --- a/datajunction-server/datajunction_server/internal/deployment/validation.py +++ b/datajunction-server/datajunction_server/internal/deployment/validation.py @@ -611,7 +611,12 @@ def _check_declared_columns_exist( query's output. A declared column that doesn't match any output column is silently dropped (its metadata is never applied), so this is surfaced as an error instead. + + Metrics are exempt: their one output column is renamed to the node's + amenable name on deploy, so no declared name can match reliably. """ + if spec.node_type == NodeType.METRIC: + return None declared_names = { col.name for col in ( diff --git a/datajunction-server/tests/internal/deployment/validation_test.py b/datajunction-server/tests/internal/deployment/validation_test.py index ebc6cda33..c8f85c0d7 100644 --- a/datajunction-server/tests/internal/deployment/validation_test.py +++ b/datajunction-server/tests/internal/deployment/validation_test.py @@ -284,6 +284,37 @@ async def test_validate_query_node_flags_unmatched_declared_column( ) assert "full_name" in message + @pytest.mark.asyncio + async def test_validate_query_node_skips_declared_columns_for_metric( + self, + session: AsyncSession, + parent_node: Node, + ): + """A metric declaring its output column stays valid whatever it's named. + + A metric's single output column is renamed to the amenable node name + on deploy, so a declared name has nothing stable to match against. + """ + context = ValidationContext( + session=session, + node_graph={"test.weekly_active_players": [parent_node.name]}, + dependency_nodes={parent_node.name: parent_node}, + ) + spec = MetricSpec( + name="test.weekly_active_players", + query="SELECT SUM(value) FROM test.parent", + description="A test metric", + mode="published", + columns=[ + ColumnSpec(name="weekly_active_players", display_name="WAP"), + ], + ) + validator = NodeSpecBulkValidator(context) + result = validator.validate_query_node(spec) + + assert result.errors == [] + assert result.status == NodeStatus.VALID + @pytest.mark.asyncio async def test_validate_query_node_flags_hardcoded_namespace( self, From 89befe39551c92daded23d357f77fab7ebed0ade Mon Sep 17 00:00:00 2001 From: Yian Shang Date: Tue, 8 Sep 2026 13:01:58 -0700 Subject: [PATCH 2/3] Reject author-declared columns on a metric spec A metric's single output column is never named by its query. On deploy the first projection's alias is overwritten with the amenable form of the node name, so `shared.main.weekly_active_players` always persists a column called `shared_DOT_main_DOT_weekly_active_players` whatever the query aliased, or whether it aliased anything at all. A declared name therefore has nothing stable to match, and the metadata attached to it was reconciled onto that one column regardless. The previous commit made metrics simply exempt from #2522's declared-column check, which left authors free to write a `columns:` block that quietly did nothing. Refuse it instead, and state the rule on the model rather than as a special case in the checker: each spec class declares the fields the server owns in `INTERNAL_FIELDS`, mapped to the remedy an author needs, and one generic check turns any authored value into a `DJError`. The next internal-only field is enforced by declaring it. Distinguishing an authored value from a server-populated one needs no new marker. `Node.to_spec` does pass `columns=` for metrics, but such specs come from the branch copy, which sets `_skip_validation` and takes the fast path above these checks; the export path drops metric columns entirely because the field is `exclude=True`. So `columns` reaching the check means a human wrote it. The check tests by value rather than by `model_fields_set` so that a server-populated empty list still reads as unset. This is a hard error with no grace period: every metric carrying a `columns:` block now fails to deploy, not only those whose declared name mismatched. `ErrorCode.INVALID_SPEC_FIELD` is new, so the generated GraphQL enum moves with it. #2522's check is restored unchanged for transform, dimension, and source, where a declared name really must match a query output. --- .../api/graphql/schema.graphql | 1 + .../datajunction_server/errors.py | 1 + .../internal/deployment/validation.py | 27 ++++-- .../datajunction_server/models/deployment.py | 29 +++++++ .../internal/deployment/validation_test.py | 87 +++++++++++++++++-- 5 files changed, 134 insertions(+), 11 deletions(-) diff --git a/datajunction-server/datajunction_server/api/graphql/schema.graphql b/datajunction-server/datajunction_server/api/graphql/schema.graphql index 6d442f339..5e70cdfe7 100644 --- a/datajunction-server/datajunction_server/api/graphql/schema.graphql +++ b/datajunction-server/datajunction_server/api/graphql/schema.graphql @@ -199,6 +199,7 @@ enum ErrorCode { TAG_NOT_FOUND CATALOG_NOT_FOUND INVALID_NAMESPACE + INVALID_SPEC_FIELD } type GeneratedSQL { diff --git a/datajunction-server/datajunction_server/errors.py b/datajunction-server/datajunction_server/errors.py index 1854b4d15..ee5857779 100644 --- a/datajunction-server/datajunction_server/errors.py +++ b/datajunction-server/datajunction_server/errors.py @@ -65,6 +65,7 @@ class ErrorCode(IntEnum): TAG_NOT_FOUND = 700 CATALOG_NOT_FOUND = 701 INVALID_NAMESPACE = 702 + INVALID_SPEC_FIELD = 703 class DebugType(TypedDict, total=False): diff --git a/datajunction-server/datajunction_server/internal/deployment/validation.py b/datajunction-server/datajunction_server/internal/deployment/validation.py index 3b32643f8..0f5f70901 100644 --- a/datajunction-server/datajunction_server/internal/deployment/validation.py +++ b/datajunction-server/datajunction_server/internal/deployment/validation.py @@ -492,6 +492,7 @@ def validate_query_node( err for err in [ self._check_inferred_columns(inferred_columns), + self._check_internal_fields(spec), self._check_declared_columns_exist( spec, validation.output_columns, @@ -601,6 +602,27 @@ def _check_inferred_columns(self, columns: list[ColumnSpec]) -> DJError | None: ) return None + @staticmethod + def _check_internal_fields(spec: NodeSpec) -> DJError | None: + """ + Reject a spec that sets a field the server owns. + + Specs built from existing nodes carry these, but they reach validation + only through the `_skip_validation` fast path above, so a value here + was written by an author. + """ + authored = spec.authored_internal_fields() + if authored: + label = spec.node_type.value.capitalize() + return DJError( + code=ErrorCode.INVALID_SPEC_FIELD, + message=" ".join( + f"{label} {spec.rendered_name} must not declare {field}. {remedy}" + for field, remedy in authored + ), + ) + return None + @staticmethod def _check_declared_columns_exist( spec: NodeSpec, @@ -611,12 +633,7 @@ def _check_declared_columns_exist( query's output. A declared column that doesn't match any output column is silently dropped (its metadata is never applied), so this is surfaced as an error instead. - - Metrics are exempt: their one output column is renamed to the node's - amenable name on deploy, so no declared name can match reliably. """ - if spec.node_type == NodeType.METRIC: - return None declared_names = { col.name for col in ( diff --git a/datajunction-server/datajunction_server/models/deployment.py b/datajunction-server/datajunction_server/models/deployment.py index bce5eefe7..46a6fb9b6 100644 --- a/datajunction-server/datajunction_server/models/deployment.py +++ b/datajunction-server/datajunction_server/models/deployment.py @@ -645,6 +645,12 @@ class NodeSpec(NamespacedSpec): "owners": ChangeTier.NONE, "tags": ChangeTier.NONE, } + + # Fields the server owns, mapped to the remedy shown when an author sets + # one. Each class declares only the fields it introduces; lookup walks the + # MRO. Validation rejects a deployment that provides any of them. + INTERNAL_FIELDS: ClassVar[dict[str, str]] = {} + _query_ast: Any | None = PrivateAttr(default=None) # Internal: marks specs from already-validated sources (e.g., branch copies) # that can skip expensive SQL parsing and validation @@ -789,6 +795,22 @@ def has_explicit_change_tier(cls, field: str) -> bool: """Whether some class in the MRO classifies `field`.""" return cls._declared_tier("FIELD_CHANGE_TIERS", field) is not None + def authored_internal_fields(self) -> list[tuple[str, str]]: + """ + Internal-only fields carrying a value, with each one's remedy. + + Tested by value rather than by `model_fields_set`, so that server code + populating a field with nothing in it reads as unset. + """ + declared: dict[str, str] = {} + for klass in reversed(type(self).__mro__): + declared.update(klass.__dict__.get("INTERNAL_FIELDS") or {}) + return [ + (field, remedy) + for field, remedy in sorted(declared.items()) + if getattr(self, field, None) + ] + @classmethod def has_explicit_order_change_tier(cls, field: str) -> bool: """Whether some class in the MRO classifies reordering `field`.""" @@ -1022,6 +1044,13 @@ class MetricSpec(NodeSpec): # Internal only - used for validation skip optimization when copying from valid nodes. # Excluded from serialization so it's never exported. columns: list[ColumnSpec] | None = Field(default=None, exclude=True) + + # A metric's one output column is always named after the node, so a + # declared name can never match it. + INTERNAL_FIELDS: ClassVar[dict[str, str]] = { + "columns": "Remove the columns block; set `unit` on the metric.", + } + required_dimensions: list[str] | None = None # Field(default_factory=list) direction: MetricDirection | None = None unit_enum: MetricUnit | None = Field(default=None, exclude=True) diff --git a/datajunction-server/tests/internal/deployment/validation_test.py b/datajunction-server/tests/internal/deployment/validation_test.py index c8f85c0d7..558b69325 100644 --- a/datajunction-server/tests/internal/deployment/validation_test.py +++ b/datajunction-server/tests/internal/deployment/validation_test.py @@ -13,6 +13,7 @@ from datajunction_server.database.node import Node, NodeRevision from datajunction_server.database.user import OAuthProvider, User from datajunction_server.errors import ErrorCode +from datajunction_server.internal.deployment.utils import extract_node_graph from datajunction_server.internal.deployment.validation import ( NodeSpecBulkValidator, NodeValidationResult, @@ -285,16 +286,12 @@ async def test_validate_query_node_flags_unmatched_declared_column( assert "full_name" in message @pytest.mark.asyncio - async def test_validate_query_node_skips_declared_columns_for_metric( + async def test_validate_query_node_rejects_declared_columns_on_metric( self, session: AsyncSession, parent_node: Node, ): - """A metric declaring its output column stays valid whatever it's named. - - A metric's single output column is renamed to the amenable node name - on deploy, so a declared name has nothing stable to match against. - """ + """A metric must not declare columns, whatever its query aliases.""" context = ValidationContext( session=session, node_graph={"test.weekly_active_players": [parent_node.name]}, @@ -312,6 +309,84 @@ async def test_validate_query_node_skips_declared_columns_for_metric( validator = NodeSpecBulkValidator(context) result = validator.validate_query_node(spec) + assert result.status == NodeStatus.INVALID + error_codes = [e.code for e in result.errors] + assert ErrorCode.INVALID_SPEC_FIELD in error_codes + message = next( + e.message for e in result.errors if e.code == ErrorCode.INVALID_SPEC_FIELD + ) + assert message == ( + "Metric test.weekly_active_players must not declare columns. " + "Remove the columns block; set `unit` on the metric." + ) + + @pytest.mark.asyncio + async def test_validate_query_node_rejects_declared_columns_on_aliased_metric( + self, + session: AsyncSession, + parent_node: Node, + ): + """A metric aliasing its own short name is still rejected. + + Going through ``extract_node_graph`` caches the metric-aliased AST on + the spec, which is the form a deploy validates. This is the shape that + failed in production. + """ + spec = MetricSpec( + name="test.weekly_active_players", + query="SELECT SUM(value) AS weekly_active_players FROM test.parent", + description="A test metric", + mode="published", + columns=[ + ColumnSpec(name="weekly_active_players", display_name="WAP"), + ], + ) + node_graph = extract_node_graph([spec]) + assert ( + spec.query_ast.select.projection[0].alias_or_name.identifier() + == "test_DOT_weekly_active_players" + ) + context = ValidationContext( + session=session, + node_graph=node_graph, + dependency_nodes={parent_node.name: parent_node}, + ) + validator = NodeSpecBulkValidator(context) + result = validator.validate_query_node(spec) + + assert result.status == NodeStatus.INVALID + error_codes = [e.code for e in result.errors] + assert ErrorCode.INVALID_SPEC_FIELD in error_codes + message = next( + e.message for e in result.errors if e.code == ErrorCode.INVALID_SPEC_FIELD + ) + assert message == ( + "Metric test.weekly_active_players must not declare columns. " + "Remove the columns block; set `unit` on the metric." + ) + + @pytest.mark.asyncio + async def test_validate_query_node_allows_metric_without_columns( + self, + session: AsyncSession, + parent_node: Node, + ): + """A metric that declares no columns validates clean.""" + spec = MetricSpec( + name="test.weekly_active_players", + query="SELECT SUM(value) AS weekly_active_players FROM test.parent", + description="A test metric", + mode="published", + ) + node_graph = extract_node_graph([spec]) + context = ValidationContext( + session=session, + node_graph=node_graph, + dependency_nodes={parent_node.name: parent_node}, + ) + validator = NodeSpecBulkValidator(context) + result = validator.validate_query_node(spec) + assert result.errors == [] assert result.status == NodeStatus.VALID From 0214434b865fd3751ec54f35506a3195f14f354f Mon Sep 17 00:00:00 2001 From: Yian Shang Date: Tue, 8 Sep 2026 15:47:41 -0700 Subject: [PATCH 3/3] Report only the cause when a spec declares an internal field An author who declared a field the server owns was told twice: once that the field is not theirs to set, and again that the column they declared doesn't match any column the query produces. The second message is a symptom of the first and actively misleading here, since the rule is that a metric shouldn't declare columns at all rather than that it named them wrongly. Skip the declared-column check for any spec that already failed the internal-field check. #2522's check is untouched; only whether it runs has changed. Assert the whole error list by equality in these tests rather than probing it for membership. Doing that to #2522's own transform test showed its fixture names the spec `transform` while keying the graph on `test.transform`, so `test.parent` never resolves and the case carries two more errors than it looks like it does: nothing is inferred, and the declared columns are unmatched partly for that reason rather than on their own merits. Recorded as found, with a comment, rather than quietly repaired. Also move `INTERNAL_FIELDS` down beside `FIELD_CHANGE_TIERS` and `authored_internal_fields` below the tier classmethods, so neither splits a block it was sitting in the middle of. --- .../internal/deployment/validation.py | 18 ++++-- .../datajunction_server/models/deployment.py | 23 ++++---- .../internal/deployment/validation_test.py | 58 +++++++++++-------- 3 files changed, 58 insertions(+), 41 deletions(-) diff --git a/datajunction-server/datajunction_server/internal/deployment/validation.py b/datajunction-server/datajunction_server/internal/deployment/validation.py index 0f5f70901..82b582c6a 100644 --- a/datajunction-server/datajunction_server/internal/deployment/validation.py +++ b/datajunction-server/datajunction_server/internal/deployment/validation.py @@ -488,15 +488,23 @@ def validate_query_node( validation.output_columns, spec, ) + internal_field_error = self._check_internal_fields(spec) + # Declaring a field the server owns makes every downstream + # complaint about that field a symptom. Report the cause only. + declared_columns_error = ( + None + if internal_field_error + else self._check_declared_columns_exist( + spec, + validation.output_columns, + ) + ) errors = [ err for err in [ self._check_inferred_columns(inferred_columns), - self._check_internal_fields(spec), - self._check_declared_columns_exist( - spec, - validation.output_columns, - ), + internal_field_error, + declared_columns_error, self._check_primary_key(inferred_columns, spec), self._check_metric_query(spec, spec.query_ast), ] diff --git a/datajunction-server/datajunction_server/models/deployment.py b/datajunction-server/datajunction_server/models/deployment.py index 46a6fb9b6..8d1931309 100644 --- a/datajunction-server/datajunction_server/models/deployment.py +++ b/datajunction-server/datajunction_server/models/deployment.py @@ -795,6 +795,11 @@ def has_explicit_change_tier(cls, field: str) -> bool: """Whether some class in the MRO classifies `field`.""" return cls._declared_tier("FIELD_CHANGE_TIERS", field) is not None + @classmethod + def has_explicit_order_change_tier(cls, field: str) -> bool: + """Whether some class in the MRO classifies reordering `field`.""" + return cls._declared_tier("FIELD_ORDER_CHANGE_TIERS", field) is not None + def authored_internal_fields(self) -> list[tuple[str, str]]: """ Internal-only fields carrying a value, with each one's remedy. @@ -811,11 +816,6 @@ def authored_internal_fields(self) -> list[tuple[str, str]]: if getattr(self, field, None) ] - @classmethod - def has_explicit_order_change_tier(cls, field: str) -> bool: - """Whether some class in the MRO classifies reordering `field`.""" - return cls._declared_tier("FIELD_ORDER_CHANGE_TIERS", field) is not None - @classmethod def unclassified_fields(cls) -> list[str]: """Fields on this spec class that nobody classified. Should always be empty.""" @@ -1044,13 +1044,6 @@ class MetricSpec(NodeSpec): # Internal only - used for validation skip optimization when copying from valid nodes. # Excluded from serialization so it's never exported. columns: list[ColumnSpec] | None = Field(default=None, exclude=True) - - # A metric's one output column is always named after the node, so a - # declared name can never match it. - INTERNAL_FIELDS: ClassVar[dict[str, str]] = { - "columns": "Remove the columns block; set `unit` on the metric.", - } - required_dimensions: list[str] | None = None # Field(default_factory=list) direction: MetricDirection | None = None unit_enum: MetricUnit | None = Field(default=None, exclude=True) @@ -1063,6 +1056,12 @@ class MetricSpec(NodeSpec): min_decimal_exponent: int | None = None max_decimal_exponent: int | None = None + # A metric's one output column is always named after the node, so a + # declared name can never match it. + INTERNAL_FIELDS: ClassVar[dict[str, str]] = { + "columns": "Remove the columns block; set `unit` on the metric.", + } + FIELD_CHANGE_TIERS: ClassVar[dict[str, ChangeTier]] = { "query": ChangeTier.MAJOR, "columns": ChangeTier.NONE, diff --git a/datajunction-server/tests/internal/deployment/validation_test.py b/datajunction-server/tests/internal/deployment/validation_test.py index 558b69325..74510e3df 100644 --- a/datajunction-server/tests/internal/deployment/validation_test.py +++ b/datajunction-server/tests/internal/deployment/validation_test.py @@ -277,13 +277,27 @@ async def test_validate_query_node_flags_unmatched_declared_column( validator = NodeSpecBulkValidator(validation_context) result = validator.validate_query_node(spec) + # `test.parent` does not resolve here: the spec is named `transform` + # while the fixture's graph is keyed on `test.transform`, so the parent + # columns map comes back empty and nothing is inferred. The declared + # columns are unmatched for that reason as well as on their own merits. + assert [(e.code, e.message) for e in result.errors] == [ + ( + ErrorCode.INVALID_SQL_QUERY, + "No columns could be inferred from the SQL query.", + ), + ( + ErrorCode.INVALID_COLUMN, + "Declared column(s) ['full_name', 'id'] on node transform do not " + "match any column produced by the query. Check for a missing or " + "mismatched column alias.", + ), + ( + ErrorCode.TYPE_INFERENCE, + "Table `test.parent` not found in parent columns map. Available: []", + ), + ] assert result.status == NodeStatus.INVALID - error_codes = [e.code for e in result.errors] - assert ErrorCode.INVALID_COLUMN in error_codes - message = next( - e.message for e in result.errors if e.code == ErrorCode.INVALID_COLUMN - ) - assert "full_name" in message @pytest.mark.asyncio async def test_validate_query_node_rejects_declared_columns_on_metric( @@ -309,16 +323,14 @@ async def test_validate_query_node_rejects_declared_columns_on_metric( validator = NodeSpecBulkValidator(context) result = validator.validate_query_node(spec) + assert [(e.code, e.message) for e in result.errors] == [ + ( + ErrorCode.INVALID_SPEC_FIELD, + "Metric test.weekly_active_players must not declare columns. " + "Remove the columns block; set `unit` on the metric.", + ), + ] assert result.status == NodeStatus.INVALID - error_codes = [e.code for e in result.errors] - assert ErrorCode.INVALID_SPEC_FIELD in error_codes - message = next( - e.message for e in result.errors if e.code == ErrorCode.INVALID_SPEC_FIELD - ) - assert message == ( - "Metric test.weekly_active_players must not declare columns. " - "Remove the columns block; set `unit` on the metric." - ) @pytest.mark.asyncio async def test_validate_query_node_rejects_declared_columns_on_aliased_metric( @@ -354,16 +366,14 @@ async def test_validate_query_node_rejects_declared_columns_on_aliased_metric( validator = NodeSpecBulkValidator(context) result = validator.validate_query_node(spec) + assert [(e.code, e.message) for e in result.errors] == [ + ( + ErrorCode.INVALID_SPEC_FIELD, + "Metric test.weekly_active_players must not declare columns. " + "Remove the columns block; set `unit` on the metric.", + ), + ] assert result.status == NodeStatus.INVALID - error_codes = [e.code for e in result.errors] - assert ErrorCode.INVALID_SPEC_FIELD in error_codes - message = next( - e.message for e in result.errors if e.code == ErrorCode.INVALID_SPEC_FIELD - ) - assert message == ( - "Metric test.weekly_active_players must not declare columns. " - "Remove the columns block; set `unit` on the metric." - ) @pytest.mark.asyncio async def test_validate_query_node_allows_metric_without_columns(