-
Notifications
You must be signed in to change notification settings - Fork 60
Add federated multisearch to MeilisearchClient #571
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.
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
base: main
Are you sure you want to change the base?
Conversation
Hello @apt-TebbeM thank you for your PR! Let me know when the PR is ready 😊 |
…e the code more readable
I think it could be reviewed. I actually thought about chaning the API for the federated search part to something like this: public async Task<ISearchable<T>> FederatedMultiSearchAsync<T>(List<FederatedSearchQuery> queries,
MultiSearchFederationOptions federationOptions = default(MultiSearchFederationOptions),
CancellationToken cancellationToken = default) to make it less likely to have an empty list of queries. |
you can go ahead |
@curquiza when will this pr be reviewed? :) |
Hello @apt-TebbeM I'm sorry but I have a loooot on my plate right now. I will maybe be able to review your PR next week. I'll do my best. But busy work period at the moment But be sure I don't forget you! |
@apt-TebbeM if you are still here, can you answer @ahmednfwela point above 👆 so that we can improve or merge the PR? |
@curquiza @ahmednfwela I've updated the PR as suggested |
Is there still a desire for this to be merged? @ahmednfwela |
/// <summary> | ||
/// Always include property in json. MultiSearchFederationOptions will be serialized as "{}" | ||
/// </summary> | ||
public class MultiSearchFederationOptionsConverter : JsonConverter<MultiSearchFederationOptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see what problem does this converter solve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API requiers a empty "federation" object when calling the /multisearch endpoint this is to respond in the expected format:
{
"hits": [ ...],
"processingTimeMs": 4,
"limit": 20,
"offset": 0,
"estimatedTotalHits": 11
}
When the empty object parameter is not included it responds in a totaly different way:
{
"results": [
{
"indexUid": "searchIndex1",
"hits": [...],
"query": "w",
"processingTimeMs": 4,
"limit": 20,
"offset": 0,
"estimatedTotalHits": 11
},
{
"indexUid": "searchIndex2",
"hits": [...],
"query": "w",
"processingTimeMs": 0,
"limit": 20,
"offset": 0,
"estimatedTotalHits": 0
},
{
"indexUid": "searchIndex3",
"hits": [...],
"query": "w",
"processingTimeMs": 0,
"limit": 20,
"offset": 0,
"estimatedTotalHits": 0
}
]
}
What are the next steps here? @curquiza @ahmednfwela |
Pull Request
Related issue
Fixes #560
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!