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

Add protovalidate lint rules #2473

Merged
merged 39 commits into from
Oct 31, 2023
Merged

Add protovalidate lint rules #2473

merged 39 commits into from
Oct 31, 2023

Conversation

oliversun9
Copy link
Contributor

@oliversun9 oliversun9 commented Oct 2, 2023

This PR adds PROTOVALIDATE rule to buf lint.
It checks

  • all CEL expressions compile
  • rules permit some value. For example, gt: 5 and lt: 1 reject all values
  • rules match the type of the field. Wrapper types can also match rules for its value type
  • rules are not obviously redundant. For example, if const is specified, other rules are obviously redundant. However, if string.min_len is 5 and string.min_bytes is 4, the latter is redundant but this is not obvious. (I initially had a quite many redundancy checks. I removed them after discussing with @rodaine)

If the .proto author wrote options in non-aggregate form, buf lint can pinpoint. If the options are written in the aggregate form, then everything between the braces gets highlighted, since that's how source code locations are generated.

Non-aggregate

 uint32 a = 5 [
   (buf.validate.field).uint32.lt = 10,
   // only the next line is highlighted
   (buf.validate.field).uint32.const = 1,
   (buf.validate.field).uint32.gt = 8
 ];

Aggregate

uint32 a = 5 [
  // the next 5 lines are highlighted
  (buf.validate.field).uint32 = {
    lt: 10,
    const: 1,
    gt: 8
  }
];

go.mod Outdated Show resolved Hide resolved
@oliversun9 oliversun9 requested review from bufdev, rodaine and jhump October 2, 2023 22:50
@oliversun9 oliversun9 requested a review from jhump October 3, 2023 20:45
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

This is approved given the following:

  • Add a test that will fail given the custom parsing of cel-go's error messages.
  • We get the PR for the rest of protovalidate rules up in parallel. I'd like to merge them together, and to evaluate the final name for the respective lint rules (and perhaps combine them into one).

@bufdev bufdev mentioned this pull request Oct 4, 2023
go.mod Outdated
Comment on lines 84 to 86
// The Buf CLI is the one library that should not depend on BSR generated SDKs
// as this could cause bootstrapping errors.
replace buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate => ./private/gen/proto/go/buf/validate
Copy link
Member

Choose a reason for hiding this comment

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

Is this true? What bootstrapping errors do we expect? If we're using the buf CLI to work on buf, we're working with a binary where the remote packages are already resolved and compiled, so I don't see how this is a problem here.

I was under the impression that it's the core repo that must not depend on generated SDKs. Note that this directive here will do nothing to prevent a cyclic dependency in core since replace directives in dependencies are ignored by the go tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're working with a binary where the remote packages are already resolved and compiled

The buf CLI's binary itself will be fine, since like you said, it's just a binary.

Note that this directive here will do nothing to prevent a cyclic dependency in core since replace directives in dependencies are ignored by the go tool.

Yes I think this is the concern and we will also need to add a replace directive there, before or around the time when this PR is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think this is the concern

So we can remove the replace directive here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, and with the reasoning above I think it's probably fine? @bufdev what do you think?

This PR adds `PROTOVALIDATE` rule to `buf lint`.
It checks 
- all CEL expressions compile
- rules permit some value. For example, `gt: 5` and `lt: 1` reject all
values
- rules match the type of the field. Wrapper types can also match rules
for its value type
- rules are not obviously redundant. For example, if `const` is
specified, other rules are obviously redundant. However, if
`string.min_len` is 5 and `string.min_bytes` is 4, the latter is
redundant but this is not obvious. (I initially had a quite many
redundancy checks. I removed them after discussing with @rodaine)

If the `.proto` author wrote options in non-aggregate form, `buf lint`
can pinpoint. If the options are written in the aggregate form, then
everything between the braces gets highlighted, since that's how source
code locations are generated.

Non-aggregate
 ```
  uint32 a = 5 [
    (buf.validate.field).uint32.lt = 10,
    // only the next line is highlighted
    (buf.validate.field).uint32.const = 1,
    (buf.validate.field).uint32.gt = 8
  ];
  ``` 
  
Aggregate
  ```
  uint32 a = 5 [
    // the next 5 lines are highlighted
    (buf.validate.field).uint32 = {
      lt: 10,
      const: 1,
      gt: 8
    }
  ];
```

---------

Co-authored-by: jchadwick-buf <[email protected]>
Co-authored-by: bufdev <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Philip K. Warren <[email protected]>
Co-authored-by: Michael Fridman <[email protected]>
Co-authored-by: buf-release-bot[bot] <116301919+buf-release-bot[bot]@users.noreply.github.com>
Co-authored-by: doriable <[email protected]>
Co-authored-by: Doria Keung <[email protected]>
Co-authored-by: Stefan VanBuren <[email protected]>
@oliversun9 oliversun9 changed the title Add cel linting to buf lint Add protovalidate lint rules Oct 31, 2023
@oliversun9 oliversun9 merged commit 6b2bc3d into main Oct 31, 2023
6 checks passed
@oliversun9 oliversun9 deleted the osun/buf-lint-cel branch October 31, 2023 16:26
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.

4 participants