Fix metric declared column check - #2529
Draft
shangyian wants to merge 3 commits into
Draft
Conversation
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.
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.
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.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan