protovalidate icon indicating copy to clipboard operation
protovalidate copied to clipboard

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

Open simonbos opened this issue 2 years ago • 4 comments

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 avatar Oct 03 '23 20:10 simonbos

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.

rodaine avatar Oct 16 '23 17:10 rodaine

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!

simonbos avatar Oct 17 '23 19:10 simonbos

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

rodaine avatar Oct 17 '23 22:10 rodaine

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

simonbos avatar Oct 18 '23 10:10 simonbos