schema validation#2403
Conversation
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 96.18% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.90% Status: PASSED ✅ Details
|
| /// which is not allowed. | ||
| #[error(transparent)] | ||
| #[diagnostic(transparent)] | ||
| EnumEntityInHierarchy(#[from] schema_errors::EnumEntityInHierarchyError), |
There was a problem hiding this comment.
Note this is new API. We could have a new error type for what try_validate, but those enums do represent "schema errors" so I think they fit here.
There was a problem hiding this comment.
Pull request overview
This PR implements ValidatorSchema::try_validate() to enforce well-formedness checks on schemas constructed programmatically (including protobuf Schema::decode), bringing validation behavior in line with schema parsing/JSON deserialization expectations.
Changes:
- Implemented
ValidatorSchema::try_validate()with explicit checks for hierarchy well-formedness, referenced types, extension types, transitive closure recomputation, and RFC 70 shadowing checks. - Improved unknown-extension-type error construction by centralizing fuzzy-suggestion generation on
UnknownExtensionTypeError. - Updated/expanded protobuf schema decoding tests to assert that invalid schemas are rejected (instead of previously succeeding due to a no-op
try_validate).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cedar-policy/src/proto/validator.rs |
Updates protobuf-schema decode tests to require rejection of invalid schemas (now that validation is implemented). |
cedar-policy-core/src/validator/schema/namespace_def.rs |
Refactors unknown-extension-type error construction to use a shared helper for suggested replacements. |
cedar-policy-core/src/validator/schema/err.rs |
Adds new schema error variants + new error types; introduces UnknownExtensionTypeError::new_with_suggestion. |
cedar-policy-core/src/validator/schema.rs |
Implements try_validate() and supporting well-formedness checks (hierarchy, references, extensions, TC recompute, RFC 70). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for descendant in action.descendants() { | ||
| if !self.action_ids.contains_key(descendant) { | ||
| return Err(ActionInvariantViolationError { | ||
| euids: NonEmpty::singleton(descendant.clone()), | ||
| } | ||
| .into()); | ||
| } |
There was a problem hiding this comment.
Will add a new enum variant, we shouldn't suggest filing an issue when protobuf decode fails.
| /// Iterate over all leaf types reachable from entity attributes, tags, and action contexts, | ||
| /// calling `visit` on each leaf type. Recursion through sets and records is handled | ||
| /// automatically. | ||
| fn for_each_type( |
There was a problem hiding this comment.
It doesn't matter too much, but I like the pattern used by ExprIterator to return an iterator instead of passing in a closure
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 92.14% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.89% Status: PASSED ✅ Details
|
| /// of an action. | ||
| #[error(transparent)] | ||
| #[diagnostic(transparent)] | ||
| UndeclaredActions(#[from] schema_errors::UndeclaredActionsError), |
There was a problem hiding this comment.
How were these issues handled before in schema validation if we don't have a schema error for them?
There was a problem hiding this comment.
There is another error type ActionNotDefined, but it's built with types available during the translation of Fragments where names are resolved on the normal parsing path.
Similarly there's already existing TypesNotDefined and UndeclaredEntityTypes variants for errors.
Another solution would be to change the underlying type of ActionNotDefined to support different ways of constructing it.
There was a problem hiding this comment.
A moral justification is that there is a difference between the two: ActionNotDefined occurs when definitions are being resolved while building the schema, while UndeclaredActions occurs when some action is used in an already built schema, but it's not declared (by this schema).
There was a problem hiding this comment.
The types should all be opaque to the public API, so supporting both kinds in ActionNotDefined sound reasonable
There was a problem hiding this comment.
Is there a DRT opportunity between the two validations: validating an already build schema vs building the same schema from fragments
| impl_diagnostic_from_method_on_nonempty_field!(types, loc); | ||
| } | ||
|
|
||
| /// Undeclared actions error: an action's descendants list references |
There was a problem hiding this comment.
Minor typo? "list references actions" -> "references actions"
Signed-off-by: Victor Nicolet <victornl@amazon.com>
Signed-off-by: Victor Nicolet <victornl@amazon.com>
| /// Returns an iterator over all leaf types reachable from entity attributes, tags, and | ||
| /// action contexts. Leaf types are all types except `Set` and `Record`, which are | ||
| /// traversed into automatically. | ||
| fn leaf_types(&self) -> TypeIterator<'_> { |
There was a problem hiding this comment.
If a type is referenced from multiple contexts, it will be iterated over it multiple times, right? I doubt it will be an issue since computing TC is probably the real bottleneck, but if performance for validation becomes an issue maybe this is an optimization we could do?
There was a problem hiding this comment.
That's correct; it's easy enough to add Itertools::unique when calling leaf_types so I'll add that and a doc comment.
victornicolet
force-pushed
the
victornicolet/schema-try-validate
branch
from
617fd74 to
ffa63fe
Compare
Signed-off-by: Victor Nicolet <victornl@amazon.com>
victornicolet
force-pushed
the
victornicolet/schema-try-validate
branch
from
ffa63fe to
0f6e539
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.14% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 87.89% Status: PASSED ✅ Details
|
Description of changes
Implements the
try_validatefunction forValidatorSchemasto enforce thatSchema::decodevalidates just as much as schema parsing or deserialization from JSON.Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):cedar-spec, and how you have tested that your updates are correct.)cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):