meilisearch / meilisearch-go

Golang wrapper for the Meilisearch API

https://www.meilisearch.com

MIT License

519 stars 85 forks source link

Add multi-search federation #563

Closed polyfloyd closed 2 weeks ago

polyfloyd commented 1 month ago

Pull Request

Fixes #573

What does this PR do?

This PR adds support for the new federated search to be introduced in v1.10: https://github.com/meilisearch/meilisearch/releases/tag/v1.10.0-rc.0

PR checklist

Please check if your PR fulfills the following requirements:

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

polyfloyd commented 1 month ago

Please note that I am running into an issue here: unaccepted status code found: 400 expected: [200], MeilisearchApiError Message: Inside .queries[0]: Using pagination options is not allowed in federated queries.

I think this patch is correct, so I'm opening this PR already. But I can not be 100% sure just yet

curquiza commented 1 month ago

Hello @polyfloyd thanks for your PR, but v1.10 is not released yet. So I will review and merge a PR for federated once v1.10 is release 😊

Thanks for your anticipation and involvement 😄

polyfloyd commented 1 month ago

Found the cause of the error message, this Go library sets the pagination limit of each query to a default. I removed it for now so I can continue testing

Ja7ad commented 1 month ago

@polyfloyd Thank you, currently you can change this PR to draft until release v1.10 and add issue for federation search.

polyfloyd commented 1 month ago

Saw that there is an issue open now: #573

Will go over the checklist to see whether there is still something to be done

Ja7ad commented 1 month ago

Saw that there is an issue open now: #573

Will go over the checklist to see whether there is still something to be done

Please do this check list:

[ ] write test stage for multi-search with federation
[ ] please add fixes #573 in PR description
[ ] add _federation for SearchResponse

"_federation": {
        "indexUid": "comics",
        "queriesPosition": 1
      }

polyfloyd commented 1 month ago

Thanks!

polyfloyd commented 1 month ago

Updated the commit title and added a test.

Adding _federation to SearchResponse does not seem correct? This object is present in each item of the hits array. See the expected payload of the new unit test to see what I mean

Ja7ad commented 1 month ago

Updated the commit title and added a test.

Adding _federation to SearchResponse does not seem correct? This object is present in each item of the hits array. See the expected payload of the new unit test to see what I mean

Don't require any do for this.

Ja7ad commented 1 month ago

@polyfloyd please add fixes #573 in PR description

polyfloyd commented 1 month ago

@Ja7ad I have added the issue ref to the PR description

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.72%. Comparing base (e09d86e) to head (d132bc1). Report is 2 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #563 +/- ## ========================================== - Coverage 86.73% 86.72% -0.02% ========================================== Files 11 11 Lines 2043 2041 -2 ========================================== - Hits 1772 1770 -2 Misses 165 165 Partials 106 106 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Ja7ad commented 2 weeks ago

bors merge

Thank you!!

meili-bors[bot] commented 2 weeks ago

Build succeeded: