connect-go icon indicating copy to clipboard operation
connect-go copied to clipboard

Need to be able to handle low-level marshalling errors

Open torkelrogstad opened this issue 10 months ago • 3 comments

Is your feature request related to a problem? Please describe. My code is somehow constructing Protobuf messages that fail to encode: timestamppb.Timestamp values with second values larger than maxSecondsInDuration. That's on me - but the current tools for picking up the error is insufficient.

Reproduce:

  1. Create a timestamppb.Timestamp message with Seconds set to 315576000001.
  2. Return this message as part of a response.
  3. Call the endpoint in such a way that protojson encoding is used: curl -H "content-type: application/json" -X POST --fail-with-body http://server/service/Endpoint
  4. curl fails: curl: (22) The requested URL returned error: 500 {"code":"internal","message":"marshal message: proto: google.protobuf.Timestamp: seconds out of range 315576000000"}
  5. Observe that there's no way within interceptors or other user-available mechanisms to handle this error.

Describe the solution you'd like From what I can observe the error is returned from h.implementation(ctx, connCloser) here: https://github.com/connectrpc/connect-go/blob/d7c0966751650b41a9f1794513592e81b9beed45/handler.go#L237

It would be nice to provide some sort of hook for picking this up, so that I can for example log it. In this case it would be great to have access to the complete Protobuf message that failed marshalling. In my case I'm getting this error when marshalling a rather large message containing a few thousands timestamps, and it's hard to decipher which one is incorrectly constructed.

torkelrogstad avatar Feb 13 '25 11:02 torkelrogstad

That is an error returned from the codec, when the message is marshaled to bytes. You can handle such errors by providing a custom codec. The custom codec could delegate to the underlying serialization library and then do something if it fails.

To keep this repo's API minimal (for flexibility reasons -- so we have more options to make future changes without breaking compatibility and needing to rev to v2), the existing codecs are not directly exposed. But you can easily create a custom JSON codec that type-asserts the messages to proto.Message and then delegates to the protojson package for actual marshaling and unmarshaling. You could even just copy the implementation of the existing internal one.

Note that there is an analogous failure for requests: you interceptor will never see a request, and the framework will automatically send back an error (similar to the case here) if it receives a message that it cannot unmarshal.

jhump avatar Feb 13 '25 13:02 jhump

A big downside with implementing my own codec (which I've actually done in another case, to customize the error message my consumers get!) is that the codec doesn't have access to any of the metadata about the request. I stick lots of metadata into a context.Context (don't we all?), including a request ID that I use for logs and traces.

Note that there is an analogous failure for requests

Yes, I've ran into that multiple times as well. Would be great to be able to hook into that, also!

torkelrogstad avatar Feb 13 '25 15:02 torkelrogstad

Last year we separately were looking into better foundations for observability, to improve on the shortcomings of otelconnect. So we may be able to address this with that work.

jhump avatar Feb 21 '25 16:02 jhump