Repository navigation
Keep back-references to message evaluators that are still being built - #543
Conversation
…done before they are actually completed. Also don't recurse into google.protobuf.Struct, Value, and ListValue. Signed-off-by: Jon Bodner <[email protected]>
…done before they are actually completed. Also don't recurse into google.protobuf.Struct, Value, and ListValue. Signed-off-by: Jon Bodner <[email protected]>
pkwarren
left a comment
pkwarren
left a comment
There was a problem hiding this comment.
Good catch. Just a couple of comments on the tests.
| Descriptor a = types.get("A"); | ||
| Descriptor b = types.get("B"); | ||
| Validator validator = ValidatorFactory.newBuilder().build(); | ||
| assertThat(validator.validate(nestedA(a, b, 2, 5)).getViolations()).hasSize(2); |
There was a problem hiding this comment.
As a reader the structure of these tests isn't clear since we're building messages dynamically. It would be easier to follow if we created .proto files under src/test/resources.
Additionally, beyond just the size if would be a stronger assertion if we verified the field paths of violations.
There was a problem hiding this comment.
Done. Most of the dynamic types are now defined in a .proto. The ones that have invalid protovalidate rules and override the google.protobuf.* types are still dynamic.
| public void testStructTypesAreTautologies() throws Exception { | ||
| assertThat(isTautology(Struct.getDescriptor())).isTrue(); | ||
| assertThat(isTautology(Value.getDescriptor())).isTrue(); | ||
| assertThat(isTautology(ListValue.getDescriptor())).isTrue(); |
There was a problem hiding this comment.
In addition to testing these types on their own should we test that a message that contains three fields of these types also returns true?
| FieldRules.getDescriptor().findFieldByNumber(FieldRules.CEL_EXPRESSION_FIELD_NUMBER)); | ||
|
|
||
| private static final Set<String> STRUCT_TYPE_NAMES = | ||
| new HashSet<>( |
There was a problem hiding this comment.
For a constant we should wrap with Collections.unmodifiableSet.
|
|
||
| @Test | ||
| @Timeout(value = 15, unit = TimeUnit.SECONDS) | ||
| public void testSharedChainBuildsInLinearTime() throws Exception { |
There was a problem hiding this comment.
Can you describe what this test is protecting against? Is this something that was failing prior to this fix?
There was a problem hiding this comment.
Yeah, this test wasn't needed. I'm removing it.
Signed-off-by: Jon Bodner <[email protected]>
Recursive and mutually recursive MessageEvaluators were treated as already processed and dropped before they finished being evaluated. This led to dropping rules that were reachable through them.
Now a MessageEvaluator only reports that it is processed after it is fully built. Also google.protobuf.Struct, Value, and ListValue are always pruned unless a non-standard descriptor adds rules to them.