Feature / v2 endpoint

[tomas862]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @tomas862, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new version of the synchronization SDK, specifically designed to interact with the V2 API. It encapsulates all V2-specific logic and configurations into new classes, ensuring a clear separation from existing API versions. The changes aim to provide a robust and well-tested client for the V2 API, covering a wide range of functionalities from index management to bulk data operations, with an emphasis on correct data handling and authentication.

Highlights

• New V2 SDK Implementation: Introduced a new SyncV2Sdk class to provide a dedicated interface for interacting with the V2 API endpoints, separating it from previous API versions.
• V2 Configuration Class: Added SyncConfigV2 to handle configuration specific to the V2 API, including robust validation for application ID (UUID), API URL, and authentication token.
• Comprehensive Test Coverage: Included an extensive PHPUnit test suite (SyncV2SdkTest.php) and BDD feature files to ensure the correctness of all V2 SDK functionalities, authentication, URL path construction, and data passthrough.
• API Endpoint Coverage: The new SDK provides methods for various V2 API operations, including index creation and management, configuration settings, synonym management, and bulk product operations.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces the V2 endpoint for the Sync SDK, including a new configuration object, the SDK class itself, and extensive unit and BDD tests. The implementation is mostly solid, but I've identified a critical issue in the testability of the SyncV2Sdk class which invalidates the current unit tests. I've also pointed out a potential security/correctness issue with URL parameter encoding and a maintainability concern regarding confusing method names. Addressing the testability issue is crucial to ensure the long-term stability and correctness of the SDK.

[tomas862]

@claude review

[claude]

Claude finished @tomas862's task in 2m 7s —— View job

---

Code Review: Feature / v2 endpoint

I've completed my review of this PR. This is a substantial implementation (27,273 additions across 100 files) introducing a new V2 SDK for Brad Search synchronization. Overall, the code demonstrates good architecture with immutable value objects, comprehensive validation, and extensive test coverage. However, I've identified several critical and high-priority issues that should be addressed.

---

Critical Issues

1. Testability Issue: Direct $httpClient Access Prevents Proper Mocking

Location: src/SyncV2Sdk.php (lines 64, 80, 94, 110, 126, 139, 154, 169, 184, 197, 213, 228, 241, 257, 273, 287, 303)

Issue: All SDK methods directly access $this->httpClient instead of using the getHttpClient() accessor method. This makes the class difficult to test properly.

The current test implementation in tests/SyncV2SdkTest.php has to work around this by creating an anonymous class that extends SyncV2Sdk and re-implements all the SDK methods. This is a critical testing flaw because the tests aren't actually testing the real SDK code - they're testing duplicate implementations.

Example of the problem:

// Current implementation (lines 62-70)
public function createIndex(IndexCreateRequest $request): IndexCreationResponse
{
    $response = $this->httpClient->post(  // ❌ Direct access
        $this->baseApiPath . 'index',
        $request->jsonSerialize()
    );
    return IndexCreationResponse::fromArray($response);
}

Solution: Use the existing getHttpClient() method throughout:

public function createIndex(IndexCreateRequest $request): IndexCreationResponse
{
    $response = $this->getHttpClient()->post(  // ✅ Use accessor
        $this->baseApiPath . 'index',
        $request->jsonSerialize()
    );
    return IndexCreationResponse::fromArray($response);
}

This change needs to be applied to all 18 occurrences throughout the file. Once fixed, the tests can be simplified to just override getHttpClient() without duplicating all the SDK methods.

Fix this →

---

High Priority Issues

2. Missing URL Encoding for Query Parameters

Location: src/SyncV2Sdk.php:214 and src/SyncV2Sdk.php:229

Issue: The $language parameter is directly concatenated into URL query strings without encoding. This can cause incorrect behavior or errors if the language code contains special characters.

// Line 213-215 (getSynonyms)
$response = $this->httpClient->get(
    $this->baseApiPath . 'synonyms?language=' . $language  // ❌ Not encoded
);

// Line 228-230 (deleteSynonyms)
return $this->httpClient->delete(
    $this->baseApiPath . 'synonyms?language=' . $language  // ❌ Not encoded
);

