bufbuild / protovalidate-go

Protocol Buffer Validation for Go
https://pkg.go.dev/github.com/bufbuild/protovalidate-go
Apache License 2.0
294 stars 19 forks source link

[BUG] Processing a message that contains multiple unique constraints causes a runtime error. #37

Closed kaz closed 1 year ago

kaz commented 1 year ago

Description

If a single message contains multiple repeated fields, each of different types, and each type has a unique constraint, a runtime error occurs.

Steps to Reproduce

  1. Uncomment the following line: https://github.com/bufbuild/protovalidate-go/blob/b7c1481c0bfc1e2f48ba4eff8299d1b4f2f7b15c/proto/tests/example/v1/validations.proto#L109
  2. Run buf generate.
  3. Execute go test -run TestValidator_ValidateRepeatedFoo.

Expected Behavior

The test passes without a runtime error.

Actual Behavior

A runtime error occurs.

Screenshots/Logs

--- FAIL: TestValidator_ValidateRepeatedFoo (0.01s)
    validator_test.go:111:
                Error Trace:    /Users/kaz/Desktop/protovalidate-go/validator_test.go:111
                Error:          Received unexpected error:
                                runtime error: error evaluating repeated.unique: no such overload: unique(list)
                Test:           TestValidator_ValidateRepeatedFoo
FAIL
exit status 1
FAIL    github.com/bufbuild/protovalidate-go    0.254s

Environment

Possible Solution

As a workaround, it's suggested to implement the unique constraint as a custom constraint. It seems to work when dynamically dispatched using dyn.

message MsgHasRepeated {
  repeated float x = 1 [
    (buf.validate.field).repeated = {
      max_items: 3,
      min_items: 1,
      items: {
        cel: {
          expression: "true",
          message: "intentional false"
        }
        float: {gt: 0}
      }
    },
    (buf.validate.field).cel = {
      /* workaround */
      expression: "dyn(this).unique()",
      message: "repeated value must contain unique items"
    }
  ];
  repeated string y = 2 [(buf.validate.field).cel = {
    /* workaround */
    expression: "dyn(this).unique()",
    message: "repeated value must contain unique items"
  }];
  repeated HasMsgExprs z = 3 [(buf.validate.field).repeated = {max_items: 2}];
}

Additional Context

The method loadOrCompileStandardConstraint in *github.com/bufbuild/protovalidate-go/internal/constraints.Cache uses only protoreflect FieldDescriptor as the cache key and does not consider the type of the field being constrained. As a result, it seems that it might return an expression.ASTSet that calls the incorrect overload of the unique.

For instance, in the example above, looking inside the AST of the unique constraint for the y field reveals that the overload double_unique_bool has been chosen. (It's expected to select string_unique_bool.)

I think that this issue could be resolved by either making the implementation of the Standard constraint dynamically dispatched or by including type information when caching the compile results of the Standard constraint.

elliotmjackson commented 1 year ago

hey @kaz, thanks for taking the time to write up so much detail! this really helps us drill down on whats gone wrong - i'll get around to investigating this further later today and get back to you with some answers shortly after.