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

Omitempty? Or some way to not validate default/zero-value? #102

Closed
veqryn opened this issue Sep 21, 2018 · 19 comments · Fixed by #412
Closed

Omitempty? Or some way to not validate default/zero-value? #102

veqryn opened this issue Sep 21, 2018 · 19 comments · Fixed by #412
Labels
Enhancement Extend or improve functionality

Comments

@veqryn
Copy link

veqryn commented Sep 21, 2018

Just about every single validation library I've seen in golang has let me opt out of validating a field if it is empty / the zero-value.

For example, here is what several common validation libraries use:

type Params struct {
        ID              string    `validate:"len=32"`
	CountryCode     string    `validate:"omitempty,len=2"`
	Size            int       `validate:"omitempty,gt=10"`
}

This means:
Validate ID is length 32 and not empty.
If CountryCode is not empty, validate the length is 2.
If Size is not zero, validate the length is greater than 10.

Can we please have some way to specify, on every single validation option and type you have, to ignore the zero-value?

This is the only thing stopping me from making full use of this library.
My use case is that I serve GRPC with the REST Gateway, and I have several proto messages that have both optional (zero value = ok and ignored) and required parameters. For the optional parameters, I'd still like to validate them if they aren't empty.

Proposed format:

string email = 1 [(validate.rules).string = { omitempty: true, email: true}];
double lat   = 1 [(validate.rules).double = { omitempty: true, gte: -90,  lte: 90 }];
@rodaine
Copy link
Member

rodaine commented Sep 21, 2018

Hey! Thanks for the report. This is pretty much a generalization of #98, and totally a reasonable addition. If you can, the WKTs carry more semantic weight in describing an optional field, but understandably it makes the data structure more complex.

@rodaine rodaine added the Enhancement Extend or improve functionality label Sep 21, 2018
@veqryn
Copy link
Author

veqryn commented Sep 21, 2018

@rodaine Thx, but two questions:

  1. Is this something being presently worked on?
  2. What is a WKT and can you give an example of a proto3 file using it with validation rules to accomplish what I need above?

@rodaine
Copy link
Member

rodaine commented Sep 24, 2018

  1. Nope, but it's a reasonable feature, so nothing's blocking it other than time/prioritization.
  2. WKT = Well Known Type. PGV supports them as if they were the scalar values, but skips validations if the field is unset.

@veqryn
Copy link
Author

veqryn commented Sep 24, 2018

Thanks.
I guess the well known types you are referring to are the 'wrapper' types.
I don't think we want to use those because we don't want to have null's to deal with (we've specifically built our api around proto3's way of treating empty values the same as values not sent). It would also change our api (including our rest gateway api).

@veqryn
Copy link
Author

veqryn commented Jun 26, 2019

Hey, just wondering if this might get prioritized (@rodaine )?
I can't think of any decent golang struct/message validation library that doesn't have this, and this feature is preventing wider use of this library for me and the rest of the teams I work with.

@alexykot
Copy link

alexykot commented Aug 30, 2019

This can be sometimes workarounded with a pattern comparison, I've managed to do it with the rule "^([a-f0-9]{64}|.{0})$", which forces it to be either hex-encoded 32 bytes, or empty. But this is not elegant, much more computation-heavy and required changes in the API since I had to switch from raw bytes to hex-encoded representation. Proper omitempty fix would be way nicer.

@hexdigest
Copy link
Contributor

Hello everyone,

The problem I see here is that all fields in protobuf v3 are optional but protoc-gen-validate already treats any field that has validation rule as required.

However there's already message.required attribute. Maybe explicitly added (validate.rules).message.required = false can help here?

Example:

  string id = 1 [
    (validate.rules).message.required = false,
    (validate.rules).string.min_len = 1
  ];

Can be interepreted like "if field is present then its min length must be greater or equal to 1"
If not maybe it's possible to introduce special "omitempty" as @veqryn proposed:

  string id = 1 [
    (validate.rules).message.omitempty = false,
    (validate.rules).string.min_len = 1
  ];

@alexykot
Copy link

Backwards compatibility wise (validate.rules).message.required = false is preferred I think because it will not change behaviour of the existing code.

@hexdigest
Copy link
Contributor

hexdigest commented Oct 15, 2019

@alexykot
(validate.rules).message.required = false actually CAN change the behaviour of the existing code because this attribute is already exist and may be used somewhere but doing nothing currently.

I think the safest option is to introduce another attribute for this purpose.

@veqryn
Copy link
Author

veqryn commented Oct 18, 2019

The lack of this feature pretty much means protoc-gen-validate is near-useless to anyone using protobuf v3.

Just about anyone building a modern api around protobuf v3 will accept missing fields and default/zero values and treat them as such. Any useful protobuf validation library needs to be able to "validate a field is empty (the default/zero value) or has some property".

I'm not as concerned about the specifics yet as I am on just getting the authors on board with this in general.

@ghost
Copy link

ghost commented Oct 20, 2019

100%, I just stumbled upon this. Sorry, but this is really unacceptable behavior for this type of project.

// Validation rules applied at the field level
extend google.protobuf.FieldOptions {
    // Rules specify the validations to be performed on this field. By default,
    // no validation is performed against a field.
    optional FieldRules rules = 1071;
    bool omitempty = 1072; // <--- not hard
}

string foo= 1 [(validate.rules).string = { max_len: 3 } (validate.omitempty) = true ];

Though I see this issue is now unaswered for a year so I'll move to another project.

@hexdigest
Copy link
Contributor

@veqryn @nonpcnpc @alexykot
You can use https://github.com/hexdigest/protoc-gen-validate/tree/omitempty as a workaround until the PR is merged

@ghost
Copy link

ghost commented Oct 29, 2019

@hexdigest thanks but i already wrote my own protoc generator :) with blackjack and ...

@hexdigest
Copy link
Contributor

@nonpcnpc I don't see any commits in your forked repo can you please point me to the changes you made?

@ghost
Copy link

ghost commented Oct 29, 2019

I ended up writing a new one from scratch. This one and mwitkow's, or any other pb validators, suffer from the simple fact that they have predetermined list of built-in validators. So I have wrote protoc generator that uses closures instead. It is go-only, but that is all I need.

@rodaine
Copy link
Member

rodaine commented Nov 18, 2019

Duplicating discussion from #283:

I'd also like to discuss the name of this rule. While omitempty is fairly well known for Go (considering its roots in the JSON struct annotations), it's literal meaning doesn't quite describe its intent in a polyglot tool such as this. I'd rather this field be something akin to ignore_empty which is clearer, and more accurately describes the behavior.

I'm open to suggestions? Something like ignore_empty or ignore_zero_value seems closer to the meaning than omitempty .

@veqryn
Copy link
Author

veqryn commented Nov 19, 2019

I think ignore_zero_value is probably the least confusing outside of golang. But I like both suggestions.

@hexdigest
Copy link
Contributor

Hi,

@rodaine @veqryn If you're ok with ignore_empty I can redo it.

@bconway
Copy link

bconway commented Jan 7, 2021

Has there been any movement on this recently? I would very much like to validate a UUID string field that accepts a UUID or "", but not "aaa".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Extend or improve functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants