From 48388f2adcee9e4ff9fd036067ed0118812dd798 Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Thu, 1 Oct 2026 15:30:31 -0400 Subject: [PATCH] fix newly discovered bugs in the native rule implementation. --- .../buf/protovalidate/EvaluatorBuilder.java | 48 ++-- .../protovalidate/NumericRulesEvaluator.java | 20 +- .../build/buf/protovalidate/RuleCache.java | 14 +- .../java/build/buf/protovalidate/Rules.java | 10 +- .../protovalidate/StringRulesEvaluator.java | 18 +- .../NativeRulesRegressionTest.java | 252 ++++++++++++++++++ .../proto/validationtest/validationtest.proto | 108 ++++++++ 7 files changed, 428 insertions(+), 42 deletions(-) create mode 100644 src/test/java/build/buf/protovalidate/NativeRulesRegressionTest.java diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index 00348f4cb..2c5fe74cf 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -15,6 +15,7 @@ package build.buf.protovalidate; import build.buf.protovalidate.exceptions.CompilationException; +import build.buf.validate.EnumRules; import build.buf.validate.FieldPath; import build.buf.validate.FieldPathElement; import build.buf.validate.FieldRules; @@ -308,7 +309,6 @@ private void buildValue( processWrapperRules(fieldDescriptor, fieldRules, valueEvaluator); processStandardRules(fieldDescriptor, fieldRules, valueEvaluator); processAnyRules(fieldDescriptor, fieldRules, valueEvaluator); - processEnumRules(fieldDescriptor, fieldRules, valueEvaluator); processMapRules(fieldDescriptor, fieldRules, valueEvaluator); processRepeatedRules(fieldDescriptor, fieldRules, valueEvaluator); } @@ -456,7 +456,13 @@ private void processWrapperRules( ValueEvaluator unwrapped = new ValueEvaluator( valueEvaluatorEval.getDescriptor(), valueEvaluatorEval.getNestedRule()); - buildValue(fieldDescriptor.getMessageType().findFieldByName("value"), fieldRules, unwrapped); + // Only the type rules apply to the inner value; the outer pipeline already + // handled the rest (cel, cel_expression, ...), which would otherwise run twice. + FieldRules innerRules = + FieldRules.newBuilder() + .setField(expectedWrapperDescriptor, fieldRules.getField(expectedWrapperDescriptor)) + .build(); + buildValue(fieldDescriptor.getMessageType().findFieldByName("value"), innerRules, unwrapped); valueEvaluatorEval.append(unwrapped); } @@ -466,7 +472,9 @@ private void processStandardRules( // If this is a wrapper field, just return. Wrapper fields are handled by // processWrapperRules and their unwrapped values are passed through the process gauntlet. - if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE) { + // A list of wrappers still needs its list-level rules (min_items, unique). + if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE + && (!fieldDescriptor.isRepeated() || valueEvaluatorEval.hasNestedRule())) { FieldDescriptor expectedWrapperDescriptor = DescriptorMappings.expectedWrapperRules(fieldDescriptor.getMessageType().getFullName()); if (expectedWrapperDescriptor != null) { @@ -474,6 +482,28 @@ private void processStandardRules( } } + // defined_only has its own evaluator; keep it in validate.proto order, + // between const and the remaining enum rules. + EnumRules enumRules = fieldRules.getEnum(); + if (fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM + && enumRules.getDefinedOnly()) { + if (enumRules.hasConst()) { + FieldRules constRules = + FieldRules.newBuilder() + .setEnum(EnumRules.newBuilder().setConst(enumRules.getConst())) + .build(); + appendStandardRules(fieldDescriptor, constRules, valueEvaluatorEval); + fieldRules = fieldRules.toBuilder().setEnum(enumRules.toBuilder().clearConst()).build(); + } + valueEvaluatorEval.append( + new EnumEvaluator(valueEvaluatorEval, fieldDescriptor.getEnumType().getValues())); + } + appendStandardRules(fieldDescriptor, fieldRules, valueEvaluatorEval); + } + + private void appendStandardRules( + FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval) + throws CompilationException { // Try native rule evaluators when opted in. Any rule covered natively is cleared on the // residual builder so CEL only compiles what's left; rules without a native implementation // remain on the residual and CEL handles them. @@ -510,18 +540,6 @@ private void processAnyRules( fieldRules.getAny().getNotInList())); } - private void processEnumRules( - FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval) { - if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.ENUM) { - return; - } - if (fieldRules.getEnum().getDefinedOnly()) { - Descriptors.EnumDescriptor enumDescriptor = fieldDescriptor.getEnumType(); - valueEvaluatorEval.append( - new EnumEvaluator(valueEvaluatorEval, enumDescriptor.getValues())); - } - } - private void processMapRules( FieldDescriptor fieldDescriptor, FieldRules fieldRules, ValueEvaluator valueEvaluatorEval) throws CompilationException { diff --git a/src/main/java/build/buf/protovalidate/NumericRulesEvaluator.java b/src/main/java/build/buf/protovalidate/NumericRulesEvaluator.java index 8e81cd951..1c182e347 100644 --- a/src/main/java/build/buf/protovalidate/NumericRulesEvaluator.java +++ b/src/main/java/build/buf/protovalidate/NumericRulesEvaluator.java @@ -227,6 +227,16 @@ public List evaluate(Value val, boolean failFast) { } } + if (lowerKind != LowerBound.NONE || upperKind != UpperBound.NONE) { + RuleViolation.Builder rangeViolation = buildRangeViolation(val, actual); + if (rangeViolation != null) { + violations = RuleBase.add(violations, rangeViolation); + if (failFast) { + return base.done(violations); + } + } + } + if (!inVals.isEmpty() && !containsValue(inVals, actual)) { violations = RuleBase.add( @@ -269,16 +279,6 @@ public List evaluate(Value val, boolean failFast) { } } - if (lowerKind != LowerBound.NONE || upperKind != UpperBound.NONE) { - RuleViolation.Builder rangeViolation = buildRangeViolation(val, actual); - if (rangeViolation != null) { - violations = RuleBase.add(violations, rangeViolation); - if (failFast) { - return base.done(violations); - } - } - } - return base.done(violations); } diff --git a/src/main/java/build/buf/protovalidate/RuleCache.java b/src/main/java/build/buf/protovalidate/RuleCache.java index 9e6962bfd..665bbad76 100644 --- a/src/main/java/build/buf/protovalidate/RuleCache.java +++ b/src/main/java/build/buf/protovalidate/RuleCache.java @@ -33,6 +33,7 @@ import dev.cel.runtime.CelRuntime.Program; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -113,8 +114,7 @@ List compile( } Message message = resolved.message; List completeProgramList = new ArrayList<>(); - for (Map.Entry entry : message.getAllFields().entrySet()) { - FieldDescriptor ruleFieldDesc = entry.getKey(); + for (FieldDescriptor ruleFieldDesc : sortedRuleFields(message)) { List programList = compileRule(fieldDescriptor, forItems, resolved.setOneof, ruleFieldDesc, message); if (programList == null) continue; @@ -134,6 +134,16 @@ List compile( return Collections.unmodifiableList(programs); } + // getAllFields orders by field number, but violations follow validate.proto declaration order + // (string.len is field 19); extensions come last, by field number. + private static List sortedRuleFields(Message message) { + List fields = new ArrayList<>(message.getAllFields().keySet()); + fields.sort( + Comparator.comparing(FieldDescriptor::isExtension) + .thenComparingInt(field -> field.isExtension() ? field.getNumber() : field.getIndex())); + return fields; + } + private @Nullable List compileRule( FieldDescriptor fieldDescriptor, boolean forItems, diff --git a/src/main/java/build/buf/protovalidate/Rules.java b/src/main/java/build/buf/protovalidate/Rules.java index 9f824bb3a..ee0127dba 100644 --- a/src/main/java/build/buf/protovalidate/Rules.java +++ b/src/main/java/build/buf/protovalidate/Rules.java @@ -62,12 +62,10 @@ private Rules() {} if (scalar == null) { return null; } - // When processWrapperRules recurses with the inner "value" field, the ValueEvaluator's - // descriptor is still the OUTER wrapper field. Detect that and wrap the scalar evaluator - // so it unwraps the wrapper Message at evaluation time before delegating. - FieldDescriptor outerDescriptor = valueEvaluator.getDescriptor(); - if (outerDescriptor != null - && outerDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE) { + // For wrapper WKTs, fieldDescriptor is the inner "value" field but the runtime value is the + // wrapper message. valueEvaluator.getDescriptor() is null for list items and map values. + if (DescriptorMappings.expectedWrapperRules(fieldDescriptor.getContainingType().getFullName()) + != null) { return new WrappedValueEvaluator(fieldDescriptor, scalar); } return scalar; diff --git a/src/main/java/build/buf/protovalidate/StringRulesEvaluator.java b/src/main/java/build/buf/protovalidate/StringRulesEvaluator.java index 5decf32fa..72f933ed8 100644 --- a/src/main/java/build/buf/protovalidate/StringRulesEvaluator.java +++ b/src/main/java/build/buf/protovalidate/StringRulesEvaluator.java @@ -564,6 +564,15 @@ public List evaluate(Value val, boolean failFast) { String strVal = (String) val.rawValue(); List violations = null; + if (constVal != null && !strVal.equals(constVal)) { + violations = + RuleBase.add( + violations, + NativeViolations.newViolation( + CONST_SITE, null, "must equal `" + constVal + "`", val, constVal)); + if (failFast) return base.done(violations); + } + if (exactLen != null || minLen != null || maxLen != null) { long runeCount = strVal.codePointCount(0, strVal.length()); violations = applyLength(violations, val, runeCount, failFast); @@ -580,15 +589,6 @@ public List evaluate(Value val, boolean failFast) { } } - if (constVal != null && !strVal.equals(constVal)) { - violations = - RuleBase.add( - violations, - NativeViolations.newViolation( - CONST_SITE, null, "must equal `" + constVal + "`", val, constVal)); - if (failFast) return base.done(violations); - } - if (pattern != null && !pattern.matches(strVal)) { violations = RuleBase.add( diff --git a/src/test/java/build/buf/protovalidate/NativeRulesRegressionTest.java b/src/test/java/build/buf/protovalidate/NativeRulesRegressionTest.java new file mode 100644 index 000000000..637cf62bc --- /dev/null +++ b/src/test/java/build/buf/protovalidate/NativeRulesRegressionTest.java @@ -0,0 +1,252 @@ +// Copyright 2023-2026 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package build.buf.protovalidate; + +import static org.assertj.core.api.Assertions.assertThat; + +import build.buf.protovalidate.exceptions.ValidationException; +import com.example.noimports.validationtest.DoubleRuleOrder; +import com.example.noimports.validationtest.EnumConstDefinedOnlyOrder; +import com.example.noimports.validationtest.EnumRuleOrder; +import com.example.noimports.validationtest.Int32RuleOrder; +import com.example.noimports.validationtest.MapStringWrapperValues; +import com.example.noimports.validationtest.MapWrapperValues; +import com.example.noimports.validationtest.MinItemsWrappers; +import com.example.noimports.validationtest.RepeatedWrapperItems; +import com.example.noimports.validationtest.RepeatedWrapperStandardAndCustom; +import com.example.noimports.validationtest.StringRuleOrder; +import com.example.noimports.validationtest.UniqueBytesWrappers; +import com.example.noimports.validationtest.UniqueWrappers; +import com.example.noimports.validationtest.WrapperCustomOnly; +import com.example.noimports.validationtest.WrapperStandardAndCelExpression; +import com.example.noimports.validationtest.WrapperStandardAndCustom; +import com.google.protobuf.ByteString; +import com.google.protobuf.BytesValue; +import com.google.protobuf.DynamicMessage; +import com.google.protobuf.Int32Value; +import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.Message; +import com.google.protobuf.StringValue; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Regression tests for wrapper handling and violation ordering. Every case runs with native rules + * enabled and disabled; both paths must agree. + */ +class NativeRulesRegressionTest { + + private static Validator validator(boolean nativeRules, boolean failFast) { + Config config = + Config.newBuilder().setEnableNativeRules(nativeRules).setFailFast(failFast).build(); + return ValidatorFactory.newBuilder().withConfig(config).build(); + } + + private static List ruleIds(ValidationResult result) { + return result.getViolations().stream() + .map(v -> v.toProto().getRuleId()) + .collect(Collectors.toList()); + } + + private static List int32s(int... vals) { + return Arrays.stream(vals).mapToObj(Int32Value::of).collect(Collectors.toList()); + } + + private static Message toDynamic(Message msg) throws InvalidProtocolBufferException { + return DynamicMessage.parseFrom(msg.getDescriptorForType(), msg.toByteString()); + } + + static Stream wrapperCases() { + return Stream.of( + Arguments.of( + "repeated_items/invalid", + RepeatedWrapperItems.newBuilder().addAllVal(int32s(5)).build(), + "int32.gt", + "val[0]"), + Arguments.of( + "repeated_items/valid", + RepeatedWrapperItems.newBuilder().addAllVal(int32s(100)).build(), + null, + null), + Arguments.of( + "map_int32/invalid", + MapWrapperValues.newBuilder().putVal("key", Int32Value.of(5)).build(), + "int32.gt", + "val[\"key\"]"), + Arguments.of( + "map_int32/valid", + MapWrapperValues.newBuilder().putVal("key", Int32Value.of(100)).build(), + null, + null), + Arguments.of( + "map_string/invalid", + MapStringWrapperValues.newBuilder().putVal("k", StringValue.of("a")).build(), + "string.min_len", + "val[\"k\"]"), + Arguments.of( + "map_string/valid", + MapStringWrapperValues.newBuilder().putVal("k", StringValue.of("abc")).build(), + null, + null), + Arguments.of( + "min_items/invalid", + MinItemsWrappers.newBuilder().build(), + "repeated.min_items", + "val"), + Arguments.of( + "min_items/valid", + MinItemsWrappers.newBuilder().addAllVal(int32s(1, 2)).build(), + null, + null), + Arguments.of( + "unique/invalid", + UniqueWrappers.newBuilder().addAllVal(int32s(1, 1)).build(), + "repeated.unique", + "val"), + Arguments.of( + "unique/valid", + UniqueWrappers.newBuilder().addAllVal(int32s(1, 2)).build(), + null, + null), + Arguments.of( + "unique_bytes/invalid", + UniqueBytesWrappers.newBuilder() + .addVal(BytesValue.of(ByteString.copyFromUtf8("a"))) + .addVal(BytesValue.of(ByteString.copyFromUtf8("a"))) + .build(), + "repeated.unique", + "val"), + Arguments.of( + "unique_bytes/valid", + UniqueBytesWrappers.newBuilder() + .addVal(BytesValue.of(ByteString.copyFromUtf8("a"))) + .addVal(BytesValue.of(ByteString.copyFromUtf8("b"))) + .build(), + null, + null)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("wrapperCases") + void wrappersInCollections( + String name, Message msg, @Nullable String wantRule, @Nullable String wantPath) + throws Exception { + for (Message candidate : Arrays.asList(msg, toDynamic(msg))) { + ValidationResult nativeResult = validator(true, false).validate(candidate); + ValidationResult celResult = validator(false, false).validate(candidate); + if (wantRule == null) { + assertThat(nativeResult.isSuccess()).isTrue(); + assertThat(celResult.isSuccess()).isTrue(); + continue; + } + assertThat(nativeResult.getViolations()).hasSize(1); + build.buf.validate.Violation violation = nativeResult.getViolations().get(0).toProto(); + assertThat(violation.getRuleId()).isEqualTo(wantRule); + assertThat(FieldPathUtils.fieldPathString(violation.getField())).isEqualTo(wantPath); + assertThat(nativeResult.toProto()).isEqualTo(celResult.toProto()); + } + } + + static Stream wrapperRulesNotDuplicatedCases() { + return Stream.of( + Arguments.of( + "standard_and_custom/both_fail", + WrapperStandardAndCustom.newBuilder().setVal(Int32Value.of(5)).build(), + Arrays.asList("custom", "int32.gt")), + Arguments.of( + "standard_and_custom/custom_fails", + WrapperStandardAndCustom.newBuilder().setVal(Int32Value.of(50)).build(), + Arrays.asList("custom")), + Arguments.of( + "custom_only", + WrapperCustomOnly.newBuilder().setVal(Int32Value.of(5)).build(), + Arrays.asList("custom")), + Arguments.of( + "repeated_items/both_fail", + RepeatedWrapperStandardAndCustom.newBuilder().addAllVal(int32s(5)).build(), + Arrays.asList("custom", "int32.gt")), + Arguments.of( + "standard_and_cel_expression/both_fail", + WrapperStandardAndCelExpression.newBuilder().setVal(Int32Value.of(5)).build(), + Arrays.asList("this > 100 ? '' : 'must be greater than 100'", "int32.gt"))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("wrapperRulesNotDuplicatedCases") + void wrapperFieldRulesNotDuplicated(String name, Message msg, List want) + throws ValidationException { + for (boolean nativeRules : new boolean[] {true, false}) { + assertThat(ruleIds(validator(nativeRules, false).validate(msg))) + .as("nativeRules=%s", nativeRules) + .isEqualTo(want); + } + } + + static Stream violationOrderCases() { + return Stream.of( + Arguments.of( + "int32", + Int32RuleOrder.newBuilder().setVal(3).build(), + Arrays.asList("int32.gt", "int32.in", "int32.not_in")), + Arguments.of( + "double", + DoubleRuleOrder.newBuilder().setVal(Double.NEGATIVE_INFINITY).build(), + Arrays.asList("double.gt", "double.in", "double.not_in", "double.finite")), + Arguments.of( + "string", + StringRuleOrder.newBuilder().setVal("x").build(), + Arrays.asList( + "string.const", "string.len", "string.min_bytes", "string.prefix", "string.in")), + Arguments.of( + "enum", + EnumRuleOrder.newBuilder().setValValue(99).build(), + Arrays.asList("enum.defined_only", "enum.in", "enum.not_in")), + Arguments.of( + "enum_const", + EnumConstDefinedOnlyOrder.newBuilder().setValValue(99).build(), + Arrays.asList("enum.const", "enum.defined_only", "enum.in"))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("violationOrderCases") + void violationsFollowValidateProtoOrder(String name, Message msg, List want) + throws Exception { + for (boolean nativeRules : new boolean[] {true, false}) { + for (Message candidate : Arrays.asList(msg, toDynamic(msg))) { + assertThat(ruleIds(validator(nativeRules, false).validate(candidate))) + .as("nativeRules=%s", nativeRules) + .isEqualTo(want); + assertThat(ruleIds(validator(nativeRules, true).validate(candidate))) + .as("nativeRules=%s failFast", nativeRules) + .containsExactly(want.get(0)); + } + } + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void violationOrderMatchesBetweenNativeAndCel(boolean failFast) throws ValidationException { + StringRuleOrder msg = StringRuleOrder.newBuilder().setVal("x").build(); + assertThat(validator(true, failFast).validate(msg).toProto()) + .isEqualTo(validator(false, failFast).validate(msg).toProto()); + } +} diff --git a/src/test/resources/proto/validationtest/validationtest.proto b/src/test/resources/proto/validationtest/validationtest.proto index 65b6d07de..2173eabc0 100644 --- a/src/test/resources/proto/validationtest/validationtest.proto +++ b/src/test/resources/proto/validationtest/validationtest.proto @@ -386,3 +386,111 @@ message Int64WrapperConst { message StringWrapperLen { google.protobuf.StringValue val = 1 [(buf.validate.field).string.min_len = 3]; } + +message RepeatedWrapperItems { + repeated google.protobuf.Int32Value val = 1 [(buf.validate.field).repeated.items.int32.gt = 10]; +} + +message MapWrapperValues { + map val = 1 [(buf.validate.field).map.values.int32.gt = 10]; +} + +message MapStringWrapperValues { + map val = 1 [(buf.validate.field).map.values.string.min_len = 3]; +} + +message UniqueWrappers { + repeated google.protobuf.Int32Value val = 1 [(buf.validate.field).repeated.unique = true]; +} + +message MinItemsWrappers { + repeated google.protobuf.Int32Value val = 1 [(buf.validate.field).repeated.min_items = 2]; +} + +message UniqueBytesWrappers { + repeated google.protobuf.BytesValue val = 1 [(buf.validate.field).repeated.unique = true]; +} + +message Int32RuleOrder { + int32 val = 1 [(buf.validate.field).int32 = { + gt: 10 + in: [1] + not_in: [3] + }]; +} + +message DoubleRuleOrder { + double val = 1 [(buf.validate.field).double = { + gt: 10 + in: [1] + not_in: [-inf] + finite: true + }]; +} + +message StringRuleOrder { + // Combines const with other rules to check violation order. + // buf:lint:ignore PROTOVALIDATE + string val = 1 [(buf.validate.field).string = { + const: "hello" + len: 5 + min_bytes: 4 + prefix: "h" + in: ["hello"] + }]; +} + +message EnumRuleOrder { + ExampleColor val = 1 [(buf.validate.field).enum = { + defined_only: true + in: [1] + not_in: [99] + }]; +} + +message EnumConstDefinedOnlyOrder { + // Combines const with other rules to check violation order. + // buf:lint:ignore PROTOVALIDATE + ExampleColor val = 1 [(buf.validate.field).enum = { + const: 1 + defined_only: true + in: [1] + }]; +} + +message WrapperStandardAndCustom { + google.protobuf.Int32Value val = 1 [(buf.validate.field) = { + int32: {gt: 10} + cel: { + id: "custom" + message: "must be greater than 100" + expression: "this > 100" + } + }]; +} + +message WrapperCustomOnly { + google.protobuf.Int32Value val = 1 [(buf.validate.field).cel = { + id: "custom" + message: "must be greater than 100" + expression: "this > 100" + }]; +} + +message WrapperStandardAndCelExpression { + google.protobuf.Int32Value val = 1 [(buf.validate.field) = { + int32: {gt: 10} + cel_expression: "this > 100 ? '' : 'must be greater than 100'" + }]; +} + +message RepeatedWrapperStandardAndCustom { + repeated google.protobuf.Int32Value val = 1 [(buf.validate.field).repeated.items = { + int32: {gt: 10} + cel: { + id: "custom" + message: "must be greater than 100" + expression: "this > 100" + } + }]; +}