Skip to content

Conversation

@jrinehart-buf
Copy link
Contributor

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 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's state provides the value of this flag, similar to how it's used in processStandardConstraints 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)

@jrinehart-buf jrinehart-buf requested a review from a user March 17, 2025 15:46
@jrinehart-buf jrinehart-buf marked this pull request as ready for review March 17, 2025 15:46
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch. LGTM.

@rodaine rodaine merged commit fac49b4 into main Mar 17, 2025
5 checks passed
@rodaine rodaine deleted the jrinehart/tcn-2782-protovalidate-java-custom-rules-cannot-use-repeated-nested branch March 17, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants