Skip to content

Commit 48388f2

Browse files
committed
fix newly discovered bugs in the native rule implementation.
1 parent 7c5cc3f commit 48388f2

7 files changed

Lines changed: 428 additions & 42 deletions

File tree

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

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package build.buf.protovalidate;
1616

1717
import build.buf.protovalidate.exceptions.CompilationException;
18+
import build.buf.validate.EnumRules;
1819
import build.buf.validate.FieldPath;
1920
import build.buf.validate.FieldPathElement;
2021
import build.buf.validate.FieldRules;
@@ -308,7 +309,6 @@ private void buildValue(
308309
processWrapperRules(fieldDescriptor, fieldRules, valueEvaluator);
309310
processStandardRules(fieldDescriptor, fieldRules, valueEvaluator);
310311
processAnyRules(fieldDescriptor, fieldRules, valueEvaluator);
311-
processEnumRules(fieldDescriptor, fieldRules, valueEvaluator);
312312
processMapRules(fieldDescriptor, fieldRules, valueEvaluator);
313313
processRepeatedRules(fieldDescriptor, fieldRules, valueEvaluator);
314314
}
@@ -456,7 +456,13 @@ private void processWrapperRules(
456456
ValueEvaluator unwrapped =
457457
new ValueEvaluator(
458458
valueEvaluatorEval.getDescriptor(), valueEvaluatorEval.getNestedRule());
459-
buildValue(fieldDescriptor.getMessageType().findFieldByName("value"), fieldRules, unwrapped);
459+
// Only the type rules apply to the inner value; the outer pipeline already
460+
// handled the rest (cel, cel_expression, ...), which would otherwise run twice.
461+
FieldRules innerRules =
462+
FieldRules.newBuilder()
463+
.setField(expectedWrapperDescriptor, fieldRules.getField(expectedWrapperDescriptor))
464+
.build();
465+
buildValue(fieldDescriptor.getMessageType().findFieldByName("value"), innerRules, unwrapped);
460466
valueEvaluatorEval.append(unwrapped);
461467
}
462468

@@ -466,14 +472,38 @@ private void processStandardRules(
466472

467473
// If this is a wrapper field, just return. Wrapper fields are handled by
468474
// processWrapperRules and their unwrapped values are passed through the process gauntlet.
469-
if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE) {
475+
// A list of wrappers still needs its list-level rules (min_items, unique).
476+
if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE
477+
&& (!fieldDescriptor.isRepeated() || valueEvaluatorEval.hasNestedRule())) {
470478
FieldDescriptor expectedWrapperDescriptor =
471479
DescriptorMappings.expectedWrapperRules(fieldDescriptor.getMessageType().getFullName());
472480
if (expectedWrapperDescriptor != null) {
473481
return;
474482
}
475483
}
476484

485+
// defined_only has its own evaluator; keep it in validate.proto order,
486+
// between const and the remaining enum rules.
487+
EnumRules enumRules = fieldRules.getEnum();
488+
if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM
489+
&& enumRules.getDefinedOnly()) {
490+
if (enumRules.hasConst()) {
491+
FieldRules constRules =
492+
FieldRules.newBuilder()
493+
.setEnum(EnumRules.newBuilder().setConst(enumRules.getConst()))
494+
.build();
495+
appendStandardRules(fieldDescriptor, constRules, valueEvaluatorEval);
496+
fieldRules = fieldRules.toBuilder().setEnum(enumRules.toBuilder().clearConst()).build();
497+
}
498+
valueEvaluatorEval.append(
499+
new EnumEvaluator(valueEvaluatorEval, fieldDescriptor.getEnumType().getValues()));
500+
}
501+
appendStandardRules(fieldDescriptor, fieldRules, valueEvaluatorEval);
502+
}
503+
504+
private void appendStandardRules(
505+
FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval)
506+
throws CompilationException {
477507
// Try native rule evaluators when opted in. Any rule covered natively is cleared on the
478508
// residual builder so CEL only compiles what's left; rules without a native implementation
479509
// remain on the residual and CEL handles them.
@@ -510,18 +540,6 @@ private void processAnyRules(
510540
fieldRules.getAny().getNotInList()));
511541
}
512542

513-
private void processEnumRules(
514-
FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval) {
515-
if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.ENUM) {
516-
return;
517-
}
518-
if (fieldRules.getEnum().getDefinedOnly()) {
519-
Descriptors.EnumDescriptor enumDescriptor = fieldDescriptor.getEnumType();
520-
valueEvaluatorEval.append(
521-
new EnumEvaluator(valueEvaluatorEval, enumDescriptor.getValues()));
522-
}
523-
}
524-
525543
private void processMapRules(
526544
FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval)
527545
throws CompilationException {

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,16 @@ public List<RuleViolation.Builder> evaluate(Value val, boolean failFast) {
227227
}
228228
}
229229

