SY-4124: Update Freighter (Go) to allow for heterogenous codecs#2271
SY-4124: Update Freighter (Go) to allow for heterogenous codecs#2271
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rc #2271 +/- ##
==========================================
- Coverage 64.30% 64.29% -0.01%
==========================================
Files 2166 2168 +2
Lines 109898 109980 +82
Branches 8298 8285 -13
==========================================
+ Hits 70669 70711 +42
- Misses 33183 33218 +35
- Partials 6046 6051 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…4-update-freighter-go-to-allow-for-heterogenous-encodings
There was a problem hiding this comment.
Do all of these types need to be public? If so, all of them need tests. Some seem to be missing public tests.
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| "github.com/synnaxlabs/x/http" | ||
| fhttp "github.com/synnaxlabs/freighter/http" |
There was a problem hiding this comment.
Does this need an alias? Double check other files in this PR that may not need aliases.
| return b | ||
| } | ||
|
|
||
| type coreConfig struct { |
|
|
||
| // streamCore is the common functionality implemented by both the client and server | ||
| // streams. | ||
| type streamCore[I, O freighter.Payload] struct { |
There was a problem hiding this comment.
Does this type belong in this file?
| . "github.com/synnaxlabs/x/testutil" | ||
| ) | ||
|
|
||
| var _ = Describe("Router", func() { |
There was a problem hiding this comment.
Are we using our gleak utilities from testutil on this test suite? If not, we should be.
| // WithAdditionalCodec registers a stream-server codec on top of the default codec list. | ||
| // The constructor is invoked once per matching request so the codec may be stateful and | ||
| // hold per-stream state. | ||
| func WithAdditionalCodec( |
There was a problem hiding this comment.
If codec already implements the ContentType method how does this make sense? There has to be a cleaner way to do this.
…ogenous-encodings
| func NewUnaryClient[RQ, RS freighter.Payload]( | ||
| configs ...UnaryClientConfig, | ||
| ) (freighter.UnaryClient[RQ, RS], error) { | ||
| cfg, err := config.New(UnaryClientConfig{}, configs...) |
There was a problem hiding this comment.
NewUnaryClient ignores documented defaults
config.New is called with UnaryClientConfig{} (zero value) as the base, so the defaultUnaryClientConfig that documents Encoder = msgpack.Codec and Decoders = defaultDecoders is never applied. Calling NewUnaryClient[RQ, RS]() with no arguments will always fail validation with "encoder is required" and "at least one decoder is required", despite the field-level doc-comments marking both fields as [OPTIONAL].
Compare with NewStreamClient, which correctly passes defaultStreamClientConfig as the base:
cfg, err := config.New(defaultStreamClientConfig, configs...)| cfg, err := config.New(UnaryClientConfig{}, configs...) | |
| cfg, err := config.New(defaultUnaryClientConfig, configs...) |
| httpRes, err := (&http.Client{}).Do(httpReq) | ||
| outCtx := parseResponseCtx(httpRes, target) | ||
| if err != nil { | ||
| return outCtx, err |
There was a problem hiding this comment.
parseResponseCtx(httpRes, target) is called before the err != nil guard. When http.Client.Do fails with a network-level error (e.g. connection refused), Go's net/http docs guarantee httpRes is nil. parseResponseCtx immediately dereferences c.Header (len(c.Header)), which panics.
The nil check must come first:
httpRes, err := (&http.Client{}).Do(httpReq)
if err != nil {
return freighter.Context{}, err
}
outCtx := parseResponseCtx(httpRes, target)| oCtx := parseResponseCtx(res, target) | ||
| if err != nil { | ||
| return oCtx, err |
There was a problem hiding this comment.
Same pattern as in unary_client.go: parseResponseCtx(res, target) is called before the err != nil guard. When ws.Dialer.DialContext cannot establish a TCP connection, res is nil and the call to parseResponseCtx will panic at len(c.Header). The nil check (and the res.StatusCode check at line 111) must come first.
…ogenous-encodings
| func (s *unaryServer[RQ, RS]) fiberHandler(fCtx fiber.Ctx) error { | ||
| decoder, err := s.resolveRequestDecoder(fCtx.Get(fiber.HeaderContentType)) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Content-Type returns HTTP 500 instead of 415
When resolveRequestDecoder returns an error (unrecognized or missing Content-Type), the error is returned bare from fiberHandler. Fiber maps a non-fiber error to a 500 Internal Server Error. Clients sending an unsupported encoding will see a confusing 500 instead of a 415 Unsupported Media Type.
This is inconsistent with the stream server, which correctly sends a 400 with a plain-text body:
return upgradeCtx.Status(fiber.StatusBadRequest).SendString(err.Error())The unary handler should mirror that pattern:
| func (s *unaryServer[RQ, RS]) fiberHandler(fCtx fiber.Ctx) error { | |
| decoder, err := s.resolveRequestDecoder(fCtx.Get(fiber.HeaderContentType)) | |
| if err != nil { | |
| return err | |
| decoder, err := s.resolveRequestDecoder(fCtx.Get(fiber.HeaderContentType)) | |
| if err != nil { | |
| return fCtx.Status(fiber.StatusUnsupportedMediaType).SendString(err.Error()) | |
| } |
2 participants
Issue Pull Request
Linear Issue
SY-4124
Description
Update Freighter (Go) to allow for different request / response Codecs. This means that request and response do not have to use the same codec (e.g., different
Accepts andContent-Typeheaders, and some endpoints could allow for multiple request or response codecs if they wanted to accept or be able to output several different types (e.g., exporting YAML or TOML or JSON)Basic Readiness
Greptile Summary
This PR refactors the Go Freighter HTTP transport to support heterogeneous request/response codecs: unary servers now negotiate the request decoder from
Content-Typeand the response encoder fromAcceptindependently, while stream servers accept a per-connection codec factory viaWithAdditionalCodec. The framer codec's custom resolver is replaced byWithAdditionalCodec(\"application/sy-framer\", ...).Content-Type:resolveRequestDecoder's error is returned bare fromfiberHandler, causing Fiber to emit a 500 Internal Server Error. The stream server correctly returns 400; this path should return 415 Unsupported Media Type.Confidence Score: 3/5
Not safe to merge as-is — a P1 regression (500 vs 415) remains unaddressed, and several previously-flagged P1s (nil dereference, missing defaults, Focus label) may also be outstanding.
One new P1 (500 on unsupported Content-Type) plus multiple P1s carried from prior review rounds that do not appear resolved in the current head commit. Combined, these pull the score below the P1 ceiling of 4.
freighter/go/http/unary_server.go (500 vs 415), freighter/go/http/unary_client.go (nil dereference + missing defaults), freighter/go/http/stream_client.go (nil dereference), core/pkg/server/http_test.go (Focus label)
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant US as UnaryServer participant SS as StreamServer Note over C,US: Unary Request (heterogeneous codecs) C->>US: POST /endpoint Content-Type: application/msgpack Accept: application/json US->>US: resolveRequestDecoder(Content-Type) alt unsupported Content-Type US-->>C: 500 Internal Server Error (should be 415) end US->>US: resolveResponseEncoder(Accept) alt no encoder matches Accept US-->>C: 406 Not Acceptable end US->>US: decoder.Decode(body) US->>US: handler(ctx, req) US->>US: encoder.Encode(res) US-->>C: 200 OK Content-Type: application/json Note over C,SS: Stream Upgrade (single negotiated codec) C->>SS: GET /endpoint Content-Type: application/msgpack WebSocket upgrade SS->>SS: resolveStreamCodec(Content-Type) alt unsupported Content-Type SS-->>C: 400 Bad Request end SS->>SS: fiberws.New(handleSocket) SS-->>C: 101 Switching Protocols loop stream lifetime C->>SS: WSMessage[RQ] SS-->>C: WSMessage[RS] end SS-->>C: WSMessage[Close]Reviews (7): Last reviewed commit: "Reorganize files" | Re-trigger Greptile