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

[Feature Request] Support specification validation for google.protobuf.Timestamp #101

Open
simonbos opened this issue Oct 3, 2023 · 4 comments
Labels
Feature New feature or request

Comments

@simonbos
Copy link

simonbos commented Oct 3, 2023

Feature description:
The well-known google.protobuf.Timestamp specification imposes some constraints on the fields (see source):

message Timestamp {
  // Represents seconds of UTC time since Unix epoch
  // 1970-01-01T00:00:00Z. Must be from 0001-01-01T00:00:00Z to
  // 9999-12-31T23:59:59Z inclusive.
  int64 seconds = 1;

  // Non-negative fractions of a second at nanosecond resolution. Negative
  // second values with fractions must still have non-negative nanos values
  // that count forward in time. Must be from 0 to 999,999,999
  // inclusive.
  int32 nanos = 2;
}

This feature request suggests to enhance protovalidate to support this validation out-of-the-box.

Problem it solves or use case:
With the current implementation, developers would have to program this validation outside of protovalidate (or maybe use some complicated CEL expression). In Go, the specification validation is typically done with CheckValid. However, this has some issues:

  • Overhead for the developer.
  • Inconsistent validation end user experience.

Proposed implementation or solution:
I propose to add an extra validation rule for timestamp. For the implementation I am not sure since I am not experienced with CEL. From what I can see, the general idea is either:

  • Have an implementation in CEL using existing expression logic. I tried implementing the constraints as a CEL expression. The validation on seconds seems easy enough, but the nanoseconds seem unavailable in a CEL expression (There might still be a way.)
  • Adding a new CEL function and implementing it for each language (like isNaN).

Contribution:
I would be willing to implement this feature; as long as there is consensus on how it should be implemented.

Examples or references:

  • protoc-gen-validate-go did seem to produce code which checked the specification validity using CheckValid. See here.

Additional context:

  • While this proposal proposes an explicit validation rule, this might actually be something which should be done implicitly. This because the validation 1) is on the specification of the message and 2) is not context-aware.
@simonbos simonbos added the Feature New feature or request label Oct 3, 2023
@rodaine
Copy link
Member

rodaine commented Oct 16, 2023

Hey @simonbos! Thanks for the thorough report! I can definitely see this as a timestamp.valid (or similar) standard constraint. The two expressions would be:

  • this.seconds >= -62135568422 && this.seconds <= 253402329599
  • this.nanos >= 0 && this.nanos <= 999999999

I believe the duration WKT has similar limitations.

While this proposal proposes an explicit validation rule, this might actually be something which should be done implicitly. This because the validation 1) is on the specification of the message and 2) is not context-aware.

While I agree the constraints are documented by the type and meant to be intrinsic, technically a valid value of a timestamp message is what's valid for the contained int64 and int32; the type itself cannot encode those semantics. To be consistent (and to not potentially break consumers of protovalidate), we will still want to require opting-into semantic validation of the field.

@simonbos
Copy link
Author

Hi @rodaine, thanks for the reply! I tried out the expressions using the following:

    google.protobuf.Timestamp input_time = 2 [(buf.validate.field).cel = {
      id: "echo_request.input_time",
      message: "Timestamp should be valid",
      expression: "-62135596800 <= this.seconds && this.seconds <= 253402300799 && this.nanos >= 0 && this.nanos <= 999999999"
    }];

But I had some issues selecting the fields:

"failed to compile expression echo_request.input_time: ERROR: <input>:1:21: type 'timestamp' does not support field selection\n | -62135596800 <= this.seconds && this.seconds <= 253402300799 && this.nanos >= 0 && this.nanos <= 999999999\n | ....................^\nERROR: <input>:1:37: type 'timestamp' does not support field selection\n | -62135596800 <= this.seconds && this.seconds <= 253402300799 && this.nanos >= 0 && this.nanos <= 999999999\n | ....................................^\nERROR: <input>:1:69: type 'timestamp' does not support field selection\n | -62135596800 <= this.seconds && this.seconds <= 253402300799 && this.nanos >= 0 && this.nanos <= 999999999\n | ....................................................................^\nERROR: <input>:1:88: type 'timestamp' does not support field selection\n | -62135596800 <= this.seconds && this.seconds <= 253402300799 && this.nanos >= 0 && this.nanos <= 999999999\n | .......................................................................................^"

I believe this behavior is documented in the cel-spec. For the seconds restriction, we can also express it using (buf.validate.field).timestamp.gt and such (or the underlying expression); but for the nanos restriction I don't see how to express it using CEL ...

I believe the duration WKT has similar limitations.

Indeed, duration.proto also documents some upper and lower bounds.

While I agree the constraints are documented by the type and meant to be intrinsic, technically a valid value of a timestamp message is what's valid for the contained int64 and int32; the type itself cannot encode those semantics. To be consistent (and to not potentially break consumers of protovalidate), we will still want to require opting-into semantic validation of the field.

Agreed!

@rodaine
Copy link
Member

rodaine commented Oct 17, 2023

Oh that's right, it's not treated as a proto message within CEL's type system.

Looking at cel-go, google.protobuf.Timestamp values are converted to Go's time.Time struct using AsTime, which per its documentation normalizes invalid timestamps (via Go's time.Unix function). Because the type is does not exist in CEL, we cannot use CEL (either an expression or function) to check that the timestamp is valid, which would mean this rule would need to be lifted into the validator's logic (similar to how we treat Any messages).

@simonbos
Copy link
Author

Ok, that's clear, I'll have a look for implementing this (first in Go).

igor-tsiglyar pushed a commit to igor-tsiglyar/protovalidate that referenced this issue Apr 16, 2024
Update protovalidate-go to work with the latest protovalidate proto
rules, including the Ignore enum. Update to the latest golangci-lint and
fix lint warnings.

---------

Co-authored-by: Chris Roche <github@rodaine.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants