-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error handling google.protobuf.Timestamp #3
Comments
grpc/grpc#10304 could be related |
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 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... |
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:
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 |
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). |
Relabeling this as an enhancement and not a bug, since using the right format fixes this issue. |
Message looks like this:
When issuing request like so:
Getting error:
The text was updated successfully, but these errors were encountered: