Skip to content

[SABRA-2456] Add V2 Facets and Searchabilities API Support#179

Open
aandrukhovich wants to merge 3 commits intomasterfrom
SABRA-2456/cio-java
Open

[SABRA-2456] Add V2 Facets and Searchabilities API Support#179
aandrukhovich wants to merge 3 commits intomasterfrom
SABRA-2456/cio-java

Conversation

@aandrukhovich
Copy link

Summary

This PR adds support for the v2 versions of the facets and searchabilities APIs, which include additional fields and capabilities not available in v1.

Key Changes

New Model Classes:

  • FacetConfigurationV2 - v2 facet configuration with path_in_metadata field support
  • SearchabilityV2 - Searchability configuration model

New Request Classes:

  • FacetConfigurationV2Request - Single facet v2 operations
  • FacetConfigurationsV2Request - Bulk facet v2 operations
  • SearchabilityV2Request - Single searchability operations
  • SearchabilitiesV2Request - Bulk searchability PATCH operations
  • SearchabilitiesV2GetRequest - Searchability list operations with filtering/pagination
  • SearchabilitiesV2DeleteRequest - Bulk searchability deletion

New API Methods in ConstructorIO:

Endpoint Method
GET /v2/facets retrieveFacetConfigurationsV2()
GET /v2/facets/{name} retrieveFacetConfigurationV2()
POST /v2/facets createFacetConfigurationV2()
PUT /v2/facets/{name} replaceFacetConfigurationV2()
PATCH /v2/facets/{name} updateFacetConfigurationV2()
PATCH /v2/facets updateFacetConfigurationsV2()
PUT /v2/facets replaceFacetConfigurationsV2()
DELETE /v2/facets/{name} deleteFacetConfigurationV2()
GET /v2/searchabilities retrieveSearchabilitiesV2()
GET /v2/searchabilities/{name} retrieveSearchabilityV2()
PATCH /v2/searchabilities/{name} createOrUpdateSearchabilityV2()
PATCH /v2/searchabilities createOrUpdateSearchabilitiesV2()
DELETE /v2/searchabilities/{name} deleteSearchabilityV2()
DELETE /v2/searchabilities deleteSearchabilitiesV2()

v2 API Differences from v1

  • Facets v2: Requires path_in_metadata field which specifies where in item metadata the facet data is located
  • Searchabilities v2: New endpoints for managing searchability configurations with support for skip_rebuild parameter

Test Plan

  • Unit tests for all model and request classes
  • Integration tests for all v2 API methods
  • Verify facet CRUD operations work correctly
  • Verify searchability CRUD operations work correctly
  • Verify bulk operations work correctly
  • Verify pagination and filtering work for list operations

@aandrukhovich aandrukhovich changed the title Add v2 versions of facets and searchabilities [SABRA-2456] Add v2 facets and searchabilities Dec 30, 2025
@aandrukhovich aandrukhovich changed the title [SABRA-2456] Add v2 facets and searchabilities [SABRA-2456] Add V2 Facets and Searchabilities API Support Dec 30, 2025
@constructor-claude-bedrock

This comment has been minimized.

Copilot AI review requested due to automatic review settings February 25, 2026 23:30
@constructor-claude-bedrock
Copy link

Code Review Summary

This PR adds well-structured support for the v2 Facets and Searchabilities APIs. The implementation follows existing project conventions (Gson, @SerializedName, clientWithRetry for reads, client for writes) and includes a good two-tier test suite (unit + integration). The code is generally clean and readable. That said, there are several issues — a couple of them functional bugs — that should be addressed before merging.


Detailed Feedback

[Bug: File: ConstructorIO.java] — Inconsistent HTTP client usage in retrieveFacetConfigurationsV2 / retrieveFacetConfigurationV2

The single-facet and list-facets GET methods correctly use clientWithRetry. However, deleteFacetConfigurationV2 and all the write methods use client (no retry), which is consistent with v1 write behaviour. This is fine. The issue is that retrieveFacetConfigurationsV2 (with offset) has a logic bug:

