From a0d1c7518327327479f68313b7c6b5611e7af8e4 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Tue, 16 Jun 2020 12:17:50 -0700 Subject: [PATCH] types/known: handle nil in IsValid and CheckValid methods Many usages of the legacy ptypes.Timestamp and ptypes.Duration rely on the functions implicitly returning an error for a nil message. In order to migrate previous usages of the ptypes constructors to the new API, we modify CheckValid to preserve similar semantics. There is some consistency with this change in that: (*M)(nil).IsValid() == (*M)(nil).ProtoReflect().IsValid() where M is Duration, Timestamp, and FieldMask. Change-Id: I05ae152511f9875ea034e78aefb7aab18d17a318 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/238007 Reviewed-by: Damien Neil --- .../internal_gengo/well_known_types.go | 33 +++++++++++++++---- types/known/durationpb/duration.pb.go | 17 +++++++--- types/known/durationpb/duration_test.go | 2 +- types/known/fieldmaskpb/field_mask.pb.go | 1 + types/known/timestamppb/timestamp.pb.go | 15 +++++++-- types/known/timestamppb/timestamp_test.go | 2 +- 6 files changed, 54 insertions(+), 16 deletions(-) diff --git a/cmd/protoc-gen-go/internal_gengo/well_known_types.go b/cmd/protoc-gen-go/internal_gengo/well_known_types.go index 7365075c..8b12823f 100644 --- a/cmd/protoc-gen-go/internal_gengo/well_known_types.go +++ b/cmd/protoc-gen-go/internal_gengo/well_known_types.go @@ -176,8 +176,11 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P("// CheckValid returns an error if the timestamp is invalid.") g.P("// In particular, it checks whether the value represents a date that is") g.P("// in the range of 0001-01-01T00:00:00Z to 9999-12-31T23:59:59Z inclusive.") + g.P("// An error is reported for a nil Timestamp.") g.P("func (x *Timestamp) CheckValid() error {") g.P(" switch x.check() {") + g.P(" case invalidNil:") + g.P(" return ", protoimplPackage.Ident("X"), ".NewError(\"invalid nil Timestamp\")") g.P(" case invalidUnderflow:") g.P(" return ", protoimplPackage.Ident("X"), ".NewError(\"timestamp (%v) before 0001-01-01\", x)") g.P(" case invalidOverflow:") @@ -190,9 +193,13 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P("}") g.P() - g.P("const invalidUnderflow = 1") - g.P("const invalidOverflow = 2") - g.P("const invalidNanos = 3") + g.P("const (") + g.P(" _ = iota") + g.P(" invalidNil") + g.P(" invalidUnderflow") + g.P(" invalidOverflow") + g.P(" invalidNanos") + g.P(")") g.P() g.P("func (x *Timestamp) check() uint {") @@ -201,6 +208,8 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P(" secs := x.GetSeconds()") g.P(" nanos := x.GetNanos()") g.P(" switch {") + g.P(" case x == nil:") + g.P(" return invalidNil") g.P(" case secs < minTimestamp:") g.P(" return invalidUnderflow") g.P(" case secs > maxTimestamp:") @@ -255,8 +264,11 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P("// CheckValid returns an error if the duration is invalid.") g.P("// In particular, it checks whether the value is within the range of") g.P("// -10000 years to +10000 years inclusive.") + g.P("// An error is reported for a nil Duration.") g.P("func (x *Duration) CheckValid() error {") g.P(" switch x.check() {") + g.P(" case invalidNil:") + g.P(" return ", protoimplPackage.Ident("X"), ".NewError(\"invalid nil Duration\")") g.P(" case invalidUnderflow:") g.P(" return ", protoimplPackage.Ident("X"), ".NewError(\"duration (%v) exceeds -10000 years\", x)") g.P(" case invalidOverflow:") @@ -271,10 +283,14 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P("}") g.P() - g.P("const invalidUnderflow = 1") - g.P("const invalidOverflow = 2") - g.P("const invalidNanosRange = 3") - g.P("const invalidNanosSign = 4") + g.P("const (") + g.P(" _ = iota") + g.P(" invalidNil") + g.P(" invalidUnderflow") + g.P(" invalidOverflow") + g.P(" invalidNanosRange") + g.P(" invalidNanosSign") + g.P(")") g.P() g.P("func (x *Duration) check() uint {") @@ -282,6 +298,8 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P(" secs := x.GetSeconds()") g.P(" nanos := x.GetNanos()") g.P(" switch {") + g.P(" case x == nil:") + g.P(" return invalidNil") g.P(" case secs < -absDuration:") g.P(" return invalidUnderflow") g.P(" case secs > +absDuration:") @@ -587,6 +605,7 @@ func genMessageKnownFunctions(g *protogen.GeneratedFile, f *fileInfo, m *message g.P("// IsValid reports whether all the paths are syntactically valid and") g.P("// refer to known fields in the specified message type.") + g.P("// It reports false for a nil FieldMask.") g.P("func (x *FieldMask) IsValid(m ", protoPackage.Ident("Message"), ") bool {") g.P(" paths := x.GetPaths()") g.P(" return numValidPaths(m, paths) == len(paths)") diff --git a/types/known/durationpb/duration.pb.go b/types/known/durationpb/duration.pb.go index 53004d28..67dbea23 100644 --- a/types/known/durationpb/duration.pb.go +++ b/types/known/durationpb/duration.pb.go @@ -158,8 +158,11 @@ func (x *Duration) IsValid() bool { // CheckValid returns an error if the duration is invalid. // In particular, it checks whether the value is within the range of // -10000 years to +10000 years inclusive. +// An error is reported for a nil Duration. func (x *Duration) CheckValid() error { switch x.check() { + case invalidNil: + return protoimpl.X.NewError("invalid nil Duration") case invalidUnderflow: return protoimpl.X.NewError("duration (%v) exceeds -10000 years", x) case invalidOverflow: @@ -173,16 +176,22 @@ func (x *Duration) CheckValid() error { } } -const invalidUnderflow = 1 -const invalidOverflow = 2 -const invalidNanosRange = 3 -const invalidNanosSign = 4 +const ( + _ = iota + invalidNil + invalidUnderflow + invalidOverflow + invalidNanosRange + invalidNanosSign +) func (x *Duration) check() uint { const absDuration = 315576000000 // 10000yr * 365.25day/yr * 24hr/day * 60min/hr * 60sec/min secs := x.GetSeconds() nanos := x.GetNanos() switch { + case x == nil: + return invalidNil case secs < -absDuration: return invalidUnderflow case secs > +absDuration: diff --git a/types/known/durationpb/duration_test.go b/types/known/durationpb/duration_test.go index 464fbf19..0e86eb12 100644 --- a/types/known/durationpb/duration_test.go +++ b/types/known/durationpb/duration_test.go @@ -56,7 +56,7 @@ func TestFromDuration(t *testing.T) { wantDur time.Duration wantErr error }{ - {in: nil, wantDur: time.Duration(0)}, + {in: nil, wantDur: time.Duration(0), wantErr: textError("invalid nil Duration")}, {in: new(durpb.Duration), wantDur: time.Duration(0)}, {in: &durpb.Duration{Seconds: -1, Nanos: 0}, wantDur: -time.Second}, {in: &durpb.Duration{Seconds: +1, Nanos: 0}, wantDur: +time.Second}, diff --git a/types/known/fieldmaskpb/field_mask.pb.go b/types/known/fieldmaskpb/field_mask.pb.go index 6d845c8c..ae1be0e0 100644 --- a/types/known/fieldmaskpb/field_mask.pb.go +++ b/types/known/fieldmaskpb/field_mask.pb.go @@ -304,6 +304,7 @@ func Intersect(mx *FieldMask, my *FieldMask, ms ...*FieldMask) *FieldMask { // IsValid reports whether all the paths are syntactically valid and // refer to known fields in the specified message type. +// It reports false for a nil FieldMask. func (x *FieldMask) IsValid(m proto.Message) bool { paths := x.GetPaths() return numValidPaths(m, paths) == len(paths) diff --git a/types/known/timestamppb/timestamp.pb.go b/types/known/timestamppb/timestamp.pb.go index 0023d262..1988eb44 100644 --- a/types/known/timestamppb/timestamp.pb.go +++ b/types/known/timestamppb/timestamp.pb.go @@ -165,8 +165,11 @@ func (x *Timestamp) IsValid() bool { // CheckValid returns an error if the timestamp is invalid. // In particular, it checks whether the value represents a date that is // in the range of 0001-01-01T00:00:00Z to 9999-12-31T23:59:59Z inclusive. +// An error is reported for a nil Timestamp. func (x *Timestamp) CheckValid() error { switch x.check() { + case invalidNil: + return protoimpl.X.NewError("invalid nil Timestamp") case invalidUnderflow: return protoimpl.X.NewError("timestamp (%v) before 0001-01-01", x) case invalidOverflow: @@ -178,9 +181,13 @@ func (x *Timestamp) CheckValid() error { } } -const invalidUnderflow = 1 -const invalidOverflow = 2 -const invalidNanos = 3 +const ( + _ = iota + invalidNil + invalidUnderflow + invalidOverflow + invalidNanos +) func (x *Timestamp) check() uint { const minTimestamp = -62135596800 // Seconds between 1970-01-01T00:00:00Z and 0001-01-01T00:00:00Z, inclusive @@ -188,6 +195,8 @@ func (x *Timestamp) check() uint { secs := x.GetSeconds() nanos := x.GetNanos() switch { + case x == nil: + return invalidNil case secs < minTimestamp: return invalidUnderflow case secs > maxTimestamp: diff --git a/types/known/timestamppb/timestamp_test.go b/types/known/timestamppb/timestamp_test.go index a358aac5..dfd666de 100644 --- a/types/known/timestamppb/timestamp_test.go +++ b/types/known/timestamppb/timestamp_test.go @@ -59,7 +59,7 @@ func TestFromTimestamp(t *testing.T) { wantTime time.Time wantErr error }{ - {in: nil, wantTime: time.Unix(0, 0)}, + {in: nil, wantTime: time.Unix(0, 0), wantErr: textError("invalid nil Timestamp")}, {in: new(tspb.Timestamp), wantTime: time.Unix(0, 0)}, {in: &tspb.Timestamp{Seconds: -62135596800, Nanos: 0}, wantTime: time.Time{}}, {in: &tspb.Timestamp{Seconds: -1, Nanos: -1}, wantTime: time.Unix(-1, -1), wantErr: textError("timestamp (seconds:-1 nanos:-1) has out-of-range nanos")},