Add field and rule value to violations #215

ghost · 2024-12-04T19:07:57Z

Adds the ability to access the captured rule and field value from a Violation.

There's a bit of refactoring toil, so I tried to split the commits cleanly to make this easier to review.

The first commit adds the Violation wrapper class and plumbs it through all uses of Violation.
The second commit makes Value non-internal so we can use it to expose protobuf values in a cleaner fashion.
The third commit actually implements filling the fieldValue and ruleValuefields.

This is a breaking change. The API changes in the following ways:

ValidationResult now provides a new wrapper Violation type instead of the buf.validate.Violation message. This new wrapper has a getProto() method to return a buf.validate.Violation message, and ValidationResult now has a toProto() method to return a buf.validate.Violations message.

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

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

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

This reverts commit 87ee0da.

ghost · 2024-12-05T15:23:25Z

I'm going to try to pick up the violation accumulator change today if possible. Please feel free to review what's here though. Particularly interested if there's any way we can clean up the Violation API surface to be a bit nicer.

ghost · 2024-12-06T10:29:44Z

I did a bit of refactorings:

Now Violation is an interface. There's an internal ConstraintViolation that implements it, and we stay in Builders until the end.
Value is still private, but there's a Violation.FieldValue interface to encapsulate the Object and FieldDescriptor for the field and rule values. This seems a bit less awkward.
Wrapper interfaces, introduced just recently for structured field paths, are removed. Instead, I just mirrored what I started doing in the pv-go PR instead: carrying the necessary data on the Value. A bit tricky, but far simpler.
(Also, no more need to awkwardly re-build violation/rule paths everywhere anymore. We can build them incrementally.)

This necessitated some reshuffling that may not be intuitive and I'm sure there's ugly things in here but I wanted to get this out in some shape before the weekend.

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

jhump · 2024-12-09T15:50:52Z

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

