[SABRA-2456] Add V2 Facets and Searchabilities API Support#237
[SABRA-2456] Add V2 Facets and Searchabilities API Support#237
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This PR adds V2 Facets and Searchabilities API support with comprehensive method coverage and TypeScript types, while also deprecating the V1 counterparts — overall a well-structured and thorough addition.
Inline comments: 8 discussions added
Overall Assessment:
| */ | ||
| addFacetConfiguration(parameters = {}, networkParameters = {}) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('ConstructorIO: addFacetConfiguration is deprecated and will be removed in the next major version. Use addFacetConfigurationV2 instead.'); |
There was a problem hiding this comment.
Important Issue: Adding console.warn deprecation notices to every V1 method is a breaking behavioral change for existing consumers who may be running in environments that treat console output as errors, or who have log-monitoring set up. Deprecation warnings are normally scoped to major version bumps; adding them mid-semver is unexpected. Additionally, there is no way for consumers to opt out of these warnings (e.g., by setting a flag like suppressDeprecationWarnings: true in options). Consider either deferring these warnings to the major version in which the methods are removed, or provide a suppression mechanism.
| * displayName: 'Color', | ||
| * }); | ||
| */ | ||
| addFacetConfigurationV2(parameters = {}, networkParameters = {}) { |
There was a problem hiding this comment.
Important Issue: addFacetConfigurationV2 validates name, pathInMetadata, and type individually, but createOrReplaceFacetConfigurationsV2 and modifyFacetConfigurationsV2 (batch methods) do no per-item validation — they pass the raw array items directly to toSnakeCaseKeys and send them to the API. This inconsistency means client-side validation errors for invalid items (e.g., missing name or pathInMetadata) are only caught at the API level for bulk operations, leading to confusing error messages. The individual item validation logic should be reused or extracted for the batch methods, or at minimum documented.
| const { fetch } = this.options; | ||
| const controller = new AbortController(); | ||
| const { signal } = controller; | ||
| const { section, name, pathInMetadata, ...rest } = parameters; |
There was a problem hiding this comment.
Suggestion: The ...rest spread in addFacetConfigurationV2 includes type, displayName, and all other optional fields, then these are snake-cased and sent as the body. Since type is destructured from rest indirectly (rest.type), this works correctly but it is somewhat implicit. For clarity and to match the pattern used in modifyFacetConfigurationV2 (which also spreads rest), this is fine — but the same approach in replaceFacetConfigurationV2 merges name back in: toSnakeCaseKeys({ name, pathInMetadata, ...rest }). Note that section is already excluded from the body correctly, but section being in rest was already guarded by the explicit destructure. This is consistent with V1 patterns, so no change needed, just noting the pattern.
| * sortOrder: 'num_matches', | ||
| * }); | ||
| */ | ||
| modifyFacetConfigurationV2(parameters = {}, networkParameters = {}) { |
There was a problem hiding this comment.
Suggestion: modifyFacetConfigurationV2 (single PATCH) sends toSnakeCaseKeys(rest) as the body, which excludes name from the body. The name is correctly used only in the URL. However, for consistency with addFacetConfigurationV2 and replaceFacetConfigurationV2 (which both include name in the body), it would be worth verifying with the API docs whether name should be included in the body for single-facet PATCH requests. This could cause subtle bugs if the API expects name in the body.
| * fuzzySearchable: true, | ||
| * }); | ||
| */ | ||
| patchSearchabilityV2(parameters = {}, networkParameters = {}) { |
There was a problem hiding this comment.
Important Issue: patchSearchabilityV2 uses ...rest to collect the body fields, but section is explicitly destructured out of parameters. However, skipRebuild is also destructured out and placed in query params — yet rest would not contain skipRebuild since it was already destructured. This is correct. However, if a caller accidentally passes unknown fields (e.g., nonExistentField: true) they will silently be included in the request body after snake_casing, while the API may reject them. The single-patch method has no field whitelist validation, unlike how the V1 patchSearchabilities validates against the server. This is inconsistent with the test that verifies patchSearchabilitiesV2 (batch) rejects unsupported fields via the API but there's no equivalent test for the single patchSearchabilityV2 with invalid fields.
| dotenv.config(); | ||
|
|
||
| const sendTimeout = 300; | ||
| const testApiKey = process.env.TEST_CATALOG_FACETS_V2_API_KEY; |
There was a problem hiding this comment.
Important Issue: The new test file uses a separate TEST_CATALOG_FACETS_V2_API_KEY environment variable instead of the existing TEST_CATALOG_API_KEY. The PR description does not explain why a separate API key is needed for V2 facets. If it's because the V2 endpoint is only available on specific accounts, this should be documented. Meanwhile, the searchabilities V2 test file uses TEST_CATALOG_API_KEY (the standard key). This inconsistency may cause the facet V2 tests to silently skip or fail in CI if the new secret is not configured, while the PR title says there are no breaking changes.
| const queryString = qs.stringify(queryParams, { indices: false }); | ||
|
|
||
| return `${serviceUrl}/${encodeURIComponent(apiVersion)}/${encodeURIComponent(path)}?${queryString}`; | ||
| const encodedPath = path.split('/').map(encodeURIComponent).join('/'); |
There was a problem hiding this comment.
Suggestion: The change to createCatalogUrl to encode each path segment individually (rather than encoding the whole path at once) is correct and necessary for multi-segment paths like facets/some-name. However, note that this change affects all existing V1 callers of createCatalogUrl as well (since it's a shared utility). Make sure V1 paths with special characters in facet names (e.g., names containing /) continue to work correctly. Since encodeURIComponent encodes / as %2F, this should be fine, but it's worth calling out as a behavior change to all V1 methods.
Add V2 Facets and Searchabilities API Support
Summary
This PR adds support for the V2 versions of the
/v2/facetsand/v2/searchabilitiesendpoints, running in parallel with the existing V1 implementations.Changes
New Facets V2 Methods
addFacetConfigurationV2- Create a single facet configurationgetFacetConfigurationsV2- Retrieve all facet configurations with paginationgetFacetConfigurationV2- Get a specific facet by namemodifyFacetConfigurationsV2- Batch partial update facets (PATCH)modifyFacetConfigurationV2- Partially update a single facetreplaceFacetConfigurationV2- Replace a single facet configuration (PUT)createOrReplaceFacetConfigurationsV2- Batch create or replace facets (PUT)removeFacetConfigurationV2- Delete a facet configurationNew Searchabilities V2 Methods
retrieveSearchabilitiesV2- Retrieve searchabilities with filtering/pagination/sortinggetSearchabilityV2- Get a specific searchability by namepatchSearchabilitiesV2- Batch create or update searchabilitiespatchSearchabilityV2- Update a single searchabilitydeleteSearchabilitiesV2- Batch delete searchabilitiesdeleteSearchabilityV2- Delete a single searchabilityNew Types (TypeScript)
FacetConfigurationV2- V2 facet model withpathInMetadatafieldSearchabilityConfigurationV2- V2 searchability model with new fields (displayable,hidden,createdAt,updatedAt)V2 API Differences from V1
Facets:
pathInMetadatafield which specifies where in item metadata the facet data is located/v2/facetsendpoint instead of/v1/facetsSearchabilities:
displayableandhiddenfieldscreatedAt/updatedAttimestamps/v2/searchabilitiesendpoint instead of/v1/searchabilitiesTest Plan
catalog-facet-configurations-v2.jswith comprehensive test coverage for all facet V2 operationscatalog-searchabilities-v2.jswith comprehensive test coverage for all searchability V2 operationsafterhooksBreaking Changes
None. V2 methods are additive and V1 methods remain unchanged.