test(authz): compiler property fuzz and generative persist suite - #758
test(authz): compiler property fuzz and generative persist suite#758adlerhurst wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
cursor
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a layered, confidence-building test strategy for the authz stack (parser → profile/compiler → storage persistence) ahead of the eventual resolver/check API, including new fuzz/property suites and a dialect-agnostic persist round-trip test harness.
Changes:
- Introduces shared
internal/authz/authztestmodel generators (mixed + valid-only) and compiler invariants tests (seeded + fuzz). - Adds
LoadCatalogMutationsplus per-dialect implementations to read back persisted catalog rows for round-trip verification. - Documents the L1–L4 authz testing approach and wires the new doc into the existing design doc index.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/stmttest/authz_generative_test.go | New generative persist round-trip integration test across dialects. |
| internal/storage/v2/dialect/sqlite/authz_catalog.go | Implements LoadCatalogMutations for SQLite. |
| internal/storage/v2/dialect/spanner/authz_catalog.go | Implements LoadCatalogMutations for Spanner. |
| internal/storage/v2/dialect/postgres/authz_catalog.go | Implements LoadCatalogMutations for Postgres. |
| internal/storage/v2/dialect/authz/catalog_rows.go | Adds parsing for persisted expression-edge kind strings. |
| internal/service/statement.go | Extends AuthzCatalogStatements with LoadCatalogMutations. |
| internal/service/mocks/statement.mock.go | Updates mocks for the new statements method. |
| internal/authz/openfga/fuzz_test.go | Adds crash fuzzing for DSL/JSON parsing. |
| internal/authz/compiler/output.go | Introduces PersistedCatalog + CatalogMutations.Persisted() helper. |
| internal/authz/compiler/invariants_test.go | Adds seeded compiler/profile invariants suite. |
| internal/authz/compiler/fuzz_test.go | Adds fuzz entrypoint reusing the invariants oracle. |
| internal/authz/authztest/recipes.go | Recipe tables for generator rewrites/refs and model defects. |
| internal/authz/authztest/generate.go | Deterministic model generators for mixed + valid-only modes. |
| internal/authz/authztest/generate_valid_test.go | Locks “valid-only generator always compiles” contract. |
| docs/design/api/README.md | Links in the new authz-testing design note. |
| docs/design/api/permission-storage.md | Adds authz-testing as a related design doc. |
| docs/design/api/authz-testing.md | New design note describing the layered fuzzy/property strategy. |
Files not reviewed (1)
- internal/service/mocks/statement.mock.go: Generated file
livio-a
left a comment
livio-a
left a comment
There was a problem hiding this comment.
Review: compiler property fuzz + generative persist suite — reviewed as top of the #666 → #677 → #758 stack.
These are genuine, well-oracled tests — not vacuous. The invariant suite asserts several independent structural properties (reflexive/same-object closure, dense edge/reference positions, plan↔edge consistency, no dangling targets), and determinism is handled carefully throughout: FNV-seeded generators, a reproducible fuzz corpus via Seed(i), catalog-order closure emission, and order-independent ElementsMatch in persist assertions. I found nothing that would flake in CI. Three things worth addressing:
[MEDIUM] The closure-depth oracle is a line-for-line copy of the production algorithm.
invariants_test.go allShortestDepths is byte-for-byte identical to production internal/authz/compiler/closure.go shortestDepths (same init, same queue, same relaxation), so it cannot falsify a depth bug — a shared BFS error passes both. Concretely, this is why nothing in the suite catches the #677 seed discrepancy where viewer→admin is hand-seeded at depth 1 but the compiler computes depth 2. Suggest an independent oracle (e.g. hand-written expected depths for the fixture models).
[MEDIUM] Generative persist round-trip never exercises the non-trivial readback columns.
GenerateValidModel restricts to refPlain (valid recipes never emit TTU/wildcard/condition), so the round-trip in authz_generative_test.go never persists a tuple_to_userset edge or a wildcard/condition reference. The new LoadCatalogMutations code that scans ref_relation/wildcard/condition and reconstructs TTU Source/Tupleset is therefore never verified end-to-end — a swapped/broken wildcard or condition scan would pass. (Computed-userset Source round-trip is covered.)
[MEDIUM / scope] ~440 lines of production readback code ship in a test-labeled PR with no product caller.
LoadCatalogMutations (postgres/spanner/sqlite), compiler.PersistedCatalog + Persisted(), the new interface method on statement.go, ParseExpressionEdgeKind, and the regenerated mocks are all non-test production surface. There's no product caller (the doc + statement.go acknowledge #423 brings it), and its only exercise is the *_integration-gated generative test — so go test ./... gives it zero coverage. The code itself looks correct, but consider whether it should land with its #423 caller instead of riding here.
Lower severity
- [LOW]
openfga/fuzz_test.gocrash-fuzz discards both returns (_, _ = ParseDSL(input)) — pure no-panic oracle. Matches the doc's L1 intent, but the equivalence property the doc (authz-testing.mdL1) also advertises isn't implemented. - [LOW] PG/SQLite catalog-exists checks use
err == pgx.ErrNoRows/err == sql.ErrNoRowsrather thanerrors.Is— brittle if the sentinel is ever wrapped. (Spanner mapscodes.NotFound, so it's fine.) - [NIT]
assertPlansMatchEdgescomparesedgesByTarget[target][i]positionally without re-sorting byPosition(unlikeassertDenseEdgePositions) — deterministic today, fragile if emission order ever diverges from plan-term order.
Generator bias and determinism otherwise check out — TestGenerateValidModelAlwaysCompiles (500 seeds) guards against trivially-empty catalogs, and the rich invariants still run on real shapes via the curated fixtures.
Rebase PR tip onto squashed Wave 0/1 main: keep recipe-table generators, L1/L2 fuzz, generative persist via LoadCatalogMutations → PersistedCatalog, and authz-testing docs without replaying duplicate Wave 1 history. Co-authored-by: Silvan <adlerhurst@users.noreply.github.com>
e495163 to
5b1cc60
Compare
Summary
Confidence-building tests for the authz stack before the grants/check API / resolver (#423).
Design
docs/design/api/authz-testing.md— L1 crash fuzz → L2 compiler invariants → L3 DB persist → L4 resolver oracle.L1/L2
authztestgenerator with named recipe tables (GenerateModelmixed /GenerateValidModelvalid-only)FuzzCompileInvariants/ parse fuzzTestGenerateValidModelAlwaysCompiles(500 seeds)L3
GenerateValidModel(no soft-skip)compiler.PersistedCatalog+LoadCatalogMutations(thin wrappers overGetAuthzCatalog+PersistedCatalogFromDomain)stmttest.TestPersistCatalogVersion_GenerativeRebuilt onto
mainafter Wave 0/1 squash merges so duplicate Wave 1 history no longer conflicts.Validation
go test ./internal/authz/...go test -tags sqlite_integration ./internal/storage/v2/stmttest/ -run 'Generative|Authz|Catalog' -count=1Release notes / changeset
No changeset required — docs/tests (+ test-oriented catalog read unused by product paths yet).
Notes