230+
if (lowerKind != LowerBound.NONE || upperKind != UpperBound.NONE) {
231+
RuleViolation.Builder rangeViolation = buildRangeViolation(val, actual);
232+
if (rangeViolation != null) {
233+
violations = RuleBase.add(violations, rangeViolation);
234+
if (failFast) {
235+
return base.done(violations);
236+
}
237+
}
238+
}
239+
230240
if (!inVals.isEmpty() && !containsValue(inVals, actual)) {
231241
violations =
232242
RuleBase.add(
@@ -269,16 +279,6 @@ public List<RuleViolation.Builder> evaluate(Value val, boolean failFast) {
269279
}
270280
}
271281

272-
if (lowerKind != LowerBound.NONE || upperKind != UpperBound.NONE) {
273-
RuleViolation.Builder rangeViolation = buildRangeViolation(val, actual);
274-
if (rangeViolation != null) {
275-
violations = RuleBase.add(violations, rangeViolation);
276-
if (failFast) {
277-
return base.done(violations);
278-
}
279-
}
280-
}
281-
282282
return base.done(violations);
283283
}
284284

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import dev.cel.runtime.CelRuntime.Program;
3434
import java.util.ArrayList;
3535
import java.util.Collections;
36+
import java.util.Comparator;
3637
import java.util.List;
3738
import java.util.Map;
3839
import java.util.concurrent.ConcurrentHashMap;
@@ -113,8 +114,7 @@ List<CompiledProgram> compile(
113114
}
114115
Message message = resolved.message;
115116
List<CelRule> completeProgramList = new ArrayList<>();
116-
for (Map.Entry<FieldDescriptor, Object> entry : message.getAllFields().entrySet()) {
117-
FieldDescriptor ruleFieldDesc = entry.getKey();
117+
for (FieldDescriptor ruleFieldDesc : sortedRuleFields(message)) {
118118
List<CelRule> programList =
119119
compileRule(fieldDescriptor, forItems, resolved.setOneof, ruleFieldDesc, message);
120120
if (programList == null) continue;
@@ -134,6 +134,16 @@ List<CompiledProgram> compile(
134134
return Collections.unmodifiableList(programs);
135135
}
136136

137+
// getAllFields orders by field number, but violations follow validate.proto declaration order
138+
// (string.len is field 19); extensions come last, by field number.
139+
private static List<FieldDescriptor> sortedRuleFields(Message message) {
140+
List<FieldDescriptor> fields = new ArrayList<>(message.getAllFields().keySet());
141+
fields.sort(
142+
Comparator.comparing(FieldDescriptor::isExtension)
143+
.thenComparingInt(field -> field.isExtension() ? field.getNumber() : field.getIndex()));
144+
return fields;
145+
}
146+
137147
private @Nullable List<CelRule> compileRule(
138148
FieldDescriptor fieldDescriptor,
139149
boolean forItems,

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,10 @@ private Rules() {}
6262
if (scalar == null) {
6363
return null;
6464
}
65-
// When processWrapperRules recurses with the inner "value" field, the ValueEvaluator's
66-
// descriptor is still the OUTER wrapper field. Detect that and wrap the scalar evaluator
67-
// so it unwraps the wrapper Message at evaluation time before delegating.
68-
FieldDescriptor outerDescriptor = valueEvaluator.getDescriptor();
69-
if (outerDescriptor != null
70-
&& outerDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE) {
65+
// For wrapper WKTs, fieldDescriptor is the inner "value" field but the runtime value is the
66+
// wrapper message. valueEvaluator.getDescriptor() is null for list items and map values.
67+
if (DescriptorMappings.expectedWrapperRules(fieldDescriptor.getContainingType().getFullName())
68+
!= null) {
7169
return new WrappedValueEvaluator(fieldDescriptor, scalar);
7270
}
7371
return scalar;

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,15 @@ public List<RuleViolation.Builder> evaluate(Value val, boolean failFast) {
564564
String strVal = (String) val.rawValue();
565565
List<RuleViolation.Builder> violations = null;
566566

567+
if (constVal != null && !strVal.equals(constVal)) {
568+
violations =
569+
RuleBase.add(
570+
violations,
571+
NativeViolations.newViolation(
572+
CONST_SITE, null, "must equal `" + constVal + "`", val, constVal));
573+
if (failFast) return base.done(violations);
574+
}
575+
567576
if (exactLen != null || minLen != null || maxLen != null) {
568577
long runeCount = strVal.codePointCount(0, strVal.length());
569578
violations = applyLength(violations, val, runeCount, failFast);
@@ -580,15 +589,6 @@ public List<RuleViolation.Builder> evaluate(Value val, boolean failFast) {
580589
}
581590
}
582591

583-
if (constVal != null && !strVal.equals(constVal)) {
584-
violations =
585-
RuleBase.add(
586-
violations,
587-
NativeViolations.newViolation(
588-
CONST_SITE, null, "must equal `" + constVal + "`", val, constVal));
589-
if (failFast) return base.done(violations);
590-
}
591-
592592
if (pattern != null && !pattern.matches(strVal)) {
593593
violations =
594594
RuleBase.add(

0 commit comments

Comments
 (0)