Schematized custom metadata APIs - #2461
Draft
shangyian wants to merge 10 commits into
Draft
Conversation
…ey expression index
…gistry API - Non-admin POST without namespace → 403; reserved=True → 403 - Namespace-scoped POST checks write access via AccessChecker - Reserved global key blocks namespace-scoped registration (409) - Persist owner, reserved, created_by_id, updated_by_id on create/update - Add owner, reserved, updated_by_id to CustomMetadataSchemaOutput - Add owner, reserved to CustomMetadataSchemaCreate - Add 7 new auth/field tests; update existing tests to use namespace scope
`pyupgrade` and `ruff --fix` both run as blocking hooks in CI over every file a branch touches, and they rewrite `Optional[X]` to `X | None` and `datetime.timezone.utc` to `datetime.UTC`. These two files were written in the older forms, so the hooks modified them and failed the build. Running the hooks over only the files a change edits is what let this survive: the rest of the branch's files were never checked locally. They are now.
A namespace that owns a repository has one writer, and this endpoint is not it. Registering a schema here is unreviewed, and once a deployment reconciles a namespace to exactly what its manifest declares it is also temporary: the row would vanish at the next push, having returned 200 to whoever created it. Refusing at the door is kinder than reverting later, and it means the ungoverned path never exists rather than being closed after the fact. The check is the one node mutations already use, so a namespace locked by an ancestor is caught too, and branch namespaces stay writable -- edits there are valid and sync back to the repo. Its message was hardcoded to talk about nodes, which would misdirect anyone registering a schema, so it now takes the closing sentence as an argument and defaults to the node wording. The registry passes its own, naming the manifest key the change belongs in. Global keys are untouched: no repository owns one, so there is nowhere to send the caller. They remain admin-only, which is a separate gate.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
The registry sat under `/custom-metadata/schemas/`, where `/custom-metadata/`
is a prefix rather than a resource -- nothing answers a GET on it. Every
neighbouring collection is a single plural noun at the top level, so this is now
`/metadata-schemas/`.
Named for metadata rather than for `custom_metadata` deliberately. Node
`custom_metadata` is the first surface, not the only plausible one: `tag_metadata`
is an authored bag with no validation today, and a materialization platform block
would be another. Neither is called `custom_metadata`, so a registry named after
one field would describe part of what it does. The entity discriminator that
extension needs is deliberately absent -- the scoping axes for a second surface
are unknown, and guessing them now is worse than adding them when there is
something to scope.
Ids stay. A row's real identity is `(key, node_type, namespace)`, which does not
fit a path segment, and the id is stable across a retire-and-re-register cycle
because upsert revives the row. What was missing is that an id could only be
spent, never read: `GET /metadata-schemas/{id}` closes that, and listing by `key`
turns resolving an id into one precise call rather than a scan.
The facet catalog was the list with one predicate. `filterable` is on every row
of the list response, so the subset is a client-side read, and the endpoint went.
Worth knowing that the flag curates which keys a UI offers as filters and gates
nothing: `customMetadataFilters` never consults the registry.
Violations report on one schema and already required its id, so they belong
under it.
Left as follow-ups: the list filters match a stored scope exactly, so they answer
"what is registered here" and not "what governs a node here" -- a schema on
`shared` applies to `shared.finance` without being registered at it. Resolution
already answers the second question and could be exposed as a filter if a caller
needs it.
Two CI gates the new endpoints had not met.
`test_every_mutating_route_has_a_denial_test` requires every mutating route to
prove its authorization check fires, not merely that it is reached. Neither
registry write was registered. The POST fits the generic case table: it has no
path parameters and authorizes the namespace named in its body, so the stub goes
there. The DELETE does not -- it addresses an integer id and authorizes the row's
own namespace, so a row has to exist before the check runs, while the generic
cases fill every path parameter with a string that would fail validation first.
It gets a real denial test next to the endpoint's other tests, registered the way
the `{preagg_id}` routes already are.
The coverage gate failed on one partial branch: nothing combined a
custom_metadata filter with another filter that had already joined the current
revision, so the "already joined" path was never taken. The neighbouring filters
mark theirs `# pragma: no branch`, but this one is worth a test rather than an
exemption -- joining the same alias twice is an error, not a slow query, so the
case proves the flag is respected instead of asserting it is unreachable.
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
The custom metadata schema registry was added in #2456. This PR adds endpoints for registering and retiring schemas, along with discovery endpoints so that nodes can be filtered by their
custom_metadataentries.Managing schemas
Authorization:
check_namespace_not_git_onlyguard node mutations already use.Finding nodes by metadata
findNodesacceptscustomMetadataFilters, a list of{key, op, value}withEQ,NE,EXISTS,CONTAINSand numeric comparisons. Equality compiles to JSONB containment (@>) so it can be served by the global GIN index from #2337, and containment is recursive, so filtering on a nested value works without a path syntax.Test Plan
Deployment Plan
Additive -- new endpoints and a new optional GraphQL argument