Add timestamp.valid evaluator #53
Conversation
pkwarren
left a comment
pkwarren
left a comment
There was a problem hiding this comment.
Changes look good - just had a few minor comments. I know this is waiting for upstream PRs to be unblocked.
| * allowing access to the message fields. | ||
| */ | ||
| class TimestampEvaluator implements Evaluator { | ||
| private final long maxTimestamp = +253402300799L; |
There was a problem hiding this comment.
| private final long maxTimestamp = +253402300799L; | |
| private static final long MAX_TIMESTAMP = ZonedDateTime.parse("9999-12-31T23:59:59Z").toEpochSecond(); |
We should make these static constants but I also think it improves readability if we use the timestamp strings instead of the epoch second here.
| ValueEvaluator valueEvaluatorEval) { | ||
| if (fieldDescriptor.getType() != FieldDescriptor.Type.MESSAGE | ||
| || !fieldDescriptor.getMessageType().getFullName().equals("google.protobuf.Timestamp")) { | ||
| return; |
There was a problem hiding this comment.
What do you think about returning early if !fieldConstraints.getTimestamp.getValid()? I don't see any sense in creating the evaluator in that case since there is only one rule and otherwise it is a no-op.
| Descriptors.FieldDescriptor secondsDescriptor, | ||
| Descriptors.FieldDescriptor nanosDescriptor, | ||
| boolean valid) { | ||
| this.secondsDescriptor = secondsDescriptor; |
There was a problem hiding this comment.
| this.secondsDescriptor = secondsDescriptor; | |
| this.secondsDescriptor = Objects.requireNonNull(secondsDescriptor, "secondsDescriptor"); |
| Descriptors.FieldDescriptor nanosDescriptor, | ||
| boolean valid) { | ||
| this.secondsDescriptor = secondsDescriptor; | ||
| this.nanosDescriptor = nanosDescriptor; |
There was a problem hiding this comment.
| this.nanosDescriptor = nanosDescriptor; | |
| this.nanosDescriptor = Objects.requireNonNull(nanosDescriptor, "nanosDescriptor"); |
| Violation.newBuilder() | ||
| .setConstraintId("timestamp.valid") | ||
| .setMessage(errorMessage) | ||
| .build(); |
There was a problem hiding this comment.
We should set the field path on the violation as well.
| .setMessage(errorMessage) | ||
| .build(); | ||
| violationList.add(violation); | ||
| if (failFast) { |
There was a problem hiding this comment.
This isn't looping over anything here, so I think we can just remove this if statement.
| } else if (seconds > maxTimestamp) { | ||
| errorMessage = "timestamp after 9999-12-31"; | ||
| } else if (nanos < 0 || nanos >= 1e9) { | ||
| errorMessage = "timestamp has out-of-range nanos"; |
There was a problem hiding this comment.
I think it would be useful if the error message included details of all parts of the timestamp which are invalid. So if both the seconds and nanos values are invalid, we should return that in the error message.
Do we also want to include in the error message the expected range of the timestamp nanos?
|
I am closing this because in recent projects I don't need this anymore. See bufbuild/protovalidate-python#85 (comment). |
2 participants
See bufbuild/protovalidate#101 for details.
This depends on bufbuild/protovalidate#114 being merged. However, locally I was able to build & run conformance tests using some edits to the
build.gradle.ktsandMakefile. (After the merge on the main repo, I would best re-generate the generated stubs.)