Skip to content

[CDX-380] Add support for variationId param in RecommendationsRequest #183

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.

Sign up for GitHub

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

Open
quiteeasy wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[CDX-380] Add support for variationId param in RecommendationsRequest #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c488f3
[CDX-380] Add support for variationId param in RecommendationsRequest
quiteeasy Mar 25, 2026
07c0301
add validation for items size
quiteeasy Mar 25, 2026
1a46519
change validation, update test
quiteeasy Mar 25, 2026
1a6c884
move expression
quiteeasy Mar 25, 2026
6a38c34
lint
quiteeasy Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,6 +1811,17 @@ public String recommendationsAsJSON(RecommendationsRequest req, UserInfo userInf
}
}

if (StringUtils.isNotBlank(req.getVariationId())) {
if (req.getItemIds() == null || req.getItemIds().size() != 1) {
throw new IllegalArgumentException(
Copy link

@constructor-claude-bedrock constructor-claude-bedrock bot Mar 25, 2026

Choose a reason for hiding this comment

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

Important Issue: The IllegalArgumentException thrown here is inside the try block of recommendationsAsJSON, which has a catch (Exception exception) that wraps everything into a ConstructorException. This means the actual exception type reaching the caller will be ConstructorException (wrapping IllegalArgumentException), not IllegalArgumentException directly.

The integration tests correctly use thrown.expect(ConstructorException.class), which confirms the wrapping happens. However, the validation could be confusing because the Javadoc on recommendationsAsJSON says it throws ConstructorException, and callers may not realize the root cause is an IllegalArgumentException.

More importantly: consider whether this validation belongs here (in ConstructorIO) or earlier in RecommendationsRequest.setVariationId(). Validating at the request level (i.e., throwing in setVariationId if itemIds is already set with != 1 item, or at build time) would surface the error closer to the misconfiguration. That said, this placement is consistent with how other cross-field validations are handled in this file (e.g., page and offset cannot be used together), so this is acceptable if the team prefers it here.

"variationId requires exactly one itemId to be specified");
}
url =
url.newBuilder()
.addQueryParameter("variation_id", req.getVariationId())
.build();
}
Comment on lines +1814 to +1823
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

variation_id is documented as usable only when exactly one item_id is specified, but the request builder will currently add variation_id even when itemIds is null or contains multiple entries. Consider validating this combination (e.g., throw an IllegalArgumentException/ConstructorException when variationId is set and itemIds == null || itemIds.size() != 1) to prevent sending invalid requests.

Copilot uses AI. Check for mistakes.

