-
Notifications
You must be signed in to change notification settings - Fork 106
feat: Revamp HTTP Client: Multi-client support, Streaming & Optimizations #1317
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a comprehensive architectural refactor of the HTTP client package, transitioning from a singleton-based client to a Factory Pattern that supports multiple named HTTP clients with isolated connection pools and thread-safe client management.
Key Changes:
- Introduces a Factory Pattern for managing multiple named HTTP clients (e.g., "default", "stripe", "github")
- Implements thread-safe client initialization with double-checked locking
- Updates configuration structure to support multiple client definitions with independent settings
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/http/client/factory.go | New Factory interface defining Client() and Request() methods for managing named clients |
| contracts/http/client/client.go | New Client interface exposing Name(), Config(), HTTPClient(), and NewRequest() methods |
| contracts/http/client/config.go | Enhanced Config struct with mapstructure tags and DefaultConfig constant |
| http/client/factory.go | Factory implementation with thread-safe client registry and lazy initialization |
| http/client/client.go | Client implementation replacing singleton pattern with per-client Transport configuration |
| http/client/config.go | FactoryConfig struct for unmarshaling multi-client configuration |
| http/client/request.go | Updated to accept Client interface instead of raw config and http.Client |
| http/service_provider.go | Changed from Bind to Singleton registration and updated config unmarshaling |
| http/setup/stubs.go | Updated configuration stub to reflect new multi-client structure |
| foundation/container.go | Updated MakeHttp() to return Factory instead of Request |
| facades/facades.go | Updated Http() facade to return Factory interface |
| mocks/http/client/Factory.go | Generated mock for new Factory interface |
| mocks/http/client/Client.go | Updated mock with new Client interface methods |
| mocks/foundation/Application.go | Updated MakeHttp() signature to return Factory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1317 +/- ##
==========================================
+ Coverage 69.99% 70.24% +0.24%
==========================================
Files 282 281 -1
Lines 16659 16875 +216
==========================================
+ Hits 11660 11853 +193
- Misses 4511 4527 +16
- Partials 488 495 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: e125fa7 | Previous: 19dbf7a | Ratio |
|---|---|---|---|
BenchmarkFile_ReadWrite |
370669 ns/op 6258 B/op 99 allocs/op |
192070 ns/op 6257 B/op 99 allocs/op |
1.93 |
BenchmarkFile_ReadWrite - ns/op |
370669 ns/op |
192070 ns/op |
1.93 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
contracts/http/client/factory.go
Outdated
| Client(name ...string) Client | ||
|
|
||
| // Request is a convenience alias for Client(name...).NewRequest(). | ||
| // It immediately starts building a request using the specified (or default) client. | ||
| Request(name ...string) Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
facades.Http().Client("stripe").Get("/") should be fine, the functions of Client can be moved to the Request interface: facades.Http().Client("stripe").Name() and facades.Http().Name().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwbrzzl To support the requirements we discussed, I see two viable approaches.
Option 1: The "Proxy" Approach (3 Layers)
In this design, we maintain a strict separation between the Client (configuration) and the Request (builder). The Client acts as a factory that spawns new requests.
type Factory interface {
Request
Client(name ...string) Client
}
type Client interface {
Request
HttpClient() *http.Client
Config() *Config
}
type Request interface {
Get(uri string) (Response, error)
WithHeaders(headers map[string]string) Request
}This setup allows us to safely handle Dependency Injection. The Client instance is immutable, so calling methods on it spawns a fresh Request every time:
facades.Http().Get(...)
// All method calls here return a NEW Request instance.
// The original 'Client' struct remains immutable.
facades.Http().Client("stripe").Get(...)DI Example:
// In ServiceProvider:
app.Bind("PaymentService", func() {
return NewPaymentService(facades.Http().Client("stripe"))
})
type PaymentService struct {
client client.Client
}
func (s *PaymentService) Charge() {
// s.client is already configured with Stripe host/timeout.
// .WithHeaders() creates a new Request instance internally.
// .Post() modifies and sends that new instance.
s.client.WithHeaders(...).Get("/charges")
}Cons: The main downside is redundancy. We have to implement the proxy methods (Get, Post, WithHeaders, etc.) twice: once for the Factory (to proxy to default) and once for the Client (to proxy to Request).
Option 2: The "Flattened" Approach (2 Layers) (Preferred)
Alternatively, we can eliminate the Client interface entirely and merge it into Request.
type Factory interface {
Request
Client(name ...string) Request
}The usage remains the same syntactically:
facades.Http().Get(...)
facades.Http().Client("name").Get(...)DI Implication (Copy-On-Write):
If we use this approach in a Service, we are injecting a Request object directly. To prevent race conditions (where one request modifies the headers of the shared service instance), we must ensure immutability.
type PaymentService struct {
client client.Request
}
func (s *PaymentService) Charge() {
// We implement Copy-On-Write here.
// .WithHeaders() must strictly return a CLONE of the request with new headers.
// The original s.client remains unmodified.
s.client.WithHeaders(...).Get("/charges")
}We can achieve this by making every configuration method (like WithHeaders, WithToken) internally call a clone() method before applying changes.
@hwbrzzl @almas-x Which approach do you prefer, or do you have a better suggestion regarding this?
Note regarding Laravel: I believe Laravel achieves its multiple-client support via macros (though I am unsure if they support distinct connection pooling per macro).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer Option 2—the flattened two-layer design. It’s more consistent with how other modules like cache, log, and ORM work in the framework: initialize a default instance from config, but allow manual initialization when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details. Yes, option 2, consistent with other modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
http/client/request.go:134
- The
ReplaceHeaders()method doesn't actually replace headers - it only adds new headers from the provided map without clearing existing ones first. This differs from the expected behavior based on the method name. The method should either callFlushHeaders()first or be renamed to something likeMergeHeaders()orAddHeaders().
func (r *Request) ReplaceHeaders(headers map[string]string) client.Request {
return r.WithHeaders(headers)
}
func (r *Request) WithBasicAuth(username, password string) client.Request {
encoded := base64.StdEncoding.EncodeToString(fmt.Appendf(nil, "%s:%s", username, password))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
This PR replaces the single-client singleton (previously initialized via
sync.Once) with a Factory Pattern. This enables support for Multiple HTTP Clients while significantly refactoring the engine for performance and safety.Note
We will need to update TestRequest to support testing these HTTP client changes. I’ll create a separate PR for that, as the current PR is already quite large.
Critical: Configuration Update (Breaking Change)
The
config/http.gostructure has changed to support multiple clients. You must update your configuration file.Old Config (Single Client)
Previously, the configuration only allowed one global client under the
"client"key.New Config (Multi-Client)
We replace the single
"client"key withdefault_clientand aclientsmap. This avoids conflicts with the HTTP Server configuration (which often uses keys like host or port at the root).Key Features
1. Multiple Clients Support
Access specific API configurations on the fly using the new Factory.
2. Streaming (Low Memory Usage)
Introduced
Stream()to handle large file downloads efficiently without loading the entire response into RAM.3. Fail-Fast & Lazy Loading
Replaces the strict initialization with a "Lazy Error" pattern. Missing configurations no longer panic at boot; they return a safe error only when a request is attempted.
Usage & Migration
Dependency Injection (Best Practice)
Inject specific
client.Clientinstances into your services instead of the entire Factory.Response Parsing
We are moving to
Response.Bind()for safer error handling.Old Way (Deprecated):
New Way (Recommended):
Breaking Changes
config/http.gostructure must be updated.client.Responsenow includesStream()andBind(). If you mock this interface in tests, you must implement these methods.✅ Checks