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.83% 64.81% -0.02%
==========================================
Files 2582 2584 +2
Lines 111582 111617 +35
Branches 8321 8309 -12
==========================================
+ Hits 72345 72348 +3
- Misses 33173 33206 +33
+ Partials 6064 6063 -1
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()) | |
| } |
…ogenous-encodings
…ogenous-encodings
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 Freighter Go HTTP transport to support heterogeneous request/response codecs, allowing the request
Content-Typeand responseAcceptheader to specify different encodings independently. The old single-codec-per-connection model is replaced with separate encoder/decoder registries on both the unary server and client.x/go/http/codec.gois removed and its role is absorbed into the newfreighter/go/http/codec.go, which definesEncoder,Decoder, andCodecinterfaces along withJSONCodec,MsgPackCodec, and shared default registries;ContentType()is removed from the underlyingx/encoding/jsonandx/encoding/msgpackpackages.stream.goandunary.goare split into focused files;NewRouternow returns an error instead of panicking; stream servers gainWithAdditionalCodecfor per-connection stateful codecs (used by the framer).router_test.goandunary_test.go, covering q-value ordering, wildcardAccept, 406 on no match, and cross-codec round-trips.Confidence Score: 4/5
The core codec-splitting logic and new content-negotiation path are well-tested, but several defects in the client and server error paths from the previous review round remain unaddressed.
The unary and stream clients call parseResponseCtx before guarding against a nil response causing a panic on network failure, NewUnaryClient silently fails validation with no arguments despite [OPTIONAL] docs, the unary server returns HTTP 500 instead of 415 for unrecognised Content-Type, and the Focus label in http_test.go will break CI. The framer codec integration, encoder/decoder separation, and router lifecycle refactor are all clean.
freighter/go/http/unary_client.go and freighter/go/http/http.go (nil dereference on network failure), freighter/go/http/unary_server.go (wrong HTTP status for bad Content-Type), core/pkg/server/http_test.go (Focus label).
Important Files Changed
Reviews (9): Last reviewed commit: "Merge branch 'rc' into sy-4124-update-fr..." | Re-trigger Greptile