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

Issues with the PROTOVALIDATE rules when linting a proto file with optional fields and protovalidate constraints #2589

Closed
mivanovcpd opened this issue Nov 14, 2023 · 5 comments · Fixed by #2590
Labels
Bug Something isn't working

Comments

@mivanovcpd
Copy link

The linting rules for protovalidate do not support optional fields with field constraints.

To reproduce:

In a proto file:
optional string abc = 1 [(buf.validate.field).required = true];

And then use as rules for running buf:

version: v1
breaking:
  use:
    - FILE
lint:
  use:
    - DEFAULT
deps:
  - buf.build/bufbuild/protovalidate

Run the linter(I use npx @bufbuild/buf lint).

You will get something similar to:

file.proto:33:39:Field "abc" has (buf.validate.field).required but is in a oneof (_abc). Oneof fields must not have (buf.validate.field).required.

Workaround is to disable the PROTOVALIDATE rules, but I think that's a bad and temporary solution as you lose out on validation.

Please ask if additional context is needed.

@mivanovcpd
Copy link
Author

As for why optional string abc = 1 [(buf.validate.field).required = true]; is meaningful => it allows you to differentiate between the field abc not being set at all and being set to "", basically allowing explicit field presence.

This is similar to how in OpenAPI terms you could have a JSON field of type string that's required, and you could set it to an empty string and that's still valid, but if you remove the field from the JSON, it will then complain it's missing.

@bufdev
Copy link
Member

bufdev commented Nov 14, 2023

This is definitely a bug - we'll fix.

@bufdev bufdev added the Bug Something isn't working label Nov 14, 2023
oliversun9 added a commit that referenced this issue Nov 14, 2023
Fixes #2589.

This PR fixes the bug where `buf lint` incorrectly report error on
proto3 optional fields.
@oliversun9
Copy link
Contributor

Thanks for reporting this bug! This is fixed in #2590 and will go out in the next release. Sorry for the inconvenience

@bufdev
Copy link
Member

bufdev commented Nov 15, 2023

This has been fixed in v1.28.1, sorry for the troubles.

@mivanovcpd
Copy link
Author

Thanks for the kind comments and quick fix. Really appreciate it 👍

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
Development

Successfully merging a pull request may close this issue.

3 participants