Conversation
This adds some tests around the expected behaviours of `CompilationError` and `Filter`: `CompilationErrorNoViolations` tests that we expect to get a `CompilationError` back from invalid constraints defined in the source file, in this case, a mismatch in field type and constraint type. This is currently behaving as expected and the test passes. `CompilationErrorWithViolations` tests that in the case where a `CompilationError` exists and a different field also violates the constraint, we are only returning the `CompilationError` that was found. This is behaving as expected and the test passes. `FilterIncludeCompilationError` tests that if there is a `CompilationError` on a field and we set a filter for the field with the `CompilationError`, we should get a `CompilationError` in return. This is currently not the behaviour, since we return early in the case of a `CompilationError`, and then we go to evaluate the message. Since the `CompilationError` is set on the message evaluator, and the message descriptor is not the field descriptor we are filtering for, the `CompilationError` is lost. `FilterExcludeCompilationError` tests that if a filter excludes a field with a `CompilationError`, we should still return the other violations. This is currently not behaving as expected depending on the field ordering due to the early return behaviour -- we may not have compiled the constraint for the field, and so it is not evaluated.
Instead of setting `CompilationError`s found in the build process on the mesasge evaluator, this sets them on their respective field evaluators. They are then returned when the field is evaluated. This ensures that all constraints ae built, even when a `CompilationError` is encountered. This makes the `Filter` API easier to use because then all constraints are built, and then `CompilationError`s are returned during the evaluation layer. The main change is that `CompilationError`s are no longer "fail fast" immediately, and instead allow for all constraintas to be built and cached.
|
The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).
|
Member
Author
|
This has been validated with the upgrade to |
ghost
approved these changes
Mar 17, 2025
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.
Instead of setting
CompilationErrors found in the build process on themesasge evaluator, this sets them on their respective field evaluators.
They are then returned when the field is evaluated. This ensures that
all constraints ae built, even when a
CompilationErroris encountered.This makes the
FilterAPI easier to use because then all constraintsare built, and then
CompilationErrors are returned during theevaluation layer.
The main change is that
CompilationErrors are no longer "fail fast"immediately, and instead allow for all constraints to be built and
cached.
Fixes #204