Implement predefined field constraints

[jchadwick-buf]

• Renames buf.validate.priv.FieldConstraints to buf.validate.PredefinedConstraint
• Renames buf.validate.priv.field to buf.validate.predefined
• Removes package buf.validate.priv to buf.validate
• Merges buf/validate/expression.proto to buf/validate/validate.proto
• Removes //proto/protovalidate/buf/validate:expression_proto
• Switches buf/validate/validate.proto from proto3 syntax to proto2 syntax to enable usage of extensions
• Adds extension 1000 to max to each ...Rules message
• Adds test cases for valid and invalid predefined constraints of every possible rule type for both proto2 and protobuf edition 2023
• Added the CEL constant rule that can be used by predefined constraints to refer to themselves specifically.

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]

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.)

see comments

[oliversun9]

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]

@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]

@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]

@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]

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]

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]

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.

[bufdev]

I don't understand the docs here - the types are the same, why can I not share something specified in field? Why is this named shared_field? I'm sure there are answers to all of this, but it's not clear to me how to use these two extensions from the documentation here.

[jchadwick-buf]

I did propose that, and even implemented it in protovalidate-go before the current implementation. As a result of many discussions, I wound up backtracking to something very close to @rodaine's original proposal instead. Personally I still like the idea of unifying it all, but I don't think I really convinced anyone else of it.

The main points are:

• There is concern that using the same extension for shared rules and custom rules would be confusing.

• It's confusing to understand when shared rules would apply. For example, consider this:

  extend buf.validate.FloatRules {
    bool is_zero [(buf.validate.field).float.const = 0]
  }
  
  message MyMessage {
    // `is_zero` applies, since it is set.
    float number_1 = 1 [(buf.validate.field).float.(is_zero) = true];
    // `is_zero` still applies, since it is set. Extensions always have field presence.
    float number_2 = 2 [(buf.validate.field).float.(is_zero) = false];
  }

  This isn't a huge problem for the current version because you can do this:

  extend buf.validate.FloatRules {
    bool is_zero [(buf.validate.shared_field).cel = "rule ? this == 0 : true"]
  }

• Moving the standard rules to use the same mechanism as custom constraints will require making changes to how custom constraints work in each runtime to allow rule values to be plumbed through.

If you would prefer a version that eliminates the need for a SharedFieldConstraints type and allows using standard constraints recursively, I do have some work already done on that front and can switch gears.

I discussed a design that would work this way here: #246 (comment) - but it would need to change at least somewhat, since we can't just blindly proto.Merge everything if we want to be able to e.g. bind rules/rule differently per CEL expression based on where it is in the chain of options.

[bufdev]

@rodaine I'd love to sync on this for 5 minutes pre-merge

@jchadwick-buf my main comment here is that regardless of design (and even assuming that this existing design stays), the comments/documentation are not clear to me as a user. I'm reading the comments and am not sure how to use protovalidate - this is a critical thing to tackle

[jchadwick-buf]

I understand what you mean, I'll try to improve the comments at least. I do worry that the reason it's hard to explain is because the design is actually a bit confusing, so it would probably be best if we really did make sure that this is what we want to go with.

[jchadwick-buf]

I just noticed a probable huge part of the confusion, which is that I have had a typo here. I need to switch the second one to SharedFieldConstraints.

[jchadwick-buf]

Now it is PredefinedConstraints and predefined. I'm leaving this comment unresolved just in case I've screwed up any details from the meeting.

[bufdev]

This isn't actually true. 50000-99999 are reserved for internal use for organizations. The random number thing is actually a somewhat-uncomfortable recommendation for me - I get the birthday paradox, but I'm not thrilled that this is what we're recommending people do. I would prefer saying "choose a number that won't conflict" and leave this up to the user.

[jchadwick-buf]

Thanks, I was mistaken on the ranges somehow, that's what I get for not actually checking.

I agree we shouldn't recommend it, so I've adjusted the documentation altogether:

• Fixed the ranges.
• No longer explicitly recommends randomly-generated integers
• Discourages use of 100000....536870911 for publicly-consumed schemas at all, due to risk of conflicts
• But keeping a suggestion from Miguel, I kept a note that suggests using a high-quality random source if the user decides to use a randomly generated integer anyway, under the belief that it's better if they do that versus merely choose arbitrary numbers.

WDYT?

[bufdev]

• Remove random number generation references
• Coalesce extension ranges

[jchadwick-buf]

Done.

[bufdev]

Note that these docs no longer match impl

[jchadwick-buf]

Documentation has been updated.

[bufdev]

I wouldn't expect these to be intermixed in order with the *Constraints definitions. These should likely be at the bottom of the file

[jchadwick-buf]

Moved Violations and Violation to end of file.

[bufdev]

I still don't understand why we are referencing random number generation in the docs - I've read the comments, but this doesn't seem like a good idea.

[jchadwick-buf]

OK, references to random number generation are just removed.

[bufdev]

I don't understand why we're splitting up the extension ranges into two sections - I've never seen that before. In the absence of a strong reason, coalesce.

[jchadwick-buf]

Was suggested here and seemed reasonable enough to me. Either way, it's coalesced again now.

[rodaine]

Thanks @jchadwick-buf! Herculean effort.