Cast federation payload explicilty to an object

[thijskuilman]

Pull Request

Related issue

Fixes #685

What does this PR do?

• It casts the federation payload explicitly to an object. This solves the error that appears when no MultiSearchFederation properties have been set.

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

[norkunas]

Please add test then to avoid regressions in future

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.71%. Comparing base (55dc98f) to head (a5fe717).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #686   +/-   ##
=======================================
  Coverage   89.71%   89.71%           
=======================================
  Files          52       52           
  Lines        1322     1322           
=======================================
  Hits         1186     1186           
  Misses        136      136           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[curquiza]

@thijskuilman

thank you for your PR ❤️

I agree with norkunas, can you add a test with an empty MultiSearchFederation to ensure it works all the time?

[thijskuilman]

@norkunas @curquiza I have added a test!

[norkunas]

@thijskuilman not sure that we need a functional test here :) better would be to assert on that what was really passed to http client, because now a test without assertions doesn't prove anything

[norkunas]

IMHO we should a dev requirement for symfony/http-client, which allows easier testing than the guzzle.

Then your test could look like this:

$mockHttpClient = new \Symfony\Component\HttpClient\MockHttpClient(static function (string $method, string $url, array $options) {
    // assert on method, url, options['body'] or whatever what was passed

    return new JsonMockResponse([]);
});
$httpClient = new \Symfony\Component\HttpClient\Psr18Client($mockHttpClient);

$meilisearch = new \Meilisearch\Client('http://meilisearch', 'api_key', $httpClient);
$meilisearch->multiSearch([], new MultiSearchFederation());

code is just an idea and is not validated :)

[thijskuilman]

better would be to assert on that what was really passed to http client, because now a test without assertions doesn't prove anything

Yeah, I agree that it seems a bit weird that no assertions are done in the test. However, the current test does prove that no exceptions are raised during the function call: if you'd revert the object casting, then the test fails because the ApiException is thrown. I could make the test more specific by wrapping it in a try catch and check specifically on ApiException if that's more desirable.

Beside that: I agree that testing the HTTP client instead might be nice (or a better solution) as well! :)

[norkunas]

My point was that unit test will be faster than the functional one :)

[ruhulfbr]

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

[ruhulfbr]

@curquiza Could you please review and process this PR or provide a solution? This is an important update.

[curquiza]

Hello @ruhulfbr
we have a lot on our plate now, but @Strift will review your PR when he will have more time for this
In the meantime if you have time @norkunas, you can review and merge of course. If you cannot, no worries, we will take care of it later

[norkunas]

I've already reviewed and my point still stands - it'd enough for unit test (current test does not do any assertion)