Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Recursive validation skipped depending on field order #141

Closed
daemonl opened this issue Aug 28, 2024 · 1 comment · Fixed by #142
Closed

[BUG] Recursive validation skipped depending on field order #141

daemonl opened this issue Aug 28, 2024 · 1 comment · Fixed by #142
Labels
Bug Something isn't working

Comments

@daemonl
Copy link

daemonl commented Aug 28, 2024

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

  • Operating System: macOS - M1
  • Version: macOS Sonoma 14.6.1
  • Compiler/Toolchain: go1.22.4 darwin/arm64
  • Protobuf Compiler & Version: buf 1.33.0
  • Protoc-gen-validate Version: ?
  • Protovalidate Version v0.6.4

Possible Solution

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

@rodaine
Copy link
Member

rodaine commented Aug 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
2 participants