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] Recursive validation skipped depending on field order #141

Closed daemonl closed 3 months ago

daemonl commented 3 months ago

Description

A bit difficult to describe, but there are circumstances where the validator will incorrectly skip a message.

https://github.com/daemonl/pvbug is a full demo

Validating an empty 'FieldWithIssue' gives a validation error, correctly.

When nested as OneTwo.F1.field, it also gives the validation error.

However, when nested as TwoOne.F1.field, and when the need_this string is set to a value, it does not fail validation.

The number of the fields doesn't seem to matter, only the order they appear in.

message OneTwo {
  F1 field1 = 1;
  F2 field2 = 2;
}

message TwoOne {
  F2 field2 = 1;
  F1 field1 = 2;
}

message F2 {
  FieldWithIssue field = 1;
}

message F1 {
  string need_this = 1; // This needs a value, leaving it empty validates correctly
  FieldWithIssue field = 2;
}

message FieldWithIssue {
  F1 f1 = 1; // This must be before the name string, but does not need a value
  string name = 2 [(buf.validate.field).string.min_len = 1];
}

Steps to Reproduce

$ git clone https://github.com/daemonl/pvbug
$ cd pvbug
$ buf generate && go test ./...

Expected Behavior

Expected to see the message marked as invalid in all cases

Actual Behavior

    --- FAIL: TestLocalJ5 (0.01s)
    --- FAIL: TestLocalJ5/Wrapped_in_order_2_1 (0.00s)
        test_test.go:75: Fresh Validation should have failed
FAIL
FAIL    github.com/daemonl/pvbug        0.271s
FAIL

Environment

Possible Solution

I'm gonna guess it's something to do with recursive lazy loading

rodaine commented 3 months ago

Thanks for the very complete report, @daemonl! #142 fixes this by skipping a very minor optimization that was subtly breaking the evaluator.