Skip to content

Cast federation payload explicilty to an object #686

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
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Cast federation payload explicilty to an object #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f3a300
Cast federation payload explicilty to an object
thijskuilman Oct 4, 2024
d666c0b
Added HandlesMultiSearchTest and test that no exceptions are thrown w…
thijskuilman Oct 11, 2024
a5fe717
Added HandlesMultiSearchTest and test that no exceptions are thrown w…
thijskuilman Oct 11, 2024
4d6c355
Merge branch 'main' into main
Strift May 29, 2025
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
2 changes: 1 addition & 1 deletion src/Endpoints/Delegates/HandlesMultiSearch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function multiSearch(array $queries = [], ?MultiSearchFederation $federat

$payload = ['queries' => $body];
if (null !== $federation) {
$payload['federation'] = $federation->toArray();
$payload['federation'] = (object) $federation->toArray();
Copy link

Choose a reason for hiding this comment

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

Why is typecasting necessary? You could use $payload['federation'] = $federation; and it would work just fine.

}

return $this->http->post('/multi-search', $payload);
Expand Down
35 changes: 35 additions & 0 deletions tests/Endpoints/HandlesMultiSearchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Endpoints;

use Meilisearch\Contracts\MultiSearchFederation;
use Meilisearch\Contracts\SearchQuery;
use Meilisearch\Endpoints\Indexes;
use Tests\TestCase;

final class HandlesMultiSearchTest extends TestCase
{
private Indexes $firstIndex;
private Indexes $secondIndex;

protected function setUp(): void
{
parent::setUp();
$this->firstIndex = $this->createEmptyIndex($this->safeIndexName());
$this->secondIndex = $this->createEmptyIndex($this->safeIndexName());
}

public function testPassingEmptyMultiSearchFederationDoesNotCauseException(): void
{
$this->expectNotToPerformAssertions();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove expectNotToPerformAssertions() and add meaningful assertions.

The expectNotToPerformAssertions() call indicates this test isn't actually verifying the fix works correctly - only that it doesn't throw exceptions. This makes the test less valuable for preventing regressions.

At minimum, consider adding an assertion that verifies the method completes successfully:

-    $this->expectNotToPerformAssertions();
-
-    $this->client->multiSearch([
+    $result = $this->client->multiSearch([
         (new SearchQuery())->setIndexUid($this->firstIndex->getUid()),
         (new SearchQuery())->setIndexUid($this->secondIndex->getUid()),
     ],
         new MultiSearchFederation()
     );
+    
+    $this->assertIsArray($result);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$this->expectNotToPerformAssertions();
$result = $this->client->multiSearch([
(new SearchQuery())->setIndexUid($this->firstIndex->getUid()),
(new SearchQuery())->setIndexUid($this->secondIndex->getUid()),
],
new MultiSearchFederation()
);
$this->assertIsArray($result);
🤖 Prompt for AI Agents 
In tests/Endpoints/HandlesMultiSearchTest.php at line 26, remove the call to
expectNotToPerformAssertions() because it prevents the test from verifying
actual outcomes. Instead, add meaningful assertions that confirm the method
under test behaves as expected, such as checking the returned data structure,
status codes, or specific values to ensure the fix works correctly and prevent
regressions.


$this->client->multiSearch([
(new SearchQuery())->setIndexUid($this->firstIndex->getUid()),
(new SearchQuery())->setIndexUid($this->secondIndex->getUid()),
],
new MultiSearchFederation()
);
}
Comment on lines +24 to +34
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider implementing unit tests with HTTP client assertions.

While this test ensures no exceptions are thrown, it lacks assertions on the actual behavior and uses a functional testing approach. As discussed in the PR comments, a more robust approach would be to unit test the HTTP client interactions directly.

Consider implementing the suggestion from the PR comments to mock the HTTP client and assert on the request payload:

public function testEmptyFederationPayloadIsCastToObject(): void
{
    $httpClient = $this->createMock(Http::class);
    $httpClient->expects($this->once())
        ->method('post')
        ->with(
            '/multi-search',
            $this->callback(function ($payload) {
                // Verify federation is an object (stdClass) not an array
                return isset($payload['federation']) 
                    && $payload['federation'] instanceof \stdClass;
            })
        );
    
    $client = new Client('http://localhost:7700');
    // Inject mocked HTTP client...
    
    $client->multiSearch(
        [(new SearchQuery())->setIndexUid('test')],
        new MultiSearchFederation()
    );
}

This approach would:

  • Verify the actual payload structure sent to the API
  • Run faster as a unit test
  • Provide more reliable regression protection

Would you like me to help implement a more comprehensive unit testing approach for this functionality?

🤖 Prompt for AI Agents 
In tests/Endpoints/HandlesMultiSearchTest.php around lines 24 to 34, the current
test only checks that no exceptions occur but does not verify the actual HTTP
request payload. To improve, mock the HTTP client used by the multiSearch
method, set an expectation that the post method is called once with a payload
where the federation field is an object (stdClass), not an array. Then inject
this mocked client into the Client instance and call multiSearch with an empty
MultiSearchFederation. This will unit test the HTTP interaction and payload
structure more robustly.

}
Loading