Solution: Use urlencode() to ensure the URL is always correctly formatted:

$response = $this->httpClient->get(
    $this->baseApiPath . 'synonyms?language=' . urlencode($language)
);

Fix this →

3. Confusing Method Naming for Configuration Endpoints

Location: src/SyncV2Sdk.php:250-306

Issue: The methods createSearchSettings, getSearchSettings, updateSearchSettings, and deleteSearchSettings operate on different API endpoints than the similarly named setConfiguration, getConfiguration, updateConfiguration, and deleteConfiguration methods:

• Configuration methods: api/v2/applications/{appId}/configuration/... (application-specific)
• Search settings methods: api/v2/configuration/... (global/different scope)

This naming similarity can confuse developers using the SDK, as it's not immediately clear that these operate on different resources.

Recommendation: Rename the search settings methods to better reflect their scope. For example:

• createSearchSettings → createGlobalSearchSettings
• getSearchSettings → getGlobalSearchSettings
• updateSearchSettings → updateGlobalSearchSettings
• deleteSearchSettings → deleteGlobalSearchSettings

This would make the distinction clear and improve API usability.

Fix this →

---

Medium Priority Issues

4. Inconsistent Exception Usage in Configuration

Location: src/Config/SyncConfigV2.php:22-30

Issue: The SyncConfigV2 class uses InvalidFieldConfigException for validation errors, but this exception type is semantically meant for field configuration issues (based on its namespace and usage in the V1 SDK). Configuration validation errors would be better represented by a more appropriate exception.

Recommendation: Consider using or creating a configuration-specific exception like InvalidConfigurationException or use the V2 exceptions (V2\Exceptions\InvalidArgumentException).

5. Potential Improvement: Add Version Information Methods

Observation: The SDK includes version management methods (activateIndexVersion, deleteIndexVersion, listIndexVersions) but doesn't provide a way to get the currently active version directly. Consider adding a convenience method like getActiveVersion() that extracts just the active version from getIndexInfo().

6. Missing PHPDoc for Some Value Object Methods

Location: Various value object classes in src/V2/ValueObjects/

Observation: While most classes have good documentation, some with* methods lack PHPDoc comments. Adding consistent documentation would improve IDE autocomplete and developer experience.

---

Positive Aspects

1. Excellent Value Object Design: The immutable value objects with proper validation and builder patterns are well-implemented
2. Comprehensive Test Coverage: 65 test files with 2,421 lines in the main SDK test alone shows thorough testing
3. Type Safety: Good use of PHP 8.4 features (readonly properties, enums, constructor property promotion)
4. Consistent Architecture: Clear separation between value objects, exceptions, and SDK logic
5. Good Error Handling: Response objects include proper error handling and validation
6. Builder Patterns: The inclusion of builder classes for complex objects improves usability

---

Summary

This is a solid V2 implementation with a well-thought-out architecture. The critical issue with httpClient access must be fixed as it invalidates the current test approach. The URL encoding issue is a potential bug that should be addressed before merging. The naming confusion issue is less critical but would improve the developer experience.

After addressing these issues, this PR will provide a robust, well-tested V2 SDK for the Brad Search API.

---

[tomas862]

@claude review latest changes

[claude]

Claude finished @tomas862's task in 1m 57s —— View job

---

Code Review: Latest Changes

I've completed my review of the latest changes on the feature/v2-endpoint branch.

Recent Commits Analyzed

• cb85e14 - "updated bulk operations and tests" (added DeleteProductsPayload functionality)
• 8870db6 - "adjusts code to actually perform tests" (major refactoring of SyncV2Sdk.php and test suite)

---

Critical Issue: FIXED ✅

The testability issue has been successfully resolved! The most significant improvement in commit 8870db6 is that all SDK methods in src/SyncV2Sdk.php now correctly use $this->getHttpClient() instead of direct $this->httpClient access. This allows proper mocking through method override.

Example of the fix (line 64):

public function createIndex(IndexCreateRequest $request): IndexCreationResponse
{
    $response = $this->getHttpClient()->post(  // ✅ Now using accessor
        $this->baseApiPath . 'index',
        $request->jsonSerialize()
    );
    return IndexCreationResponse::fromArray($response);
}

