Skip to content

Allow injecting a custom HTTP transport into the SSE client - #105

Merged
alejovazquezsplit merged 1 commit into
mainfrom
FME-17477
Aug 19, 2026
Merged

Allow injecting a custom HTTP transport into the SSE client#105
alejovazquezsplit merged 1 commit into
mainfrom
FME-17477

Conversation

@alejovazquezsplit

Copy link
Copy Markdown
Contributor

What
Adds a functional-options parameter to sse.NewClient and a WithCustomTransport(http.RoundTripper) option, letting callers supply the HTTP transport used for the SSE connection. When no option is passed, behavior is unchanged (a cloned http.DefaultTransport with ProxyFromEnvironment).

Why
Consumers (e.g. go-split-commons) need to control the streaming connection's transport — specifically to pin the SSE connection to HTTP/1.1 and avoid Go's http2 client stack, which emits noisy protocol error: received DATA after END_STREAM logs on the long-lived stream. Rather than baking that policy into the toolkit, this exposes a generic hook so callers decide the transport.

Changes

  • sse/sse.go: introduce type Option func(*options), WithCustomTransport, and a variadic opts ...Option on NewClient; build the default transport only when none is injected.
  • sse/sse_test.go: add TestNewClientDefaultTransport and TestNewClientWithCustomTransport.

Compatibility
Non-breaking: NewClient's new parameter is variadic, so existing call sites compile and behave unchanged. No go.mod changes.

Testing
go build ./... and go test ./sse/... pass.

@alejovazquezsplit
alejovazquezsplit requested a review from a team August 18, 2026 21:38
@sonarqube-pull-requests

Copy link
Copy Markdown

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@sanzmauro sanzmauro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, i leave a minor comment

Comment thread sse/sse.go
transportToUse := o.transport
if transportToUse == nil {
t := http.DefaultTransport.(*http.Transport).Clone()
t.Proxy = http.ProxyFromEnvironment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this line needs to go in here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it has to. That if builds the default transport, and Proxy = ProxyFromEnvironment is part of it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's ok to be able to configure it, but you'll have to make sure you set it whenever passing a custom transport layer from one of our apps, since all of them are expected to honor that env var.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Our only custom transport caller (go-split-commons NewStreamingClient) already sets transport.Proxy = http.ProxyFromEnvironment, so the env var is honored there too. I'll keep that as the rule for any future app-side transport

Comment thread sse/sse.go
transportToUse := o.transport
if transportToUse == nil {
t := http.DefaultTransport.(*http.Transport).Clone()
t.Proxy = http.ProxyFromEnvironment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's ok to be able to configure it, but you'll have to make sure you set it whenever passing a custom transport layer from one of our apps, since all of them are expected to honor that env var.

@alejovazquezsplit
alejovazquezsplit merged commit 6e7cf0d into main Aug 19, 2026
3 checks passed
@alejovazquezsplit
alejovazquezsplit deleted the FME-17477 branch August 19, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants