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

[quiteeasy]

No description provided.

[Copilot]

Pull request overview

Adds a variationId option to RecommendationsRequest and forwards it as a variation_id query parameter in the recommendations endpoint, with corresponding unit/integration test updates.

Changes:

• Add variationId field + getter/setter to RecommendationsRequest.
• Append variation_id query parameter when building the recommendations URL.
• Extend tests to cover the new request property and a variation-id recommendations call.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
constructorio-client/src/main/java/io/constructor/client/RecommendationsRequest.java	Introduces variationId as a first-class request property.
constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java	Adds variation_id to the recommendations request URL when provided.
constructorio-client/src/test/java/io/constructor/client/RecommendationsRequestTest.java	Verifies default/null and setter behavior for variationId.
constructorio-client/src/test/java/io/constructor/client/ConstructorIORecommendationsTest.java	Adds a recommendations test that includes both item_id and variation_id.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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.

[constructor-claude-bedrock]

Code Review

This PR adds support for the variationId parameter in RecommendationsRequest, including validation that exactly one itemId is present when it's used. The implementation is clean and follows existing patterns, with a few issues to address.

Inline comments: 4 discussions added

Overall Assessment: ⚠️ Needs Work

[constructor-claude-bedrock]

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.

[constructor-claude-bedrock]

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());

[constructor-claude-bedrock]

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.

[constructor-claude-bedrock]

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.