[CDX-628] Expose Response Headers on Browse by Mudaafi · Pull Request #191 · Constructor-io/constructorio-java

Mudaafi · 2026-09-14T16:53:29Z

Similar to #187, but just for Browse

Copilot

🔵 Needs a closer look

One or more issues must be addressed before approval.

Pull request overview

Exposes HTTP response headers for Browse-related response objects and propagates them through synchronous and asynchronous requests.

Changes:

Added header access to Browse, Browse Facets, and Facet Options responses.
Propagated headers to successful responses and ConstructorException.
Added success and error header tests for Browse endpoints.

File summaries

File	Description
constructorio-client/src/test/java/io/constructor/client/ConstructorIOBrowseResponseHeadersTest.java	Updated as part of this pull request.
constructorio-client/src/main/java/io/constructor/client/models/BrowseResponse.java	Updated as part of this pull request.
constructorio-client/src/main/java/io/constructor/client/models/BrowseFacetsResponse.java	Updated as part of this pull request.
constructorio-client/src/main/java/io/constructor/client/models/BrowseFacetOptionsResponse.java	Updated as part of this pull request.
constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java	Updated as part of this pull request.

Review details

Suppressed comments (2)

constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java:1467

The new asynchronous browse path is not covered by the added header tests: ConstructorIOBrowseAsyncTest exercises callback success but never checks response.getHeaders() or the failure callback's ConstructorException.getHeaders(). Add MockWebServer-backed async success and error cases so this distinct propagation path cannot regress independently of the synchronous methods.

                                        headers = response.headers().toMultimap();
                                        String json = getResponseBody(response);
                                        BrowseResponse res = createBrowseResponse(json, headers);

constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java:1656

The new asynchronous browseItems path is not covered by the added header tests: the existing async tests do not assert headers on either the response or the failure exception. Add MockWebServer-backed async success and error cases for this overload so its separate header propagation logic is verified.

                                        headers = response.headers().toMultimap();
                                        String json = getResponseBody(response);
                                        BrowseResponse res = createBrowseResponse(json, headers);

Files reviewed: 5/5 changed files
Comments generated: 0
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot's review flagged that the callback-based browse/browseItems paths have their own header-propagation logic in ConstructorIO.java that wasn't covered by any test, so a regression there could slip through independently of the synchronous methods. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Also removes the unused ExpectedException rule, which had no remaining purpose once none of the tests in the file used it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…e-headers-ootb-browse

TarekAlQaddy

LGTM, left one small comment.

side q, wondering why we didn't add the headers to the "AsJson" methods for Search, Autocomplete and Browse?

TarekAlQaddy · 2026-09-18T12:40:12Z

+        ConstructorIO constructor =
+                new ConstructorIO("", apiKey, false, "127.0.0.1", mockServer.getPort());
+        BrowseRequest request = new BrowseRequest("Color", "Blue");
+        BrowseResponse response = constructor.browse(request, null);


Could you add another test to the browse() overload that takes a callback? Same applies to the browseItems() overload

Mudaafi · 2026-09-21T13:55:43Z

A little weird to add the headers in the JSON string we return imo. Would require quite a bit more convoluted parsing and re-stringing which idt the JSON methods are really looking for.

constructor-claude-bedrock

Code Review

This PR mirrors the pattern from #187 to expose HTTP response headers on Browse-family responses (BrowseResponse, BrowseFacetsResponse, BrowseFacetOptionsResponse) and on ConstructorException, with solid test coverage using MockWebServer.

Inline comments: 4 discussions added

Overall Assessment: ⚠️ Needs Work

constructor-claude-bedrock · 2026-09-21T14:00:10Z

+                    }
+                });
+
+        await().atMost(2, SECONDS).until(responseIsResolved());


Important Issue: The instance fields responseResolved and exceptionResolved are never reset between tests. JUnit does not create a new test-class instance per test by default (it does with JUnit 5's default lifecycle, but this is JUnit 4). If an async callback from a previous test fires late, it can set responseResolved or exceptionResolved before the next test's await() check, causing a false-positive assertion on stale data.

Add a @Before method to reset the fields before each test:

@Before public void resetState() { responseResolved = null; exceptionResolved = null; }

(Also applies to all other async tests in this file that rely on these fields.)

TarekAlQaddy

LGTM ✅

expose headers on Browse

37bc3c5

Mudaafi requested review from a team and a lite review from Copilot September 14, 2026 16:53

Mudaafi requested a review from a team as a code owner September 14, 2026 16:53

Copilot started reviewing on behalf of Mudaafi September 14, 2026 16:54 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

Revert async header propagation tests for browse and browseItems

69a6ea1

Also removes the unused ExpectedException rule, which had no remaining purpose once none of the tests in the file used it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

This comment was marked as outdated.

Sign in to view

update tests

e31dc4c

This comment was marked as outdated.

Sign in to view

Merge branch 'master' into cdx-628-java-sdk-expose-rate-limit-respons…

9ceb817

…e-headers-ootb-browse

This comment was marked as outdated.

Sign in to view

TarekAlQaddy previously approved these changes Sep 18, 2026

View reviewed changes

add more tests

1e66cdf

Mudaafi dismissed TarekAlQaddy’s stale review via 1e66cdf September 21, 2026 13:55

constructor-claude-bedrock Bot reviewed Sep 21, 2026

View reviewed changes

TarekAlQaddy approved these changes Sep 21, 2026

View reviewed changes

Mudaafi merged commit 88260bf into master Sep 21, 2026
5 checks passed

Mudaafi deleted the cdx-628-java-sdk-expose-rate-limit-response-headers-ootb-browse branch September 21, 2026 15:07

Conversation

Mudaafi commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

TarekAlQaddy left a comment

Choose a reason for hiding this comment

Uh oh!

TarekAlQaddy Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Mudaafi Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

Mudaafi commented Sep 21, 2026

Uh oh!

constructor-claude-bedrock Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

constructor-claude-bedrock Bot Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TarekAlQaddy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mudaafi commented Sep 14, 2026 •

edited

Loading