Refactor validator creation to support various configurations (#283)

Fixes #51 

This makes a series of changes to the way that a validator is created.
Doing so enables more flexible ways to configure a validator and also
provides a bit more safety when attempting to create a validator with
invalid state. The impetus behind this change was to allow for the
ability to specify a list of seed descriptors when building a validator
and for the optional ability to disable lazy-loading of any descriptors
not provided.

Previously, a validator could be created either with a default
constructor (`new Validator()`) or via a `Config` object (`new
Validator(config)`). However, the `Config` object had no way to provide
a list of descriptors, it only had a `disableLazy` field. Simply adding
a list of descriptors to the `Config` presented a few problems though:

* Since these were two unrelated fields in the config, users had a
serious footgun to play with because it was possible to set
`disableLazy` to true and not specify any descriptors. Doing so would
return an error at validation time with a validation message about an
unknown evaluator.
* Even if the above was tolerable, building validation rules from
descriptors necessitates catching a `CompilationException`. This meant
that if the seed descriptors were added to the config, all users
creating a validator would now have to catch a checked exception even if
they weren't using the seed descriptors.

The solution in this PR converts validator creation from using
constructors into a builder pattern with the following changes:
* `disableLazy` and the new `descriptors` list have been moved from the
config and are now parameters at instantiation time.
* There are two options for building once the builder is created:
  *  `build()`
* `buildWithDescriptors(List<Descriptor>, boolean disableLazy) throws
CompilationException, InvalidStateException`

The overload that accepts seed descriptors is the only one that throws a
`CompilationException`. If users choose to use `build` with no
arguments, no checked exception needs to be handled.

Note that the above also allows us to provide a helpful check at
instantiation time if a user sets `disableLazy` to true but doesn't
provide any descriptors. In this case, an `IllegalStateException` is
thrown. It is still possible to provide an _incorrect_ list of
descriptors, but that is impossible to catch at instantiation time (but
hopefully is now a bit harder to do).

The new API now adds a `ValidatorFactory` class with static methods for
creating a validator. In addition, the type returned is now an interface
(still named `Validator`) which provides us with a bit more flexibility
both for testing and for making non-breaking changes in the future.

Examples of creating a validator with no seed descriptors:

```java
// Create a Validator with a default config
Validator validator = ValidatorFactory.newBuilder().build();

// Create a Validator with a custom config
Config cfg = Config.newBuilder().setFailFast(true).build();
Validator validator = ValidatoryFactory.newBuilder().withConfig(cfg).build();
```

Creating a validator with seed descriptors is similar. You would still
call `newBuilder`, but at build time, you call `buildWithDescriptors`
and pass the descriptors and set `disableLazy` accordingly. Using this
approach requires catching exceptions at build time:

```java
// Create a validator with seed descriptors
MyMessage msg = MyMessage.newBuilder().build();
List<Descriptor> seedDescriptors = new ArrayList<Descriptor>();
seedDescriptors.add(msg.getDescriptorForType());

try {
    Validator validator = ValidatorFactory.newBuilder().buildWithDescriptors(seedDescriptors, false);

    // Note you can also create a validator with a config
    // Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).buildWithDescriptors(seedDescriptors, false);

    // Also note that the buildWithDescriptors function is what throws the exception so the validator 
    // builder can be created ahead of time with no need for catching an exception.
} catch (CompilationException ce) {
    // Exception pre-warming the cache with the seed descriptors
} catch (IllegalStateException ise) {
    // This is thrown if seedDescriptors is empty and disableLazy is true
}
```

Note: **THIS IS A BREAKING CHANGE**, but the hope is that this makes it
easier and more intuitive for creating a validator while providing
future-proof flexibility. To migrate, users should make the following
changes:

**With no config**

```diff
import build.buf.protovalidate.Validator;
+ import build.buf.protovalidate.ValidatorFactory;

- Validator validator = new Validator();
+ Validator validator = ValidatorFactory.newBuilder().build();
```

**With config**

```diff
import build.buf.protovalidate.Validator;
+ import build.buf.protovalidate.ValidatorFactory;

Config cfg = Config.newBuilder().setFailFast(true).build();
- Validator validator = new Validator(cfg);
+ Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build();
```

In addition, the `loadMessages` and `loadDescriptors` methods from the
validator have been removed. Users should instead use the
`newBuilder(List<Descriptor> descriptors, boolean disableLazy)` approach
to load descriptors ahead of time.

---------

Co-authored-by: Philip K. Warren <[email protected]>

Author: smaye81

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,12 +61,13 @@ static TestConformanceResponse testConformance(TestConformanceRequest request) {
       TypeRegistry typeRegistry = FileDescriptorUtil.createTypeRegistry(fileDescriptorMap.values());
       ExtensionRegistry extensionRegistry =
           FileDescriptorUtil.createExtensionRegistry(fileDescriptorMap.values());
-      Validator validator =
-          new Validator(
-              Config.newBuilder()
-                  .setTypeRegistry(typeRegistry)
-                  .setExtensionRegistry(extensionRegistry)
-                  .build());
+      Config cfg =
+          Config.newBuilder()
+              .setTypeRegistry(typeRegistry)
+              .setExtensionRegistry(extensionRegistry)
+              .build();
+      Validator validator = ValidatorFactory.newBuilder().withConfig(cfg).build();
+
       TestConformanceResponse.Builder responseBuilder = TestConformanceResponse.newBuilder();
       Map<String, TestResult> resultsMap = new HashMap<>();
       for (Map.Entry<String, Any> entry : request.getCasesMap().entrySet()) {

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

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);
     }
   }
 }

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

