From 4a051ad9e2311bdcff67d785a269dc04010c8143 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Thu, 8 May 2025 16:31:35 -0400 Subject: [PATCH 01/14] Factory --- build.gradle.kts | 1 + .../buf/protovalidate/conformance/Main.java | 7 +- .../java/build/buf/protovalidate/Config.java | 26 +--- .../buf/protovalidate/EvaluatorBuilder.java | 22 +++- .../build/buf/protovalidate/IValidator.java | 35 ++++++ .../build/buf/protovalidate/Validator.java | 112 ++++++++++++------ .../buf/protovalidate/ValidatorFactory.java | 0 .../ValidatorCelExpressionTest.java | 4 +- .../ValidatorConstructionTest.java | 89 ++++++++++++++ .../ValidatorDifferentJavaPackagesTest.java | 2 +- .../ValidatorDynamicMessageTest.java | 14 +-- .../proto/validationtest/validationtest.proto | 18 +++ 12 files changed, 252 insertions(+), 78 deletions(-) create mode 100644 src/main/java/build/buf/protovalidate/IValidator.java create mode 100644 src/main/java/build/buf/protovalidate/ValidatorFactory.java create mode 100644 src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java diff --git a/build.gradle.kts b/build.gradle.kts index 8f17533e..a7d15608 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -208,6 +208,7 @@ allprojects { tasks.withType().configureEach { useJUnitPlatform() this.testLogging { + this.showStandardStreams = true events("failed") exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL showExceptions = true diff --git a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java index 9c2cd915..38ea67c1 100644 --- a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java +++ b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java @@ -60,12 +60,11 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) { TypeRegistry typeRegistry = FileDescriptorUtil.createTypeRegistry(fileDescriptorMap.values()); ExtensionRegistry extensionRegistry = FileDescriptorUtil.createExtensionRegistry(fileDescriptorMap.values()); - Validator validator = - new Validator( - Config.newBuilder() + Config cfg = Config.newBuilder() .setTypeRegistry(typeRegistry) .setExtensionRegistry(extensionRegistry) - .build()); + .build(); + Validator validator = Validator.newBuilder().withConfig(cfg).build(); TestConformanceResponse.Builder responseBuilder = TestConformanceResponse.newBuilder(); Map resultsMap = new HashMap<>(); for (Map.Entry entry : request.getCasesMap().entrySet()) { diff --git a/src/main/java/build/buf/protovalidate/Config.java b/src/main/java/build/buf/protovalidate/Config.java index 7c9f3ea7..8c1214b9 100644 --- a/src/main/java/build/buf/protovalidate/Config.java +++ b/src/main/java/build/buf/protovalidate/Config.java @@ -24,19 +24,16 @@ public final class Config { ExtensionRegistry.getEmptyRegistry(); private final boolean failFast; - private final boolean disableLazy; private final TypeRegistry typeRegistry; private final ExtensionRegistry extensionRegistry; private final boolean allowUnknownFields; private Config( boolean failFast, - boolean disableLazy, TypeRegistry typeRegistry, ExtensionRegistry extensionRegistry, boolean allowUnknownFields) { this.failFast = failFast; - this.disableLazy = disableLazy; this.typeRegistry = typeRegistry; this.extensionRegistry = extensionRegistry; this.allowUnknownFields = allowUnknownFields; @@ -60,15 +57,6 @@ public boolean isFailFast() { return failFast; } - /** - * Checks if the configuration for disabling lazy evaluation is enabled. - * - * @return if disabling lazy evaluation is enabled - */ - public boolean isDisableLazy() { - return disableLazy; - } - /** * Gets the type registry used for reparsing protobuf messages. * @@ -99,7 +87,6 @@ public boolean isAllowingUnknownFields() { /** Builder for configuration. Provides a forward compatible API for users. */ public static final class Builder { private boolean failFast; - private boolean disableLazy; private TypeRegistry typeRegistry = DEFAULT_TYPE_REGISTRY; private ExtensionRegistry extensionRegistry = DEFAULT_EXTENSION_REGISTRY; private boolean allowUnknownFields; @@ -117,17 +104,6 @@ public Builder setFailFast(boolean failFast) { return this; } - /** - * Set the configuration for disabling lazy evaluation. - * - * @param disableLazy the boolean for enabling - * @return this builder - */ - public Builder setDisableLazy(boolean disableLazy) { - this.disableLazy = disableLazy; - return this; - } - /** * Set the type registry for reparsing protobuf messages. This option should be set alongside * setExtensionRegistry to allow dynamic resolution of predefined rule extensions. It should be @@ -187,7 +163,7 @@ public Builder setAllowUnknownFields(boolean allowUnknownFields) { * @return the configuration. */ public Config build() { - return new Config(failFast, disableLazy, typeRegistry, extensionRegistry, allowUnknownFields); + return new Config(failFast, typeRegistry, extensionRegistry, allowUnknownFields); } } } diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index 201448b0..de952a60 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -51,6 +51,7 @@ class EvaluatorBuilder { private final Env env; private final boolean disableLazy; + private final List descriptors; private final RuleCache rules; /** @@ -61,10 +62,29 @@ class EvaluatorBuilder { */ public EvaluatorBuilder(Env env, Config config) { this.env = env; - this.disableLazy = config.isDisableLazy(); + this.descriptors = new ArrayList(); + this.disableLazy = false; this.rules = new RuleCache(env, config); } + /** + * Constructs a new {@link EvaluatorBuilder}. + * + * @param env The CEL environment for evaluation. + * @param config The configuration to use for the evaluation. + */ + public EvaluatorBuilder(Env env, Config config, List descriptors, boolean disableLazy) + throws CompilationException { + this.env = env; + this.descriptors = descriptors; + this.disableLazy = disableLazy; + this.rules = new RuleCache(env, config); + + for (Descriptor descriptor : this.descriptors) { + this.build(descriptor); + } + } + /** * Returns a pre-cached {@link Evaluator} for the given descriptor or, if the descriptor is * unknown, returns an evaluator that always throws a {@link CompilationException}. diff --git a/src/main/java/build/buf/protovalidate/IValidator.java b/src/main/java/build/buf/protovalidate/IValidator.java new file mode 100644 index 00000000..cf4e1220 --- /dev/null +++ b/src/main/java/build/buf/protovalidate/IValidator.java @@ -0,0 +1,35 @@ +// Copyright 2023-2024 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 build.buf.protovalidate.exceptions.ValidationException; +import com.google.protobuf.Message; + +interface IValidator { + /** + * Checks that message satisfies its rules. Rules are defined within the Protobuf file as options + * from the buf.validate package. A {@link ValidationResult} is returned which contains a list of + * violations. If the list is empty, the message is valid. If the list is non-empty, the message + * is invalid. An exception is thrown if the message cannot be validated because the evaluation + * logic for the message cannot be built ({@link CompilationException}), or there is a type error + * when attempting to evaluate a CEL expression associated with the message ({@link + * ExecutionException}). + * + * @param msg the {@link Message} to be validated. + * @return the {@link ValidationResult} from the evaluation. + * @throws ValidationException if there are any compilation or validation execution errors. + */ + ValidationResult validate(Message msg) throws ValidationException; +} diff --git a/src/main/java/build/buf/protovalidate/Validator.java b/src/main/java/build/buf/protovalidate/Validator.java index bc6880dc..0bf0a98e 100644 --- a/src/main/java/build/buf/protovalidate/Validator.java +++ b/src/main/java/build/buf/protovalidate/Validator.java @@ -20,35 +20,37 @@ import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Message; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import org.jspecify.annotations.Nullable; import org.projectnessie.cel.Env; import org.projectnessie.cel.Library; -/** Performs validation on any proto.Message values. The Validator is safe for concurrent use. */ -public class Validator { +public class Validator implements IValidator { /** evaluatorBuilder is the builder used to construct the evaluator for a given message. */ private final EvaluatorBuilder evaluatorBuilder; + + // For convenience + public static IValidator defaultInstance() { + return newBuilder().build(); + } + + public static SafeBuilder newBuilder() { + return new SafeBuilder(); + } + + public static EagerBuilder newBuilder(List descriptors, boolean disableLazy) { + return new EagerBuilder(descriptors, disableLazy); + } + /** * failFast indicates whether the validator should stop evaluating rules after the first * violation. */ private final boolean failFast; - /** - * Constructs a new {@link Validator}. - * - * @param config specified configuration. - */ - public Validator(Config config) { - Env env = Env.newEnv(Library.Lib(new ValidateLibrary())); - this.evaluatorBuilder = new EvaluatorBuilder(env, config); - this.failFast = config.isFailFast(); - } - - /** Constructs a new {@link Validator} with a default configuration. */ - public Validator() { - Config config = Config.newBuilder().build(); + Validator(Config config) { Env env = Env.newEnv(Library.Lib(new ValidateLibrary())); this.evaluatorBuilder = new EvaluatorBuilder(env, config); this.failFast = config.isFailFast(); @@ -73,7 +75,7 @@ public ValidationResult validate(Message msg) throws ValidationException { } Descriptor descriptor = msg.getDescriptorForType(); Evaluator evaluator = evaluatorBuilder.load(descriptor); - List result = evaluator.evaluate(new MessageValue(msg), failFast); + List result = evaluator.evaluate(new MessageValue(msg), this.failFast); if (result.isEmpty()) { return ValidationResult.EMPTY; } @@ -83,31 +85,65 @@ public ValidationResult validate(Message msg) throws ValidationException { } return new ValidationResult(violations); } +} - /** - * Loads messages that are expected to be validated, allowing the {@link Validator} to warm up. - * Messages included transitively (i.e., fields with message values) are automatically handled. - * - * @param messages the list of {@link Message} to load. - * @throws CompilationException if there are any compilation errors during warm-up. - */ - public void loadMessages(Message... messages) throws CompilationException { - for (Message message : messages) { - this.evaluatorBuilder.load(message.getDescriptorForType()); +abstract class Builder> { + @Nullable protected Config config; + + public T withConfig(Config config) { + this.config = config; + return self(); + } + + abstract T self(); +} + +public class EagerBuilder extends Builder { + private final List descriptors; + private final boolean disableLazy; + + EagerBuilder(List descriptors, boolean disableLazy) { + this.descriptors = Collections.unmodifiableList(descriptors); + this.disableLazy = disableLazy; + } + + IValidator build() throws CompilationException, IllegalStateException { + if (disableLazy && this.descriptors.size() == 0) { + throw new IllegalStateException(); + } + + Config cfg = this.config; + if (cfg == null) { + cfg = Config.newBuilder().build(); } + return new Validator(cfg); } - /** - * Loads message descriptors that are expected to be validated, allowing the {@link Validator} to - * warm up. Messages included transitively (i.e., fields with message values) are automatically - * handled. - * - * @param descriptors the list of {@link Descriptor} to load. - * @throws CompilationException if there are any compilation errors during warm-up. - */ - public void loadDescriptors(Descriptor... descriptors) throws CompilationException { - for (Descriptor descriptor : descriptors) { - this.evaluatorBuilder.load(descriptor); + @Override + EagerBuilder self() { + return this; + } + + boolean getDisableLazy() { + return this.disableLazy; + } + + List getDescriptors() { + return this.descriptors; + } +} + +public class SafeBuilder extends Builder { + public IValidator build() { + Config cfg = this.config; + if (cfg == null) { + cfg = Config.newBuilder().build(); } + return new Validator(cfg); + } + + @Override + SafeBuilder self() { + return this; } } diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java new file mode 100644 index 00000000..e69de29b diff --git a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java index b709a680..75c6ede1 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java @@ -75,7 +75,7 @@ public void testFieldExpressionRepeatedMessage() throws Exception { .setMessage("test message field_expression.repeated.message") .build(); - Validator validator = new Validator(); + Validator validator = Validator.defaultInstance(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); @@ -144,7 +144,7 @@ public void testFieldExpressionRepeatedMessageItems() throws Exception { .setMessage("test message field_expression.repeated.message.items") .build(); - Validator validator = new Validator(); + Validator validator = Validator.defaultInstance(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java new file mode 100644 index 00000000..ffa27005 --- /dev/null +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -0,0 +1,89 @@ +// Copyright 2023-2024 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 com.example.imports.validationtest.FieldExpressionMapInt32; +import com.google.protobuf.Descriptors.Descriptor; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Test; + +public class ValidatorConstructionTest { + @Test + public void testNoWarmup() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + Config cfg = Config.newBuilder().setFailFast(true).build(); + Validator validator = Validator.newBuilder().withConfig(cfg).build(); + try { + ValidationResult result = validator.validate(msg); + // assertThat(result.isSuccess()).isFalse(); + // assertThat(result.getViolations().size()).isEqualTo(1); + System.err.println(result); + } catch (Exception e) { + System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); + } + } + + @Test + public void testWarmup() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + // Config cfg = Config.newBuilder().setFailFast(true).build(); + List seedDescriptors = new ArrayList(); + FieldExpressionMapInt32 reg = FieldExpressionMapInt32.newBuilder().build(); + seedDescriptors.add(reg.getDescriptorForType()); + + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = Validator.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + validator.validate(msg); + // assertThat(result.isSuccess()).isFalse(); + // assertThat(result.getViolations().size()).isEqualTo(1); + // System.err.println(result); + } catch (Exception e) { + System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); + } + } + + // @Test + // public void testDisableLazy() { + // Map testMap = new HashMap(); + // testMap.put(42, 42); + // FieldExpressionMapInt32 msg = + // FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + // Config cfg = Config.newBuilder().setDisableLazy(true).build(); + // Validator validator = new Validator(cfg); + // try { + // System.err.println("goosh"); + // validator.loadMessages(FieldExpressionMapInt32.newBuilder().build()); + // ValidationResult result = validator.validate(msg); + // assertThat(result.isSuccess()).isFalse(); + // assertThat(result.getViolations().size()).isEqualTo(1); + // System.err.println(result); + // } catch (Exception e) { + // System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); + // } + // } +} diff --git a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java index 917f9c43..066920ce 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java @@ -186,7 +186,7 @@ private void expectViolation(Message msg, Violation violation) throws Validation } private void expectViolations(Message msg, List expected) throws ValidationException { - Validator validator = new Validator(); + Validator validator = Validator.defaultInstance(); List violations = validator.validate(msg).toProto().getViolationsList(); assertThat(violations).containsExactlyInAnyOrderElementsOf(expected); } diff --git a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java index 1b0a542d..89bd4b36 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java @@ -78,7 +78,7 @@ public void testFieldRuleDynamicMessage() throws Exception { .setRuleId("string.pattern") .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); - ValidationResult result = new Validator().validate(messageBuilder.build()); + ValidationResult result = Validator.defaultInstance().validate(messageBuilder.build()); assertThat(result.toProto().getViolationsList()).containsExactly(expectedViolation); assertThat(result.getViolations().get(0).getFieldValue().getValue()).isEqualTo("0123456789"); assertThat(result.getViolations().get(0).getRuleValue().getValue()) @@ -97,7 +97,7 @@ public void testOneofRuleDynamicMessage() throws Exception { .setRuleId("required") .setMessage("exactly one field is required in oneof") .build(); - assertThat(new Validator().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) .containsExactly(expectedViolation); } @@ -113,7 +113,7 @@ public void testMessageRuleDynamicMessage() throws Exception { .setRuleId("secondary_email_depends_on_primary") .setMessage("cannot set a secondary email without setting a primary one") .build(); - assertThat(new Validator().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) .containsExactly(expectedViolation); } @@ -123,7 +123,7 @@ public void testRequiredFieldRuleDynamicMessage() throws Exception { createMessageWithUnknownOptions(ExampleRequiredFieldRules.getDefaultInstance()); messageBuilder.setField( messageBuilder.getDescriptorForType().findFieldByName("regex_string_field"), "abc123"); - assertThat(new Validator().validate(messageBuilder.build()).getViolations()).isEmpty(); + assertThat(Validator.defaultInstance().validate(messageBuilder.build()).getViolations()).isEmpty(); } @Test @@ -154,7 +154,7 @@ public void testRequiredFieldRuleDynamicMessageInvalid() throws Exception { .setRuleId("string.pattern") .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); - assertThat(new Validator().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) .containsExactly(expectedViolation); } @@ -170,7 +170,7 @@ public void testPredefinedFieldRuleDynamicMessage() throws Exception { TypeRegistry.newBuilder().add(isIdent.getDescriptor().getContainingType()).build(); Config config = Config.newBuilder().setExtensionRegistry(registry).setTypeRegistry(typeRegistry).build(); - assertThat(new Validator(config).validate(messageBuilder.build()).getViolations()).isEmpty(); + assertThat(Validator.newBuilder().withConfig(config).build().validate(messageBuilder.build()).getViolations()).isEmpty(); } @Test @@ -203,7 +203,7 @@ public void testPredefinedFieldRuleDynamicMessageInvalid() throws Exception { TypeRegistry.newBuilder().add(isIdent.getDescriptor().getContainingType()).build(); Config config = Config.newBuilder().setExtensionRegistry(registry).setTypeRegistry(typeRegistry).build(); - assertThat(new Validator(config).validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat(Validator.newBuilder().withConfig(config).build().validate(messageBuilder.build()).toProto().getViolationsList()) .containsExactly(expectedViolation); } diff --git a/src/test/resources/proto/validationtest/validationtest.proto b/src/test/resources/proto/validationtest/validationtest.proto index 68d5c0d4..cf388719 100644 --- a/src/test/resources/proto/validationtest/validationtest.proto +++ b/src/test/resources/proto/validationtest/validationtest.proto @@ -17,6 +17,7 @@ syntax = "proto3"; package validationtest; import "buf/validate/validate.proto"; +import "google/protobuf/duration.proto"; message ExampleFieldRules { string regex_string_field = 1 [(buf.validate.field).string.pattern = "^[a-z0-9]{1,9}$"]; @@ -51,3 +52,20 @@ message ExampleMessageRules { string primary_email = 1; string secondary_email = 2; } + +message FieldExpressionMapInt32 { + map val = 1 [(buf.validate.field).cel = { + id: "field_expression.map.int32" + message: "all map values must equal 1" + expression: "this.all(k, this[k] == 1)" + }]; +} + +message FieldExpressionRepeatedScalar { + repeated int32 val = 1 [(buf.validate.field).cel = { + id: "field_expression.repeated.scalar" + message: "test message field_expression.repeated.scalar" + expression: "this.all(e, e == 1)" + }]; +} + From 999b9300c776ad20ae6e3cc70511f22120120fe8 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Thu, 8 May 2025 17:20:07 -0400 Subject: [PATCH 02/14] Docs --- .../buf/protovalidate/conformance/Main.java | 12 +- .../buf/protovalidate/ValidatorTest.java | 2 +- .../build/buf/protovalidate/Validator.java | 117 +--------------- .../buf/protovalidate/ValidatorFactory.java | 131 ++++++++++++++++++ .../{IValidator.java => ValidatorImpl.java} | 42 +++++- .../ValidatorCelExpressionTest.java | 4 +- .../ValidatorConstructionTest.java | 20 +-- .../ValidatorDifferentJavaPackagesTest.java | 2 +- .../ValidatorDynamicMessageTest.java | 39 +++++- 9 files changed, 228 insertions(+), 141 deletions(-) rename src/main/java/build/buf/protovalidate/{IValidator.java => ValidatorImpl.java} (51%) diff --git a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java index 38ea67c1..55f5ae65 100644 --- a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java +++ b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java @@ -17,6 +17,7 @@ import build.buf.protovalidate.Config; import build.buf.protovalidate.ValidationResult; import build.buf.protovalidate.Validator; +import build.buf.protovalidate.ValidatorFactory; import build.buf.protovalidate.exceptions.CompilationException; import build.buf.protovalidate.exceptions.ExecutionException; import build.buf.validate.ValidateProto; @@ -60,11 +61,12 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) { TypeRegistry typeRegistry = FileDescriptorUtil.createTypeRegistry(fileDescriptorMap.values()); ExtensionRegistry extensionRegistry = FileDescriptorUtil.createExtensionRegistry(fileDescriptorMap.values()); - Config cfg = Config.newBuilder() - .setTypeRegistry(typeRegistry) - .setExtensionRegistry(extensionRegistry) - .build(); - Validator validator = Validator.newBuilder().withConfig(cfg).build(); + Config cfg = + Config.newBuilder() + .setTypeRegistry(typeRegistry) + .setExtensionRegistry(extensionRegistry) + .build(); + Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build(); TestConformanceResponse.Builder responseBuilder = TestConformanceResponse.newBuilder(); Map resultsMap = new HashMap<>(); for (Map.Entry entry : request.getCasesMap().entrySet()) { diff --git a/conformance/src/test/java/build/buf/protovalidate/ValidatorTest.java b/conformance/src/test/java/build/buf/protovalidate/ValidatorTest.java index e9929c91..d7d55453 100644 --- a/conformance/src/test/java/build/buf/protovalidate/ValidatorTest.java +++ b/conformance/src/test/java/build/buf/protovalidate/ValidatorTest.java @@ -60,7 +60,7 @@ public class ValidatorTest { @BeforeEach public void setUp() { Config config = Config.newBuilder().build(); - validator = new Validator(config); + validator = ValidatorFactory.newBuilder().withConfig(config).build(); } @Test diff --git a/src/main/java/build/buf/protovalidate/Validator.java b/src/main/java/build/buf/protovalidate/Validator.java index 0bf0a98e..53cbe2ee 100644 --- a/src/main/java/build/buf/protovalidate/Validator.java +++ b/src/main/java/build/buf/protovalidate/Validator.java @@ -17,45 +17,10 @@ import build.buf.protovalidate.exceptions.CompilationException; import build.buf.protovalidate.exceptions.ExecutionException; import build.buf.protovalidate.exceptions.ValidationException; -import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Message; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import org.jspecify.annotations.Nullable; -import org.projectnessie.cel.Env; -import org.projectnessie.cel.Library; - -public class Validator implements IValidator { - /** evaluatorBuilder is the builder used to construct the evaluator for a given message. */ - private final EvaluatorBuilder evaluatorBuilder; - - - // For convenience - public static IValidator defaultInstance() { - return newBuilder().build(); - } - - public static SafeBuilder newBuilder() { - return new SafeBuilder(); - } - - public static EagerBuilder newBuilder(List descriptors, boolean disableLazy) { - return new EagerBuilder(descriptors, disableLazy); - } - - /** - * failFast indicates whether the validator should stop evaluating rules after the first - * violation. - */ - private final boolean failFast; - - Validator(Config config) { - Env env = Env.newEnv(Library.Lib(new ValidateLibrary())); - this.evaluatorBuilder = new EvaluatorBuilder(env, config); - this.failFast = config.isFailFast(); - } +/** A validator that can be used to validate messages */ +public interface Validator { /** * Checks that message satisfies its rules. Rules are defined within the Protobuf file as options * from the buf.validate package. A {@link ValidationResult} is returned which contains a list of @@ -69,81 +34,5 @@ public static EagerBuilder newBuilder(List descriptors, boolean disa * @return the {@link ValidationResult} from the evaluation. * @throws ValidationException if there are any compilation or validation execution errors. */ - public ValidationResult validate(Message msg) throws ValidationException { - if (msg == null) { - return ValidationResult.EMPTY; - } - Descriptor descriptor = msg.getDescriptorForType(); - Evaluator evaluator = evaluatorBuilder.load(descriptor); - List result = evaluator.evaluate(new MessageValue(msg), this.failFast); - if (result.isEmpty()) { - return ValidationResult.EMPTY; - } - List violations = new ArrayList<>(result.size()); - for (RuleViolation.Builder builder : result) { - violations.add(builder.build()); - } - return new ValidationResult(violations); - } -} - -abstract class Builder> { - @Nullable protected Config config; - - public T withConfig(Config config) { - this.config = config; - return self(); - } - - abstract T self(); -} - -public class EagerBuilder extends Builder { - private final List descriptors; - private final boolean disableLazy; - - EagerBuilder(List descriptors, boolean disableLazy) { - this.descriptors = Collections.unmodifiableList(descriptors); - this.disableLazy = disableLazy; - } - - IValidator build() throws CompilationException, IllegalStateException { - if (disableLazy && this.descriptors.size() == 0) { - throw new IllegalStateException(); - } - - Config cfg = this.config; - if (cfg == null) { - cfg = Config.newBuilder().build(); - } - return new Validator(cfg); - } - - @Override - EagerBuilder self() { - return this; - } - - boolean getDisableLazy() { - return this.disableLazy; - } - - List getDescriptors() { - return this.descriptors; - } -} - -public class SafeBuilder extends Builder { - public IValidator build() { - Config cfg = this.config; - if (cfg == null) { - cfg = Config.newBuilder().build(); - } - return new Validator(cfg); - } - - @Override - SafeBuilder self() { - return this; - } + ValidationResult validate(Message msg) throws ValidationException; } diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index e69de29b..190640f3 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -0,0 +1,131 @@ +// Copyright 2023-2024 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 build.buf.protovalidate.exceptions.CompilationException; +import com.google.protobuf.Descriptors.Descriptor; +import java.util.Collections; +import java.util.List; +import org.jspecify.annotations.Nullable; + +/** ValidatorFactory is used to create a validator */ +public class ValidatorFactory { + + public static class EagerBuilder extends BaseBuilder { + private final List descriptors; + private final boolean disableLazy; + + /** + * An eager builder behaves the same as a regular Builder, but attempts to warmup the validator + * cache with a given list of Descriptors. + */ + EagerBuilder(List descriptors, boolean disableLazy) { + this.descriptors = Collections.unmodifiableList(descriptors); + this.disableLazy = disableLazy; + } + + /** + * Build the validator, warming up the cache with any provided descriptors. + * + * @return A Validator instance + * @throws CompilationException If any of the given descriptors' validation rules fail + * processing while warming up the cache. + * @throws IllegalStateException If disableLazy is set to true and no descriptors are passed. + */ + public Validator build() throws CompilationException, IllegalStateException { + if (disableLazy && this.descriptors.size() == 0) { + throw new IllegalStateException(); + } + + Config cfg = this.config; + if (cfg == null) { + cfg = Config.newBuilder().build(); + } + return new ValidatorImpl(cfg); + } + + @Override + EagerBuilder self() { + return this; + } + + boolean getDisableLazy() { + return this.disableLazy; + } + + List getDescriptors() { + return this.descriptors; + } + } + + public static class Builder extends BaseBuilder { + public Validator build() { + Config cfg = this.config; + if (cfg == null) { + cfg = Config.newBuilder().build(); + } + return new ValidatorImpl(cfg); + } + + @Override + Builder self() { + return this; + } + } + + /** + * A convenience function for creating a new validator with a default configuration. + * + * @return A Validator instance + **/ + public static Validator defaultInstance() { + return newBuilder().build(); + } + + /** + * Creates a new builder for a validator. + * + * @return A Validator instance + **/ + public static Builder newBuilder() { + return new Builder(); + } + + /** + * Creates a new eager builder for a validator. + * + * @param descriptors the list of descriptors to warm up the cache. + * @param disableLazy whether to disable lazy loading of validation rules. When validation is + * performed, a message's rules will be looked up in a cache. If they are not found, by + * default they will be processed and lazily-loaded into the cache. Setting this to false will + * not attempt to lazily-load descriptor information not found in the cache and essentially + * makes the entire cache read-only, eliminating thread contention and race conditions. + * @return An eager builder instance. + */ + public static EagerBuilder newBuilder(List descriptors, boolean disableLazy) { + return new EagerBuilder(descriptors, disableLazy); + } +} + +abstract class BaseBuilder> { + @Nullable protected Config config; + + public T withConfig(Config config) { + this.config = config; + return self(); + } + + abstract T self(); +} diff --git a/src/main/java/build/buf/protovalidate/IValidator.java b/src/main/java/build/buf/protovalidate/ValidatorImpl.java similarity index 51% rename from src/main/java/build/buf/protovalidate/IValidator.java rename to src/main/java/build/buf/protovalidate/ValidatorImpl.java index cf4e1220..e5c6adb5 100644 --- a/src/main/java/build/buf/protovalidate/IValidator.java +++ b/src/main/java/build/buf/protovalidate/ValidatorImpl.java @@ -14,10 +14,32 @@ package build.buf.protovalidate; +import build.buf.protovalidate.exceptions.CompilationException; +import build.buf.protovalidate.exceptions.ExecutionException; import build.buf.protovalidate.exceptions.ValidationException; +import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Message; +import java.util.ArrayList; +import java.util.List; +import org.projectnessie.cel.Env; +import org.projectnessie.cel.Library; + +class ValidatorImpl implements Validator { + /** evaluatorBuilder is the builder used to construct the evaluator for a given message. */ + private final EvaluatorBuilder evaluatorBuilder; + + /** + * failFast indicates whether the validator should stop evaluating rules after the first + * violation. + */ + private final boolean failFast; + + ValidatorImpl(Config config) { + Env env = Env.newEnv(Library.Lib(new ValidateLibrary())); + this.evaluatorBuilder = new EvaluatorBuilder(env, config); + this.failFast = config.isFailFast(); + } -interface IValidator { /** * Checks that message satisfies its rules. Rules are defined within the Protobuf file as options * from the buf.validate package. A {@link ValidationResult} is returned which contains a list of @@ -31,5 +53,21 @@ interface IValidator { * @return the {@link ValidationResult} from the evaluation. * @throws ValidationException if there are any compilation or validation execution errors. */ - ValidationResult validate(Message msg) throws ValidationException; + @Override + public ValidationResult validate(Message msg) throws ValidationException { + if (msg == null) { + return ValidationResult.EMPTY; + } + Descriptor descriptor = msg.getDescriptorForType(); + Evaluator evaluator = evaluatorBuilder.load(descriptor); + List result = evaluator.evaluate(new MessageValue(msg), this.failFast); + if (result.isEmpty()) { + return ValidationResult.EMPTY; + } + List violations = new ArrayList<>(result.size()); + for (RuleViolation.Builder builder : result) { + violations.add(builder.build()); + } + return new ValidationResult(violations); + } } diff --git a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java index 75c6ede1..8f1ecc9f 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java @@ -75,7 +75,7 @@ public void testFieldExpressionRepeatedMessage() throws Exception { .setMessage("test message field_expression.repeated.message") .build(); - Validator validator = Validator.defaultInstance(); + Validator validator = ValidatorFactory.defaultInstance(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); @@ -144,7 +144,7 @@ public void testFieldExpressionRepeatedMessageItems() throws Exception { .setMessage("test message field_expression.repeated.message.items") .build(); - Validator validator = Validator.defaultInstance(); + Validator validator = ValidatorFactory.defaultInstance(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index ffa27005..7bed2ede 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -14,10 +14,12 @@ package build.buf.protovalidate; -// import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; import com.example.imports.validationtest.FieldExpressionMapInt32; import com.google.protobuf.Descriptors.Descriptor; +import build.buf.protovalidate.exceptions.ValidationException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -26,20 +28,19 @@ public class ValidatorConstructionTest { @Test - public void testNoWarmup() { + public void testLazyBuilderWithConfig() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); Config cfg = Config.newBuilder().setFailFast(true).build(); - Validator validator = Validator.newBuilder().withConfig(cfg).build(); + Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build(); try { ValidationResult result = validator.validate(msg); - // assertThat(result.isSuccess()).isFalse(); - // assertThat(result.getViolations().size()).isEqualTo(1); - System.err.println(result); - } catch (Exception e) { - System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); } } @@ -56,7 +57,8 @@ public void testWarmup() { Config cfg = Config.newBuilder().setFailFast(true).build(); try { - Validator validator = Validator.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + Validator validator = + ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); validator.validate(msg); // assertThat(result.isSuccess()).isFalse(); // assertThat(result.getViolations().size()).isEqualTo(1); diff --git a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java index 066920ce..c7e87251 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java @@ -186,7 +186,7 @@ private void expectViolation(Message msg, Violation violation) throws Validation } private void expectViolations(Message msg, List expected) throws ValidationException { - Validator validator = Validator.defaultInstance(); + Validator validator = ValidatorFactory.defaultInstance(); List violations = validator.validate(msg).toProto().getViolationsList(); assertThat(violations).containsExactlyInAnyOrderElementsOf(expected); } diff --git a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java index 89bd4b36..7f32bc1e 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java @@ -78,7 +78,7 @@ public void testFieldRuleDynamicMessage() throws Exception { .setRuleId("string.pattern") .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); - ValidationResult result = Validator.defaultInstance().validate(messageBuilder.build()); + ValidationResult result = ValidatorFactory.defaultInstance().validate(messageBuilder.build()); assertThat(result.toProto().getViolationsList()).containsExactly(expectedViolation); assertThat(result.getViolations().get(0).getFieldValue().getValue()).isEqualTo("0123456789"); assertThat(result.getViolations().get(0).getRuleValue().getValue()) @@ -97,7 +97,11 @@ public void testOneofRuleDynamicMessage() throws Exception { .setRuleId("required") .setMessage("exactly one field is required in oneof") .build(); - assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat( + ValidatorFactory.defaultInstance() + .validate(messageBuilder.build()) + .toProto() + .getViolationsList()) .containsExactly(expectedViolation); } @@ -113,7 +117,11 @@ public void testMessageRuleDynamicMessage() throws Exception { .setRuleId("secondary_email_depends_on_primary") .setMessage("cannot set a secondary email without setting a primary one") .build(); - assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat( + ValidatorFactory.defaultInstance() + .validate(messageBuilder.build()) + .toProto() + .getViolationsList()) .containsExactly(expectedViolation); } @@ -123,7 +131,8 @@ public void testRequiredFieldRuleDynamicMessage() throws Exception { createMessageWithUnknownOptions(ExampleRequiredFieldRules.getDefaultInstance()); messageBuilder.setField( messageBuilder.getDescriptorForType().findFieldByName("regex_string_field"), "abc123"); - assertThat(Validator.defaultInstance().validate(messageBuilder.build()).getViolations()).isEmpty(); + assertThat(ValidatorFactory.defaultInstance().validate(messageBuilder.build()).getViolations()) + .isEmpty(); } @Test @@ -154,7 +163,11 @@ public void testRequiredFieldRuleDynamicMessageInvalid() throws Exception { .setRuleId("string.pattern") .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); - assertThat(Validator.defaultInstance().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat( + ValidatorFactory.defaultInstance() + .validate(messageBuilder.build()) + .toProto() + .getViolationsList()) .containsExactly(expectedViolation); } @@ -170,7 +183,13 @@ public void testPredefinedFieldRuleDynamicMessage() throws Exception { TypeRegistry.newBuilder().add(isIdent.getDescriptor().getContainingType()).build(); Config config = Config.newBuilder().setExtensionRegistry(registry).setTypeRegistry(typeRegistry).build(); - assertThat(Validator.newBuilder().withConfig(config).build().validate(messageBuilder.build()).getViolations()).isEmpty(); + assertThat( + ValidatorFactory.newBuilder() + .withConfig(config) + .build() + .validate(messageBuilder.build()) + .getViolations()) + .isEmpty(); } @Test @@ -203,7 +222,13 @@ public void testPredefinedFieldRuleDynamicMessageInvalid() throws Exception { TypeRegistry.newBuilder().add(isIdent.getDescriptor().getContainingType()).build(); Config config = Config.newBuilder().setExtensionRegistry(registry).setTypeRegistry(typeRegistry).build(); - assertThat(Validator.newBuilder().withConfig(config).build().validate(messageBuilder.build()).toProto().getViolationsList()) + assertThat( + ValidatorFactory.newBuilder() + .withConfig(config) + .build() + .validate(messageBuilder.build()) + .toProto() + .getViolationsList()) .containsExactly(expectedViolation); } From 26ae718ada1879a15ed79288066ee1f702c64311 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Thu, 8 May 2025 17:32:48 -0400 Subject: [PATCH 03/14] Validation tests --- .../buf/protovalidate/conformance/Main.java | 1 + .../buf/protovalidate/ValidatorFactory.java | 15 +++--- .../ValidatorConstructionTest.java | 51 +++++++++++++++---- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java index 55f5ae65..6c8aae59 100644 --- a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java +++ b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java @@ -73,6 +73,7 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) { TestResult testResult = testCase(validator, descriptorMap, entry.getValue()); resultsMap.put(entry.getKey(), testResult); } + responseBuilder.putAllResults(resultsMap); return responseBuilder.build(); } catch (Exception e) { diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 190640f3..47f6b2ef 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -46,7 +46,8 @@ public static class EagerBuilder extends BaseBuilder { */ public Validator build() throws CompilationException, IllegalStateException { if (disableLazy && this.descriptors.size() == 0) { - throw new IllegalStateException(); + throw new IllegalStateException( + "a list of descriptors is required when disableLazy is true"); } Config cfg = this.config; @@ -85,20 +86,20 @@ Builder self() { } } - /** - * A convenience function for creating a new validator with a default configuration. + /** + * A convenience function for creating a new validator with a default configuration. * * @return A Validator instance - **/ + */ public static Validator defaultInstance() { return newBuilder().build(); } - /** - * Creates a new builder for a validator. + /** + * Creates a new builder for a validator. * * @return A Validator instance - **/ + */ public static Builder newBuilder() { return new Builder(); } diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index 7bed2ede..1da36f8e 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -15,11 +15,12 @@ package build.buf.protovalidate; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.fail; +import build.buf.protovalidate.exceptions.ValidationException; import com.example.imports.validationtest.FieldExpressionMapInt32; import com.google.protobuf.Descriptors.Descriptor; -import build.buf.protovalidate.exceptions.ValidationException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -27,6 +28,24 @@ import org.junit.jupiter.api.Test; public class ValidatorConstructionTest { + @Test + public void testDefaultInstance() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + Validator validator = ValidatorFactory.defaultInstance(); + try { + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + @Test public void testLazyBuilderWithConfig() { Map testMap = new HashMap(); @@ -39,18 +58,19 @@ public void testLazyBuilderWithConfig() { ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); } catch (ValidationException e) { - fail("unexpected exception thrown", e); + fail("unexpected exception thrown", e); } } @Test - public void testWarmup() { + public void testEagerBuilderWithConfig() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); - // Config cfg = Config.newBuilder().setFailFast(true).build(); List seedDescriptors = new ArrayList(); FieldExpressionMapInt32 reg = FieldExpressionMapInt32.newBuilder().build(); seedDescriptors.add(reg.getDescriptorForType()); @@ -59,15 +79,26 @@ public void testWarmup() { try { Validator validator = ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); - validator.validate(msg); - // assertThat(result.isSuccess()).isFalse(); - // assertThat(result.getViolations().size()).isEqualTo(1); - // System.err.println(result); - } catch (Exception e) { - System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); } } + @Test + public void testEagerBuilderWithInvalidState() { + List seedDescriptors = new ArrayList(); + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy( + () -> { + ValidatorFactory.newBuilder(seedDescriptors, true).build(); + }); + } + // @Test // public void testDisableLazy() { // Map testMap = new HashMap(); From 58bfad417f7aadbd0a00b14dacd60494d7c39447 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 10:39:39 -0400 Subject: [PATCH 04/14] Javadoc --- .../buf/protovalidate/conformance/Main.java | 3 +- .../buf/protovalidate/EvaluatorBuilder.java | 6 +-- .../buf/protovalidate/ValidatorFactory.java | 47 +++++++++++++---- .../buf/protovalidate/ValidatorImpl.java | 7 +++ .../ValidatorConstructionTest.java | 50 ++++++++++++++++++- 5 files changed, 97 insertions(+), 16 deletions(-) diff --git a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java index 6c8aae59..ae7bdced 100644 --- a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java +++ b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java @@ -67,13 +67,14 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) { .setExtensionRegistry(extensionRegistry) .build(); Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build(); + TestConformanceResponse.Builder responseBuilder = TestConformanceResponse.newBuilder(); Map resultsMap = new HashMap<>(); for (Map.Entry entry : request.getCasesMap().entrySet()) { TestResult testResult = testCase(validator, descriptorMap, entry.getValue()); resultsMap.put(entry.getKey(), testResult); } - + responseBuilder.putAllResults(resultsMap); return responseBuilder.build(); } catch (Exception e) { diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index de952a60..56060db5 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -60,7 +60,7 @@ class EvaluatorBuilder { * @param env The CEL environment for evaluation. * @param config The configuration to use for the evaluation. */ - public EvaluatorBuilder(Env env, Config config) { + EvaluatorBuilder(Env env, Config config) { this.env = env; this.descriptors = new ArrayList(); this.disableLazy = false; @@ -73,7 +73,7 @@ public EvaluatorBuilder(Env env, Config config) { * @param env The CEL environment for evaluation. * @param config The configuration to use for the evaluation. */ - public EvaluatorBuilder(Env env, Config config, List descriptors, boolean disableLazy) + EvaluatorBuilder(Env env, Config config, List descriptors, boolean disableLazy) throws CompilationException { this.env = env; this.descriptors = descriptors; @@ -93,7 +93,7 @@ public EvaluatorBuilder(Env env, Config config, List descriptors, bo * @return An evaluator for the descriptor type. * @throws CompilationException If an evaluator can't be created for the specified descriptor. */ - public Evaluator load(Descriptor desc) throws CompilationException { + Evaluator load(Descriptor desc) throws CompilationException { Evaluator evaluator = evaluatorCache.get(desc); if (evaluator == null && disableLazy) { return new UnknownDescriptorEvaluator(desc); diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 47f6b2ef..34ff8c25 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -20,17 +20,26 @@ import java.util.List; import org.jspecify.annotations.Nullable; -/** ValidatorFactory is used to create a validator */ +/** + * ValidatorFactory is used to create a validator. + * + *

Validators can be created with an optional {@link Config} to customize behavior. They can also + * be created with a list of seed descriptors to warmup the validator cache ahead of time as well as + * an indicator to lazily-load any descriptors not provided into the cache. + */ public class ValidatorFactory { + // Prevent instantiation + private ValidatorFactory() {} + ; + /** + * An eager builder behaves the same as a regular Builder, but attempts to warmup the validator + * cache with a given list of Descriptors. + */ public static class EagerBuilder extends BaseBuilder { private final List descriptors; private final boolean disableLazy; - /** - * An eager builder behaves the same as a regular Builder, but attempts to warmup the validator - * cache with a given list of Descriptors. - */ EagerBuilder(List descriptors, boolean disableLazy) { this.descriptors = Collections.unmodifiableList(descriptors); this.disableLazy = disableLazy; @@ -54,7 +63,7 @@ public Validator build() throws CompilationException, IllegalStateException { if (cfg == null) { cfg = Config.newBuilder().build(); } - return new ValidatorImpl(cfg); + return new ValidatorImpl(cfg, this.descriptors, this.disableLazy); } @Override @@ -71,7 +80,16 @@ List getDescriptors() { } } + /** A builder class used for building a validator. */ public static class Builder extends BaseBuilder { + // Default constructor + private Builder() {} + + /** + * Build a new validator + * + * @return A new {@link Validator} instance. + */ public Validator build() { Config cfg = this.config; if (cfg == null) { @@ -98,35 +116,44 @@ public static Validator defaultInstance() { /** * Creates a new builder for a validator. * - * @return A Validator instance + * @return A Validator builder */ public static Builder newBuilder() { return new Builder(); } /** - * Creates a new eager builder for a validator. + * Creates a new builder for a validator. * * @param descriptors the list of descriptors to warm up the cache. * @param disableLazy whether to disable lazy loading of validation rules. When validation is * performed, a message's rules will be looked up in a cache. If they are not found, by * default they will be processed and lazily-loaded into the cache. Setting this to false will * not attempt to lazily-load descriptor information not found in the cache and essentially - * makes the entire cache read-only, eliminating thread contention and race conditions. - * @return An eager builder instance. + * makes the entire cache read-only, eliminating thread contention. + * @return A Validator builder */ public static EagerBuilder newBuilder(List descriptors, boolean disableLazy) { return new EagerBuilder(descriptors, disableLazy); } } +/** An abstract class that all validator builders should extend. */ abstract class BaseBuilder> { + /** The config object to use for instantiating a validator. */ @Nullable protected Config config; + /** + * Create a validator with the given config + * + * @param config The {@link Config} to configure the validator. + * @return T the builder instance + */ public T withConfig(Config config) { this.config = config; return self(); } + // Subclasses must specify their identity. abstract T self(); } diff --git a/src/main/java/build/buf/protovalidate/ValidatorImpl.java b/src/main/java/build/buf/protovalidate/ValidatorImpl.java index e5c6adb5..b452bdda 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorImpl.java +++ b/src/main/java/build/buf/protovalidate/ValidatorImpl.java @@ -40,6 +40,13 @@ class ValidatorImpl implements Validator { this.failFast = config.isFailFast(); } + ValidatorImpl(Config config, List descriptors, boolean disableLazy) + throws CompilationException { + Env env = Env.newEnv(Library.Lib(new ValidateLibrary())); + this.evaluatorBuilder = new EvaluatorBuilder(env, config, descriptors, disableLazy); + this.failFast = config.isFailFast(); + } + /** * Checks that message satisfies its rules. Rules are defined within the Protobuf file as options * from the buf.validate package. A {@link ValidationResult} is returned which contains a list of diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index 1da36f8e..62a5968a 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.fail; import build.buf.protovalidate.exceptions.ValidationException; +import com.example.imports.validationtest.ExampleFieldRules; import com.example.imports.validationtest.FieldExpressionMapInt32; import com.google.protobuf.Descriptors.Descriptor; import java.util.ArrayList; @@ -66,7 +67,7 @@ public void testLazyBuilderWithConfig() { } @Test - public void testEagerBuilderWithConfig() { + public void testSeedDescriptorsLazyDisabled() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); @@ -90,7 +91,31 @@ public void testEagerBuilderWithConfig() { } @Test - public void testEagerBuilderWithInvalidState() { + public void testSeedDescriptorsWithWrongDescriptorAndLazyDisabled() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + List seedDescriptors = new ArrayList(); + ExampleFieldRules wrong = ExampleFieldRules.newBuilder().build(); + seedDescriptors.add(wrong.getDescriptorForType()); + + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = + ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("No evaluator available for " + msg.getDescriptorForType().getFullName()); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + + @Test + public void testSeedDescriptorsWithInvalidState() { List seedDescriptors = new ArrayList(); assertThatExceptionOfType(IllegalStateException.class) .isThrownBy( @@ -99,6 +124,27 @@ public void testEagerBuilderWithInvalidState() { }); } + @Test + public void testEmptySeedDescriptorsLazyEnabled() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + List seedDescriptors = new ArrayList(); + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = + ValidatorFactory.newBuilder(seedDescriptors, false).withConfig(cfg).build(); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + // @Test // public void testDisableLazy() { // Map testMap = new HashMap(); From 99ddf43dd362903e319c5e0b584c129f7aac8137 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 11:50:07 -0400 Subject: [PATCH 05/14] More tests --- .../buf/protovalidate/ValidatorImpl.java | 15 +-- .../ValidatorConstructionTest.java | 109 ++++++++++++++---- .../proto/validationtest/validationtest.proto | 7 ++ 3 files changed, 96 insertions(+), 35 deletions(-) diff --git a/src/main/java/build/buf/protovalidate/ValidatorImpl.java b/src/main/java/build/buf/protovalidate/ValidatorImpl.java index b452bdda..d40304a9 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorImpl.java +++ b/src/main/java/build/buf/protovalidate/ValidatorImpl.java @@ -15,7 +15,6 @@ package build.buf.protovalidate; import build.buf.protovalidate.exceptions.CompilationException; -import build.buf.protovalidate.exceptions.ExecutionException; import build.buf.protovalidate.exceptions.ValidationException; import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Message; @@ -47,19 +46,7 @@ class ValidatorImpl implements Validator { this.failFast = config.isFailFast(); } - /** - * Checks that message satisfies its rules. Rules are defined within the Protobuf file as options - * from the buf.validate package. A {@link ValidationResult} is returned which contains a list of - * violations. If the list is empty, the message is valid. If the list is non-empty, the message - * is invalid. An exception is thrown if the message cannot be validated because the evaluation - * logic for the message cannot be built ({@link CompilationException}), or there is a type error - * when attempting to evaluate a CEL expression associated with the message ({@link - * ExecutionException}). - * - * @param msg the {@link Message} to be validated. - * @return the {@link ValidationResult} from the evaluation. - * @throws ValidationException if there are any compilation or validation execution errors. - */ + /** {@inheritDoc} */ @Override public ValidationResult validate(Message msg) throws ValidationException { if (msg == null) { diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index 62a5968a..4cd24abe 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -21,6 +21,7 @@ import build.buf.protovalidate.exceptions.ValidationException; import com.example.imports.validationtest.ExampleFieldRules; import com.example.imports.validationtest.FieldExpressionMapInt32; +import com.example.imports.validationtest.FieldExpressionMultiple; import com.google.protobuf.Descriptors.Descriptor; import java.util.ArrayList; import java.util.HashMap; @@ -29,6 +30,8 @@ import org.junit.jupiter.api.Test; public class ValidatorConstructionTest { + + // Tests validation works as planned with default instance. @Test public void testDefaultInstance() { Map testMap = new HashMap(); @@ -47,8 +50,9 @@ public void testDefaultInstance() { } } + // Tests validation works as planned with normal builder and config @Test - public void testLazyBuilderWithConfig() { + public void testNormalBuilderWithConfig() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); @@ -66,6 +70,8 @@ public void testLazyBuilderWithConfig() { } } + // Tests that if the correct seed descriptors are provided and lazy is disabled, + // validation works as planned. @Test public void testSeedDescriptorsLazyDisabled() { Map testMap = new HashMap(); @@ -90,6 +96,39 @@ public void testSeedDescriptorsLazyDisabled() { } } + // Tests that the seed descriptor list is immutable inside the validator and that if + // a descriptor is removed after the validator is created, validation still works as planned. + @Test + public void testSeedDescriptorsImmutable() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + List seedDescriptors = new ArrayList(); + FieldExpressionMapInt32 reg = FieldExpressionMapInt32.newBuilder().build(); + seedDescriptors.add(reg.getDescriptorForType()); + + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = + ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + + // Remove descriptor from list after the validator is created to verify validation still works + seedDescriptors.clear(); + + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + + // Tests that if a message is attempted to be validated and it wasn't in the initial + // list of seed descriptors AND lazy is disabled, that a message is returned that + // no evaluator is available. @Test public void testSeedDescriptorsWithWrongDescriptorAndLazyDisabled() { Map testMap = new HashMap(); @@ -114,6 +153,8 @@ public void testSeedDescriptorsWithWrongDescriptorAndLazyDisabled() { } } + // Tests that an IllegalStateException is thrown if an empty descriptor list is given + // and lazy is disabled. @Test public void testSeedDescriptorsWithInvalidState() { List seedDescriptors = new ArrayList(); @@ -124,6 +165,8 @@ public void testSeedDescriptorsWithInvalidState() { }); } + // Tests that when an empty list of seed descriptors is provided and lazy is enabled + // that the missing message descriptor is successfully built and validation works as planned. @Test public void testEmptySeedDescriptorsLazyEnabled() { Map testMap = new HashMap(); @@ -145,24 +188,48 @@ public void testEmptySeedDescriptorsLazyEnabled() { } } - // @Test - // public void testDisableLazy() { - // Map testMap = new HashMap(); - // testMap.put(42, 42); - // FieldExpressionMapInt32 msg = - // FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); - - // Config cfg = Config.newBuilder().setDisableLazy(true).build(); - // Validator validator = new Validator(cfg); - // try { - // System.err.println("goosh"); - // validator.loadMessages(FieldExpressionMapInt32.newBuilder().build()); - // ValidationResult result = validator.validate(msg); - // assertThat(result.isSuccess()).isFalse(); - // assertThat(result.getViolations().size()).isEqualTo(1); - // System.err.println(result); - // } catch (Exception e) { - // System.err.println("AAAAAAAAAAAAAAAAAAAAAAAAAa"); - // } - // } + // Tests that the config is applied when building a validator. + @Test + public void testConfigApplied() { + // Value must be at most 5 characters and must be lowercase alpha chars or numbers. + FieldExpressionMultiple msg = FieldExpressionMultiple.newBuilder().setVal("INVALID").build(); + + // Set fail fast to true, so we exit after the first validation failure. + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build(); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("value length must be at most 5 characters"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + + // Tests that the config is applied when building a validator with seed descriptors. + @Test + public void testConfigAppliedWithSeedDescriptors() { + // Value must be at most 5 characters and must be lowercase alpha chars or numbers. + FieldExpressionMultiple msg = FieldExpressionMultiple.newBuilder().setVal("INVALID").build(); + + FieldExpressionMultiple desc = FieldExpressionMultiple.newBuilder().build(); + List seedDescriptors = new ArrayList(); + seedDescriptors.add(desc.getDescriptorForType()); + + // Set fail fast to true, so we exit after the first validation failure. + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = + ValidatorFactory.newBuilder(seedDescriptors, false).withConfig(cfg).build(); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("value length must be at most 5 characters"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } } diff --git a/src/test/resources/proto/validationtest/validationtest.proto b/src/test/resources/proto/validationtest/validationtest.proto index cf388719..9d83fc68 100644 --- a/src/test/resources/proto/validationtest/validationtest.proto +++ b/src/test/resources/proto/validationtest/validationtest.proto @@ -53,6 +53,13 @@ message ExampleMessageRules { string secondary_email = 2; } +message FieldExpressionMultiple { + string val = 1 [ + (buf.validate.field).string.max_len = 5, + (buf.validate.field).string.pattern = "^[a-z0-9]$" + ]; +} + message FieldExpressionMapInt32 { map val = 1 [(buf.validate.field).cel = { id: "field_expression.map.int32" From 188c9250d88f63795e7f86d9c8bd46b34c26325e Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 11:51:49 -0400 Subject: [PATCH 06/14] More tests --- build.gradle.kts | 1 - 1 file changed, 1 deletion(-) diff --git a/build.gradle.kts b/build.gradle.kts index a7d15608..8f17533e 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -208,7 +208,6 @@ allprojects { tasks.withType().configureEach { useJUnitPlatform() this.testLogging { - this.showStandardStreams = true events("failed") exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL showExceptions = true From 87f095a9ae12b6144bdc375b290758858d1bb391 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 11:52:15 -0400 Subject: [PATCH 07/14] More tests --- .../src/main/java/build/buf/protovalidate/conformance/Main.java | 1 - 1 file changed, 1 deletion(-) diff --git a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java index ae7bdced..d0240912 100644 --- a/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java +++ b/conformance/src/main/java/build/buf/protovalidate/conformance/Main.java @@ -74,7 +74,6 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) { TestResult testResult = testCase(validator, descriptorMap, entry.getValue()); resultsMap.put(entry.getKey(), testResult); } - responseBuilder.putAllResults(resultsMap); return responseBuilder.build(); } catch (Exception e) { From ead86d2328f1ec2a1294679a8c9b8a171da04a45 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 11:53:03 -0400 Subject: [PATCH 08/14] More tests --- src/test/resources/proto/validationtest/validationtest.proto | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/resources/proto/validationtest/validationtest.proto b/src/test/resources/proto/validationtest/validationtest.proto index 9d83fc68..a2e9f213 100644 --- a/src/test/resources/proto/validationtest/validationtest.proto +++ b/src/test/resources/proto/validationtest/validationtest.proto @@ -17,7 +17,6 @@ syntax = "proto3"; package validationtest; import "buf/validate/validate.proto"; -import "google/protobuf/duration.proto"; message ExampleFieldRules { string regex_string_field = 1 [(buf.validate.field).string.pattern = "^[a-z0-9]{1,9}$"]; From c47dd61cc2700b70751f1a66d858441438905eb9 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Fri, 9 May 2025 12:40:07 -0400 Subject: [PATCH 09/14] Remove unused message --- .../resources/proto/validationtest/validationtest.proto | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/test/resources/proto/validationtest/validationtest.proto b/src/test/resources/proto/validationtest/validationtest.proto index a2e9f213..b73409b4 100644 --- a/src/test/resources/proto/validationtest/validationtest.proto +++ b/src/test/resources/proto/validationtest/validationtest.proto @@ -66,12 +66,3 @@ message FieldExpressionMapInt32 { expression: "this.all(k, this[k] == 1)" }]; } - -message FieldExpressionRepeatedScalar { - repeated int32 val = 1 [(buf.validate.field).cel = { - id: "field_expression.repeated.scalar" - message: "test message field_expression.repeated.scalar" - expression: "this.all(e, e == 1)" - }]; -} - From 1f176085edfa93616e45b6582ab664931c3b7a0f Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Tue, 13 May 2025 16:45:58 -0400 Subject: [PATCH 10/14] Feedback --- .../build/buf/protovalidate/EvaluatorBuilder.java | 5 +---- .../build/buf/protovalidate/ValidatorFactory.java | 15 +++------------ 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index 56060db5..bcc74ea7 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -51,7 +51,6 @@ class EvaluatorBuilder { private final Env env; private final boolean disableLazy; - private final List descriptors; private final RuleCache rules; /** @@ -62,7 +61,6 @@ class EvaluatorBuilder { */ EvaluatorBuilder(Env env, Config config) { this.env = env; - this.descriptors = new ArrayList(); this.disableLazy = false; this.rules = new RuleCache(env, config); } @@ -76,11 +74,10 @@ class EvaluatorBuilder { EvaluatorBuilder(Env env, Config config, List descriptors, boolean disableLazy) throws CompilationException { this.env = env; - this.descriptors = descriptors; this.disableLazy = disableLazy; this.rules = new RuleCache(env, config); - for (Descriptor descriptor : this.descriptors) { + for (Descriptor descriptor : descriptors) { this.build(descriptor); } } diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 34ff8c25..377bd30b 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -27,10 +27,10 @@ * be created with a list of seed descriptors to warmup the validator cache ahead of time as well as * an indicator to lazily-load any descriptors not provided into the cache. */ -public class ValidatorFactory { +public final class ValidatorFactory { // Prevent instantiation private ValidatorFactory() {} - ; + /** * An eager builder behaves the same as a regular Builder, but attempts to warmup the validator @@ -54,7 +54,7 @@ public static class EagerBuilder extends BaseBuilder { * @throws IllegalStateException If disableLazy is set to true and no descriptors are passed. */ public Validator build() throws CompilationException, IllegalStateException { - if (disableLazy && this.descriptors.size() == 0) { + if (disableLazy && this.descriptors.isEmpty()) { throw new IllegalStateException( "a list of descriptors is required when disableLazy is true"); } @@ -104,15 +104,6 @@ Builder self() { } } - /** - * A convenience function for creating a new validator with a default configuration. - * - * @return A Validator instance - */ - public static Validator defaultInstance() { - return newBuilder().build(); - } - /** * Creates a new builder for a validator. * From 77aa8654d228846cee6e460dc064e5f1d8944426 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 14 May 2025 11:25:44 -0400 Subject: [PATCH 11/14] Feedback --- .../buf/protovalidate/EvaluatorBuilder.java | 6 +- .../buf/protovalidate/ValidatorFactory.java | 126 ++++++------------ .../ValidatorCelExpressionTest.java | 4 +- .../ValidatorConstructionTest.java | 66 +++++++-- .../ValidatorDifferentJavaPackagesTest.java | 2 +- .../ValidatorDynamicMessageTest.java | 15 ++- 6 files changed, 109 insertions(+), 110 deletions(-) diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index bcc74ea7..63c7c36e 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -77,8 +77,10 @@ class EvaluatorBuilder { this.disableLazy = disableLazy; this.rules = new RuleCache(env, config); - for (Descriptor descriptor : descriptors) { - this.build(descriptor); + if (descriptors != null) { + for (Descriptor descriptor : descriptors) { + this.build(descriptor); + } } } diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 377bd30b..45463880 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -16,7 +16,6 @@ import build.buf.protovalidate.exceptions.CompilationException; import com.google.protobuf.Descriptors.Descriptor; -import java.util.Collections; import java.util.List; import org.jspecify.annotations.Nullable; @@ -30,60 +29,25 @@ public final class ValidatorFactory { // Prevent instantiation private ValidatorFactory() {} - - /** - * An eager builder behaves the same as a regular Builder, but attempts to warmup the validator - * cache with a given list of Descriptors. - */ - public static class EagerBuilder extends BaseBuilder { - private final List descriptors; - private final boolean disableLazy; - - EagerBuilder(List descriptors, boolean disableLazy) { - this.descriptors = Collections.unmodifiableList(descriptors); - this.disableLazy = disableLazy; - } + /** A builder class used for building a validator. */ + public static class ValidatorBuilder { + /** The config object to use for instantiating a validator. */ + @Nullable protected Config config; /** - * Build the validator, warming up the cache with any provided descriptors. + * Create a validator with the given config * - * @return A Validator instance - * @throws CompilationException If any of the given descriptors' validation rules fail - * processing while warming up the cache. - * @throws IllegalStateException If disableLazy is set to true and no descriptors are passed. + * @param config The {@link Config} to configure the validator. + * @return T the builder instance */ - public Validator build() throws CompilationException, IllegalStateException { - if (disableLazy && this.descriptors.isEmpty()) { - throw new IllegalStateException( - "a list of descriptors is required when disableLazy is true"); - } - - Config cfg = this.config; - if (cfg == null) { - cfg = Config.newBuilder().build(); - } - return new ValidatorImpl(cfg, this.descriptors, this.disableLazy); - } - - @Override - EagerBuilder self() { + public ValidatorBuilder withConfig(Config config) { + this.config = config; return this; } - boolean getDisableLazy() { - return this.disableLazy; - } - - List getDescriptors() { - return this.descriptors; - } - } - - /** A builder class used for building a validator. */ - public static class Builder extends BaseBuilder { - // Default constructor - private Builder() {} + // Prevent instantiation + private ValidatorBuilder() {} /** * Build a new validator @@ -98,53 +62,41 @@ public Validator build() { return new ValidatorImpl(cfg); } - @Override - Builder self() { - return this; - } - } + /** + * Build the validator, warming up the cache with any provided descriptors. + * + * @param descriptors the list of descriptors to warm up the cache. + * @param disableLazy whether to disable lazy loading of validation rules. When validation is + * performed, a message's rules will be looked up in a cache. If they are not found, by + * default they will be processed and lazily-loaded into the cache. Setting this to false + * will not attempt to lazily-load descriptor information not found in the cache and + * essentially makes the entire cache read-only, eliminating thread contention. + * @return A new {@link Validator} instance. + * @throws CompilationException If any of the given descriptors' validation rules fail + * processing while warming up the cache. + * @throws IllegalStateException If disableLazy is set to true and no descriptors are passed. + */ + public Validator buildWithDescriptors(List descriptors, boolean disableLazy) + throws CompilationException, IllegalStateException { + if (disableLazy && (descriptors == null || descriptors.isEmpty())) { + throw new IllegalStateException( + "a list of descriptors is required when disableLazy is true"); + } - /** - * Creates a new builder for a validator. - * - * @return A Validator builder - */ - public static Builder newBuilder() { - return new Builder(); + Config cfg = this.config; + if (cfg == null) { + cfg = Config.newBuilder().build(); + } + return new ValidatorImpl(cfg, descriptors, disableLazy); + } } /** * Creates a new builder for a validator. * - * @param descriptors the list of descriptors to warm up the cache. - * @param disableLazy whether to disable lazy loading of validation rules. When validation is - * performed, a message's rules will be looked up in a cache. If they are not found, by - * default they will be processed and lazily-loaded into the cache. Setting this to false will - * not attempt to lazily-load descriptor information not found in the cache and essentially - * makes the entire cache read-only, eliminating thread contention. * @return A Validator builder */ - public static EagerBuilder newBuilder(List descriptors, boolean disableLazy) { - return new EagerBuilder(descriptors, disableLazy); - } -} - -/** An abstract class that all validator builders should extend. */ -abstract class BaseBuilder> { - /** The config object to use for instantiating a validator. */ - @Nullable protected Config config; - - /** - * Create a validator with the given config - * - * @param config The {@link Config} to configure the validator. - * @return T the builder instance - */ - public T withConfig(Config config) { - this.config = config; - return self(); + public static ValidatorBuilder newBuilder() { + return new ValidatorBuilder(); } - - // Subclasses must specify their identity. - abstract T self(); } diff --git a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java index 8f1ecc9f..44288f69 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorCelExpressionTest.java @@ -75,7 +75,7 @@ public void testFieldExpressionRepeatedMessage() throws Exception { .setMessage("test message field_expression.repeated.message") .build(); - Validator validator = ValidatorFactory.defaultInstance(); + Validator validator = ValidatorFactory.newBuilder().build(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); @@ -144,7 +144,7 @@ public void testFieldExpressionRepeatedMessageItems() throws Exception { .setMessage("test message field_expression.repeated.message.items") .build(); - Validator validator = ValidatorFactory.defaultInstance(); + Validator validator = ValidatorFactory.newBuilder().build(); // Valid message checks ValidationResult validResult = validator.validate(validMsg); diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index 4cd24abe..e5b3f6de 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -31,14 +31,14 @@ public class ValidatorConstructionTest { - // Tests validation works as planned with default instance. + // Tests validation works as planned with default builder. @Test - public void testDefaultInstance() { + public void testDefaultBuilder() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); - Validator validator = ValidatorFactory.defaultInstance(); + Validator validator = ValidatorFactory.newBuilder().build(); try { ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); @@ -50,9 +50,9 @@ public void testDefaultInstance() { } } - // Tests validation works as planned with normal builder and config + // Tests validation works as planned with default builder and config @Test - public void testNormalBuilderWithConfig() { + public void testDefaultBuilderWithConfig() { Map testMap = new HashMap(); testMap.put(42, 42); FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); @@ -83,9 +83,12 @@ public void testSeedDescriptorsLazyDisabled() { seedDescriptors.add(reg.getDescriptorForType()); Config cfg = Config.newBuilder().setFailFast(true).build(); + + // Note that buildWithDescriptors throws the exception so the validator builder + // can be created ahead of time without having to catch an exception. + ValidatorFactory.ValidatorBuilder bldr = ValidatorFactory.newBuilder().withConfig(cfg); try { - Validator validator = - ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + Validator validator = bldr.buildWithDescriptors(seedDescriptors, true); ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); assertThat(result.getViolations().size()).isEqualTo(1); @@ -111,7 +114,7 @@ public void testSeedDescriptorsImmutable() { Config cfg = Config.newBuilder().setFailFast(true).build(); try { Validator validator = - ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + ValidatorFactory.newBuilder().withConfig(cfg).buildWithDescriptors(seedDescriptors, true); // Remove descriptor from list after the validator is created to verify validation still works seedDescriptors.clear(); @@ -142,7 +145,7 @@ public void testSeedDescriptorsWithWrongDescriptorAndLazyDisabled() { Config cfg = Config.newBuilder().setFailFast(true).build(); try { Validator validator = - ValidatorFactory.newBuilder(seedDescriptors, true).withConfig(cfg).build(); + ValidatorFactory.newBuilder().withConfig(cfg).buildWithDescriptors(seedDescriptors, true); ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); assertThat(result.getViolations().size()).isEqualTo(1); @@ -156,12 +159,23 @@ public void testSeedDescriptorsWithWrongDescriptorAndLazyDisabled() { // Tests that an IllegalStateException is thrown if an empty descriptor list is given // and lazy is disabled. @Test - public void testSeedDescriptorsWithInvalidState() { + public void testEmptySeedDescriptorsInvalidState() { List seedDescriptors = new ArrayList(); assertThatExceptionOfType(IllegalStateException.class) .isThrownBy( () -> { - ValidatorFactory.newBuilder(seedDescriptors, true).build(); + ValidatorFactory.newBuilder().buildWithDescriptors(seedDescriptors, true); + }); + } + + // Tests that an IllegalStateException is thrown if a null descriptor list is given + // and lazy is disabled. + @Test + public void testNullSeedDescriptorsInvalidState() { + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy( + () -> { + ValidatorFactory.newBuilder().buildWithDescriptors(null, true); }); } @@ -177,7 +191,31 @@ public void testEmptySeedDescriptorsLazyEnabled() { Config cfg = Config.newBuilder().setFailFast(true).build(); try { Validator validator = - ValidatorFactory.newBuilder(seedDescriptors, false).withConfig(cfg).build(); + ValidatorFactory.newBuilder() + .withConfig(cfg) + .buildWithDescriptors(seedDescriptors, false); + ValidationResult result = validator.validate(msg); + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getViolations().size()).isEqualTo(1); + assertThat(result.getViolations().get(0).toProto().getMessage()) + .isEqualTo("all map values must equal 1"); + } catch (ValidationException e) { + fail("unexpected exception thrown", e); + } + } + + // Tests that when a null list of seed descriptors is provided and lazy is enabled + // that the missing message descriptor is successfully built and validation works as planned. + @Test + public void testNullSeedDescriptorsLazyEnabled() { + Map testMap = new HashMap(); + testMap.put(42, 42); + FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); + + Config cfg = Config.newBuilder().setFailFast(true).build(); + try { + Validator validator = + ValidatorFactory.newBuilder().withConfig(cfg).buildWithDescriptors(null, false); ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); assertThat(result.getViolations().size()).isEqualTo(1); @@ -222,7 +260,9 @@ public void testConfigAppliedWithSeedDescriptors() { Config cfg = Config.newBuilder().setFailFast(true).build(); try { Validator validator = - ValidatorFactory.newBuilder(seedDescriptors, false).withConfig(cfg).build(); + ValidatorFactory.newBuilder() + .withConfig(cfg) + .buildWithDescriptors(seedDescriptors, false); ValidationResult result = validator.validate(msg); assertThat(result.isSuccess()).isFalse(); assertThat(result.getViolations().size()).isEqualTo(1); diff --git a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java index c7e87251..35b2642e 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDifferentJavaPackagesTest.java @@ -186,7 +186,7 @@ private void expectViolation(Message msg, Violation violation) throws Validation } private void expectViolations(Message msg, List expected) throws ValidationException { - Validator validator = ValidatorFactory.defaultInstance(); + Validator validator = ValidatorFactory.newBuilder().build(); List violations = validator.validate(msg).toProto().getViolationsList(); assertThat(violations).containsExactlyInAnyOrderElementsOf(expected); } diff --git a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java index 7f32bc1e..fc7fd73b 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorDynamicMessageTest.java @@ -78,7 +78,8 @@ public void testFieldRuleDynamicMessage() throws Exception { .setRuleId("string.pattern") .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); - ValidationResult result = ValidatorFactory.defaultInstance().validate(messageBuilder.build()); + ValidationResult result = + ValidatorFactory.newBuilder().build().validate(messageBuilder.build()); assertThat(result.toProto().getViolationsList()).containsExactly(expectedViolation); assertThat(result.getViolations().get(0).getFieldValue().getValue()).isEqualTo("0123456789"); assertThat(result.getViolations().get(0).getRuleValue().getValue()) @@ -98,7 +99,8 @@ public void testOneofRuleDynamicMessage() throws Exception { .setMessage("exactly one field is required in oneof") .build(); assertThat( - ValidatorFactory.defaultInstance() + ValidatorFactory.newBuilder() + .build() .validate(messageBuilder.build()) .toProto() .getViolationsList()) @@ -118,7 +120,8 @@ public void testMessageRuleDynamicMessage() throws Exception { .setMessage("cannot set a secondary email without setting a primary one") .build(); assertThat( - ValidatorFactory.defaultInstance() + ValidatorFactory.newBuilder() + .build() .validate(messageBuilder.build()) .toProto() .getViolationsList()) @@ -131,7 +134,8 @@ public void testRequiredFieldRuleDynamicMessage() throws Exception { createMessageWithUnknownOptions(ExampleRequiredFieldRules.getDefaultInstance()); messageBuilder.setField( messageBuilder.getDescriptorForType().findFieldByName("regex_string_field"), "abc123"); - assertThat(ValidatorFactory.defaultInstance().validate(messageBuilder.build()).getViolations()) + assertThat( + ValidatorFactory.newBuilder().build().validate(messageBuilder.build()).getViolations()) .isEmpty(); } @@ -164,7 +168,8 @@ public void testRequiredFieldRuleDynamicMessageInvalid() throws Exception { .setMessage("value does not match regex pattern `^[a-z0-9]{1,9}$`") .build(); assertThat( - ValidatorFactory.defaultInstance() + ValidatorFactory.newBuilder() + .build() .validate(messageBuilder.build()) .toProto() .getViolationsList()) From 1e854ac9e03b56ee02f2677087ed9356c54f12cf Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 14 May 2025 12:06:11 -0400 Subject: [PATCH 12/14] Update src/main/java/build/buf/protovalidate/ValidatorFactory.java Co-authored-by: Philip K. Warren --- src/main/java/build/buf/protovalidate/ValidatorFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 45463880..0eb82a1c 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -39,7 +39,7 @@ public static class ValidatorBuilder { * Create a validator with the given config * * @param config The {@link Config} to configure the validator. - * @return T the builder instance + * @return The builder instance */ public ValidatorBuilder withConfig(Config config) { this.config = config; From 42c1f374a3449525e8f4ba4a60be7dc316f7c3bd Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 14 May 2025 12:07:59 -0400 Subject: [PATCH 13/14] Update src/main/java/build/buf/protovalidate/ValidatorFactory.java Co-authored-by: Philip K. Warren --- src/main/java/build/buf/protovalidate/ValidatorFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/build/buf/protovalidate/ValidatorFactory.java b/src/main/java/build/buf/protovalidate/ValidatorFactory.java index 0eb82a1c..99595e9d 100644 --- a/src/main/java/build/buf/protovalidate/ValidatorFactory.java +++ b/src/main/java/build/buf/protovalidate/ValidatorFactory.java @@ -33,7 +33,7 @@ private ValidatorFactory() {} /** A builder class used for building a validator. */ public static class ValidatorBuilder { /** The config object to use for instantiating a validator. */ - @Nullable protected Config config; + @Nullable private Config config; /** * Create a validator with the given config From a8b78ba4d6212624b1491bce510aad60592c6cff Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 14 May 2025 12:26:00 -0400 Subject: [PATCH 14/14] Add Objects.requireNonNull instead of null check --- .../buf/protovalidate/EvaluatorBuilder.java | 7 +++-- .../ValidatorConstructionTest.java | 27 +++++++------------ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java index 63c7c36e..b2e05efb 100644 --- a/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java +++ b/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java @@ -73,14 +73,13 @@ class EvaluatorBuilder { */ EvaluatorBuilder(Env env, Config config, List descriptors, boolean disableLazy) throws CompilationException { + Objects.requireNonNull(descriptors, "descriptors must not be null"); this.env = env; this.disableLazy = disableLazy; this.rules = new RuleCache(env, config); - if (descriptors != null) { - for (Descriptor descriptor : descriptors) { - this.build(descriptor); - } + for (Descriptor descriptor : descriptors) { + this.build(descriptor); } } diff --git a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java index e5b3f6de..85177646 100644 --- a/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java +++ b/src/test/java/build/buf/protovalidate/ValidatorConstructionTest.java @@ -204,26 +204,17 @@ public void testEmptySeedDescriptorsLazyEnabled() { } } - // Tests that when a null list of seed descriptors is provided and lazy is enabled - // that the missing message descriptor is successfully built and validation works as planned. + // Tests that when a null list of seed descriptors is provided, a NullPointerException + // is thrown with a message that descriptors cannot be null. @Test public void testNullSeedDescriptorsLazyEnabled() { - Map testMap = new HashMap(); - testMap.put(42, 42); - FieldExpressionMapInt32 msg = FieldExpressionMapInt32.newBuilder().putAllVal(testMap).build(); - - Config cfg = Config.newBuilder().setFailFast(true).build(); - try { - Validator validator = - ValidatorFactory.newBuilder().withConfig(cfg).buildWithDescriptors(null, false); - ValidationResult result = validator.validate(msg); - assertThat(result.isSuccess()).isFalse(); - assertThat(result.getViolations().size()).isEqualTo(1); - assertThat(result.getViolations().get(0).toProto().getMessage()) - .isEqualTo("all map values must equal 1"); - } catch (ValidationException e) { - fail("unexpected exception thrown", e); - } + assertThatExceptionOfType(NullPointerException.class) + .isThrownBy( + () -> { + ValidatorFactory.newBuilder().buildWithDescriptors(null, false); + }) + .withMessageContaining("descriptors must not be null"); + ; } // Tests that the config is applied when building a validator.