From a779982cc50cee3158d24cd98718ee57eb31dc95 Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Wed, 21 Oct 2026 18:08:23 -0400 Subject: [PATCH 1/3] Prevent recursive and mutually recursive messages from evaluating as done before they are actually completed. Also don't recurse into google.protobuf.Struct, Value, and ListValue. Signed-off-by: Jon Bodner --- .../protovalidate/RecursiveMessageTest.java | 276 ++++++++++++++++++ 1 file changed, 276 insertions(+) create mode 100644 src/test/java/build/buf/protovalidate/RecursiveMessageTest.java diff --git a/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java new file mode 100644 index 00000000..653fbff9 --- /dev/null +++ b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java @@ -0,0 +1,276 @@ +// 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.validate.FieldRules; +import build.buf.validate.Int32Rules; +import build.buf.validate.RepeatedRules; +import build.buf.validate.StringRules; +import build.buf.validate.ValidateProto; +import com.google.protobuf.DescriptorProtos.DescriptorProto; +import com.google.protobuf.DescriptorProtos.FieldDescriptorProto; +import com.google.protobuf.DescriptorProtos.FieldOptions; +import com.google.protobuf.DescriptorProtos.FileDescriptorProto; +import com.google.protobuf.Descriptors.Descriptor; +import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.protobuf.DynamicMessage; +import com.google.protobuf.ListValue; +import com.google.protobuf.Struct; +import com.google.protobuf.Value; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +/** + * Message evaluators are cached before they are built so recursive references can resolve to them. + * These tests cover references made while the referenced evaluator is still incomplete. + */ +public class RecursiveMessageTest { + private static final FieldOptions INT32_GT_100 = + rules(FieldRules.newBuilder().setInt32(Int32Rules.newBuilder().setGt(100))); + + @Test + public void testMutuallyRecursiveMessagesValidateEveryDepth() throws Exception { + Map types = mutuallyRecursiveTypes(); + 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); + assertThat(validator.validate(nestedA(a, b, 3, 5)).getViolations()).hasSize(3); + } + + @Test + public void testMutuallyRecursiveMessagesValidateFromEitherRoot() throws Exception { + Map types = mutuallyRecursiveTypes(); + Descriptor a = types.get("A"); + Descriptor b = types.get("B"); + DynamicMessage root = + DynamicMessage.newBuilder(b) + .setField(b.findFieldByName("v"), 5) + .setField(b.findFieldByName("a"), nestedA(a, b, 1, 5)) + .build(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(root).getViolations()).hasSize(2); + } + + @Test + public void testMutuallyRecursiveMessagesAcceptValidValues() throws Exception { + Map types = mutuallyRecursiveTypes(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(nestedA(types.get("A"), types.get("B"), 2, 101)).isSuccess()) + .isTrue(); + } + + @Test + public void testSelfRecursiveFieldDeclaredBeforeRule() throws Exception { + Descriptor node = + compile( + "recursive", + DescriptorProto.newBuilder() + .setName("Node") + .addField(messageField("next", 1, ".recursive.Node")) + .addField( + field("v", 2, FieldDescriptorProto.Type.TYPE_INT32) + .setOptions(INT32_GT_100))) + .get("Node"); + DynamicMessage inner = + DynamicMessage.newBuilder(node).setField(node.findFieldByName("v"), 5).build(); + DynamicMessage root = + DynamicMessage.newBuilder(node) + .setField(node.findFieldByName("v"), 5) + .setField(node.findFieldByName("next"), inner) + .build(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(root).getViolations()).hasSize(2); + } + + @Test + public void testMessageWithoutRulesIsTautology() throws Exception { + Map types = + compile( + "recursive", + DescriptorProto.newBuilder() + .setName("Mid") + .addField(messageField("leaf", 1, ".recursive.Leaf")) + .addField(field("s", 2, FieldDescriptorProto.Type.TYPE_STRING)), + DescriptorProto.newBuilder() + .setName("Leaf") + .addField(field("v", 1, FieldDescriptorProto.Type.TYPE_INT32))); + assertThat(isTautology(types.get("Mid"))).isTrue(); + } + + @Test + public void testRuleFreeCycleIsKept() throws Exception { + Map types = + compile( + "recursive", + DescriptorProto.newBuilder() + .setName("Root") + .addField( + field("name", 1, FieldDescriptorProto.Type.TYPE_STRING) + .setOptions( + rules( + FieldRules.newBuilder() + .setString(StringRules.newBuilder().setMinLen(1))))) + .addField(messageField("node", 2, ".recursive.Node")), + DescriptorProto.newBuilder() + .setName("Node") + .addField(messageField("next", 1, ".recursive.Node"))); + Descriptor root = types.get("Root"); + Descriptor node = types.get("Node"); + // Detecting that a cycle has no rules isn't worth the complexity, so it is walked. + assertThat(isTautology(node)).isFalse(); + DynamicMessage message = + DynamicMessage.newBuilder(root) + .setField(root.findFieldByName("node"), DynamicMessage.getDefaultInstance(node)) + .build(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(message).getViolations()).hasSize(1); + } + + @Test + public void testStructTypesAreTautologies() throws Exception { + assertThat(isTautology(Struct.getDescriptor())).isTrue(); + assertThat(isTautology(Value.getDescriptor())).isTrue(); + assertThat(isTautology(ListValue.getDescriptor())).isTrue(); + } + + @Test + public void testNonStandardListValueWithRulesIsValidated() throws Exception { + Map types = + compile( + "google.protobuf", + DescriptorProto.newBuilder() + .setName("Value") + .addField(field("string_value", 3, FieldDescriptorProto.Type.TYPE_STRING)), + DescriptorProto.newBuilder() + .setName("ListValue") + .addField( + messageField("values", 1, ".google.protobuf.Value") + .setLabel(FieldDescriptorProto.Label.LABEL_REPEATED) + .setOptions( + rules( + FieldRules.newBuilder() + .setRepeated(RepeatedRules.newBuilder().setMaxItems(1))))), + DescriptorProto.newBuilder() + .setName("Root") + .addField(messageField("list", 1, ".google.protobuf.ListValue"))); + Descriptor value = types.get("Value"); + Descriptor listValue = types.get("ListValue"); + Descriptor root = types.get("Root"); + DynamicMessage item = + DynamicMessage.newBuilder(value) + .setField(value.findFieldByName("string_value"), "x") + .build(); + DynamicMessage list = + DynamicMessage.newBuilder(listValue) + .addRepeatedField(listValue.findFieldByName("values"), item) + .addRepeatedField(listValue.findFieldByName("values"), item) + .build(); + DynamicMessage message = + DynamicMessage.newBuilder(root).setField(root.findFieldByName("list"), list).build(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(message).getViolations()).hasSize(1); + } + + @Test + @Timeout(value = 15, unit = TimeUnit.SECONDS) + public void testSharedChainBuildsInLinearTime() throws Exception { + // Each M{i} has two required fields of type M{i+1}, so descending into shared evaluators + // when checking for tautologies would do 2^n work. + int n = 26; + DescriptorProto.Builder[] messages = new DescriptorProto.Builder[n + 1]; + FieldOptions required = rules(FieldRules.newBuilder().setRequired(true)); + for (int i = 0; i < n; i++) { + String next = ".recursive.M" + (i + 1); + messages[i] = + DescriptorProto.newBuilder() + .setName("M" + i) + .addField(messageField("a", 1, next).setOptions(required)) + .addField(messageField("b", 2, next).setOptions(required)); + } + messages[n] = DescriptorProto.newBuilder().setName("M" + n); + Descriptor m0 = compile("recursive", messages).get("M0"); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThat(validator.validate(DynamicMessage.getDefaultInstance(m0)).getViolations()) + .hasSize(2); + } + + private static Map mutuallyRecursiveTypes() throws Exception { + return compile( + "recursive", + DescriptorProto.newBuilder().setName("A").addField(messageField("b", 1, ".recursive.B")), + DescriptorProto.newBuilder() + .setName("B") + .addField(messageField("a", 1, ".recursive.A")) + .addField( + field("v", 2, FieldDescriptorProto.Type.TYPE_INT32).setOptions(INT32_GT_100))); + } + + /** Returns an A whose chain of B messages is {@code depth} long, each with v set to {@code v}. */ + private static DynamicMessage nestedA(Descriptor a, Descriptor b, int depth, int v) { + DynamicMessage.Builder bBuilder = + DynamicMessage.newBuilder(b).setField(b.findFieldByName("v"), v); + if (depth > 1) { + bBuilder.setField(b.findFieldByName("a"), nestedA(a, b, depth - 1, v)); + } + return DynamicMessage.newBuilder(a).setField(a.findFieldByName("b"), bBuilder.build()).build(); + } + + private static boolean isTautology(Descriptor descriptor) throws Exception { + EvaluatorBuilder builder = + new EvaluatorBuilder(ValidateLibrary.newCel(), Config.newBuilder().build()); + return builder.load(descriptor).tautology(); + } + + private static FieldDescriptorProto.Builder field( + String name, int number, FieldDescriptorProto.Type type) { + return FieldDescriptorProto.newBuilder().setName(name).setNumber(number).setType(type); + } + + private static FieldDescriptorProto.Builder messageField( + String name, int number, String typeName) { + return field(name, number, FieldDescriptorProto.Type.TYPE_MESSAGE).setTypeName(typeName); + } + + private static FieldOptions rules(FieldRules.Builder rules) { + return FieldOptions.newBuilder().setExtension(ValidateProto.field, rules.build()).build(); + } + + private static Map compile(String pkg, DescriptorProto.Builder... messages) + throws Exception { + FileDescriptorProto.Builder file = + FileDescriptorProto.newBuilder() + .setName("recursive_test.proto") + .setPackage(pkg) + .setSyntax("proto3") + .addDependency("buf/validate/validate.proto"); + for (DescriptorProto.Builder message : messages) { + file.addMessageType(message); + } + FileDescriptor descriptor = + FileDescriptor.buildFrom( + file.build(), new FileDescriptor[] {ValidateProto.getDescriptor()}); + Map types = new HashMap<>(); + for (Descriptor message : descriptor.getMessageTypes()) { + types.put(message.getName(), message); + } + return types; + } +} From afe89e67be74317d43e28b6a3a7d0264a3015082 Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Wed, 21 Oct 2026 18:09:42 -0400 Subject: [PATCH 2/3] Prevent recursive and mutually recursive messages from evaluating as done before they are actually completed. Also don't recurse into google.protobuf.Struct, Value, and ListValue. Signed-off-by: Jon Bodner --- .../buf/protovalidate/EvaluatorBuilder.java | 61 +++++++++++++++++- .../buf/protovalidate/MessageEvaluator.java | 14 +++++ .../protovalidate/RecursiveMessageTest.java | 62 +++++++++++++++++++ 3 files changed, 136 insertions(+), 1 deletion(-) diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index 2c5fe74c..ce8673ee 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -35,9 +35,13 @@ import dev.cel.common.types.StructTypeReference; import dev.cel.common.values.CelByteString; import dev.cel.runtime.CelEvaluationException; +import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; +import java.util.Deque; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -57,6 +61,11 @@ final class EvaluatorBuilder { FieldPathUtils.fieldPathElement( FieldRules.getDescriptor().findFieldByNumber(FieldRules.CEL_EXPRESSION_FIELD_NUMBER)); + private static final Set STRUCT_TYPE_NAMES = + new HashSet<>( + Arrays.asList( + "google.protobuf.Struct", "google.protobuf.Value", "google.protobuf.ListValue")); + private volatile Map evaluatorCache = Collections.emptyMap(); private final Cel cel; @@ -182,10 +191,60 @@ private MessageEvaluator createMessageEvaluator(Descriptor desc) throws Compilat } MessageEvaluator msgEval = new MessageEvaluator(); cache.put(desc, msgEval); - buildMessage(desc, msgEval); + if (!isRuleFreeStructType(desc)) { + buildMessage(desc, msgEval); + } + msgEval.finishBuilding(); return msgEval; } + /** + * Struct, Value and ListValue form a rule-free cycle, which would otherwise be walked on every + * validation. Treat them as tautologies unless a non-standard descriptor adds rules. + */ + private boolean isRuleFreeStructType(Descriptor desc) throws CompilationException { + try { + Set seen = new HashSet<>(); + Deque pending = new ArrayDeque<>(); + pending.push(desc); + while (!pending.isEmpty()) { + Descriptor message = pending.pop(); + if (!seen.add(message)) { + continue; + } + if (!STRUCT_TYPE_NAMES.contains(message.getFullName()) + || !resolver.resolveMessageRules(message).equals(MessageRules.getDefaultInstance())) { + return false; + } + for (Descriptors.OneofDescriptor oneof : message.getOneofs()) { + if (!resolver.resolveOneofRules(oneof).equals(OneofRules.getDefaultInstance())) { + return false; + } + } + for (FieldDescriptor field : message.getFields()) { + if (!resolver.resolveFieldRules(field).equals(FieldRules.getDefaultInstance())) { + return false; + } + if (field.getJavaType() != FieldDescriptor.JavaType.MESSAGE) { + continue; + } + if (field.isMapField()) { + FieldDescriptor mapValue = field.getMessageType().findFieldByNumber(2); + if (mapValue.getJavaType() == FieldDescriptor.JavaType.MESSAGE) { + pending.push(mapValue.getMessageType()); + } + } else { + pending.push(field.getMessageType()); + } + } + } + return true; + } catch (InvalidProtocolBufferException e) { + throw new CompilationException( + "failed to parse proto definition: " + desc.getFullName(), e); + } + } + private void buildMessage(Descriptor desc, MessageEvaluator msgEval) throws CompilationException { try { diff --git a/src/main/java/build/buf/protovalidate/MessageEvaluator.java b/src/main/java/build/buf/protovalidate/MessageEvaluator.java index 150cae21..265d8227 100644 --- a/src/main/java/build/buf/protovalidate/MessageEvaluator.java +++ b/src/main/java/build/buf/protovalidate/MessageEvaluator.java @@ -23,8 +23,22 @@ final class MessageEvaluator implements Evaluator { /** List of {@link Evaluator}s that are applied to a message. */ private final List evaluators = new ArrayList<>(); + /** + * A recursive message type references its own evaluator before that evaluator is complete. Until + * then it must not look like a tautology, or the back-reference would be dropped for good. + */ + private boolean building = true; + + /** Marks the evaluator as fully built, so {@link #tautology()} can report an accurate result. */ + void finishBuilding() { + building = false; + } + @Override public boolean tautology() { + if (building) { + return false; + } for (Evaluator evaluator : evaluators) { if (!evaluator.tautology()) { return false; diff --git a/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java index 653fbff9..55672801 100644 --- a/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java +++ b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java @@ -15,10 +15,13 @@ package build.buf.protovalidate; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import build.buf.protovalidate.exceptions.CompilationException; import build.buf.validate.FieldRules; import build.buf.validate.Int32Rules; import build.buf.validate.RepeatedRules; +import build.buf.validate.Rule; import build.buf.validate.StringRules; import build.buf.validate.ValidateProto; import com.google.protobuf.DescriptorProtos.DescriptorProto; @@ -212,6 +215,65 @@ public void testSharedChainBuildsInLinearTime() throws Exception { .hasSize(2); } + @Test + public void testCompilationErrorReportedWhenInnerIsBuiltFirst() throws Exception { + Map types = compilationErrorTypes(); + Validator validator = ValidatorFactory.newBuilder().build(); + assertThatThrownBy(() -> validator.validate(innerWithOuter(types))) + .isInstanceOf(CompilationException.class); + } + + @Test + public void testCompilationErrorReportedForInnerAfterOuterFailed() throws Exception { + Map types = compilationErrorTypes(); + Validator validator = ValidatorFactory.newBuilder().build(); + DynamicMessage outer = DynamicMessage.getDefaultInstance(types.get("Outer")); + assertThatThrownBy(() -> validator.validate(outer)).isInstanceOf(CompilationException.class); + assertThatThrownBy(() -> validator.validate(innerWithOuter(types))) + .isInstanceOf(CompilationException.class); + } + + @Test + public void testCompilationErrorReportedForOuterOnEveryValidation() throws Exception { + Map types = compilationErrorTypes(); + Validator validator = ValidatorFactory.newBuilder().build(); + DynamicMessage outer = DynamicMessage.getDefaultInstance(types.get("Outer")); + for (int i = 0; i < 2; i++) { + assertThatThrownBy(() -> validator.validate(outer)).isInstanceOf(CompilationException.class); + } + } + + /** + * Outer's invalid rule is on a field after its Inner field, so building Outer fails after Inner + * has been built against the incomplete Outer evaluator. + */ + private static Map compilationErrorTypes() throws Exception { + return compile( + "recursive", + DescriptorProto.newBuilder() + .setName("Outer") + .addField(messageField("inner", 1, ".recursive.Inner")) + .addField( + field("s", 2, FieldDescriptorProto.Type.TYPE_STRING) + .setOptions( + rules( + FieldRules.newBuilder() + .addCel(Rule.newBuilder().setId("bad").setExpression("this +"))))), + DescriptorProto.newBuilder() + .setName("Inner") + .addField( + messageField("outer", 1, ".recursive.Outer") + .setOptions(rules(FieldRules.newBuilder().setRequired(true))))); + } + + private static DynamicMessage innerWithOuter(Map types) { + Descriptor inner = types.get("Inner"); + return DynamicMessage.newBuilder(inner) + .setField( + inner.findFieldByName("outer"), DynamicMessage.getDefaultInstance(types.get("Outer"))) + .build(); + } + private static Map mutuallyRecursiveTypes() throws Exception { return compile( "recursive", From 3f5d28214d95f781b0775427db0e57f18d9fda7f Mon Sep 17 00:00:00 2001 From: Jon Bodner Date: Thu, 22 Oct 2026 19:08:07 -0400 Subject: [PATCH 3/3] update tests in response to code review feedback Signed-off-by: Jon Bodner --- .../buf/protovalidate/EvaluatorBuilder.java | 7 +- .../protovalidate/RecursiveMessageTest.java | 198 ++++++++---------- .../proto/validationtest/recursive.proto | 59 ++++++ 3 files changed, 146 insertions(+), 118 deletions(-) create mode 100644 src/test/resources/proto/validationtest/recursive.proto diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index ce8673ee..9ef0c48d 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -62,9 +62,10 @@ final class EvaluatorBuilder { FieldRules.getDescriptor().findFieldByNumber(FieldRules.CEL_EXPRESSION_FIELD_NUMBER)); private static final Set STRUCT_TYPE_NAMES = - new HashSet<>( - Arrays.asList( - "google.protobuf.Struct", "google.protobuf.Value", "google.protobuf.ListValue")); + Collections.unmodifiableSet( + new HashSet<>( + Arrays.asList( + "google.protobuf.Struct", "google.protobuf.Value", "google.protobuf.ListValue"))); private volatile Map evaluatorCache = Collections.emptyMap(); diff --git a/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java index 55672801..d495f87c 100644 --- a/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java +++ b/src/test/java/build/buf/protovalidate/RecursiveMessageTest.java @@ -19,11 +19,16 @@ import build.buf.protovalidate.exceptions.CompilationException; import build.buf.validate.FieldRules; -import build.buf.validate.Int32Rules; import build.buf.validate.RepeatedRules; import build.buf.validate.Rule; -import build.buf.validate.StringRules; import build.buf.validate.ValidateProto; +import com.example.imports.validationtest.HasGoogleFields; +import com.example.imports.validationtest.NoRuleParent; +import com.example.imports.validationtest.Node; +import com.example.imports.validationtest.RecursiveA; +import com.example.imports.validationtest.RecursiveB; +import com.example.imports.validationtest.Root; +import com.example.imports.validationtest.SelfRecursive; import com.google.protobuf.DescriptorProtos.DescriptorProto; import com.google.protobuf.DescriptorProtos.FieldDescriptorProto; import com.google.protobuf.DescriptorProtos.FieldOptions; @@ -35,116 +40,106 @@ import com.google.protobuf.Struct; import com.google.protobuf.Value; import java.util.HashMap; +import java.util.List; import java.util.Map; -import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.Timeout; /** * Message evaluators are cached before they are built so recursive references can resolve to them. * These tests cover references made while the referenced evaluator is still incomplete. */ public class RecursiveMessageTest { - private static final FieldOptions INT32_GT_100 = - rules(FieldRules.newBuilder().setInt32(Int32Rules.newBuilder().setGt(100))); - @Test public void testMutuallyRecursiveMessagesValidateEveryDepth() throws Exception { - Map types = mutuallyRecursiveTypes(); - 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); - assertThat(validator.validate(nestedA(a, b, 3, 5)).getViolations()).hasSize(3); + + // depth 2 + List violations = validator.validate(nestedA(2, 5)).getViolations(); + assertThat(violations).hasSize(2); + assertThat(violations) + .extracting(violation -> FieldPathUtils.fieldPathString(violation.toProto().getField())) + .containsExactlyInAnyOrder("b.v", "b.a.b.v"); + for (Violation violation : violations) { + assertThat(violation.getRuleValue().getDescriptor().getFullName()) + .isEqualTo("buf.validate.Int32Rules.gt"); + assertThat(violation.getRuleValue().getValue()).isEqualTo(100); + } + + // depth 3 also + List violations1 = validator.validate(nestedA(3, 5)).getViolations(); + assertThat(violations1).hasSize(3); + assertThat(violations1) + .extracting(violation -> FieldPathUtils.fieldPathString(violation.toProto().getField())) + .containsExactlyInAnyOrder("b.v", "b.a.b.v", "b.a.b.a.b.v"); + for (Violation violation : violations1) { + assertThat(violation.getRuleValue().getDescriptor().getFullName()) + .isEqualTo("buf.validate.Int32Rules.gt"); + assertThat(violation.getRuleValue().getValue()).isEqualTo(100); + } } @Test public void testMutuallyRecursiveMessagesValidateFromEitherRoot() throws Exception { - Map types = mutuallyRecursiveTypes(); - Descriptor a = types.get("A"); - Descriptor b = types.get("B"); - DynamicMessage root = - DynamicMessage.newBuilder(b) - .setField(b.findFieldByName("v"), 5) - .setField(b.findFieldByName("a"), nestedA(a, b, 1, 5)) - .build(); + RecursiveB recursiveB = RecursiveB.newBuilder().setV(5).setA(nestedA(1, 5)).build(); Validator validator = ValidatorFactory.newBuilder().build(); - assertThat(validator.validate(root).getViolations()).hasSize(2); + List violations = validator.validate(recursiveB).getViolations(); + assertThat(violations).hasSize(2); + assertThat(violations) + .extracting(violation -> FieldPathUtils.fieldPathString(violation.toProto().getField())) + .containsExactlyInAnyOrder("v", "a.b.v"); + for (Violation violation : violations) { + assertThat(violation.getRuleValue().getDescriptor().getFullName()) + .isEqualTo("buf.validate.Int32Rules.gt"); + assertThat(violation.getRuleValue().getValue()).isEqualTo(100); + } } @Test public void testMutuallyRecursiveMessagesAcceptValidValues() throws Exception { - Map types = mutuallyRecursiveTypes(); Validator validator = ValidatorFactory.newBuilder().build(); - assertThat(validator.validate(nestedA(types.get("A"), types.get("B"), 2, 101)).isSuccess()) - .isTrue(); + assertThat(validator.validate(nestedA(2, 101)).isSuccess()).isTrue(); } @Test public void testSelfRecursiveFieldDeclaredBeforeRule() throws Exception { - Descriptor node = - compile( - "recursive", - DescriptorProto.newBuilder() - .setName("Node") - .addField(messageField("next", 1, ".recursive.Node")) - .addField( - field("v", 2, FieldDescriptorProto.Type.TYPE_INT32) - .setOptions(INT32_GT_100))) - .get("Node"); - DynamicMessage inner = - DynamicMessage.newBuilder(node).setField(node.findFieldByName("v"), 5).build(); - DynamicMessage root = - DynamicMessage.newBuilder(node) - .setField(node.findFieldByName("v"), 5) - .setField(node.findFieldByName("next"), inner) + SelfRecursive selfRecursive = + SelfRecursive.newBuilder() + .setV(5) + .setNext(SelfRecursive.newBuilder().setV(5).build()) .build(); Validator validator = ValidatorFactory.newBuilder().build(); - assertThat(validator.validate(root).getViolations()).hasSize(2); + List violations = validator.validate(selfRecursive).getViolations(); + assertThat(violations).hasSize(2); + assertThat(violations) + .extracting(violation -> FieldPathUtils.fieldPathString(violation.toProto().getField())) + .containsExactlyInAnyOrder("v", "next.v"); + for (Violation violation : violations) { + assertThat(violation.getRuleValue().getDescriptor().getFullName()) + .isEqualTo("buf.validate.Int32Rules.gt"); + assertThat(violation.getRuleValue().getValue()).isEqualTo(100); + } } @Test public void testMessageWithoutRulesIsTautology() throws Exception { - Map types = - compile( - "recursive", - DescriptorProto.newBuilder() - .setName("Mid") - .addField(messageField("leaf", 1, ".recursive.Leaf")) - .addField(field("s", 2, FieldDescriptorProto.Type.TYPE_STRING)), - DescriptorProto.newBuilder() - .setName("Leaf") - .addField(field("v", 1, FieldDescriptorProto.Type.TYPE_INT32))); - assertThat(isTautology(types.get("Mid"))).isTrue(); + assertThat(isTautology(NoRuleParent.getDescriptor())).isTrue(); } @Test public void testRuleFreeCycleIsKept() throws Exception { - Map types = - compile( - "recursive", - DescriptorProto.newBuilder() - .setName("Root") - .addField( - field("name", 1, FieldDescriptorProto.Type.TYPE_STRING) - .setOptions( - rules( - FieldRules.newBuilder() - .setString(StringRules.newBuilder().setMinLen(1))))) - .addField(messageField("node", 2, ".recursive.Node")), - DescriptorProto.newBuilder() - .setName("Node") - .addField(messageField("next", 1, ".recursive.Node"))); - Descriptor root = types.get("Root"); - Descriptor node = types.get("Node"); // Detecting that a cycle has no rules isn't worth the complexity, so it is walked. - assertThat(isTautology(node)).isFalse(); - DynamicMessage message = - DynamicMessage.newBuilder(root) - .setField(root.findFieldByName("node"), DynamicMessage.getDefaultInstance(node)) - .build(); + // node has a single field that's itself, but it has no rules + assertThat(isTautology(Node.getDescriptor())).isFalse(); + // root has a single rule on its name field, which must have a length > 1 + Root root = Root.newBuilder().setNode(Node.newBuilder().build()).build(); Validator validator = ValidatorFactory.newBuilder().build(); - assertThat(validator.validate(message).getViolations()).hasSize(1); + List violations = validator.validate(root).getViolations(); + assertThat(violations).hasSize(1); + Violation violation = violations.get(0); + assertThat(FieldPathUtils.fieldPathString(violation.toProto().getField())).isEqualTo("name"); + assertThat(violation.getRuleValue().getDescriptor().getFullName()) + .isEqualTo("buf.validate.StringRules.min_len"); + assertThat(violation.getRuleValue().getValue()).isEqualTo(1L); } @Test @@ -152,10 +147,12 @@ public void testStructTypesAreTautologies() throws Exception { assertThat(isTautology(Struct.getDescriptor())).isTrue(); assertThat(isTautology(Value.getDescriptor())).isTrue(); assertThat(isTautology(ListValue.getDescriptor())).isTrue(); + assertThat(isTautology(HasGoogleFields.getDescriptor())).isTrue(); } @Test public void testNonStandardListValueWithRulesIsValidated() throws Exception { + // these are left dynamic intentionally Map types = compile( "google.protobuf", @@ -192,29 +189,6 @@ public void testNonStandardListValueWithRulesIsValidated() throws Exception { assertThat(validator.validate(message).getViolations()).hasSize(1); } - @Test - @Timeout(value = 15, unit = TimeUnit.SECONDS) - public void testSharedChainBuildsInLinearTime() throws Exception { - // Each M{i} has two required fields of type M{i+1}, so descending into shared evaluators - // when checking for tautologies would do 2^n work. - int n = 26; - DescriptorProto.Builder[] messages = new DescriptorProto.Builder[n + 1]; - FieldOptions required = rules(FieldRules.newBuilder().setRequired(true)); - for (int i = 0; i < n; i++) { - String next = ".recursive.M" + (i + 1); - messages[i] = - DescriptorProto.newBuilder() - .setName("M" + i) - .addField(messageField("a", 1, next).setOptions(required)) - .addField(messageField("b", 2, next).setOptions(required)); - } - messages[n] = DescriptorProto.newBuilder().setName("M" + n); - Descriptor m0 = compile("recursive", messages).get("M0"); - Validator validator = ValidatorFactory.newBuilder().build(); - assertThat(validator.validate(DynamicMessage.getDefaultInstance(m0)).getViolations()) - .hasSize(2); - } - @Test public void testCompilationErrorReportedWhenInnerIsBuiltFirst() throws Exception { Map types = compilationErrorTypes(); @@ -274,25 +248,19 @@ private static DynamicMessage innerWithOuter(Map types) { .build(); } - private static Map mutuallyRecursiveTypes() throws Exception { - return compile( - "recursive", - DescriptorProto.newBuilder().setName("A").addField(messageField("b", 1, ".recursive.B")), - DescriptorProto.newBuilder() - .setName("B") - .addField(messageField("a", 1, ".recursive.A")) - .addField( - field("v", 2, FieldDescriptorProto.Type.TYPE_INT32).setOptions(INT32_GT_100))); - } - /** Returns an A whose chain of B messages is {@code depth} long, each with v set to {@code v}. */ - private static DynamicMessage nestedA(Descriptor a, Descriptor b, int depth, int v) { - DynamicMessage.Builder bBuilder = - DynamicMessage.newBuilder(b).setField(b.findFieldByName("v"), v); - if (depth > 1) { - bBuilder.setField(b.findFieldByName("a"), nestedA(a, b, depth - 1, v)); + private static RecursiveA nestedA(int depth, int v) { + RecursiveA recursiveA = null; + RecursiveB recursiveB = null; + for (int i = 0; i < depth; i++) { + RecursiveB.Builder recursiveBBuilder = RecursiveB.newBuilder().setV(v); + if (recursiveA != null) { + recursiveBBuilder.setA(recursiveA); + } + recursiveB = recursiveBBuilder.build(); + recursiveA = RecursiveA.newBuilder().setB(recursiveB).build(); } - return DynamicMessage.newBuilder(a).setField(a.findFieldByName("b"), bBuilder.build()).build(); + return recursiveA; } private static boolean isTautology(Descriptor descriptor) throws Exception { diff --git a/src/test/resources/proto/validationtest/recursive.proto b/src/test/resources/proto/validationtest/recursive.proto new file mode 100644 index 00000000..728bb9e4 --- /dev/null +++ b/src/test/resources/proto/validationtest/recursive.proto @@ -0,0 +1,59 @@ +// 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. + +syntax = "proto3"; + +package validationtest; + +import "buf/validate/validate.proto"; +import "google/protobuf/struct.proto"; +import "google/protobuf/wrappers.proto"; + +message RecursiveA { + RecursiveB b = 1; +} + +message RecursiveB { + RecursiveA a = 1; + int32 v = 2 [(buf.validate.field).int32.gt = 100]; +} + +message SelfRecursive { + SelfRecursive next = 1; + int32 v = 2 [(buf.validate.field).int32.gt = 100]; +} + +message NoRuleParent { + NoRuleChild leaf = 1; + string s = 2; +} + +message NoRuleChild { + int32 v = 1; +} + +message Root { + string name = 1 [(buf.validate.field).string.min_len = 1]; + Node node = 2; +} + +message Node { + Node next = 1; +} + +message HasGoogleFields { + google.protobuf.Struct s = 1; + google.protobuf.Value v = 2; + google.protobuf.ListValue l = 3; +}