@@ -59,12 +59,30 @@ 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.disableLazy = config.isDisableLazy();
+    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.
+   */
+  EvaluatorBuilder(Env env, Config config, List<Descriptor> 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);
+
+    for (Descriptor descriptor : 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}.
@@ -73,7 +91,7 @@ public EvaluatorBuilder(Env env, Config config) {
    * @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);

src/main/java/build/buf/protovalidate/Validator.java

@@ -17,43 +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.List;
-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 {
-  /** 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;
-
-  /**
-   * 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();
-    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
@@ -67,47 +34,5 @@ public Validator() {
    * @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<RuleViolation.Builder> result = evaluator.evaluate(new MessageValue(msg), failFast);
-    if (result.isEmpty()) {
-      return ValidationResult.EMPTY;
-    }
-    List<Violation> violations = new ArrayList<>(result.size());
-    for (RuleViolation.Builder builder : result) {
-      violations.add(builder.build());
-    }
-    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());
-    }
-  }
-
-  /**
-   * 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);
-    }
-  }
+  ValidationResult validate(Message msg) throws ValidationException;
 }

src/main/java/build/buf/protovalidate/ValidatorFactory.java

@@ -0,0 +1,102 @@
+// 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.List;
+import org.jspecify.annotations.Nullable;
+
+/**
+ * ValidatorFactory is used to create a validator.
+ *
+ * <p>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 final class ValidatorFactory {
+  // Prevent instantiation
+  private ValidatorFactory() {}
+
+  /** A builder class used for building a validator. */
+  public static class ValidatorBuilder {
+    /** The config object to use for instantiating a validator. */
+    @Nullable private Config config;
+
+    /**
+     * Create a validator with the given config
+     *
+     * @param config The {@link Config} to configure the validator.
+     * @return The builder instance
+     */
+    public ValidatorBuilder withConfig(Config config) {
+      this.config = config;
+      return this;
+    }
+
+    // Prevent instantiation
+    private ValidatorBuilder() {}
+
+    /**
+     * Build a new validator
+     *
+     * @return A new {@link Validator} instance.
+     */
+    public Validator build() {
+      Config cfg = this.config;
+      if (cfg == null) {
+        cfg = Config.newBuilder().build();
+      }
+      return new ValidatorImpl(cfg);
+    }
+
+    /**
+     * 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<Descriptor> 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");
+      }
+
+      Config cfg = this.config;
+      if (cfg == null) {
+        cfg = Config.newBuilder().build();
+      }
+      return new ValidatorImpl(cfg, descriptors, disableLazy);
+    }
+  }
+
+  /**
+   * Creates a new builder for a validator.
+   *
+   * @return A Validator builder
+   */
+  public static ValidatorBuilder newBuilder() {
+    return new ValidatorBuilder();
+  }
+}