-    @Nullable Value fieldValue;
-    @Nullable Value ruleValue;
-
+public interface Violation {


I like it!!

jhump · 2024-12-09T15:52:53Z

src/main/java/build/buf/protovalidate/internal/errors/ConstraintViolation.java

+ * {@link ConstraintViolation} contains all of the collected information about an individual
+ * constraint violation.
+ */
+public class ConstraintViolation implements Violation {


I guess this is the pattern everything else uses -- public access but in an "internal" package.

It's a bit unfortunate that this uses Go idioms instead of Java ones, though I guess the Java idiom (using default/package-private access) would require dumping everything into one package, which is admittedly nasty in other ways...

Yep, same feelings here. I don't love it exactly, but actually it seems like a lot of Java software winds up doing basically this sort of thing, since there's really not much flexibility w.r.t. visibility.

src/main/java/build/buf/protovalidate/internal/evaluator/EmbeddedMessageEvaluator.java

jhump · 2024-12-09T16:57:10Z

src/main/java/build/buf/protovalidate/internal/evaluator/EvaluatorBase.java

+import java.util.List;
+import javax.annotation.Nullable;
+
+class EvaluatorBase {


Instead of extending this in every evaluator type, I think it would be better to use composition and maybe call it a ConstraintViolationHelper -- since it's really just used to get values for building a violation.

There's a lot of places that now call super(evaluator) that IMO don't look intuitive. Also, this doesn't provide any base Evaluator method implementations so it is awkward as a base class.

Yeah, I was going to try to extract more common functionality but it didn't work out so it's a bit pointless now. Now it's ConstraintViolationHelper and not being used as a base class.

jhump · 2024-12-09T16:59:58Z

src/main/java/build/buf/protovalidate/internal/evaluator/AnyEvaluator.java

+      ConstraintViolation.Builder violation =
+          ConstraintViolation.newBuilder()
+              .addAllRulePathElements(getRulePrefixElements())
+              .addAllRulePathElements(IN_RULE_PATH.getElementsList())
+              .addFirstFieldPathElement(getFieldPathElement())
+              .setConstraintId("any.in")
+              .setMessage("type URL must be in the allow list")
+              .setFieldValue(new ConstraintViolation.FieldValue(val))
+              .setRuleValue(new ConstraintViolation.FieldValue(this.inValue, IN_DESCRIPTOR));


It would be nice to try to wrap some of this up into a helper in EvaluatorBase, to de-boiler-plate the various calls that do this.

It looks like they all call ConstraintViolation.newBuilder().addAllRulePathElements(getRulePrefixElements()), but the other methods vary slightly. Many also call .addFirstFieldPathElement(getFieldPathElement()), but some do not. Anyhow, maybe not worth looking into since they all seem to vary a little -- moving the builder pattern steps into method arguments would certainly make call sites more concise, but potentially at the cost of readability, so maybe not worth doing more here...

I am leaning on the other direction (not having EvaluatorBase) instead, since I'm on the side of this not being worth it, at least for now. It does feel like there's likely something more elegant to pull out of this, but with all of the edge cases we have right now it's not currently apparent.

jhump

LGTM. One tiny nit, but not blocking if you disagree with it. If you do make the change, feel free to merge w/out further review.

jhump · 2024-12-12T16:53:15Z

src/main/java/build/buf/protovalidate/internal/evaluator/AnyEvaluator.java

 */
-class AnyEvaluator extends EvaluatorBase implements Evaluator {
+class AnyEvaluator implements Evaluator {
+  private final ConstraintViolationHelper constraintViolationHelper;


nit: I would call the field helper (here and throughout) to make the call sites more readable. They are so verbose, it's a bit of a chore to read or scan the code with this long name.

I accidentally made my Java a little too Java. Fixed.

…d nested (#246) `protovalidate-java` cannot use a CEL expression where `this` represents a repeated message. `EvaluatorBuilder`'s `processFieldExpression` is currently directly using `Decls.newObjectType()` to create an object type based on the name of the message type in the field instead of honoring whether or not the field is repeated and returning a `Decls.newListType()`. `provalidate` [recently added](bufbuild/protovalidate#329) conformance tests that illustrate this problem. To fix this, this PR: 1. Adds JUnit tests and `.proto` that demonstrate the issue. 2. Updates `EvaluatorBuilder`'s `processFieldExpression` to delegate the determination of the CEL type to `DescriptorMappings.getCELType()`, which uses the `FieldDescriptor` to determine the correct type to return. This method requires passing a `forItems` flag stating if the type returned should be the list (or map) type for a field or the type of the items in the collection. Using the [new `nestedRule` property](#215 state provides the value of this flag, [similar to how it's used in `processStandardConstraints`](https://github.com/bufbuild/protovalidate-java/blob/b3cd73121d04182dc989b76902e468f94b5f0b99/src/main/java/build/buf/protovalidate/EvaluatorBuilder.java#L421) to pass the `forItems` flag to `constraintCache.compile()`. 3. Because there were now 7 places where `valueEvaluator.getNestedRule() == null` was used to determine behavior, I moved this comparison into a `hasNestedRule()` method on `ValueEvaluator`, using conformance state to check regression. Conformance notes: This PR passes conformance in `protovalidate-java`. It has been tested against the conformance suite at the current head of `protovalidate` (which now includes the standard library, so there are many new failures). Before changes in this PR, there were 137 failures. After, there were 135: ``` BEFORE: FAIL (failed: 137, skipped: 0, passed: 2681, total: 2818) AFTER : FAIL (failed: 135, skipped: 0, passed: 2683, total: 2818) ``` Running `field_expression/repeated/message` shows that the two conformance tests fixed were for the desired issue: ``` BEFORE: FAIL (failed: 2, skipped: 0, passed: 2, total: 4) AFTER: PASS (failed: 0, skipped: 0, passed: 4, total: 4) ```

John Chadwick added 3 commits December 4, 2024 12:42

Add Violation wrapper class

20051a6

Make the Value interface non-internal

87ee0da

Add fieldValue and ruleValue to Violation

6014a64

ghost requested review from jhump and pkwarren December 4, 2024 19:07

jhump reviewed Dec 4, 2024

View reviewed changes

John Chadwick added 5 commits December 5, 2024 09:37

Violation.Builder: Add missing visibility modifiers

69e84df

Violation.Builder: Move required arg to constructor

456273e

Violation: Rename getProto -> toProto

9123d25

Revert "Make the Value interface non-internal"

a512d02

This reverts commit 87ee0da.

Remove Value usages from public API

c5f3d18

Refactor Violation entirely

867f237

jhump approved these changes Dec 9, 2024

View reviewed changes

Address review feedback

362b5bf

jhump approved these changes Dec 12, 2024

View reviewed changes

constraintViolationHelper -> helper

485ebea

ghost merged commit df11bf2 into main Dec 12, 2024
4 checks passed

ghost deleted the jchadwick/rule-field-value branch December 12, 2024 20:03

ghost mentioned this pull request Jan 15, 2025

Add structured field and rule paths to Violation bufbuild/protovalidate#265

Merged

jrinehart-buf mentioned this pull request Mar 17, 2025

Jrinehart/tcn 2782 protovalidate java custom rules cannot use repeated nested #246

Merged

This pull request was closed.

Add field and rule value to violations #215

Add field and rule value to violations #215

Uh oh!

Conversation

ghost commented Dec 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 5, 2024

Uh oh!

ghost commented Dec 6, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhump left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost commented Dec 6, 2024 •

edited by ghost

Loading