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

Update cel expressions for standard constraints with bool type #252

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

oliversun9
Copy link
Contributor

This fixes the problem where

message Foo {
  string bar = 1 [(buf.validate.field).string.tuuid = false];
}

still validates for tuuid by updating cel expressions to pass if the option is set to false.

Copy link

github-actions bot commented Sep 11, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 11, 2024, 9:38 PM

@rodaine
Copy link
Member

rodaine commented Sep 11, 2024

Can we add conformance tests that check these as well as run it against protovalidate-go to verify? Thanks @oliversun9!

Copy link
Member

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

I think the changes mostly LGTM though I echo the sentiment re: adding conformance test cases, definitely a good idea.

proto/protovalidate/buf/validate/validate.proto Outdated Show resolved Hide resolved
@oliversun9
Copy link
Contributor Author

Can we add conformance tests that check these as well as run it against protovalidate-go to verify? Thanks @oliversun9!

I added tests and build pvgo's conformance test binary with locally generated protovalidate code, and they pass 😄

go build -o .tmp/bin/protovalidate-conformance ./tools/protovalidate-conformance
PASS (failed: 0, skipped: 0, passed: 1800, total: 1800)

Copy link
Member

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

LGTM, passes just fine. This does however make me realize that I need to improve the reparse logic on my branch. Even with my new improvements, I still couldn't get conformance passing without updating the gencode. (This is wrong, conformance should pass in the future even with completely mismatched gencode.)

@oliversun9 oliversun9 merged commit f492418 into main Sep 16, 2024
5 checks passed
@oliversun9 oliversun9 deleted the osun/fix-bool-rules-presence branch September 16, 2024 18:28
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