The test suite in tests/SyncV2SdkTest.php has been properly refactored:

• Removed duplicate SDK method implementations (reduced from ~2,421 lines to 1,371 lines)
• Now correctly overrides getHttpClient() to inject mocks
• Tests are now actually testing the real SDK code, not duplicates

---

Remaining Issues

High Priority: Missing URL Encoding ⚠️

Location: src/SyncV2Sdk.php:214 and src/SyncV2Sdk.php:229

The $language parameter in getSynonyms() and deleteSynonyms() is still concatenated directly into URL query strings without encoding. This can cause issues if language codes contain special characters.

Current code (lines 213-215):

public function getSynonyms(string $language): SynonymResponse
{
    $response = $this->getHttpClient()->get(
        $this->baseApiPath . 'synonyms?language=' . $language  // ❌ Not encoded
    );

Recommended fix:

$response = $this->getHttpClient()->get(
    $this->baseApiPath . 'synonyms?language=' . urlencode($language)
);

The same issue exists in deleteSynonyms() at line 229.

Fix this →

---

Medium Priority: Method Naming Ambiguity

Location: src/SyncV2Sdk.php:250-306

The methods createSearchSettings, getSearchSettings, updateSearchSettings, and deleteSearchSettings operate on different API endpoints (api/v2/configuration/) compared to the similarly named configuration methods (setConfiguration, getConfiguration, etc.) which use api/v2/applications/{appId}/configuration/.

Current endpoints:

• Configuration methods: api/v2/applications/{appId}/configuration/... (application-specific)
• Search settings methods: api/v2/configuration/... (different scope, takes appId as parameter)

Recommendation: Consider renaming to clarify scope, for example:

• createSearchSettings → createGlobalSearchSettings
• getSearchSettings → getGlobalSearchSettings
• updateSearchSettings → updateGlobalSearchSettings
• deleteSearchSettings → deleteGlobalSearchSettings

This would make it immediately clear these methods operate on a different resource than the application-specific configuration methods.

---

New Code: Bulk Operations (Commit cb85e14) ✅

The newly added DeleteProductsPayload class is well-implemented:

Strengths:

• ✅ Immutable value object with proper readonly properties
• ✅ Comprehensive validation (non-empty array, string type checks, no empty strings)
• ✅ Clean API with factory methods and immutable updates
• ✅ Proper exception handling with InvalidArgumentException
• ✅ Extensive test coverage (157 lines of tests in DeleteProductsPayloadTest.php)

Updated BulkOperation class:

• ✅ Now supports both IndexProductsPayload and DeleteProductsPayload via union type
• ✅ Static factory methods indexProducts() and deleteProducts() for easy creation
• ✅ Proper serialization to match API expectations

Test Quality:
The new tests are comprehensive and cover:

• Basic construction and validation
• Immutability patterns (withProductIds(), withAddedProductId())
• JSON serialization correctness
• Error cases (empty arrays, non-string IDs, empty strings)
• Integration with BulkOperation class

---

Overall Assessment

The recent changes represent significant improvements to the codebase:

Major Improvements:

1. ✅ Critical testability issue fixed - SDK now properly testable with mocked HTTP client
2. ✅ Test suite refactored - 1,050+ lines removed, tests now validate actual SDK code
3. ✅ New bulk delete functionality - Well-designed with proper validation and tests
4. ✅ Consistent architecture - New code follows established patterns

Remaining Work:

1. ⚠️ Fix URL encoding in getSynonyms() and deleteSynonyms() (2 locations)
2. 💡 Consider renaming search settings methods to clarify their scope vs configuration methods

Code Quality:

• Excellent use of PHP 8.4 features (readonly properties, enums, union types)
• Comprehensive validation with descriptive error messages
• Strong test coverage across all new functionality
• Consistent naming and structure in value objects

The PR is in very good shape. The critical issue has been resolved, and only minor improvements remain. I recommend addressing the URL encoding issue before merging, as it's a potential bug. The method naming issue is more of a usability enhancement and could be addressed in a follow-up if needed.

---

[claude]

---

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

---

[claude]

Code review\n\nNo issues found. Checked for bugs and CLAUDE.md compliance.