[RBAC PR 6] Enforce mutation grants for governed namespaces - #2467
Open
philipfweiss wants to merge 65 commits into
Open
philipfweiss wants to merge 65 commits into
philipfweiss wants to merge 65 commits into
Conversation
Derive restrictive WRITE, DELETE, and MANAGE rules from persisted namespace boundaries so provisioning activates exclusive RBAC without a deployment allowlist.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
Treat hard deletion of a governed root as a MANAGE operation so a deployer cannot remove the persisted policy marker and reopen the namespace to fallback access.
philipfweiss
changed the title
…#2468) * Declare custom_metadata schemas in the repo that owns the namespace A namespace's manifest can now carry `custom_metadata_schemas`, so the keys a namespace expects -- and the JSON Schema each value must satisfy -- are declared alongside the nodes that carry them, and reviewed in the same PR. Registration goes through the same helpers as the REST API, so two writers to one table cannot enforce different rules. The manifest is the whole truth for its own namespace: declared keys are upserted, and rows it no longer declares are retired. The `namespace` column is what makes that safe -- it already records who owns a row, and a global row has no owning namespace, so no deployment may retire one. `custom_metadata_schemas` is nullable because None and `[]` are different manifests. None does not manage schemas at all; `[]` manages them and declares none, which retires the namespace's rows. With a list default, every deployment predating this field would have read as the latter. Registration revives a soft-deleted row rather than inserting beside it. The unique index spans deactivated rows while every read filters them out, so an insert next to a tombstone violates the constraint -- which made a retired key permanently unregisterable, by either writer. Reviving also preserves the row's id and created_at, so a key that comes back is the same registration rather than a new one. The transaction belongs to the caller. The orchestrator opens a SAVEPOINT so a dry run can be rolled back, and committing here releases it -- so `POST /deployments/impact`, which exists only to report what a deployment would do, would have permanently registered the schemas it was asked to analyse. Index DDL is skipped for a dry run, since a rolled-back CREATE INDEX is work done for nothing. * Let a manifest scope a schema to a sub-namespace A schema took the deploying namespace and nothing else, so a repo could only govern its whole graph at once. That is the wrong granularity for a staged rollout: a vocabulary usually starts on the part of the graph that matters most -- conformed dimensions, say -- and widens once it holds. A spec may now name its own namespace. Omitted, it still defaults to the deployment's, so nothing existing changes. Named, it must be that namespace or one beneath it: narrower is a rollout choice, while wider or sideways would let one repo govern another repo's nodes. The check lives in `set_namespaces` alongside the defaulting it already does for nodes, hierarchies and pre-aggs, and rejects a prefix that only looks like a descendant -- `shared_other` is not under `shared`. Reconciliation now covers the deploying namespace plus whatever sub-namespaces the specs name, rather than the deploying namespace alone. Both halves matter: declaring a schema for `shared.conformed` must not retire one for `shared.finance`, which another deployment owns, and an empty list must still retire the deploying namespace's own rows, which is the only thing that distinguishes "declares none" from "does not manage these".
* Seed test fixtures once per module instead of once per test Two local fixtures created their nodes over HTTP on every test that used them. Both were declared with a bare @pytest.fixture, so the default function scope re-ran the whole seeding sequence per test -- 6 node creations across 23 tests in the fan-out guard file, 7 tag creations across 10 tests in the tags GraphQL file. Each POST re-parses and re-validates SQL through the full app, so the repeats dominated those modules. Widen both to module scope and point them, and the tests that share their data, at the module-scoped client. Neither module mutates the seeded state -- the fan-out tests only read through /sql/measures/v3/ and the tags tests issue read-only GraphQL queries -- so one seeding pass per module is equivalent. fanout_guard_test.py 122.15s -> 56.14s (23 passed) graphql/tags_test.py 53.64s -> 49.25s (10 passed) Excluding the one-time template build those modules now spend 2.4s and 2.5s on setup, down from 68.3s and ~7s. * Seed the edge-shape transforms once per module Same change as the fan-out and tags fixtures: client_with_edge_shapes created four transforms over HTTP on each of its three consuming tests. Nothing in the module mutates them, so one seeding pass per module is equivalent. transform_query_shapes_test.py 56.44s -> 51.79s (3 passed)
These 250 tests only compile expressions and assert inferred types -- there is not a single session.add/commit/delete/merge in the module -- but each one requested the function-scoped `session` fixture, so each paid its own database clone from the template. Shadow `session` with a module-scoped fixture delegating to `module__session`. No test changes needed: a fixture defined in the module overrides the conftest one for that module only. tests/sql/functions_test.py 72.94s -> 53.55s (250 passed)
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Measure coverage on one Python version, not three
The merge gate is whichever matrix leg finishes last, and all three
server legs pay coverage instrumentation -- worth roughly a third of the
run. Measuring it three times answers the same question three times:
coverage reports which lines the suite exercises, and there is no
sys.version_info branch anywhere in datajunction_server outside the
generated ANTLR grammar, so the result does not vary by version.
Instrument the 3.13 leg only. It is the fastest of the three, and
coverage's sys.monitoring backend needs 3.12+ and costs far less than the
trace-function one, so the leg that still carries coverage is also the
one best able to. 3.11 and 3.12 run the identical suite, uninstrumented.
This keeps 3.11 support rather than dropping it -- sysmon alone would not
have helped, since 3.11 cannot use it and was the slowest leg, so the
gate would not have moved.
Tests still execute on every version; only the measurement moves. A
3.11-specific regression would still fail its own leg.
* Run the tests when the workflow that runs them changes
A change to test.yml matches none of the paths-filter patterns, so every
test step is skipped and the PR goes green having run nothing. That is
how a broken workflow reaches main: it is discovered on somebody else's
PR. This branch is the example -- its first CI run finished in 9 seconds
with Run Tests skipped.
Add a `workflow` filter and OR it into each library's condition. It needs
its own filter rather than an extra pattern on the existing ones:
predicate-quantifier is `every`, so patterns are ANDed and appending this
path would demand a change touch both the library and the workflow,
matching nothing.
Also drop the COVERAGE_CORE=sysmon export added in the previous commit.
Measured on 3.13 with tests/sql at -n 4: 29.70s uninstrumented, 51.60s
with the default core, 53.01s with sysmon requested -- no gain, because
coverage 7.13.4 refuses it ("sys.monitoring can't measure branches in
this version") and silently falls back to the trace core. Left a comment
so nobody re-adds it.
Consequence for the previous commit, worth stating plainly: with sysmon
inert, moving coverage off two legs takes those legs to roughly 18-19min
but leaves the 3.13 leg carrying it at full cost. Since the gate is the
slowest leg, that is about 2min off the gate, not the 8-10 the sysmon
assumption implied. Total CI minutes still fall substantially.
* Measure coverage across parallel shards, not inside a build leg
Coverage roughly doubles this suite -- CI measured 14.7-15.9min for the
uninstrumented legs against 28.8min for the instrumented one -- and it
cannot be made cheaper. sys.monitoring is refused by coverage 7.13.4
("can't measure branches in this version") and silently falls back to the
trace core, measured at no gain. Dropping greenlet from `concurrency` is
17.5% faster but loses 2895 statements, because SQLAlchemy's async layer
runs DB code inside greenlets, so it cannot go either.
Since the cost is irreducible, stop paying it serially. The build legs now
run uninstrumented, and a `coverage-shard` matrix measures the suite in
five parallel pieces; a `coverage` job combines the fragments and enforces
the 100% threshold. That job, not the shards, is what branch protection
should require -- the shards enforce nothing on their own.
The last shard is `tests/` minus the other four rather than an explicit
list. The first version listed directories and silently dropped 2038
tests -- dj_mcp, instrumentation, query_clients and the root-level test
files belonged to no shard -- and the only symptom would have been the
combined total landing under 100% for no visible reason. With the
catch-all the shards sum to 5528 against 5528 for the whole suite.
Fragments are written via COVERAGE_FILE to a name coverage's own erase()
will not match. pytest-cov erases every `.coverage.*` file in the
directory on startup, not just its own, so a fragment named that way is
destroyed by a sibling sharing the workspace -- and on a reused runner a
stale one could instead survive and skew the total. Verified by running
two shards in one directory: both fragments survive and combine.
relative_files is set because each runner has a different absolute
checkout path; without it combine treats the same file from two shards as
two files and the merged number is quietly wrong rather than failing.
Verified locally that combining fragments raises the total above either
input while the statement count stays fixed at 28209.
* Import every module in the coverage shards
The combined total came to 99%, short by 22 statements and 2 branches
across four files: models/table.py, __about__.py, internal/git/__init__.py
and query_clients/__init__.py. All four are module-level code that only
runs when something imports the module, and no test imports them directly.
The build legs cover them incidentally by passing --doctest-modules, which
imports the whole package while collecting doctests. The shards did not
pass it, so those modules were never imported and their import-time lines
never executed. Give it to the catch-all shard, along with the alembic
ignore the build legs use, so the doctests still run exactly once.
Verified: shard 5 alone now reports all four files at 100%, which is
exactly the shortfall.
* Forward custom_metadata_schemas from dj.yaml to the deployment The client assembles the deployment payload key by key, so a manifest section it does not name is read out of dj.yaml and dropped. The server has accepted `custom_metadata_schemas` since #2468, but no client could reach it: a repo declaring the block deployed cleanly and registered nothing, which is indistinguishable from success. Forwarded only when the key is present. Absent and empty mean different things to the server -- absent leaves registered schemas alone, while an empty list retires them -- so defaulting to [] would silently retire a namespace's schemas on the next push from any manifest that never mentioned them. * Forward hierarchies from dj.yaml to the deployment Same defect as the previous commit, found while fixing it: the payload names its keys one by one, and `hierarchies` was never among them, so a manifest declaring a hierarchy deployed cleanly and created nothing. Defaulted to [] rather than omitted, because the server's hierarchy phase returns early on an empty list -- hierarchies are upsert-only and never retired, so [] is a no-op there rather than the destructive signal it is for custom_metadata_schemas. * Restore the 100% coverage gate on client, djqs and djrs Before #2474 every build leg ran with `--cov-fail-under=100 --cov=$MODULE`, so all four packages were held at 100% inside their own leg. That change moved coverage into sharded jobs to cut the merge gate, but the shards only measure datajunction_server -- so client, djqs and djrs went from an enforced threshold to no measurement anywhere. The symptom is quiet: a client-only PR skips `coverage-shard` on its `server` filter and reports success having measured nothing. Instrument every leg except the server's again. The server is the one worth sharding (28.8min instrumented against ~15min not); the other three suites are seconds, so measuring them in place costs the gate nothing and is the only place they can be measured at all. Verified all three are still at 100% today, so this restores the gate without a backfill: client 2356 statements, djqs 539, djrs 52.
* Plan the preagg fixtures once into a template database
client_with_preaggs planned ten pre-aggregations over /preaggs/plan on
every test that used it. That is ~1.2s of setup repeated across 90
tests, and it cannot be collapsed by widening the fixture's scope: 67 of
those tests mutate the preaggs they were given, so they need their own
database.
Plan them once into a template instead, and let each test clone it. A
clone costs ~90ms against ~1.2s of planning, so every test keeps a
private database while paying almost nothing for it. This is the same
trick the suite already uses for the shared examples -- the new template
is itself a clone of that one, so the BUILD_V3 nodes the preaggs sit on
are already present.
The clone source is chosen per test rather than for the whole module.
Several tests here drive client_with_build_v3 directly to plan their own
preaggs and assert on the result; starting those from the seeded
template makes their assertions see rows they did not create.
tests/api/preaggregations_test.py 179.09s -> 108.69s (90 passed)
* Share the template-population bootstrap between both scripts
The new preaggs script had copied 44% of its lines from
populate_template.py: the environment-variable dance that has to happen
before datajunction_server is imported, the Settings construction, the
dialect plugin registration, the stubbed query service and the four
dependency overrides. None of that is interesting to either caller, and
having it twice means the next person changing how a test app is
bootstrapped has to find both copies.
Move it into tests/helpers/template_app.py behind a
`template_app_client` context manager that yields the session and an
authenticated client for a given database. Neither script imports
datajunction_server before configure_database_env has run, so the
settings are still read in the right order. populate_template.py drops
from 299 to 135 lines and now differs from the preaggs script only in
what it asks the client to do.
Also fixes the two hooks that failed CI: the session-scoped fixture
yields, so its return type has to be Generator rather than the tuple it
yields, and add-trailing-comma wanted a rewrite. Both are caught by
`make check`, which I had skipped in favour of running ruff alone.
Verified: tests/api/users_test.py passes (exercises the main template
path), tests/api/preaggregations_test.py 90 passed, and all ten
pre-commit hooks pass including mypy.
* Clone a template for the dimension link tests
These 14 tests went through isolated_client, which builds an empty
database per test: create_all for every table, then the default
attribute types, catalogs and user, then the COMPLEX_DIMENSION_LINK
examples over HTTP. Around 2s per test, all of it producing identical
state, and it cannot be shared by widening a fixture's scope because the
tests mutate the links they were given.
Give isolated_client an opt-in template instead. It defaults to none, so
every other caller keeps building an empty database and loading its own
data; a module that wants the same state in each test overrides
isolated_client_template with a template name and gets a ~90ms clone,
skipping the schema creation and seeding a clone already carries.
populate_template.py now takes an optional list of example names, so the
smaller template reuses it rather than needing a script of its own. Some
fixture sets are only module-level constants in tests/examples.py rather
than EXAMPLES keys, so names resolve against both.
tests/api/dimension_links_test.py 34.59s -> 18.32s (14 passed)
Verified the shared change: tests/api/engine_test.py, the other
isolated_client caller, passes, as do users_test.py and
preaggregations_test.py. All ten pre-commit hooks pass.
* Register every model before creating the template schema
The extraction moved the `datajunction_server.api.main` import out of
populate_template.py and into the lazily-imported helper, so it now ran
after Base.metadata.create_all rather than before it. That import is
what registers the models on the metadata, and create_all says nothing
about models it has never seen -- it just created the subset that
happened to be imported, leaving the template without the hierarchies
and hierarchy_levels tables.
Move schema creation into template_app.create_schema, which imports the
app first, and say why the import is load-bearing so it does not get
tidied away.
Fixes the 16 CI failures in models/hierarchy_test.py,
api/hierarchies_test.py and internal/deployment_test.py.
* Let workers share one Postgres and its templates
Under pytest-xdist "session" scope means per worker process, so every
worker starts its own Postgres container and builds its own copy of the
all-examples template: N times the same ~42s of work, all of it at
startup where the machine is already oversubscribed. Verified with
docker ps -- `-n 4` runs four containers.
Setting DJ_TEST_POSTGRES_URL now points the suite at a Postgres it did
not start, and any template already present on that server is reused
rather than rebuilt. Workers then only do the cheap part, cloning a
template per module or per test at ~90ms. Unset, everything behaves
exactly as before.
Generated database names now carry the xdist worker id. They previously
leaned on id(request), which is unique only within a process -- fine
when each worker had its own server, a latent collision once they share
one.
Measured on tests/api + tests/models at -n 8, same machine, identical
results (1653 passed, 8 skipped, 1 xfailed):
per-worker containers 217.25s
shared, pre-built 165.81s -24%
Unlike per-module fixture work, this removes duplicated startup rather
than trimming one module, so it shows up in wall clock.
Known gap, addressed next: a template that exists but was never
populated is treated as usable, so a failed build leaves a database that
breaks later runs with a confusing UndefinedTable. Nothing uses the
shared path yet -- CI still starts its own containers -- so this is
inert until the workflow is wired up.
* Refuse to run against a half-built shared template
Checking that a template database exists is not the same as checking it
is usable. A build that dies after CREATE DATABASE but before loading
anything leaves one that looks fine and is not, and on a shared server
nothing cleans it up, so every later run fails somewhere downstream with
an opaque UndefinedTable. That is exactly how the first shared run broke.
Verify the template holds a schema, not merely that it exists, and stop
building shared templates from inside pytest at all. Workers racing to
create the same database is the other half of the problem, and building
them in the step before pytest avoids both. When one is missing or empty
the failure now names the database, says whether it is absent or empty,
and prints the command to build it plus the DROP needed to clear a
poisoned one.
Verified by deliberately leaving template_preaggs empty:
RuntimeError: Shared template database `template_preaggs` exists
but is empty. DJ_TEST_POSTGRES_URL is set, so templates must be
built before pytest starts. Build it with: ...
Both paths still pass (104 tests across preaggregations and
dimension_links, shared and default), and make check is clean.
* Give CI one Postgres and build the templates once
The server jobs spend ~42s per xdist worker starting a container and
building the same all-examples template, all of it at startup. Add a
Postgres service to the build job and a setup section that creates the
role and the three templates before pytest, then point the suite at it
with DJ_TEST_POSTGRES_URL. Workers now only clone, at ~90ms each.
Also check the readonly_user role up front. For containers it starts
itself the suite creates that role, but on a server it does not own it
cannot, and the reader database URL needs it -- so a missing role used to
surface as an authentication error deep inside an unrelated test. It now
fails immediately, names the role and prints the CREATE ROLE to run.
Services cannot be conditional, so the client/djqs/djrs legs get a
Postgres they ignore; it starts in a couple of seconds.
Verified by replaying the workflow's setup commands against a clean
server and running the full suite against it at -n 4, matching the
runners' worker count: 4721 passed, 803 skipped, 3 xfailed, with only
the pre-existing integration failure that make test skips. The role
check was verified by dropping the role and confirming the message.
* Share one Postgres with the coverage shards too
Rebasing onto the sharded workflow applied without conflict but left the
shared Postgres wired only to the build job, while coverage-shard -- the
expensive half now -- still started its own containers and rebuilt every
template. Five shards means that duplicated startup is paid five times.
Give coverage-shard the same service container and the same template
setup, so the shards clone templates somebody else built rather than each
building their own.
Worth doing because CI measured the effect on the unsharded workflow: the
gate went 30.1 -> 28.3min and the 3.13 leg 28.1 -> 20.9min. Contention
between four workers over four vCPUs costs much more than the same work
spread across a developer machine, which is why a local -n 4 comparison
had suggested this was break-even.
Co-authored-by: GitHub Actions Bot <actions@github.com>
`CustomMetadataSchema.owner` was a free-text string that was written and never read. Nothing filtered, notified, or authorized on it. Authorization for a schema is already fully resolved by namespace -- the API gates on WRITE for the namespace plus an admin tier for global and reserved keys -- so a per-row string added nothing an authz check could use, while sitting next to a real authorization system under a name that invites the assumption that it grants something. DJ's only real ownership is `Node.owners`, a relationship to users through an association table.
* add display_name to semantic metadata * run formatter --------- Co-authored-by: Joel Lubinitsky <jlubinitsky@netflix.com>
Co-authored-by: GitHub Actions Bot <actions@github.com>
`custom_metadata` has been free-form since it was added, and the schema registry that landed in #2337, #2456, #2461 and #2468 had no user-facing docs at all. Adds a page under data-modeling covering how to register a schema in the deployment manifest or through the API, how scoping and precedence work, what reconciliation does when a manifest stops naming a key, and how to filter nodes by metadata. Two behaviours get called out because they surprise people: validation is lax for unregistered keys and strict for registered ones, so a schema is a hard gate from the moment it registers; and an omitted schema block leaves existing schemas alone where an empty list retires them.
Co-authored-by: Robin Davis <robind@netflix.com>
A role-qualified attribute such as default.special_country_dim.name[birth_country] parses as a Subscript wrapping a Column, so the projection loop in parse_dj_sql rejected it even though the same text is accepted in GROUP BY, WHERE and ORDER BY. Widen the guard to also accept a Subscript over a Column and use its string form, which matches the GROUP BY rendering exactly, so the metric-vs-dimension split keeps working. Other expressions are still rejected.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Propagate an upstream change into the cubes below it A cube was filtered out of downstream propagation, and even reached it would only have had its status revalidated -- nothing computed it a version. So an upstream edit left the cube pointing at a materialized table built against a definition that no longer existed, and the only way to recompile it was a no-op edit to the cube itself. Propagation now includes cubes and gives each one a new revision resolved from its own current metrics and dimensions, so its columns, elements and parents recompile against what the upstream became. The revision is built by the same code the cube update endpoint uses, extracted as `save_new_cube_revision`, so the two writers cannot produce differently-shaped revisions, and it swaps materializations the same way so the superseded revision's workflows are stopped rather than left pointing at a dead revision. The cube inherits the upstream's change tier rather than being classified on its own shape. Widening a transform's WHERE clause changes no metric, no dimension and no metric component identity -- `is_non_trivial_cube_change` returns False for it -- yet every row in the cube's table was computed under the old filter. Over-rebuilding costs compute; under-rebuilding serves wrong numbers. A cube that cannot be rebuilt is logged and skipped rather than costing the remaining downstreams their propagation. * Compare stored columns against the validator's on revalidation Fold the column comparison into a tier so propagation can hand the same value to bump_version for downstream cubes, rather than each caller deciding what a column change is worth. * Backfill a missing column order without a new revision `order_fixed` fires when a stored column has no `order` and DJ fills in the projection index -- its own bookkeeping on a row written before the field existed. No query changed and no name, type or value moved, so it earned a version bump that described nothing, and since downstream cubes inherit their upstream's tier, it would now rebuild every cube's materialized table for a metadata fix. It is `ChangeTier.NONE`, and the fix is applied to the current revision in place. Leaving it unset is not free: readers sort columns by `order` with unset last, so an unordered column drifts to the end of the projection and every `to_spec` logs the node as unordered. Writing the projection index onto the stored row misrepresents nothing, since the rows were inserted in that order to begin with -- and where a revision is only partly ordered, it puts a column back where the query always projected it. The audit trail survives the loss of the bump. A backfill still writes a history event naming the columns it filled and the version the node kept, because nothing else would record that DJ rewrote the row. Three existing tests used a cleared column order as a cheap way to force revalidation to fork a revision, which it no longer does; they now disagree with a stored column type instead, so they keep testing the fork path rather than passing vacuously. Also documents, next to the cube rebuild check, why the churn there is deliberate: any query edit is a major bump, so every upstream query edit rebuilds every cube below it. Narrowing that by diffing resolved output columns was considered and rejected -- a query edit can move a filter, a join or a CASE threshold while leaving every column and type identical, and each changes every row served. * Detect a column the query no longer produces The merge loop walked the validator's columns and looked each up in the stored ones, so it could only ever find what the validator produced. A column the revision still stores but the query no longer selects was never visited: no bump, and it rode onto the next revision advertising a value the node cannot supply. describe_column_changes already compares both directions, so use it. A removal is major -- anything referencing the column is broken. That occasionally rebuilds a downstream cube that never used it, which is affordable because removals are rare and additions are not. * Cover the order backfill riding along on someone else's revision An order-only change no longer earns a revision, so the in-loop backfill and the history detail that reports it are reachable only when a revision is created for another reason and a legacy column also has no order. The suite exercised those separately and never together. * Pin that a removed column invalidates the metric and its cube Dropping a column a metric aggregates leaves the transform valid -- its own query is fine -- while the metric can no longer infer a type and the cube built on it follows. Each bumps, so nothing is left serving a definition that no longer resolves. Worth pinning because the failure is only visible downstream: the edit that causes it looks entirely successful at the node being edited. * Trim the commentary to what isn't in the code * Don't tell an operator the old cube table is adoptable previous_table_usable was derived from is_non_trivial_cube_change on every path, including propagation -- so a filter change upstream, the case that motivated not using that predicate to decide rebuilds, recorded the old table as reusable exactly when its data is what went stale. Propagation now passes False, since identical shapes say nothing about the rows. Also: correct the claim that both cube-revision writers route through here (deploy builds and swaps on its own path), say why node survives the recovery rollback, and cover a cube failing after one already succeeded. * Add a failing test: deploy does not bump a cube its upstream changed The sibling of test_patch_and_deployment_agree_on_version, which only covers edits to the cube itself. Here the cube's spec is byte-identical across two deploys and only the metric under it changes, which filter_nodes_to_deploy cannot see -- an unchanged spec is skipped, so the cube keeps a revision compiled against the old definition while the PATCH path propagates into it. Fails on the cube assertion (v2.0 on PATCH, v1.0 on deploy); the metric assertion above it passes, so the divergence is the cube alone. Red until the deploy path shares the propagation bump. * Bump a cube on a deploy when its upstream changed The deploy path skipped a cube whose own spec was byte-identical, so a deployment that edited a metric left the cube beneath it on a revision compiled against the metric that no longer existed -- the same gap the PATCH path had before _propagate_update_downstream started including cubes. filter_nodes_to_deploy now walks up from each skipped cube through the parents already loaded with the namespace's nodes, which costs no queries and no parsing, and pulls in any cube sitting above a node this deploy is changing. The walk is transitive: an edited source under an unchanged metric still reaches the cube. The cube inherits the most significant tier among those upstreams rather than earning the MINOR floor an identical spec would, and its materialization swap records the old table as unusable, both matching what propagation does on the PATCH path. Makes test_patch_and_deployment_agree_on_an_upstream_change pass. * Treat an added column as a major change again Demoting an added column to MINOR was meant to stop an additive upstream change from rebuilding every cube below it, but it does not: the cube rebuild in _propagate_update_downstream skips only ChangeTier.NONE, so a minor bump rebuilds all the same. The demotion changed the version the cube earned and nothing else. Fold additions back in with type changes and removals. The column-change detection and the order backfill are unaffected.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* feat: annotate the sqlglot AST * Add test with user submitted error * Map types between DJ/sqlglot * Fix lint
DruidCubeMaterializationInput.is_branch_deploy defaulted to False and was never set. Orchestrator's _is_branch_deploy() is now carried on CubeMaterializationSwap and threaded through schedule_materialization_jobs into MaterializationJob.schedule(), so the query service can tell a branch-preview deploy from a main one when it schedules a cube.
config.cube.version only exists for some job types (e.g. not DruidCubeMaterializationJob), so listing all revisions silently grouped every materialization under the current node version. The practical effect: the UI showed an old, orphaned materialization as if it belonged to the live revision, and its delete button inherited the same wrong version, so deleting it failed with a confusing not-found error. node_version is now recorded server-side from the revision the materialization actually belongs to, and the UI groups and deletes by that instead of falling back to the node's current version.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Add semantic node fingerprints Derive node digests and canonical comparisons from existing change tiers and equality rules so semantic classification remains server-owned. * Compose semantic fingerprints from parent hashes Use a versioned Merkle-style digest so persisted object metadata changes whenever any semantic ancestor changes. * Harden fingerprint extension contracts Require explicit list-order tiers, dispatch immutable fingerprint versions, and route link equality through the shared comparison key. * Update the cube reorder assertion Reflect that NONE-tier filter ordering is omitted from the deployment changelog while the dimension reorder remains minor. * Isolate and freeze semantic fingerprint construction Move canonicalization and structural SQL serialization behind a dedicated package while preserving version 1 digests and aligning metric field semantics. * Use semantic normalization terminology Reserve canonical for final JSON serialization and name comparison helpers by behavior so the fingerprint API is easier to read. * Cover semantic fingerprint edge cases * Represent unavailable semantic fingerprints explicitly * Compare unparseable queries without blocking deployment * Remove redundant semantic fingerprint work * Move semantic fingerprint composition out of node specs Fingerprint values depend on graph context, so keep local and composition builders internal until the graph evaluator owns parent resolution. --------- Co-authored-by: Philip Weiss <pweiss@netflix.com>
* Keep columns a CTE reads without a qualifier The v3 builder prunes each CTE down to the columns its readers name, and it works out who reads what by the qualifier in front of a column. A column written bare therefore belonged to nobody, so the CTE producing it dropped it and the consumer was left selecting columns its source no longer had. Attribute unqualified columns to every node the query reads from. Scope is only knowable after compilation, and the two mistakes are not symmetric: keeping a column nobody wanted costs one entry in a projection, while losing one emits SQL that cannot run. Naming a column the node does not have is a no-op, since the pruner simply never finds it. * Ask a CTE's readers what it must project Pruning ran inside the CTE-assembly loop, where a CTE's consumers may not exist yet. Unable to ask them, it guessed from an enumeration of every way a column could be demanded, and the enumeration kept missing cases: nodes it never visited, expressions it never walked, reference forms it did not recognise. Each miss emitted SQL referencing a column its source had stopped projecting, and Spark found out hours later. Pruning now runs once the query is whole, in a single pass over the scopes ordered readers-first. One pass suffices because pruning a reader only narrows what it asks of its own sources. It needs no database and no compilation: every table reference is already a CTE name or a physical table, and a CTE's output names read straight off its own projection. Each set-operation arm is read as its own scope, so a UNION CTE is pruned per arm and keeps the same positions in all of them -- these were never pruned at all before. A qualified reference is charged to the CTE its qualifier names, taking the segment right after the qualifier so a struct path leaves the struct column itself in place. A reference whose qualifier we cannot place is charged to every CTE in scope, since keeping a name a CTE lacks costs nothing. * Follow the pruner's own reasoning through the expectations Two things the reader-first pass got right that the old enumeration did not, now visible in the generated SQL. A column only a pushed-down filter used is no longer projected: the WHERE reads it off the physical table the CTE scans, so nothing downstream needs it in the SELECT. And pruning an intermediate CTE narrows what it asks of its own upstream in turn, so a chain settles tighter than a single node's keep-set ever could. Resolve a column's qualifier from the table hung off the column as well as from the dotted name. The builder writes its own projections and GROUP BY the first way, so those references were all falling through to the unqualified case and being charged to every CTE in scope -- safe, but it held columns open in CTEs that never supplied them. ``_col_table_name`` already knew both forms; it moves to utils so the pruner can use it instead of growing a second copy. Every rewritten expectation is gated on ``unprojected_references``: whatever a CTE reads from another CTE, that other CTE projects. The oracle now excuses a filter that lands on a name its own arm introduced with AS, which is a shape DJ has always generated and the source never had a column for. * Keep the columns a select's own clauses still need Pruning protected the producer's projection from GROUP BY alone. Three other constructs address that projection too, and an audit that executed the generated SQL rather than reading it found all three, plus one that is not about protecting a column at all. DISTINCT makes the whole projection the dedup key, so narrowing it folds together rows that were distinct: a transform lost a column and a downstream SUM quietly halved. Under DISTINCT every position now stays. HAVING and ORDER BY can both name an output alias, and pruning that alias leaves SQL the binder rejects. ORDER BY positions need the renumbering GROUP BY positions already got, or they point at whichever column slid into the slot -- or off the end of it. WHERE is deliberately not on that list. A column only the producer's filter reads resolves against its FROM, not its projection, so it is still free to go; there is a test pinning that, because protecting it would undo most of what this pass is for. The four tests run their SQL against DuckDB and compare pruned to unpruned. Two of these faults return a wrong number or a wrong order rather than failing, so asserting that the SQL parses or matches a string would not have caught them coming back. * Keep dedup set operations whole when pruning A CTE's projected row is the match key for every set operation but UNION ALL, so trimming a column changes which rows survive: a metric over a UNION under-counts, and INTERSECT/EXCEPT begin matching rows that should not match. DISTINCT was already guarded in _keep_positions; the set-op form of the same hazard was not. Marking the scope unprunable rather than skipping only its own pruning also stops the narrowed demand propagating into a source it stars, which would shrink the match key one level down instead. * Match projection names without regard to case The pruner compared a projection's output name to the demanded names with Python string equality, so a `Region` projection read downstream as `x.region` was dropped while its reader kept referencing it. Only the no-match fallback in _apply_keep_positions hid this: as soon as one other column matched, the query went out broken. Every dialect DJ targets resolves identifiers case-insensitively, so fold case on both sides of the comparison. GROUP BY, HAVING and ORDER BY names resolve the same way and get the same treatment. * Keep a source starred under DISTINCT whole Keeping every position under DISTINCT protected the deduping select's own projection but not the CTE behind a star it selects. The narrowed demand still propagated into that source, so `SELECT DISTINCT * FROM s` trimmed `s` underneath the star and shrank the dedup key anyway. DISTINCT is asking the same question set operations already ask here, so it joins _dedups_rows and the scope is marked unprunable. Checking each arm rather than the outer select also covers arms that dedup on their own under a UNION ALL. The guard in _keep_positions stays: it is the only protection a direct filter_cte_projection caller gets. * Prove the dedup guards by running the SQL A string comparison only shows that pruning left a query alone; it does not show what going wrong would cost. These run the SQL through duckdb both ways, so a UNION whose dedup key was narrowed reports 30 where it owes 40 -- no error raised, just a quieter number, which is how it would have reached a Spark job. Confirmed non-vacuous by turning the guard off: UNION 40->30, EXCEPT 30->20, starred DISTINCT 40->30. Also covers the DISTINCT guard left in _keep_positions. Marking the scope unprunable settles it earlier for a CTE, so the only caller still reaching that line is filter_cte_projection -- which is the caller the guard exists for, and now the one the test uses. * Assert the SQL the pruning tests actually build Each of these spent a long fixture on one boolean: whatever a CTE reads, its producer projects. That holds for a pruner that prunes nothing, and the check re-implements the pruner's own attribution, so it catches disagreement between two copies of one idea rather than errors in it. Neither the generated SQL nor the point of the test was visible. Pin the SQL instead. parent_dim keeping code_a, code_b and code_c that nothing downstream names is the behaviour under test, and it now reads that way. The transitive case replaces `"channel" in item_cte` after a string split, which passed on the word appearing anywhere in the slice. The invariant assertion stays as a secondary check. * Name the pruning order after its guarantee _consumers_first said which end it started from but not that it is a topological sort, nor what it returns -- the CTE names plus "" for the outer select, which nothing reads and which is never pruned. That empty key is why every mutation in the loop is guarded by `if key and ...`, and nothing said so. * Explain the pruning pass in three steps prune_cte_projections had a paragraph on what it does not need (a database, a compilation pass) and nothing on how it works. Say the three steps instead: sort the scopes on read order, walk them once keeping the live and unprunable records, and at each one mark, prune, then record what it reads. The empty-string scope key gets named too -- it is the outer select, it is never pruned, and it is why every mutation in the loop is guarded by `if key and ...`. Rename _dedups_rows to _compares_whole_rows. EXCEPT ALL does not dedup but still matches on the full row, so it needs the same guard, and the comparison is the reason the guard exists.
…2508) Two deployments to the same namespace that overlap in time could both claim the same version. A deployment reads every node's `current_version` once, when it loads the registry snapshot, and computes the next version from that. If another deployment commits in between, every version the first one planned is already taken and the batched revision insert dies on uq_noderevision_version -- taking the whole deploy with it, so a user's CI build fails on something that has nothing to do with their change. `_lock_versions` now re-reads `current_version` for the nodes about to be written, under `FOR UPDATE`, so the version each node earns is derived from committed state at write time: a concurrent writer has either already committed, and we see its version, or it waits behind the lock until this deployment commits. Both writers go through it -- regular nodes via `create_nodes_from_validation` and cubes via `_create_cubes_from_validation` -- and it batches into two queries per level rather than one per node, since levels carry hundreds of nodes. The bump floors on the node's highest revision rather than on `current_version`, which also closes the second half of the old behavior: the losing deployment blind-overwrote `current_version`, so without the unique index it would have stranded a node whose `current_version` lagged its revisions. Such a node now heals on its next deploy instead of colliding. Versions compare as (major, minor) pairs, since v10.0 sorts below v9.0 as a string. Dry runs skip the lock -- they roll back, so their versions never persist, and a read-only preview should not hold row locks against live deploys.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Check a base-db marker instead of connecting to the template clone_database_from_template's pg_terminate_backend targets every connection to the shared template right before cloning from it, on every clone, from every worker, for the life of a run. A check connection made straight to the template (template_is_populated) is a legitimate target of that call whenever the timing overlaps, and gets killed as collateral damage (psycopg.errors.AdminShutdown). Write a marker row on the base database once population finishes, and check that instead -- it's never a database name pg_terminate_backend is aimed at. * Exclude test_template_status from migration schema diff The marker table lives on the shared base 'dj' database used by test_migrations_are_current's postgres_container fixture, so it was showing up as an untracked table in the alembic/model comparison. * Trim docstring verbosity, switch double backticks to single No functional change.
* Round-trip a reference link's role through to_spec() Node.to_spec() reconstructed a reference link's dimension field straight from the stored Column.dimension_column, which bakes a role into a "[role]" suffix whenever one is set, without ever splitting that back out into the link's own role field. A role-qualified reference link therefore could never compare equal to the spec it was authored from, redeploying as a changed link on every push. * Reuse parse_dimension_ref instead of hand-rolled bracket splitting Node.to_spec() had its own [role] suffix parsing for reference link dimension_column values; parse_dimension_ref already does this (plus the nested hop->role form), so reuse it via a local import. * Exclude test_template_status from migration schema diff The marker table lives on the shared base 'dj' database used by test_migrations_are_current's postgres_container fixture, so it was showing up as an untracked table in the alembic/model comparison. * Revert "Exclude test_template_status from migration schema diff" This reverts commit 20168dd. * Move reference-link spec construction onto Column Node.to_spec() no longer needs to import parse_dimension_ref itself; Column.to_reference_link_spec() owns building the DimensionReferenceLinkSpec for its own dimension_column encoding.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Fix two more dimension_links/description export round-trip gaps
DimensionLink.to_spec() dropped default_value, and NodeSpec.diff()
only rendered ${prefix} on one side of the comparison -- both caused
permanent redeploy churn independent of the earlier required_dimensions
and reference-link role fixes.
* Trim comment verbosity, add full deploy/redeploy noop regression test
test_full_redeploy_is_noop covers required_dimensions, reference link
roles, join link default_value, and description/custom_metadata
rendering together in one deploy-then-redeploy cycle.
Co-authored-by: GitHub Actions Bot <actions@github.com>
… column (#2522) A declared column name that doesn't match the query's actual SELECT output was silently ignored: the metadata attached to it (display_name, description) never applied, and every redeploy re-reported a spurious column add/remove diff and bumped the version, even though the persisted state never changed.
A join link written with no join_on reached the database with join_sql
NULL, and the NOT NULL constraint turned a validation error into an
unhandled IntegrityError on POST /deployments/impact. Validation already
caught both halves of the malformed spec — a stray node_column and the
missing join_on — but the orchestrator wrote the link anyway on the
INVALID-node path, so the message never reached the user.
_process_node_dimension_link now checks a join link before any write and
returns a FAILED DeploymentResult carrying validation's own wording, which
moves into models/dimensionlink.py so both callers share one string.
POST /nodes/{name}/link had the same hole. A missing join_on on a normal
join reached a confusing "does not reference both" error, and a CROSS join
with no ON clause went straight to the NOT NULL violation. Both entry
points now reject a missing join_on up front and store an empty join_sql
for CROSS, which is what the SQL builders already expect.
On SIGTERM the streamable-HTTP session manager cancelled its task group while /mcp requests were still streaming, so each one returned without finishing its response and uvicorn logged an error per request. The mount now counts in-flight requests. When the lifespan exits it stops taking new ones, answering them 503, and waits up to five seconds for the rest to respond before the session manager tears the streams down. Five seconds sits well under gunicorn's 30s graceful timeout, so a slow drain can never cost the worker a SIGKILL.
* Support --format json on dj push, not just dryrun push() computed a full structured DeploymentInfo but only ever printed it as a rich text panel, discarding the structured result. A caller that wants to consume a wet deployment's result programmatically (e.g. a CI script posting a PR comment) had no way to get it -- --format json existed on the flag already but was silently dropped before reaching push(). * Add coverage for --format json's suppressed-warning branches push(format="json") skips the rich text warnings/errors on several paths (git config failure, invalid nodes, file name mismatches) so they don't pollute the JSON on stdout; those branches had no test.
Co-authored-by: GitHub Actions Bot <actions@github.com>
* feat: metricless queries * fix: preserve cube filters in metricless queries * refactor: validate metrics at measure entry points * feat: add dimensions SQL v3 endpoint
Co-authored-by: GitHub Actions Bot <actions@github.com>
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Add SWR * fix bug * Add additional test coverage * fix lint error
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Pin the dimension-attribute-in-same-push failure A push that both adds a column to an existing dimension and adds a node linking to that new attribute marks the linking node INVALID with "Dimension attribute '...' not found". Four tests pin the behavior: - reference link: the deploy fails, and the identical retry succeeds - join link: the node is INVALID but the deploy reports success - with query lineage on the dimension: the identical link passes - brand-new dimension in an empty namespace: passes The failure needs the dimension to already be persisted with its old column set. Link validation reads the persisted revision's columns (_prefetch_dimension_link_nodes), and the prefetch runs once per topological level, which is built from query lineage only -- dimension link edges are not ordering edges. With no query dependency between them, the dimension and the linking node share a level, so the link resolves against pre-push columns. Query lineage or an absent dimension both avoid it, which is why the last two cases pass. * Assert the link resolves the new dimension attribute The two characterization tests now assert the intended behavior: a push that adds a dimension column and a link onto it deploys the link successfully, for both the reference and the join form. Two cycle cases join them -- a self-join with a role and a mutually linked pair of dimensions -- which link ordering must not turn into a hard failure. * Order a linked dimension ahead of the node linking to it Link validation resolves a dimension attribute against the dimension's persisted columns, and the deploy order came from query lineage alone. A push that added a column to an existing dimension and a link onto that column put both in the same topological level, so the link resolved against the pre-push column set and the linking node landed INVALID; the identical push then succeeded on a retry. Each spec's dimension_links now contribute deploy-ordering edges alongside required_dimensions, kept out of plan.node_graph so they never become parents. Links can be cyclic where query lineage cannot, so an edge that would close a cycle -- a self-join with a role, or two dimensions linking to each other -- is dropped instead, leaving those nodes ordered by lineage as before. Deploy order now differs for specs whose links are not query parents, which reorders the results list of three existing deployment tests.
Co-authored-by: GitHub Actions Bot <actions@github.com>
…2535) _warn_about_unmatched_cube_columns only appended to the free-text warnings list, giving no structured way to tell which node a warning belongs to. It now also appends a matching DeploymentResult per unmatched column, with status WARNING and the cube's rendered name, matching the pattern used by the materialization warning helpers. Also use spec.rendered_name instead of spec.name in the warning message, so it shows the resolved node name instead of a raw ${prefix} placeholder.
* Add DELETE /tags/{name}/ endpoint
Tags could be created but never removed, leaving stale empty tags in
listings. Deleting a tag returns 204; a tag that still has active nodes
attached is refused with a 409 naming the node count.
* Exclude DELETE /tags/{name} from the route-coverage guard
Tag CRUD is not RBAC-governed yet; POST /tags and PATCH /tags/{name} sit in
the same follow-up group.
Co-authored-by: GitHub Actions Bot <actions@github.com>
…mpact (#2483) * Add semantic fingerprints to deployment impact Return server-owned fingerprints and change tiers for deployment results and downstream impacts, including client parsing and unknown-state propagation. * Align deletion result expectations * Use graph snapshots in fingerprint expectations Impact tests now derive expected hashes from complete graph snapshots, matching production parent resolution. * Keep dry-run graph extraction tolerant Recover per node during impact analysis so invalid SQL can produce unknown fingerprints without weakening normal deployment parsing. * Fix deployment impact regressions after rebase Canonicalize required-dimension identities during change detection so equivalent metric specs remain no-ops, and align deployment assertions with the additive fingerprint response fields. * Cover deployment impact comparison fallbacks * Cover unchanged metric comparison path * Address semantic fingerprint impact review * Initialize deletable specs in empty deployment test * Canonicalize required dimensions with multiple parents * Preserve namespace-aware deployment comparisons * test: cover invalid required dimension normalization --------- Co-authored-by: Philip Weiss <pweiss@netflix.com>
Co-authored-by: Philip Weiss <pweiss@netflix.com>
Co-authored-by: GitHub Actions Bot <actions@github.com>
* Distinguish a re-deployed pre-existing failure from a new one A node whose spec is unchanged but whose stored status is INVALID is deliberately promoted into the update set so that the deployment gets a chance to revalidate it. That is the right thing to do -- the node may well have been broken by something outside this deployment that has since been fixed -- but it made an already-broken, untouched node indistinguishable in the response from one the author just broke. A deployment then failed on nodes its change never touched. Those nodes are now marked `revalidation_only`, so a caller can tell the failures a deployment caused from the ones it inherited, and still sees the recovery when a later deploy fixes the node's upstream. The flag is not on its own enough to excuse a failure, and a later gate must not treat it that way: it does not compare failure reasons, so a node whose own spec never moved can fail for a new reason caused by an edit upstream of it. Attribution has to be "changed, or reachable from something changed", and the inherited breakage surfaces separately as a downstream impact. Additive and observational. Exit codes, gating, and overall pass/fail are untouched, and the field is nullable so deployment rows persisted before it existed still rehydrate. * Trim the comments
Co-authored-by: GitHub Actions Bot <actions@github.com>
…nt' into pw--governed_delete_refresh--2026-09-14
philipfweiss
marked this pull request as ready for review
September 14, 2026 16:01
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
6 participants
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.
Tracking: #2234 (step 6).
Provisioning a governed namespace stores its owner and deployer grants. This change enforces those grants for
WRITE,DELETE, andMANAGEon the boundary and its descendants. Governed boundaries require explicit grants, including while deactivated.READ,EXECUTE, ungoverned namespaces, and the admin bypass retain their existing behavior.Hard deletion requires explicit
MANAGEon every governed boundary it removes, including empty descendants of an ungoverned ancestor and descendants removed withcascade=true. All authorization completes before mutation. The transaction holds the existing provisioning lock so a boundary cannot appear between the check and deletion, and explicit checks cover boundaries created after the request's authorization context was loaded.Authorization and deletion use literal namespace prefixes. For example, deleting
lunch.taco_truckpreserveslunch.tacoXtruckand its nodes, whether the neighboring namespace is governed or ungoverned.The dimension query-count regression now matches complete table names, so the governed-boundary lookup is accounted for as authorization work. The endpoint's three-query expectation and the total request query cap remain enforced.
Validation:
main(a7f632549) without rewriting its history.main; Linux CI verifies the repository's expected ordering.d875982d1passes the full GitHub matrix, including server tests on Python 3.11, 3.12 and 3.13 and the 100% coverage gate. The Python 3.13 job passed after an isolated retry of an order-sensitive failure in unchanged node deletion code.