[WIP] feat: converged error taxonomy - #660
Draft
SoulPancake wants to merge 6 commits into
Draft
Conversation
Validation findings carried only a message, a slug and a source position.
Callers wanting to know what went wrong had to match on message text, and the
slug's meaning — how badly the model is affected, what part of the model the
finding is about — lived nowhere.
Add a table that decides severity, category and cause in one place for every
code the validator emits, and attach the result to each finding:
- pkg/go/errors: Severity, ModelErrorType and sentinel errors, plus an
AuthorizationModelError wrapper carrying the offending object type,
relation or condition. Shape follows openfga/openfga pkg/typesystem —
sentinels matched with errors.Is, scope reached with errors.As.
- pkg/go/validation: a package-level table maps each emitted code to its
severity, category and cause;
raise sites pass the scope they already receive, so no Raise* signature
changes. ValidationError gains Severity, Category and Unwrap, and its
Metadata type/relation/condition fields are now populated — derived from
the wrapper, so the serialised scope and the errors.As payload cannot
drift.
Exhaustiveness is derived from the source rather than a hand-written list: the
tests walk this package's AST for addError/addScopedError calls and parse the
ValidationErrorType const block, so a new Raise* method without a table
entry fails the build. Codes that are declared but never emitted are listed
separately and deliberately left unclassified.
The shared cross-language corpus in tests/data is untouched; severity and
category are additive and omitempty. Unknown codes fall back to blocking, so a
stale table can never downgrade an invalid model.
Builds on the error classification added in the previous commit, and puts it to work. Three changes: Severity now decides validity. HasErrors, Count and GetErrors on ValidationErrors consider only findings that block, and IsValid follows from them; HasFindings, CountAll and AllFindings report everything. A model carrying only warnings or advisories is valid, which is the whole reason severity exists: the weighted-graph cutover will produce findings about models that are correct today and stay correct, and reporting those as errors would fail working models. This also fixes a latent bug in the cascade. RunAllValidations gates its later phases on ErrorCollector.HasErrors, which counted every finding, so one advisory raised early would have skipped duplicate detection, entry-point, tupleset, complex-operation and wildcard validation for the entire model — hiding real errors behind a finding that says nothing is wrong. Criticality moves into the classification table. It was a second hand-maintained map in validation_engine.go, so each code had two places to be classified and no test tying them together; a code could be critical in one and non-blocking in the other. It is now a field on the entry, and a test asserts critical implies blocking. Behaviour is unchanged for all 22 emitted codes, verified against the old table; the only difference is CyclicRelation and InvalidSchemaVersion, which were listed critical there but are emitted by nothing and so never once matched. Renames the classification table from "taxonomy" to errorInfo/errorInfoByType. The word appears nowhere else in this repo and Go names tables after what they hold — stateName, statusText — rather than after the idea of classification. Severity, category and criticality fixtures go in pkg/go/validation/testdata rather than tests/data, and a test keeps them out of the shared corpus: pkg/java deserialises it with a bare YAMLMapper and no @JsonIgnoreProperties, so an unknown key is fatal there, while pkg/js casts and subset-matches and would ignore it. Documented that constraint in a header comment on the corpus, since the next person to add a field will hit it. The corpus is otherwise untouched; pkg/go and pkg/js suites both pass against it, and pkg/java could not be built here to confirm.
- make ModelErrorType a plain string type like Severity: the int enum's first value was the zero value, so json omitempty dropped "category":"object-type" from serialised findings while every other category survived; deletes the custom marshalling, the name map and the addressable-constant workaround for category overrides - populate metadata.offendingType for invalid-relation-type with the enclosing type, matching the reference implementation's output; the raise site received the value and discarded it - attach the object type to reserved-relation-keywords findings, and render relation-scoped errors without an object type as "relation 'x'" rather than naming an empty type - drop a category override on invalid-type that restated the table default - add a regression test asserting every category survives serialisation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- switch error assertions in the classification tests to require and assert.Error, per testifylint's require-error and error-nil rules - npm audit fix for fast-uri and js-yaml advisories in pkg/js
The validator reports a malformed schema version as invalid-schema with the message "invalid schema 0.9"; pkg/js and pkg/java have no invalid-schema-version member at all, and the shared corpus asserts invalid-schema for that case. So the two pages have their content swapped relative to the implementation: invalid-schema-version.md accurately describes what is emitted under the wrong code, and invalid-schema.md documents structural cases that fail during DSL transformation and therefore carry no error code, line range or metadata. Note both on the pages and leave the resolution as a TODO, since it has to be settled before these codes are published at a stable public URL.
Category is a real enum on the Go side and still serialises as its name. The constants count from iota + 1, so the zero value is not a category and a finding that never set one is dropped by omitempty rather than reported as an object-type finding. MarshalText and UnmarshalText map each value to its wire name, since encoding/json consults neither String nor a package-level map: without them the field would ship as 1-5 to every consumer of the validation JSON. A value with no name fails to marshal, and an unrecognised name fails to decode, so neither direction can invent a category. Adds a relation-condition fixture case, which nothing covered before.
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.
Adds severity, category and cause classification to
pkg/go/validationfindings, via a newpkg/go/errorspackage.Description
What problem is being solved?
pkg/go/validationreported findings as a message, a kebab-case slug and a source position. Three things were missing:errors.Istarget — callers wanting to know what went wrong had to match message text.ErrorMetadata'sType/Relation/Conditionfields existed but nothing populated them.len(errors) > 0.So a consumer (CLI, VS Code extension,
frontend-utils) cannot ask "what specifically is wrong with this model, and does it actually block?" without parsing English.How is it being solved?
New
pkg/go/errorspackage with an error vocabulary that mirrorspkg/typesystem/error.goinopenfga/openfga(five sentinel names match the server exactly):Severitytype (error/warning/advisory) with aBlocks()predicate.ModelErrorTypecategory — a plain string type with hyphenated wire names, so it serialises without custom marshalling and its zero value means "unset".AuthorizationModelError— a typed wrapper carrying the object type / relation / condition a finding is about, implementingUnwrap.The vocabulary originates on the
feat/language-new-versionbranch. Two enum constants are renamed relative to it for consistency with the other three (ErrorRelationCondition→ErrorTypeRelationCondition,ErrorCondition→ErrorTypeCondition), and constructors return the concrete*AuthorizationModelErrorrather thanerror. That branch is unmerged, so nothing on develop consumed the old names.One classification table in
pkg/go/validation:error_info.goholdserrorInfoByType(22 entries) mapping each emitted code to severity, category, cause and aCriticalflag. Every raise site attaches all four, and serialised metadata is derived from the wrapped cause, so the JSON scope and theerrors.Aspayload cannot drift.What changes are made to solve it?
Type/Relation/Conditioncome from the wrapper, andoffendingTypeis emitted for type-restriction findings, matching whatpkg/jsalready puts on the wire for the same cases.HasErrors,Count,GetErrorsandIsValidconsider only blocking findings;AllFindings,HasFindings,CountAllreport everything. Nothing emits a non-blocking severity yet, soCount() == CountAll()today — a test asserts exactly that.criticalErrorTypesmap invalidation_engine.gois folded into the same table;TestCriticalityMatchesTheReplacedTableembeds the old map verbatim and proves the fold-in changes no behaviour for any code anything emits.RunAllValidationsgated later phases on "no errors so far", counting every finding — once anything non-blocking could be raised, a single advisory would have skipped duplicate detection, entry-point, tupleset, complex-operation and wildcard validation for the whole model. The gate now counts blocking findings only.pkg/go/validation/testdata/severity-category-cases.yaml, not the shared corpus. That split is measured:pkg/javadeserialisestests/data/dsl-semantic-validation-cases.yamlwith a bareYAMLMapperand no@JsonIgnoreProperties, so an unknown key at any nesting level fails./gradlew testwithUnrecognizedPropertyException(verified at case level, insideexpected_errors, and insidemetadata). A guard test keeps Go-only keys out of the shared file; the corpus itself gains only a header comment documenting the constraint.Behaviour is unchanged for existing consumers:
SeverityError, soCount() == CountAll()and every verdict is what it was.severity,category,offendingType) are additive andomitempty; a regression test serialises a finding from every category and asserts none drops the field.pkg/go/graphuntouched.Out of scope (recorded, deliberately not fixed here):
RaiseInvalidSchemaVersiontags its findinginvalid-schema, leaving theinvalid-schema-versionslug unreachable; fixing it means editing the shared corpus, which is cross-language work.Testing
From
pkg/go, on a cold cache:go build ./... && go vet ./... && go test ./... -count=1green (vet noise is the four generatedgen/openfga*.gofiles, same as develop), andgolangci-lint run -c .golangci.yaml ./...clean.Contract tests:
TestRealValidationStillFails(the no-behaviour-change proof),TestCriticalityMatchesTheReplacedTable,TestCascadeGateIgnoresNonBlockingFindings,TestEverySemanticFindingCarriesErrorInfo,TestSeverityFixtures,TestSeverityFixturesAreNotInTheSharedCorpus,TestCategorySerialisesForEveryCategory.Exhaustiveness is derived from the source: tests walk the package AST for
addError/addScopedErrorcalls and parse the const block, so an unclassified new code fails the build.Cross-language:
pkg/js666 passing (lockfile refreshed for two npm audit advisories published since develop's last green run),pkg/java373 passing against the edited corpus header.References
Review Checklist
main