if (offset != null && offset > 0 && page == null) {

An offset of 0 is a valid pagination offset (meaning "start from the beginning") but is silently ignored. This is the same bug reproduced in retrieveSearchabilitiesV2. The condition should be:

if (offset != null && offset >= 0 && page == null) {

[Bug: File: ConstructorIO.java]DEFAULT_SECTION constant placement

DEFAULT_SECTION is declared as a public static final field in ConstructorIO.java at the point where the v2 methods begin, in the middle of the file. This is unusual — constants are typically declared at the top of the class alongside other field declarations. More importantly, since it's public, it becomes part of the public API of ConstructorIO, which is fine, but the placement mid-file makes it easy to miss. It should be moved to the top of the class with other constants/fields.


[Bug: File: ConstructorIO.java]createFacetConfigurationV2 and write methods do not validate the section

retrieveFacetConfigurationsV2 normalises a null/blank section to DEFAULT_SECTION:

urlBuilder.addQueryParameter("section",
    (section != null && !section.trim().isEmpty()) ? section : DEFAULT_SECTION);

But createFacetConfigurationV2, replaceFacetConfigurationV2, updateFacetConfigurationV2, etc. use the section from the request object directly with no null check:

urlBuilder.addQueryParameter("section", facetConfigurationV2Request.getSection())

Since FacetConfigurationV2Request's constructor already enforces section != null, this is safe. However, setSection(null) on the request object after construction would bypass the constructor guard and cause a NullPointerException when OkHttp calls addQueryParameter. Consider either adding a null check in the setter, or adding a guard in the ConstructorIO methods.


[Issue: File: ConstructorIO.java]retrieveFacetConfigurationsV2 overloads bypass request object pattern

All other v2 methods accept a dedicated request object (e.g., FacetConfigurationV2Request, SearchabilitiesV2GetRequest). The retrieveFacetConfigurationsV2 overloads are the only methods that expose raw parameters (String section, Integer page, etc.) directly on ConstructorIO, rather than accepting a FacetConfigurationsV2GetRequest. This inconsistency will become a maintenance burden if more pagination/filter parameters are added to the GET /v2/facets endpoint later. Consider introducing a FacetConfigurationsV2GetRequest class (analogous to the already-present SearchabilitiesV2GetRequest) and delegating to it.


[Issue: File: ConstructorIO.java]Gson instantiated per-call instead of using a shared instance

Throughout the new methods, new Gson() is constructed inline:

String params = new Gson().toJson(facetConfigurationV2Request.getFacetConfiguration());

This is the same pattern used in the existing v1 code, so it's not a regression, but it is wasteful. Each Gson construction involves reflection scanning. A shared, static private static final Gson GSON = new Gson() instance (which is thread-safe) would be more efficient. This is worth raising as a project-wide improvement.


[Issue: File: FacetConfigurationV2.java Line: L71]matchType downgraded from enum to String

The v1 FacetConfiguration model uses a typed MatchType enum (all, any, none). The v2 model uses a plain String. This removes compile-time safety for callers setting matchType. If the API only accepts a fixed set of values, consider reusing the v1 enum or introducing a v2-specific one. If the API is genuinely open-ended here, this is acceptable but worth documenting with a comment.


[Issue: File: FacetConfigurationV2.java] — Missing options field

The v1 FacetConfiguration model includes a List<FacetOptionConfiguration> options field for per-facet option overrides. The v2 model omits this entirely. If the v2 API still returns option configurations in its response body, they will be silently dropped during deserialization. Please confirm with the API spec whether options is part of the v2 response and, if so, add the field.


[Issue: File: SearchabilityV2Request.java] — Constructor allows null searchability for write operations without documentation

The SearchabilityV2Request(String name, String section) and SearchabilityV2Request(String name) constructors explicitly pass null for searchability:

public SearchabilityV2Request(String name, String section) {
    this(null, name, section);
}

This is intentional for GET/DELETE operations, but createOrUpdateSearchabilityV2 calls searchabilityV2Request.getSearchability() directly and passes it to Gson().toJson(...). If a caller constructs the request with the read-only constructor and then passes it to createOrUpdateSearchabilityV2, Gson will serialize null as the JSON body, resulting in a malformed API request. Add a guard in createOrUpdateSearchabilityV2:

if (searchabilityV2Request.getSearchability() == null) {
    throw new IllegalArgumentException("searchability body is required for create/update operations");
}

[Issue: File: ConstructorIOFacetConfigurationV2Test.java / ConstructorIOSearchabilityV2Test.java] — Cleanup registration happens after assertions

In integration tests like testCreateFacetConfigurationV2, the call to addFacetToCleanupArray(...) is placed after the assert* statements. If an assertion fails, the cleanup entry is never registered and the test resource leaks into the API environment. Move addFacetToCleanupArray (and the equivalent for searchabilities) to before assertions, immediately after a successful API call is confirmed:

// After: String response = constructor.createFacetConfigurationV2(request);
addFacetToCleanupArray(facetName); // register cleanup BEFORE asserting
assertNotNull(response);
// ...

[Issue: File: ConstructorIOSearchabilityV2Test.java Line: ~16] — Misleading environment variable name

private static String apiKey = System.getenv("TEST_CATALOG_FACETS_V2_API_KEY");

Both ConstructorIOFacetConfigurationV2Test and ConstructorIOSearchabilityV2Test use TEST_CATALOG_FACETS_V2_API_KEY. If the same key is intentionally shared, that's fine, but it should be documented with a comment. If the searchability tests are meant to use a different key, rename the variable and add the corresponding secret.


[Issue: File: ConstructorIOSearchabilityV2Test.java Lines: ~90-92] — Integration test assertions hardcode fixture values

assertEquals(true, jsonObj.get("fuzzy_searchable"));
assertEquals(false, jsonObj.get("exact_searchable"));

These values are duplicated from searchability.v2.json. The facet integration tests avoid this by loading facetConfigurationJson and comparing against loadedJsonObj, which is more resilient to fixture changes. Apply the same pattern here.


[Minor: File: FacetConfigurationV2.java] — Setter Javadocs are missing

All getters in FacetConfigurationV2 have Javadoc comments. The setters do not. This is inconsistent with the getter documentation style and with SearchabilityV2.java (which also omits setter docs). Either add brief @param Javadoc to setters, or remove getter Javadoc to be consistent — whichever matches the project convention.


[Minor: File: SearchabilitiesV2GetRequest.java Line: ~53] — Constructor with explicit null check differs from sibling classes

public SearchabilitiesV2GetRequest(String section) {
    if (section == null) {
        throw new IllegalArgumentException("section is required");
    }
    this.section = section;
}

FacetConfigurationV2Request and FacetConfigurationsV2Request use the same null-check pattern for section. This is consistent and correct. However, none of the request classes validate that section is non-blank (only non-null). An empty string "" for section would pass validation and result in a malformed query parameter. Consider using the same section == null || section.trim().isEmpty() check that is used for facetName and name.


Conclusion

The PR is a solid foundation for the v2 API surface. The overall structure, documentation, and test coverage are good. The two items that need fixing before merge are the offset >= 0 boundary condition bug and the null searchability body issue in createOrUpdateSearchabilityV2. The inconsistency of retrieveFacetConfigurationsV2 not using a request object is worth addressing for long-term API consistency. The remaining points are lower-priority but should be tracked.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Java client support for Constructor.io v2 Facets and v2 Searchabilities APIs, introducing new models/request objects plus ConstructorIO methods and accompanying unit/integration tests.

Changes:

  • Added v2 model + request classes for facet configurations and searchabilities (single/bulk, plus GET filtering/pagination and bulk delete).
  • Implemented v2 endpoints in ConstructorIO for facets and searchabilities CRUD/bulk operations.
  • Added test fixtures, unit tests, integration tests, and CI env wiring for the new v2 API key.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
constructorio-client/src/test/resources/searchability.v2.json Test fixture JSON for v2 searchability serialization/deserialization.
constructorio-client/src/test/resources/facet.configuration.v2.json Test fixture JSON for v2 facet configuration (includes path_in_metadata).
constructorio-client/src/test/java/io/constructor/client/SearchabilityV2Test.java Unit tests for SearchabilityV2 model + v2 request objects.
constructorio-client/src/test/java/io/constructor/client/FacetConfigurationV2Test.java Unit tests for FacetConfigurationV2 model + v2 request objects.
constructorio-client/src/test/java/io/constructor/client/ConstructorIOSearchabilityV2Test.java Integration tests covering v2 searchabilities endpoints.
constructorio-client/src/test/java/io/constructor/client/ConstructorIOFacetConfigurationV2Test.java Integration tests covering v2 facets endpoints.
constructorio-client/src/main/java/io/constructor/client/models/SearchabilityV2.java New v2 searchability model (Gson-mapped fields).
constructorio-client/src/main/java/io/constructor/client/models/FacetConfigurationV2.java New v2 facet configuration model (adds path_in_metadata and range fields).
constructorio-client/src/main/java/io/constructor/client/SearchabilityV2Request.java New request type for single searchability v2 operations (incl. skip_rebuild).
constructorio-client/src/main/java/io/constructor/client/SearchabilitiesV2Request.java New request type for bulk searchabilities v2 PATCH operations.
constructorio-client/src/main/java/io/constructor/client/SearchabilitiesV2GetRequest.java New request type for v2 searchabilities listing (filters + pagination).
constructorio-client/src/main/java/io/constructor/client/SearchabilitiesV2DeleteRequest.java New request type for v2 bulk searchability deletion.
constructorio-client/src/main/java/io/constructor/client/FacetConfigurationsV2Request.java New request type for v2 bulk facet update/replace operations.
constructorio-client/src/main/java/io/constructor/client/FacetConfigurationV2Request.java New request type for single facet v2 create/update/replace/delete operations.
constructorio-client/src/main/java/io/constructor/client/ConstructorIO.java Added v2 Facets/Searchabilities API methods and introduced DEFAULT_SECTION.
.github/workflows/run-tests.yml Added TEST_CATALOG_FACETS_V2_API_KEY env var for CI.

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

Comment on lines +3893 to +3897
HttpUrl url = urlBuilder.build();

String params = new Gson().toJson(searchabilityV2Request.getSearchability());
RequestBody body =
RequestBody.create(params, MediaType.parse("application/json; charset=utf-8"));
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

createOrUpdateSearchabilityV2 serializes searchabilityV2Request.getSearchability() without validating it is non-null. Because SearchabilityV2Request is also used for GET/DELETE (and those constructors set searchability to null), passing the wrong request type here will send a literal null JSON body and likely fail at runtime. Add a guard that getSearchability() is non-null (and ideally that its name matches the path name) and throw IllegalArgumentException if invalid.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants