Fix unenforced field constraints (#133) and unique-on-tenant-root (#134); acton-service 0.39.0 - #135
Merged
Merged
Conversation
`enum`, `text(max:)`, and `integer(min:/max:)` were declared in the DSL but never checked in-process. The only thing refusing them was the generated database CHECK (or the VARCHAR width), and neither SQLSTATE 23514 nor 22001 is mapped to a client error: `map_write_error` special-cases only 23505, so everything else became `BackendError::QueryError` and then a 502. A caller sending an unknown enum variant got a retryable status for a request that could never succeed, carrying a raw driver message with no field name. `FieldType::check_value` now checks every constraint the DSL can express, and the write path answers 422 naming the field — and, for an enum, the allowed variants. `bytes(max:)` was already enforced here and moves onto the same code path so the wording lives in one place. The check runs at the last seam before the backend rather than during JSON conversion, for two reasons. Conversion is shared with the query filter parser, where a bound outside the declared range is a legitimate filter and must not 422. And by that seam the field map also holds `@default` and `@compute` output and anything a `before_*` hook substituted, so those are covered too — not only client input. Float `precision` stays unenforced: no backend applies it (#7), so rejecting on it would be a new restriction rather than a fix. Fixes #133
`is_tenanted()` matches `Annotation::Tenant(_)` regardless of kind, and the migration planner used it for two unrelated decisions: whether the table needs a `_tenant` column, and whether a `unique` field is scoped by that column. The first is true for a root; the second is not. Scoping a root's unique to `(_tenant, field)` does not weaken the constraint, it removes it. `_tenant` is NULL on a platform-level create and PostgreSQL treats NULLs as distinct, so the index accepts every duplicate; a tenant-scoped caller instead gets their own tenant id stamped there, which partitions the constraint by whoever happened to write the row. Either way two organizations could carry the same short code and the migration would apply cleanly. Root rows are also the ones that most need global uniqueness: they *are* the tenants, so there is no outer tenant to scope them to. Splits the decision out as `SchemaDefinition::unique_scoped_by_tenant`, true only for `@tenant(parent: ...)`. `is_tenanted` keeps its meaning and still drives the `_tenant` column, so the DDL ordering fix from #56 is untouched. The two #56 end-to-end codegen tests asserted a per-tenant index off a `TenantKind::Root` schema. That pairing was incidental to what they pin — `_tenant` must exist before anything references it — so they move to a child schema, which is what actually produces the index, and the root case gets its own test in each backend. Existing databases keep the stale index: the stored schema is unchanged, so the diff emits no step. The one-time SQL is in the CHANGELOG. Fixes #134
Bumps schema-forge-acton, schema-forge-backend, schema-forge-cli, and schema-forge-mssql. The release is additive: it adds a SAML 2.0 service provider behind a new `saml` feature and changes nothing in the features this workspace enables, so no code changes were needed. `saml` is not enabled here. acton-service ships the SP as a library (`SamlServiceProvider`, `SamlConfig`, replay and pending stores), not as mounted routes, so consuming it means wiring /saml/metadata, /saml/login, and /saml/acs, plumbing an `[auth.saml]` section, and deciding how an assertion maps onto a User entity and a tenant. That is a feature, not a flag, and is left for its own change.
CHANGELOG entries for #133 and #134, each with the migration note an operator needs: the status-code change from 502 to 422, and the one-time DROP INDEX / ADD CONSTRAINT that an existing database needs because the schema diff cannot see a scope change that happened in the code. The skill said `unique` was per-tenant "for `@tenant(...)` schemas", and examples.md asserted that tenant roots were "scoped by their own id, so two different organizations cannot share a slug" — the exact belief #134 disproves. Both now distinguish root from child, and the DSL and REST references gain the in-process constraint behavior.
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
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.
Closes #133. Closes #134.
Both bugs were found by an end-to-end assertion suite run against a live PostgreSQL instance, and both were verified there again after the fix.
#133 — declared constraints were never checked in-process
enum(...),text(max:), andinteger(min:/max:)reached the database unchecked, so the only thing refusing them was the generatedCHECK(or theVARCHARwidth).map_write_errorspecial-cases only SQLSTATE23505, so a23514check violation — and a22001string truncation — becameBackendError::QueryErrorand then a 502. A caller sending a bad enum variant got a retryable status for a request that could never succeed, carrying a raw driver message with no field name.FieldType::check_valuenow checks every constraint the DSL can express.bytes(max:)was already enforced in this layer and moves onto the same code path so the wording lives in one place.Two placement decisions worth reviewing:
It is not in
convert_json_with_type_hint, which is where the existingbytescheck lives and where the issue pointed. That function is shared with the query filter parser (coerce_json_filter_value), so enforcing there would reject?capacity_gte=0on aninteger(min: 1)field — a legitimate filter. The check instead runs at the last seam before the backend send.That seam is also strictly more complete. By that point the field map holds
@defaultand@computerule output and anything abefore_*hook substituted, none of which was covered by anything before this change. Only client JSON would have been guarded by a conversion-time check.FloatConstraints::precisionstays unenforced — no backend applies it (#7), so rejecting on it would be a new restriction rather than a fix.Before:
{"error":"backend_unavailable", "message":"backend unavailable: failed to create entity: error returned from database: new row for relation \"Amenity\" violates check constraint \"chk_Amenity_category_enum\""}After:
{"error":"validation_failed", "message":"validation failed: field 'category': 'not_a_category' is not one of the allowed values: furniture, technology, storage, accessibility, other"}#134 —
uniqueon a@tenant(root)schema enforced nothingis_tenanted()matchesAnnotation::Tenant(_)regardless of kind, and the planner used it for two unrelated decisions: whether the table needs a_tenantcolumn, and whether auniquefield is scoped by that column. The first is true for a root. The second is not.Scoping a root's unique to
(_tenant, field)does not weaken the constraint, it removes it._tenantis NULL on a platform-level create and PostgreSQL treats NULLs as distinct, so the index accepts every duplicate; a tenant-scoped caller instead gets their own tenant id stamped there, which partitions the constraint by whoever happened to write the row. Root rows are also the ones that most need global uniqueness — they are the tenants, so there is no outer tenant to scope them to.The decision is split out as
SchemaDefinition::unique_scoped_by_tenant, true only for@tenant(parent: ...).is_tenantedkeeps its meaning and still drives the_tenantcolumn, so the DDL-ordering fix from #56 is untouched.Emitted DDL, verified against a live database:
On the #56 tests
Both backends had an end-to-end codegen test pairing a
TenantKind::Rootschema with an assertion that the unique index is per-tenant. As suspected in #134, that pairing was incidental to what those tests pin —_tenantmust be established before anything references it. They now build a@tenant(parent:)schema, which is what actually produces the per-tenant index and therefore what actually exercises the ordering hazard, and the root case gets its own dedicated test in each backend. The #56 regression coverage is preserved, not traded away.Upgrade note — this one is not self-healing
An existing database keeps the stale index. The stored schema is unchanged, so the diff emits no step; the scope changed in the code, not in the schema. The CHANGELOG carries the one-time
DROP INDEX/ADD CONSTRAINT, along with the duplicate-detection query to run first — a deployment that has been live on the broken index may already hold rows that global uniqueness would reject, and that is a data decision rather than a migration step.acton-service 0.39.0
Bumped across
schema-forge-acton,schema-forge-backend,schema-forge-cli, andschema-forge-mssql. The release is additive — it adds a SAML 2.0 service provider behind a newsamlfeature and changes nothing in the features this workspace enables — so no code changes were required.samlis not enabled here. acton-service ships the SP as a library (SamlServiceProvider,SamlConfig, replay and pending stores), not as mounted routes, so consuming it means wiring/saml/metadata,/saml/login, and/saml/acs, plumbing an[auth.saml]section, and deciding how an assertion maps onto aUserentity and a tenant. That is a feature, not a flag, and belongs in its own change.Testing
cargo nextest run --workspace --exclude schema-forge-mssql --no-default-features --features surrealdb— 2389 passed, 0 failedcargo nextest run --workspace --exclude schema-forge-mssql --exclude schema-forge-surrealdb --no-default-features --features postgres— 2256 passed, 0 failedcargo clippy --workspace --exclude schema-forge-mssql --all-targets --no-default-features --features surrealdb -- -D warnings— cleanNew coverage: 17
check_valueunit tests in core (including thattext(max:)counts characters rather than bytes, so multi-byte text that fitsVARCHAR(n)is not rejected), 7check_field_constraintstests in the acton layer, 3 tenancy-predicate tests, 3 planner tests, and a root-scope codegen test per backend.End-to-end, against PostgreSQL 16: an external 45-assertion suite that had five
xfaillines for exactly these two defects now reports40 passed, 0 failed, 5 XPASS.Docs
The skill said
uniquewas per-tenant "for@tenant(...)schemas", andexamples.mdasserted that tenant roots were "scoped by their own id, so two different organizations cannot share a slug" — precisely the belief #134 disproves. Both now distinguish root from child. The DSL and REST references gain the in-process constraint behavior and the 422 contract.