[CDX-480] Expose headers for Autocomplete and Search Responses#187
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the autocomplete and search call paths so the resulting AutocompleteResponse and SearchResponse can expose HTTP response headers (e.g., rate-limit information) via a new getHeaders() API.
Changes:
- Capture OkHttp response headers for
autocompleteandsearch, and attach them to the parsed response objects. - Add
headersstorage +getHeaders()/setHeaders()toAutocompleteResponseandSearchResponse. - Add/extend tests to validate rate-limit headers are surfaced.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java | Refactors autocomplete/search execution to capture headers and plumb them into response creation. |
| constructorio-client/src/main/java/io/constructor/client/models/AutocompleteResponse.java | Adds headers field + accessor methods. |
| constructorio-client/src/main/java/io/constructor/client/models/SearchResponse.java | Adds headers field + accessor methods. |
| constructorio-client/src/test/java/io/constructor/client/ConstructorIOAutocompleteUrlEncodingTest.java | Adds header exposure assertions for autocomplete responses. |
| constructorio-client/src/test/java/io/constructor/client/ConstructorIOSearchUrlEncodingTest.java | Adds header exposure assertions for search responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
esezen
left a comment
There was a problem hiding this comment.
Looking good. I left some small comments. We can do them in a follow up PR if this needs to go out this week
| String json = autocompleteAsJSON(req, userInfo); | ||
| return createAutocompleteResponse(json); | ||
| Request request = createAutocompleteRequest(req, userInfo); | ||
| Response response = clientWithRetry.newCall(request).execute(); |
There was a problem hiding this comment.
Not sure where to put this comment but I think we should also expose these headers on 429. I might be missing something but I think right now we throw a ConstructorException for those without exposing the rate limit headers. Wdyt?
There was a problem hiding this comment.
oooh, hmm you make a good point. I'm not sure if the Exception we throw exposes anything ngl, but I agree it should be present there so that if you do get a 429, you can adjust accordingly (impt for configuration endpoints)
There was a problem hiding this comment.
Yes, I'm not too worried about the rest of the errors but the rate limiting headers are important specifically for 429
| * @return a string of JSON | ||
| * @throws ConstructorException if the request is invalid. | ||
| */ | ||
| public String naturalLanguageSearchAsJSON(NaturalLanguageSearchRequest req, UserInfo userInfo) |
There was a problem hiding this comment.
Doesn't have to be in this PR but this is also "search" and I think we should add the headers to this response as well
There was a problem hiding this comment.
fair, but, we'll bundle it in a later PR anyways
There was a problem hiding this comment.
Do we need to pass headers here or no?
There was a problem hiding this comment.
good catch, missed the overloads
| @SerializedName("request") | ||
| private Map<String, Object> request; | ||
|
|
||
| private transient Map<String, List<String>> headers = |
There was a problem hiding this comment.
Doesn't need to be in this PR but given how we are going to repeat this in all responses, we might want to extract header getHeaders setHeaders
There was a problem hiding this comment.
something like an inherited class?
There was a problem hiding this comment.
Code Review
This PR exposes HTTP response headers on AutocompleteResponse and SearchResponse via a clean ResponseHeaders base class, and propagates headers to ConstructorException on error paths. The implementation is well-structured and the error-path header propagation is correctly handled.
Inline comments: 6 discussions added
Overall Assessment:
PR Description
This change refactors the internal calling structure of
autocompleteandsearchto allow headers to be exposed. Headers are now exposed via the new methodgetHeaders()available on the types:AutocompleteResponse&SearchResponsegetHeadersreturns aMap<String, List<String>>object containing the headers.E.g.
Considerations