feat: remove headers from client, move to CallResponse for public api#197
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the OpenAIClient request APIs to remove the need for mutable borrows by returning response headers alongside decoded response bodies via a new CallResponse<T> wrapper, improving thread-shareability and concurrency ergonomics.
Changes:
- Introduces
CallResponse<T> { headers, inner }to carry per-call headers with the response payload. - Updates
OpenAIClientinternal helpers (get/post/delete/post_form) and all public API methods to take&selfand returnResult<CallResponse<T>, APIError>. - Updates examples to use non-mutable clients and access payload data via
.inner(and headers via.headers).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/v1/responses/responses.rs |
Adds the new CallResponse<T> wrapper type. |
src/v1/audio.rs |
Removes per-response header storage from AudioSpeechResponse (headers now come from CallResponse). |
src/v1/api.rs |
Switches all client methods to &self and returns CallResponse<T> from non-stream API calls. |
examples/*.rs |
Updates example clients to be non-mutable and uses result.inner/result.headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Ok(parsed) => Ok(CallResponse { | ||
| headers, | ||
| inner: parsed, | ||
| }), |
There was a problem hiding this comment.
OpenAIClient::response_headers is no longer updated anywhere (the assignment was removed from handle_response), so it will always remain None even after successful calls. Since the new CallResponse<T> exposes headers per-call, this field should either be removed (breaking but consistent) or kept in sync (e.g., set it on success) to avoid a silently broken public field.
There was a problem hiding this comment.
Ohh damn, got it, removed.
| #[derive(Debug, Clone)] | ||
| pub struct CallResponse<T> { | ||
| pub headers: HeaderMap, | ||
| pub inner: T, | ||
| } |
There was a problem hiding this comment.
CallResponse<T> is a generic wrapper used by many API endpoints (completions, files, models, etc.), but it’s defined under v1::responses::responses, which makes the public API surface confusing and couples unrelated modules to the Responses API module path. Consider moving CallResponse to a more central module (e.g., v1::api, v1::common, or a new v1::response module) and re-exporting it from there.
There was a problem hiding this comment.
@dongri not sure about this one tbh, would be up to you to decide. In my world I would keep it with the other respones to just have a clean definition where responses are live.
|
@MichaelProjects Could you please review the issues raised by Copilot? |
Hey, I took another spin on #168
The previous PR was declined primarily because it removed the header entirely #170 — this change addresses that by moving headers out of the client and into the response itself via CallResponse, keeping them co-located with the data they describe.
Beyond headers, the &mut self pattern has a real concurrency cost: it prevents OpenAIClient from being Sync, meaning it cannot be shared across threads without a Mutex. But even with a Mutex, concurrent requests become serialized — and if multiple requests do run concurrently, response_headers on the client becomes last-write-wins, meaning you can no longer reliably tell which request a given header came from. Moving headers into CallResponse eliminates this entirely — each response owns its headers, scoped to that call.
This PR fixes both:
The added wrapper is a small ergonomic cost, but it keeps headers and response data together without any shared mutable state on the client.
Breaking change
This would be a breaking change to the api so a major release needed but it will definitely make the library more easy and performance to use for concurrent use cases.