Error handling google.protobuf.Timestamp

[catherinetcai]

Message looks like this:

message MyMessage {
  // Some fields here
  google.protobuf.Timestamp time = 2;
}

When issuing request like so:

./gurl -u localhost:50051/<redacted>/<redacted> -d '{"time": { "seconds": 123 }}'

Getting error:

2017-11-21T13:11:09.688-0800    ERROR   grpc/validator.go:24    invalid syntax
go.uber.org/zap.Stack
    /Users/ccai/go/src/go.uber.org/zap/field.go:209
go.uber.org/zap.(*Logger).check
    /Users/ccai/go/src/go.uber.org/zap/logger.go:273
go.uber.org/zap.(*Logger).Error
    /Users/ccai/go/src/go.uber.org/zap/logger.go:176
github.com/wearefair/gurl/grpc.(*Validator).Validate
    /Users/ccai/go/src/github.com/wearefair/gurl/grpc/validator.go:24
github.com/wearefair/gurl/cmd.curl
    /Users/ccai/go/src/github.com/wearefair/gurl/cmd/root.go:90
github.com/spf13/cobra.(*Command).execute
    /Users/ccai/go/src/github.com/spf13/cobra/command.go:700
github.com/spf13/cobra.(*Command).ExecuteC
    /Users/ccai/go/src/github.com/spf13/cobra/command.go:785
github.com/spf13/cobra.(*Command).Execute
    /Users/ccai/go/src/github.com/spf13/cobra/command.go:738
github.com/wearefair/gurl/cmd.Execute
    /Users/ccai/go/src/github.com/wearefair/gurl/cmd/root.go:44
main.main
    /Users/ccai/go/src/github.com/wearefair/gurl/main.go:6

[catherinetcai]

https://github.com/grpc/grpc/issues/10304 could be related

[catherinetcai]

Check to see if the fix appended at the end of the thread can be resolved via protoreflect's standard imports: https://github.com/jhump/protoreflect/blob/master/desc/protoparse/std_imports.go

https://github.com/golang/protobuf/issues/414

The default protobuf jsonpb marshaler can handle Timestamp types - https://github.com/golang/protobuf/blob/master/jsonpb/jsonpb.go#L770 so figure out why it's not hitting that point...

[catherinetcai]

Looks like this can be resolved as long as it's using the default format of JSON -> Proto (and vice versa) defined by JSON mapping.

This means requests have to look like this:

go run main.go -u localhost:50051/service.Name/method.Name -d '{"time": "1972-01-01T10:00:20.021Z" }'

Also, after further inspection of the Timestamp type (and implementing it), it seems like it hits here. It might actually be using something on protoreflect's side to handle unmarshaling. Worth further inspection here

[catherinetcai]

Did a ton more digging.

Quite a few things are going to have to change if we decide to do a custom JSONPBUnmarshaler. It's really nasty.

All of the unmarshalling logic on the protoreflect side is going to have to swap to use the custom type, since it calls the jsonpb struct directly because it implements the JSONPBUnmarshaler interface. This is because underneath the covers, the jsonpb struct calls the proto's Unmarshal method first. The dynamic message's unmarshal json stuff wraps some of the logic around the dynamic message, then calls the original unmarshal method per field on the proto.

Anyway, here's where the current invalid syntax error is propagating from, because whatever's returned is not going to just be a string. (If curious, here's where the error is originating from).

[catherinetcai]

Relabeling this as an enhancement and not a bug, since using the right format fixes this issue.