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.

Sign up for GitHub

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

Deprecate skipped rule + add IGNORE_ALWAYS to ignore rule + more explicit naming #172

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Deprecate skipped rule + add IGNORE_ALWAYS to ignore rule + more explicit naming #172

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a395f22
Deprecate skipped + add to ignore rule
rodaine Feb 20, 2024
1bc44ca
checkpoint
rodaine Feb 20, 2024
e0dded4
checkpoint
rodaine Feb 20, 2024
ceb87ce
checkpoint
rodaine Feb 20, 2024
c019679
feedback
rodaine Feb 21, 2024
c897cf5
checkpoint
rodaine Feb 21, 2024
da0aa48
checkpoint
rodaine Feb 21, 2024
d545416
checkpoint
rodaine Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
build --experimental_proto_descriptor_sets_include_source_info
1 change: 1 addition & 0 deletions proto/protovalidate/buf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ lint:
ignore_only:
PACKAGE_VERSION_SUFFIX:
- buf/validate
allow_comment_ignores: true
94 changes: 53 additions & 41 deletions proto/protovalidate/buf/validate/validate.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,27 +131,18 @@ message FieldConstraints {
// }
// ```
repeated Constraint cel = 23;
// `skipped` is an optional boolean attribute that specifies that the
// validation rules of this field should not be evaluated. If skipped is set to
// true, any validation rules set for the field will be ignored.
// DEPRECATED: use ignore=IGNORE_ALWAYS instead.
bool skipped = 24 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would move deprecated fields to the end of the message body

// If `required` is true, the field must be populated. A populated field can be
// described as "serialized in the wire format," which includes:
//
// ```proto
// message MyMessage {
// // The field `value` must not be set.
// optional MyOtherMessage value = 1 [(buf.validate.field).skipped = true];
// }
// ```
bool skipped = 24;
// If `required` is true, the field must be populated. Field presence can be
// described as "serialized in the wire format," which follows the following rules:
//
// - the following "nullable" fields must be explicitly set to be considered present:
// - the following "nullable" fields must be explicitly set to be considered populated:
// - singular message fields (whose fields may be unpopulated/default values)
// - member fields of a oneof (may be their default value)
// - proto3 optional fields (may be their default value)
// - proto2 scalar fields
// - proto3 scalar fields must be non-zero to be considered present
// - repeated and map fields must be non-empty to be considered present
// - proto2 scalar fields (both optional and required)
// - proto3 scalar fields must be non-zero to be considered populated
// - repeated and map fields must be non-empty to be considered populated
//
// ```proto
// message MyMessage {
Expand All @@ -160,16 +151,17 @@ message FieldConstraints {
// }
// ```
bool required = 25;
// DEPRECATED: use ignore=IGNORE_EMPTY instead.
// DEPRECATED: use ignore=IGNORE_IF_UNPOPULATED instead.
bool ignore_empty = 26 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

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

same

// Skip validation on the field if its value matches the specified rule.
// Skip validation on the field if its value matches the specified criteria.
// See Ignore enum for details.
//
// ```proto
// message UpdateRequest {
// // The uri rule only applies if the field is populated and not an empty
// // string.
// optional string url = 1 [
// (buf.validate.field).ignore = IGNORE_DEFAULT,
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE,
// (buf.validate.field).string.uri = true,
// ];
// }
Expand Down Expand Up @@ -209,6 +201,8 @@ message FieldConstraints {
// Specifies how FieldConstraints.ignore behaves. See the documentation for
// FieldConstraints.required for definitions of "populated" and "nullable".
enum Ignore {
// buf:lint:ignore ENUM_NO_ALLOW_ALIAS
Copy link
Member

Choose a reason for hiding this comment

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

Can you do this in the buf.yaml?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to disable it for the whole file, just this one enum. Similar discussion as nolint comments vs .golangci.yaml.

option allow_alias = true;
Copy link
Member

Choose a reason for hiding this comment

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

I'm really hesitant to do this - https://buf.build/docs/lint/rules#enum_no_allow_alias we recommend against doing this explicitly, and we even have it in the BASIC category, which is shorthand for "really, please don't do this, we really recommend you don't do this." I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only temporary until we hit v1, where all pre-v1 deprecations will be removed. Otherwise implementations will need to juggle both of these (or we make a breaking change).

The reasoning for the rule makes sense when we talk about messages going over the wire, but this is used exclusively as options, which don't typically show up serialized in JSON. It will break a protoc/buf build once they upgrade the protovalidate module however, which is appropriate IMO when transitioning to v1.

Copy link
Member

Choose a reason for hiding this comment

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

dont want to block anything, but would love if we had a comment or a TODO to the effect of "this will be removed pre-v1" and same with deprecated stuff

// Validation is only skipped if it's an unpopulated nullable fields.
//
// ```proto
Expand Down Expand Up @@ -251,33 +245,35 @@ enum Ignore {
// // The uri rule applies only if the value is not the empty string.
// string foo = 1 [
// (buf.validate.field).string.uri = true,
// (buf.validate.field).ignore = IGNORE_EMPTY
// (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
// ];
//
// // IGNORE_EMPTY is equivalent to IGNORE_UNSPECIFIED in this case: the
// // uri rule only applies if the field is set, including if it's set
// // to the empty string.
// // IGNORE_IF_UNPOPULATED is equivalent to IGNORE_UNSPECIFIED in this
// // case: the uri rule only applies if the field is set, including if
// // it's set to the empty string.
// optional string bar = 2 [
// (buf.validate.field).string.uri = true,
// (buf.validate.field).ignore = IGNORE_EMPTY
// (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
// ];
//
// // The min_items rule only applies if the list has at least one item.
// repeated string baz = 3 [
// (buf.validate.field).repeated.min_items = 3,
// (buf.validate.field).ignore = IGNORE_EMPTY
// (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
// ];
//
// // IGNORE_EMPTY is equivalent to IGNORE_UNSPECIFIED in this case: the
// // custom CEL rule applies only if the field is set, including if it's
// // the "zero" value of that message.
// // IGNORE_IF_UNPOPULATED is equivalent to IGNORE_UNSPECIFIED in this
// // case: the custom CEL rule applies only if the field is set, including
// // if it's the "zero" value of that message.
// SomeMessage quux = 4 [
// (buf.validate.field).cel = {/* ... */},
// (buf.validate.field).ignore = IGNORE_EMPTY
// (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
// ];
// }
// ```
IGNORE_EMPTY = 1;
IGNORE_IF_UNPOPULATED = 1;
// Deprecated: Use IGNORE_IF_UNPOPULATED
IGNORE_EMPTY = 1 [deprecated = true];

// Validation is skipped if the field is unpopulated or if it is a nullable
Copy link
Member

Choose a reason for hiding this comment

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

If there's a link to docs where unpopulated is defined, can we include a link to that here?

Copy link
Member Author

@rodaine rodaine Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

"populated" is defined on the required constraint above (the enum docs point up there). The protobuf docs define presence which we are explicitly avoiding here to prevent confusion.

// field populated with its default value. This is typically the zero or
Expand All @@ -288,32 +284,34 @@ enum Ignore {
// syntax="proto3
//
// message Request {
// // IGNORE_DEFAULT is equivalent to IGNORE_EMPTY in this case; the uri
// // rule applies only if the value is not the empty string.
// // IGNORE_IF_DEFAULT_VALUE is equivalent to IGNORE_IF_UNPOPULATED in
// // this case; the uri rule applies only if the value is not the empty
// // string.
// string foo = 1 [
// (buf.validate.field).string.uri = true,
// (buf.validate.field).ignore = IGNORE_DEFAULT
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
// ];
//
// // The uri rule only applies if the field is set to a value other than
// // the empty string.
// optional string bar = 2 [
// (buf.validate.field).string.uri = true,
// (buf.validate.field).ignore = IGNORE_DEFAULT
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
// ];
//
// // IGNORE_DEFAULT is equivalent to IGNORE_EMPTY in this case; the
// // min_items rule only applies if the list has at least one item.
// // IGNORE_IF_DEFAULT_VALUE is equivalent to IGNORE_IF_UNPOPULATED in
// // this case; the min_items rule only applies if the list has at least
// // one item.
// repeated string baz = 3 [
// (buf.validate.field).repeated.min_items = 3,
// (buf.validate.field).ignore = IGNORE_DEFAULT
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
// ];
//
// // The custom CEL rule only applies if the field is set to a value other
// // than an empty message (i.e., fields are unpopulated).
// SomeMessage quux = 4 [
// (buf.validate.field).cel = {/* ... */},
// (buf.validate.field).ignore = IGNORE_DEFAULT
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
// ];
// }
// ```
Expand All @@ -330,10 +328,24 @@ enum Ignore {
// optional int32 value = 1 [
// default = -42,
// (buf.validate.field).int32.gt = 0,
// (buf.validate.field).ignore = IGNORE_DEFAULT
// (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
// ];
// }
IGNORE_DEFAULT = 2;
IGNORE_IF_DEFAULT_VALUE = 2;
// Deprecated: Use IGNORE_IF_DEFAULT_VALUE
IGNORE_DEFAULT = 2 [deprecated = true];

// The validation rules of this field will not be evaluated.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a sentence such as "this is useful for situations where you want to temporarily turn off a rule, or [insert other reasons this is useful". Perhaps also add another sentence "this replaces skipped from protoc-gen-validate".

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool with the first, I don't really want to document this based off its predecessor however.

//
// ```proto
// message MyMessage {
// // The field's rules will always be ignored, including any validation's
// // on value's fields.
// MyOtherMessage value = 1 [
// (buf.validate.field).ignore = IGNORE_ALWAYS];
// }
// ```
IGNORE_ALWAYS = 3;
}

// FloatRules describes the constraints applied to `float` values. These
Expand Down
Loading
Loading