jsonpb: fix handling of illegal and negative nanoseconds #492
Conversation
|
Note that the regression is present on |
dsnet
changed the title
|
Argh... I regret merging the last change into master since the two branches differ by that change now. |
jsonpb/jsonpb.go
Outdated
| @@ -201,8 +201,13 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU | |||
| // "Generated output always contains 3, 6, or 9 fractional digits, | |||
| // depending on required precision." | |||
| s, ns := s.Field(0).Int(), s.Field(1).Int() | |||
| d := time.Duration(s)*time.Second + time.Duration(ns)*time.Nanosecond | |||
| x := fmt.Sprintf("%.9f", d.Seconds()) | |||
| if (s > 0 && ns < 0) || (s < 0 && ns > 0) { | |||
There was a problem hiding this comment.
This check seems wrong. The documentation for Timestamp.nanos says:
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.
Which seems to indicate a negative value for ns is invalid regardless of s value.
There was a problem hiding this comment.
Sigh. The spec here says the opposite:
“For durations of one second or more, a non-zero value for the nanos field must be of the same sign as the seconds field. Must be from -999,999,999 to +999,999,999 inclusive.”
There was a problem hiding this comment.
Oh wait. This is Duration, not Timestamp....
There was a problem hiding this comment.
Oh, my mistake. I'm looking at Timestamp.
Also, to what degree should we validate durations?
- Should we also verify that they are within -999999999 to +999999999 inclusive? Note that the logic below breaks if the value exceeds those bounds.
- If we're going to invalidate bad Durations here, should we be consistent and apply similar checks to Timestamps down below?
There was a problem hiding this comment.
Also, to what degree should we validate durations?
I'll leave that up to you. If you want this CL to be consistent with the rest of the code, I can remove the validation and testing of illegal inputs here. If you want validation elsewhere, I would rather change the other places in a different commit, as this commit fixes a very real regression and it would be best to get a fix in ASAP.
Shall we roll that one back then? |
|
ns range validation (and tests) added to Duration and Timestamp handling. |
jsonpb/jsonpb_test.go
Outdated
| func TestMarshalIllegalTime(t *testing.T) { | ||
| tests := []struct { | ||
| pb proto.Message | ||
| errWant error |
There was a problem hiding this comment.
I don't think we care that much about the exact error message. Let's just change this to a "fail bool" that checks whether it passes or not.
dsnet
changed the title
|
Overall change LGTM. Just a nit with regards to the test. |
Fixes regression caused by #453.
@dsnet