bufbuild / protovalidate

Protocol Buffer Validation - Go, Java, Python, and C++ Beta Releases!
https://buf.build/bufbuild/protovalidate
Apache License 2.0
908 stars 37 forks source link

Implement predefined field constraints #246

Closed jchadwick-buf closed 1 month ago

jchadwick-buf commented 2 months ago

A predefined constraint in this schema looks like this:

edition = "2023";

package example.v1;

import "buf/validate/validate.proto";

extend buf.validate.StringRules {
  bool valid_path = 1162 [
    (buf.validate.predefined).cel = {
      id: "string.valid_path"
      expression: "!rule && !this.matches('^([^/.][^/]?|[^/][^/.]|[^/]{3,})(/([^/.][^/]?|[^/][^/.]|[^/]{3,}))*$') ? 'not a valid path: `%s`'.format([this]) : ''"
    }
  ];
}

message File {
  string path = 1 [(buf.validate.field).string.(valid_path) = true];
}

This is a breaking change.

jchadwick-buf commented 2 months ago

After merging main back in, it looks like there is a new lint error:

Error: Enum option "allow_alias" on enum "Ignore" must be false.

After some inspection it appears this may have to do with Buf v1.40.0, as the same code seems to have been passing in Buf v1.39. Further inspection may be needed. (In any case, it doesn't have anything to do with the changes in this PR as best as I can ascertain.)

oliversun9 commented 2 months ago

After merging main back in, it looks like there is a new lint error:

Error: Enum option "allow_alias" on enum "Ignore" must be false.

After some inspection it appears this may have to do with Buf v1.40.0, as the same code seems to have been passing in Buf v1.39. Further inspection may be needed. (In any case, it doesn't have anything to do with the changes in this PR as best as I can ascertain.)

I think this is a CLI bug, I will take a look.

rodaine commented 2 months ago

@oliversun9 @jchadwick-buf once the CLI issue is patched (and released) let's get that in here (or a separate pr that's merged in) as well as fixing the deprecation warnings. I'll merge bypass once the only remaining issue is the breaking change detection.

oliversun9 commented 2 months ago

@oliversun9 @jchadwick-buf once the CLI issue is patched (and released) let's get that in here (or a separate pr that's merged in) as well as fixing the deprecation warnings. I'll merge bypass once the only remaining issue is the breaking change detection.

I have a CLI PR up for the fix, but one thing you could do to avoid waiting on the CLI fix to land is to break the ignore comment into two lines, i.e.

// buf:lint:ignore ENUM_NO_ALLOW_ALIAS
// allowance for deprecations. TODO: remove pre-v1.
jchadwick-buf commented 1 month ago

@oliversun9 Thanks for the quick fix! It worked, so no need to workaround this. (I did contemplate doing the workaround but it's probably for the best to not merge a PR this big on a Friday afternoon anyways.) I'll try to get all of the ducks in a row here and if there are no objections I'll get this merged and get the runtime PRs under review.

There's definitely more stuff that people will want (rules on arbitrary types, shared message constraints, recursively applying constraints) but I think this is a good opportunity to launch-and-iterate.

Thanks for all of the help, feedback, reviews, etc.!

jchadwick-buf commented 1 month ago

We got held up a bit due to a somewhat esoteric issue with protoc-gen-buf-lint that impacts us as a result of using Bazel/buf_rules. For now I put a workaround while we work on a fix for the buf issue. @rodaine I'll leave the decision up to you if you want to merge ahead with the workaround in place or if you think it might be a better idea to just push forward with the upstream fixes first.

bufdev commented 1 month ago
bufdev commented 1 month ago

Also why not just merge this all into validate.proto for simplicity of imports

jchadwick-buf commented 1 month ago

Why do we need the shared package at all?

No particular reason, if anything it's really just a matter of coming up with a good naming scheme for them to be merged into one package. We could just prefix the shared constraint identifiers with Shared/shared_. I'll ask a couple other folks and see if anyone has any particularly nice ideas.

bufdev commented 1 month ago

There's only two types in the shared package and neither look like they overlap with anything in the main package, why does anything need prefixing?

jchadwick-buf commented 1 month ago

FieldConstraints and field are both present in both packages.

I just pushed a commit that does what you asked (merges everything into validate.proto and converts it all to proto2), but buf.validate.shared.FieldConstraints becomes buf.validate.SharedFieldConstraints and buf.validate.shared.field becomes buf.validate.shared_field. I'm not too concerned about undoing it later because the vast majority of the work here was converting the Go code to use the proto2 interface for Violation which I assume we want to do either way if we want it to all be proto2.

rodaine commented 1 month ago

:shipit: Thanks @jchadwick-buf! Herculean effort.