Skip to content

Commit fac49b4

Browse files
jrinehart-bufjrinehart-buf
authored
Jrinehart/tcn 2782 protovalidate java custom rules cannot use repeated nested (#246)
`protovalidate-java` cannot use a CEL expression where `this` represents a repeated message. `EvaluatorBuilder`'s `processFieldExpression` is currently directly using `Decls.newObjectType()` to create an object type based on the name of the message type in the field instead of honoring whether or not the field is repeated and returning a `Decls.newListType()`. `provalidate` [recently added](bufbuild/protovalidate#329) conformance tests that illustrate this problem. To fix this, this PR: 1. Adds JUnit tests and `.proto` that demonstrate the issue. 2. Updates `EvaluatorBuilder`'s `processFieldExpression` to delegate the determination of the CEL type to `DescriptorMappings.getCELType()`, which uses the `FieldDescriptor` to determine the correct type to return. This method requires passing a `forItems` flag stating if the type returned should be the list (or map) type for a field or the type of the items in the collection. Using the [new `nestedRule` property](#215 state provides the value of this flag, [similar to how it's used in `processStandardConstraints`](https://github.com/bufbuild/protovalidate-java/blob/b3cd73121d04182dc989b76902e468f94b5f0b99/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java#L421) to pass the `forItems` flag to `constraintCache.compile()`. 3. Because there were now 7 places where `valueEvaluator.getNestedRule() == null` was used to determine behavior, I moved this comparison into a `hasNestedRule()` method on `ValueEvaluator`, using conformance state to check regression. Conformance notes: This PR passes conformance in `protovalidate-java`. It has been tested against the conformance suite at the current head of `protovalidate` (which now includes the standard library, so there are many new failures). Before changes in this PR, there were 137 failures. After, there were 135: ``` BEFORE: FAIL (failed: 137, skipped: 0, passed: 2681, total: 2818) AFTER : FAIL (failed: 135, skipped: 0, passed: 2683, total: 2818) ``` Running `field_expression/repeated/message` shows that the two conformance tests fixed were for the desired issue: ``` BEFORE: FAIL (failed: 2, skipped: 0, passed: 2, total: 4) AFTER: PASS (failed: 0, skipped: 0, passed: 4, total: 4) ```
1 parent 78c55f0 commit fac49b4

File tree

4 files changed

+213
-7
lines changed

4 files changed

+213
-7
lines changed

‎src/main/java/build/buf/protovalidate/EvaluatorBuilder.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ private void processIgnoreEmpty(
270270
FieldConstraints fieldConstraints,
271271
ValueEvaluator valueEvaluatorEval)
272272
throws CompilationException {
273-
if (valueEvaluatorEval.getNestedRule() != null && shouldIgnoreEmpty(fieldConstraints)) {
273+
if (valueEvaluatorEval.hasNestedRule() && shouldIgnoreEmpty(fieldConstraints)) {
274274
valueEvaluatorEval.setIgnoreEmpty(zeroValue(fieldDescriptor, true));
275275
}
276276
}
@@ -348,7 +348,8 @@ private void processFieldExpressions(
348348
EnvOption.declarations(
349349
Decls.newVar(
350350
Variable.THIS_NAME,
351-
Decls.newObjectType(fieldDescriptor.getMessageType().getFullName()))));
351+
DescriptorMappings.getCELType(
352+
fieldDescriptor, valueEvaluatorEval.hasNestedRule()))));
352353
} catch (InvalidProtocolBufferException e) {
353354
throw new CompilationException("field descriptor type is invalid " + e.getMessage(), e);
354355
}
@@ -376,7 +377,7 @@ private void processEmbeddedMessage(
376377
if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE
377378
|| shouldSkip(fieldConstraints)
378379
|| fieldDescriptor.isMapField()
379-
|| (fieldDescriptor.isRepeated() && valueEvaluatorEval.getNestedRule() == null)) {
380+
|| (fieldDescriptor.isRepeated() && !valueEvaluatorEval.hasNestedRule())) {
380381
return;
381382
}
382383
Evaluator embedEval =
@@ -393,7 +394,7 @@ private void processWrapperConstraints(
393394
if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE
394395
|| shouldSkip(fieldConstraints)
395396
|| fieldDescriptor.isMapField()
396-
|| (fieldDescriptor.isRepeated() && valueEvaluatorEval.getNestedRule() == null)) {
397+
|| (fieldDescriptor.isRepeated() && !valueEvaluatorEval.hasNestedRule())) {
397398
return;
398399
}
399400
FieldDescriptor expectedWrapperDescriptor =
@@ -418,7 +419,7 @@ private void processStandardConstraints(
418419
throws CompilationException {
419420
List<CompiledProgram> compile =
420421
constraintCache.compile(
421-
fieldDescriptor, fieldConstraints, valueEvaluatorEval.getNestedRule() != null);
422+
fieldDescriptor, fieldConstraints, valueEvaluatorEval.hasNestedRule());
422423
if (compile.isEmpty()) {
423424
return;
424425
}
@@ -429,7 +430,7 @@ private void processAnyConstraints(
429430
FieldDescriptor fieldDescriptor,
430431
FieldConstraints fieldConstraints,
431432
ValueEvaluator valueEvaluatorEval) {
432-
if ((fieldDescriptor.isRepeated() && valueEvaluatorEval.getNestedRule() == null)
433+
if ((fieldDescriptor.isRepeated() && !valueEvaluatorEval.hasNestedRule())
433434
|| fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE
434435
|| !fieldDescriptor.getMessageType().getFullName().equals("google.protobuf.Any")) {
435436
return;
@@ -484,7 +485,7 @@ private void processRepeatedConstraints(
484485
throws CompilationException {
485486
if (fieldDescriptor.isMapField()
486487
|| !fieldDescriptor.isRepeated()
487-
|| valueEvaluatorEval.getNestedRule() != null) {
488+
|| valueEvaluatorEval.hasNestedRule()) {
488489
return;
489490
}
490491
ListEvaluator listEval = new ListEvaluator(valueEvaluatorEval);

‎src/main/java/build/buf/protovalidate/ValueEvaluator.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ class ValueEvaluator implements Evaluator {
5959
return nestedRule;
6060
}
6161

62+
public boolean hasNestedRule() {
63+
return this.nestedRule != null;
64+
}
65+
6266
@Override
6367
public boolean tautology() {
6468
return evaluators.isEmpty();

‎src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
// Copyright 2023-2024 Buf Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package build.buf.protovalidate;
16+
17+
import static org.assertj.core.api.Assertions.assertThat;
18+
19+
import build.buf.validate.FieldConstraints;
20+
import build.buf.validate.FieldPath;
21+
import build.buf.validate.Violation;
22+
import com.example.imports.buf.validate.RepeatedRules;
23+
import java.util.Arrays;
24+
import org.junit.jupiter.api.Test;
25+
26+
/**
27+
* This test verifies that custom (CEL-based) field and/or message constraints evaluate as expected.
28+
*/
29+
public class ValidatorCelExpressionTest {
30+
31+
@Test
32+
public void testFieldExpressionRepeatedMessage() throws Exception {
33+
// Nested message wrapping the int 1
34+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.Msg one =
35+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.Msg.newBuilder()
36+
.setA(1)
37+
.build();
38+
39+
// Nested message wrapping the int 2
40+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.Msg two =
41+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.Msg.newBuilder()
42+
.setA(2)
43+
.build();
44+
45+
// Create a valid message (1, 1)
46+
com.example.imports.validationtest.FieldExpressionRepeatedMessage validMsg =
47+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.newBuilder()
48+
.addAllVal(Arrays.asList(one, one))
49+
.build();
50+
51+
// Create an invalid message (1, 2, 1)
52+
com.example.imports.validationtest.FieldExpressionRepeatedMessage invalidMsg =
53+
com.example.imports.validationtest.FieldExpressionRepeatedMessage.newBuilder()
54+
.addAllVal(Arrays.asList(one, two, one))
55+
.build();
56+
57+
// Build a model of the expected violation
58+
Violation expectedViolation =
59+
Violation.newBuilder()
60+
.setField(
61+
FieldPath.newBuilder()
62+
.addElements(
63+
FieldPathUtils.fieldPathElement(
64+
invalidMsg.getDescriptorForType().findFieldByName("val"))
65+
.toBuilder()
66+
.build()))
67+
.setRule(
68+
FieldPath.newBuilder()
69+
.addElements(
70+
FieldPathUtils.fieldPathElement(
71+
FieldConstraints.getDescriptor()
72+
.findFieldByNumber(FieldConstraints.CEL_FIELD_NUMBER))
73+
.toBuilder()
74+
.setIndex(0)
75+
.build()))
76+
.setConstraintId("field_expression.repeated.message")
77+
.setMessage("test message field_expression.repeated.message")
78+
.build();
79+
80+
Validator validator = new Validator();
81+
82+
// Valid message checks
83+
ValidationResult validResult = validator.validate(validMsg);
84+
assertThat(validResult.isSuccess()).isTrue();
85+
86+
// Invalid message checks
87+
ValidationResult invalidResult = validator.validate(invalidMsg);
88+
assertThat(invalidResult.isSuccess()).isFalse();
89+
assertThat(invalidResult.toProto().getViolationsList()).containsExactly(expectedViolation);
90+
}
91+
92+
@Test
93+
public void testFieldExpressionRepeatedMessageItems() throws Exception {
94+
// Nested message wrapping the int 1
95+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.Msg one =
96+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.Msg.newBuilder()
97+
.setA(1)
98+
.build();
99+
100+
// Nested message wrapping the int 2
101+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.Msg two =
102+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.Msg.newBuilder()
103+
.setA(2)
104+
.build();
105+
106+
// Create a valid message (1, 1)
107+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems validMsg =
108+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.newBuilder()
109+
.addAllVal(Arrays.asList(one, one))
110+
.build();
111+
112+
// Create an invalid message (1, 2, 1)
113+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems invalidMsg =
114+
com.example.imports.validationtest.FieldExpressionRepeatedMessageItems.newBuilder()
115+
.addAllVal(Arrays.asList(one, two, one))
116+
.build();
117+
118+
// Build a model of the expected violation
119+
Violation expectedViolation =
120+
Violation.newBuilder()
121+
.setField(
122+
FieldPath.newBuilder()
123+
.addElements(
124+
FieldPathUtils.fieldPathElement(
125+
invalidMsg.getDescriptorForType().findFieldByName("val"))
126+
.toBuilder()
127+
.setIndex(1)
128+
.build()))
129+
.setRule(
130+
FieldPath.newBuilder()
131+
.addElements(
132+
FieldPathUtils.fieldPathElement(
133+
FieldConstraints.getDescriptor()
134+
.findFieldByNumber(FieldConstraints.REPEATED_FIELD_NUMBER)))
135+
.addElements(
136+
FieldPathUtils.fieldPathElement(
137+
RepeatedRules.getDescriptor().findFieldByName("items")))
138+
.addElements(
139+
FieldPathUtils.fieldPathElement(
140+
FieldConstraints.getDescriptor()
141+
.findFieldByNumber(FieldConstraints.CEL_FIELD_NUMBER))
142+
.toBuilder()
143+
.setIndex(0)
144+
.build()))
145+
.setConstraintId("field_expression.repeated.message.items")
146+
.setMessage("test message field_expression.repeated.message.items")
147+
.build();
148+
149+
Validator validator = new Validator();
150+
151+
// Valid message checks
152+
ValidationResult validResult = validator.validate(validMsg);
153+
assertThat(validResult.isSuccess()).isTrue();
154+
155+
// Invalid message checks
156+
ValidationResult invalidResult = validator.validate(invalidMsg);
157+
assertThat(invalidResult.isSuccess()).isFalse();
158+
assertThat(invalidResult.toProto().getViolationsList()).containsExactly(expectedViolation);
159+
}
160+
}

‎src/test/resources/proto/validationtest/custom_constraints.proto‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Copyright 2023-2024 Buf Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
syntax = "proto3";
16+
17+
package validationtest;
18+
19+
import "buf/validate/validate.proto";
20+
21+
message FieldExpressionRepeatedMessage {
22+
repeated Msg val = 1 [(buf.validate.field).cel = {
23+
id: "field_expression.repeated.message"
24+
message: "test message field_expression.repeated.message"
25+
expression: "this.all(e, e.a == 1)"
26+
}];
27+
message Msg {
28+
int32 a = 1;
29+
}
30+
}
31+
32+
message FieldExpressionRepeatedMessageItems {
33+
repeated Msg val = 1 [(buf.validate.field).repeated.items.cel = {
34+
id: "field_expression.repeated.message.items"
35+
message: "test message field_expression.repeated.message.items"
36+
expression: "this.a == 1"
37+
}];
38+
message Msg {
39+
int32 a = 1;
40+
}
41+
}

0 commit comments

Comments
 (0)