if (StringUtils.isNotBlank(req.getTerm())) {
url = url.newBuilder().addQueryParameter("term", req.getTerm()).build();
}
Expand Down
18 changes: 18 additions & 0 deletions constructorio-client/src/main/java/io/constructor/client/RecommendationsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public class RecommendationsRequest {
private String term;
private int numResults;
private List<String> itemIds;
private String variationId;
private Map<String, List<String>> facets;
private String section;
private String preFilterExpression;
Expand All @@ -32,6 +33,7 @@ public RecommendationsRequest(String podId) throws IllegalArgumentException {
this.podId = podId;
this.numResults = 10;
this.itemIds = null;
this.variationId = null;
this.term = null;
this.section = "Products";
this.variationsMap = null;
Expand Down Expand Up @@ -97,6 +99,22 @@ public List<String> getItemIds() {
return itemIds;
}

/**
* @param variationId the variation id to set. Can be used with exactly one item_id specified in
* the request. Applicable for alternative_items, complementary_items, and bundles pod
* types.
*/
public void setVariationId(String variationId) {
this.variationId = variationId;
}

/**
* @return the variation id
*/
public String getVariationId() {
return variationId;
}

/**
* @param section the section to set
*/
Expand Down
37 changes: 37 additions & 0 deletions ...ructorio-client/src/test/java/io/constructor/client/ConstructorIORecommendationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,43 @@ public void getRecommendationsShouldReturnAResultWithSingleItemId() throws Excep
assertTrue("recommendation result id exists", response.getResultId() != null);
}

@Test
public void getRecommendationsShouldReturnAResultWithItemIdAndVariationId() throws Exception {
ConstructorIO constructor = new ConstructorIO("", apiKey, true, null);
UserInfo userInfo = new UserInfo(3, "c62a-2a09-faie");
RecommendationsRequest request = new RecommendationsRequest("item_page_1");
request.setItemIds(Arrays.asList("power_drill"));
request.setVariationId("power_drill_variation");
RecommendationsResponse response = constructor.recommendations(request, userInfo);
assertTrue("recommendation results exist", response.getResponse().getResults().size() >= 0);
Copy link

@constructor-claude-bedrock constructor-claude-bedrock bot Mar 25, 2026

Choose a reason for hiding this comment

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

Important Issue: The assertion response.getResponse().getResults().size() >= 0 is a vacuous truth — a List size can never be negative. This makes the assertion meaningless and will always pass even if the response is completely empty or broken. This same pattern exists in many other tests in this file (not introduced by this PR), but since you're adding a new test here, it's worth fixing.

Consider asserting something more meaningful. For example, if a valid power_drill_variation exists in the test dataset, assert size() > 0. If the variation may not match any results, at least assert the response itself is not null:

assertNotNull("recommendation response exists", response.getResponse());
assertNotNull("recommendation result id exists", response.getResultId());

assertTrue("recommendation result id exists", response.getResultId() != null);
Comment on lines +39 to +41
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The new test’s assertion response.getResponse().getResults().size() >= 0 is always true for any non-null list, so it won’t catch regressions. Prefer asserting that the results list is non-null (and/or size() > 0 if non-empty results are expected) and/or asserting the server-understood request includes the expected variation_id value.

Copilot uses AI. Check for mistakes.
}

@Test
public void getRecommendationsShouldErrorWithVariationIdAndNoItemIds() throws Exception {
ConstructorIO constructor = new ConstructorIO("", apiKey, true, null);
UserInfo userInfo = new UserInfo(3, "c62a-2a09-faie");
RecommendationsRequest request = new RecommendationsRequest("item_page_1");
request.setVariationId("power_drill_variation");

thrown.expect(ConstructorException.class);
Copy link

@constructor-claude-bedrock constructor-claude-bedrock bot Mar 25, 2026

Choose a reason for hiding this comment

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

Suggestion: The two error-case tests (getRecommendationsShouldErrorWithVariationIdAndNoItemIds and getRecommendationsShouldErrorWithVariationIdAndMultipleItemIds) correctly expect ConstructorException, which is the type that actually propagates to the caller after the catch (Exception e) { throw new ConstructorException(e); } wrapping in recommendationsAsJSON.

For completeness, consider also asserting on the cause: the wrapped IllegalArgumentException contains the real validation message. While thrown.expectMessage(...) checks the message transitively (it searches through the exception chain), it would make the test more explicit to verify the cause type as well:

thrown.expectCause(instanceOf(IllegalArgumentException.class));

This is a minor suggestion and is not blocking.

thrown.expectMessage("variationId requires exactly one itemId to be specified");
constructor.recommendations(request, userInfo);
}

@Test
public void getRecommendationsShouldErrorWithVariationIdAndMultipleItemIds() throws Exception {
ConstructorIO constructor = new ConstructorIO("", apiKey, true, null);
UserInfo userInfo = new UserInfo(3, "c62a-2a09-faie");
RecommendationsRequest request = new RecommendationsRequest("item_page_1");
request.setItemIds(Arrays.asList("power_drill", "drill"));
request.setVariationId("power_drill_variation");

thrown.expect(ConstructorException.class);
thrown.expectMessage("variationId requires exactly one itemId to be specified");
constructor.recommendations(request, userInfo);
}

@Test
public void getRecommendationsShouldReturnAResultWithMultipleItemIds() throws Exception {
ConstructorIO constructor = new ConstructorIO("", apiKey, true, null);
Expand Down
3 changes: 3 additions & 0 deletions constructorio-client/src/test/java/io/constructor/client/RecommendationsRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public void newShouldReturnDefaultProperties() throws Exception {
assertEquals(request.getPodId(), podId);
assertEquals(request.getNumResults(), 10);
assertNull(request.getItemIds());
assertNull(request.getVariationId());
assertNull(request.getTerm());
assertEquals(request.getSection(), "Products");
assertEquals(request.getFacets().size(), 0);
Expand All @@ -54,6 +55,7 @@ public void settersShouldSet() throws Exception {
request.setPodId("zero_results_1");
request.setNumResults(3);
request.setItemIds(Arrays.asList("1", "2", "3"));
request.setVariationId("variation-1");
request.setSection("Search Suggestions");
request.setFacets(facets);
request.setTerm(term);
Expand All @@ -62,6 +64,7 @@ public void settersShouldSet() throws Exception {

assertEquals(request.getPodId(), "zero_results_1");
assertEquals(request.getNumResults(), 3);
assertEquals(request.getVariationId(), "variation-1");
Copy link

@constructor-claude-bedrock constructor-claude-bedrock bot Mar 25, 2026

Choose a reason for hiding this comment

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

Suggestion: The settersShouldSet test sets variationId alongside itemIds with 3 items (Arrays.asList("1", "2", "3")). This combination would fail validation in ConstructorIO.recommendationsAsJSON (variationId with != 1 item). While the RecommendationsRequest POJO itself doesn't enforce this constraint (it's enforced at request-building time), it's worth noting this inconsistency for readers of the test. A test with a consistent state (1 itemId + 1 variationId) would be clearer and more representative of valid usage.

assertEquals(request.getSection(), "Search Suggestions");
assertEquals(request.getFacets(), facets);
assertEquals(request.getTerm(), term);
Expand Down
Loading