Implement federated multi search #625
Conversation
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
2 times, most recently
from
67c4c36 to
4051890
Compare
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
4051890 to
93e52d8
Compare
|
""" WalkthroughThe changes introduce federated multi-search support by adding new query and response types, federation-specific options, and related metadata structures. The client gains an asynchronous method to execute federated multi-search queries. Documentation is updated to explain usage and type requirements for federated queries. No changes to error handling or unrelated functionality are made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Server
User->>Client: Build MultiSearchQuery
User->>Client: Call with_federation(federation_options)
User->>Client: Call execute() on FederatedMultiSearchQuery
Client->>Server: POST /multi-search with federated query
Server-->>Client: FederatedMultiSearchResponse<T>
Client-->>User: Merged federated search results
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
93e52d8 to
9d73cb9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/client.rs (1)
131-144: Reduce duplication by factoring out the common HTTP call
execute_federated_multi_search_queryandexecute_multi_search_queryare identical except for the body/response types. This means we now have two almost-copy-pasted code blocks that have to stay in sync (endpoint path, status code, error handling, etc.).A tiny generic helper would remove this repetition and the chance that one of the two branches silently drifts:
+ async fn post_multi_search<B, R>(&self, body: &B) -> Result<R, Error> + where + Http: HttpClient, + B: Serialize + ?Sized, + R: DeserializeOwned, + { + self.http_client + .request::<(), &B, R>( + &format!("{}/multi-search", &self.host), + Method::Post { body, query: () }, + 200, + ) + .await + } + pub async fn execute_federated_multi_search_query< T: 'static + DeserializeOwned + Send + Sync, >( &self, body: &FederatedMultiSearchQuery<'_, '_, Http>, ) -> Result<FederatedMultiSearchResponse<T>, Error> { - self.http_client - .request::<(), &FederatedMultiSearchQuery<Http>, FederatedMultiSearchResponse<T>>( - &format!("{}/multi-search", &self.host), - Method::Post { body, query: () }, - 200, - ) - .await + self.post_multi_search(body).await }This keeps one single source-of-truth for the HTTP details and scales if more multi-search flavours appear later.
src/search.rs (6)
368-377: Consider documenting federation-specific weight semantics
QueryFederationOptions::weightis the only supported option for now, but its valid range/behaviour (e.g. 0–1? negative allowed?) isn’t obvious from the type alone. A short doc comment and, if applicable,#[serde(skip_serializing_if = "is_zero")]would improve ergonomics.
617-624: Guard against accidental use outside a federated query
with_federation_optionscan be called on anySearchQuery, including ones that will later be executed as single-index searches – the backend will then choke on the unexpectedfederationfield.To make misuse impossible you could:
- Gate the method behind a feature flag on
FederatedMultiSearchQueryonly, or- Make it
pub(crate)and funnel all access throughMultiSearchQuery::with_federation.Either way the public surface stays self-consistent.
666-676: Ownership ergonomics: return&mut selfinwith_federationConsuming
selfforces a temporary variable:let resp = client.multi_search() .with_search_query(q1) .with_search_query(q2) .with_federation(fed_opts) // returns FederatedMultiSearchQuery .execute::<Foo>() .await?;Returning
FederatedMultiSearchQueryby value is fine, but to stay symmetrical with the other builder methods you could implementwith_federation_mut(&mut self, …) -> &mut Selfas sugar.
Not critical, just API nicety.
690-698: Missing#[serde(bound(serialize = \"\"))]on the whole structLike
MultiSearchQuery,FederatedMultiSearchQuerycan end up with an unnecessaryHttp: Serializebound when the derive macro kicks in. Only thequeriesfield is patched – adding the same attribute to the struct ensures consistency:-#[derive(Debug, Serialize, Clone)] +#[derive(Debug, Serialize, Clone)] +#[serde(bound(serialize = ""))] pub struct FederatedMultiSearchQuery …
702-713: Nit: useVec<String>forfacets_by_indexvalues?
facets_by_indexisHashMap<String, Vec<String>>, but facet names are usually&strand never repeated, soHashSetorVecis fine. If order doesn’t matter a set avoids duplicates.Not blocking – purely optional.
716-725: Propagate lifetimes correctly inexecuteThe method returns
Result<FederatedMultiSearchResponse<T>, Error>by borrowingselffor'a, but the response doesn’t referenceself. You can shorten the borrow to&selflike the non-federated variant:- pub async fn execute<T: 'static + DeserializeOwned + Send + Sync>( - &'a self, + pub async fn execute<T: 'static + DeserializeOwned + Send + Sync>( + &self, ) -> Result<FederatedMultiSearchResponse<T>, Error> {No semantic change, but simpler signature.
🛑 Comments failed to post (1)
src/search.rs (1)
69-72:
⚠️ Potential issue
weighted_ranking_scoreis treated as mandatory – will panic on missing dataThe Meilisearch docs currently don’t guarantee that
_federation.weighted_ranking_scoreis always present (the comment below even hints at that).
Deserialising into a non-Option<f32>will make the whole response fail if that single field is omitted in a minor server update.- pub weighted_ranking_score: f32, + /// Score contributed by the federation weighting. + /// Making this optional avoids hard-fail deserialisation if the server omits it. + #[serde(default)] + pub weighted_ranking_score: Option<f32>,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/search.rs around lines 69 to 72, the field weighted_ranking_score is currently defined as a non-optional f32, which will cause a panic if the field is missing in the response. Change the type of weighted_ranking_score to Option<f32> to safely handle cases where this field is absent, preventing deserialization failures and panics.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/search.rs (1)
636-1406: 🛠️ Refactor suggestionConsider adding tests for the federated search functionality.
The code has good test coverage for other search features, but there are no tests for the new federated search functionality. Adding tests would help verify that everything works as expected and provide examples of how to use the new features.
Would you like me to help draft test cases for the federated search functionality?
🧹 Nitpick comments (1)
src/search.rs (1)
751-759: Fix typo in TODO comment and consider field optionality.There's a typo in the comment on line 757 ("TOOD" should be "TODO"). Also, if
weighted_ranking_scoremight be optional as the comment suggests, consider usingOption<f32>for the field type.- // TOOD: not mentioned in the docs, is that optional? - pub weighted_ranking_score: f32, + // TODO: not mentioned in the docs, is that optional? + pub weighted_ranking_score: Option<f32>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/client.rs(2 hunks)src/search.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (9)
src/search.rs (9)
69-71: LGTM: Clear field addition for federation metadata.This new field enables storing federation-related information on search results, correctly following the pattern used for other metadata fields with proper serde attribute and documentation.
368-376: LGTM: Federation options field and struct look good.The federation options field is properly marked as crate-internal with appropriate serialization handling, and the QueryFederationOptions struct is well-structured with the needed weight field.
409-409: LGTM: Default initialization for federation_options.Properly initializing the new field as None in the constructor.
617-624: LGTM: Builder method for federation options.This follows the established builder pattern consistently with other methods in the class.
636-646: LGTM: Serialization configuration for MultiSearchQuery.The custom serde bound is correctly used here as explained in the previous review comment.
667-676: LGTM: Transformation method from MultiSearchQuery to FederatedMultiSearchQuery.This provides a clean way to transform a regular multi-search query into a federated one, following the Builder pattern approach.
690-698: LGTM: FederatedMultiSearchQuery struct definition.The structure properly includes all necessary fields with correct serialization configuration.
700-713: LGTM: FederationOptions struct with clear documentation.The struct provides all the necessary options for federation with a helpful link to the documentation.
715-725: LGTM: Execute method implementation for FederatedMultiSearchQuery.This implementation correctly delegates to the client's method for executing federated searches.
| /// Returned by federated multi search. | ||
| #[derive(Debug, Deserialize, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct FederatedMultiSearchResponse<T> { | ||
| /// Merged results of the query. | ||
| pub hits: Vec<SearchResult<T>>, | ||
|
|
||
| // TODO: are offset, limit and estimated_total_hits really non-optional? In | ||
| // my tests they are always returned, but that's not a proof. | ||
| /// Number of documents skipped. | ||
| pub offset: usize, | ||
| /// Number of results returned. | ||
| pub limit: usize, | ||
| /// Estimated total number of matches. | ||
| pub estimated_total_hits: usize, | ||
|
|
||
| /// Distribution of the given facets. | ||
| pub facet_distribution: Option<HashMap<String, HashMap<String, usize>>>, | ||
| /// facet stats of the numerical facets requested in the `facet` search parameter. | ||
| pub facet_stats: Option<HashMap<String, FacetStats>>, | ||
| /// Processing time of the query. | ||
| pub processing_time_ms: usize, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TODO comment about field optionality.
There's an open question about whether offset, limit, and estimated_total_hits fields should be optional. Consider investigating this further to ensure the API is consistent and robust.
🏁 Script executed:
#!/bin/bash
# Check Meilisearch API documentation and actual responses to determine if these fields are always present
# Search the codebase for clues about these fields' optionality in other response structures
echo "Checking other response structures for field optionality patterns:"
grep -r "pub offset:" --include="*.rs" .
grep -r "pub limit:" --include="*.rs" .
grep -r "pub estimated_total_hits:" --include="*.rs" .
# Look for any test files that might verify these fields
echo -e "\nSearching for tests that might verify these fields:"
grep -r "offset" --include="*test*.rs" .
grep -r "limit" --include="*test*.rs" .
grep -r "estimated_total_hits" --include="*test*.rs" .Length of output: 1546
Align optionality of offset, limit, and estimated_total_hits across response structs
We currently see both plain and Option<usize> definitions for these fields in several places—this should be standardized based on the API spec:
• src/documents.rs
– pub offset: u32 vs pub offset: Option<usize>
– pub limit: u32 vs pub limit: Option<usize>
• src/indexes.rs
– same pattern for offset and limit
• src/key.rs
– same pattern for offset and limit
• src/search.rs
– other search structs use pub offset: Option<usize>, pub limit: Option<usize>, and pub estimated_total_hits: Option<usize>
Action items:
- Check the Meilisearch API docs (or real responses) to confirm whether these fields are always returned for federated multi‐search.
- If guaranteed, change them to non‐optional (
usizeoru32) everywhere and remove theTODO. - If they can be omitted, update this struct to use
Option<usize>, remove theTODO, and add/update tests accordingly.
🤖 Prompt for AI Agents
In src/search.rs around lines 727 to 749, the fields offset, limit, and
estimated_total_hits are currently non-optional but there is uncertainty if they
are always present in federated multi-search responses. Investigate the
Meilisearch API documentation or actual API responses to confirm if these fields
are guaranteed. If always present, keep them as non-optional usize and remove
the TODO comment. If they can be omitted, change these fields to Option<usize>,
remove the TODO, and update or add tests to reflect this optionality for
consistency with other search response structs.
|
@curquiza Any chances you got time to look at this? Also: am I supposed to pay attention to the AI bot? I assumed I can ignore it like most other AI slop (I merely skimmed it), but if you think its comments make sense, I will take another look at it. |
| client: &'a Client<Http>, | ||
| #[serde(bound(serialize = ""))] | ||
| pub queries: Vec<SearchQuery<'b, Http>>, | ||
| pub federation: Option<FederationOptions>, |
There was a problem hiding this comment.
| pub federation: Option<FederationOptions>, | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub federation: Option<FederationOptions>, |
This is useful for testing federated stuff.
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
9d73cb9 to
91ab096
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/search.rs (2)
698-698: Addskip_serializing_ifto the federation field.- pub federation: Option<FederationOptions>, + #[serde(skip_serializing_if = "Option::is_none")] + pub federation: Option<FederationOptions>,
735-742: Resolve the TODO about field optionality.The uncertainty about whether
offset,limit, andestimated_total_hitsshould be optional needs resolution. Please verify against the Meilisearch API documentation or actual responses to ensure correctness.Run the following script to check how federated search responses handle these fields:
#!/bin/bash # Description: Search for examples or tests that might reveal the actual response structure # Look for federated search response examples in tests rg -A 10 "FederatedMultiSearchResponse" --type rust # Search for any JSON response examples or fixtures fd -e json -e jsonl | xargs rg -l "federatedMultiSearch\|federated.*search" 2>/dev/null || true # Check if there are any integration tests that might show the actual API responses rg -A 20 "test.*federated.*multi.*search" --type rust
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
meilisearch-test-macro/README.md(1 hunks)meilisearch-test-macro/src/lib.rs(1 hunks)src/client.rs(2 hunks)src/search.rs(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- meilisearch-test-macro/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch-test-macro/src/lib.rs (2)
src/client.rs (2)
delete_index(475-483)create_index(452-470)src/tasks.rs (4)
wait_for_completion(284-291)is_failure(402-404)unwrap_failure(366-373)try_make_index(317-332)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (6)
meilisearch-test-macro/src/lib.rs (1)
108-184: Well-implemented support for multiple Index parameters!The macro enhancement correctly handles multiple indices by generating unique variable names and UIDs for each index, which is essential for testing federated multi-search functionality across different indices.
src/search.rs (5)
69-71: Correct addition of federation metadata to search results.The optional
federationfield appropriately extendsSearchResultto support federated multi-search while maintaining backward compatibility.
617-624: Appropriate builder method for federation options.The method correctly follows the established builder pattern and is properly documented for its federated search context.
666-676: Clean API design for enabling federation.The conversion method provides an intuitive way to transform a regular multi-search query into a federated one while maintaining type safety.
701-714: Well-structured federation options.The struct correctly models the federation API with optional fields and proper serialization attributes.
894-953: Excellent test coverage for federated multi-search!The test effectively demonstrates:
- Searching across multiple indices with different document schemas
- Using an enum to handle heterogeneous results
- Validating federation metadata
- Proper use of the new multi-index test macro support
|
@martin-g Thanks for the review! I updated the PR to include the change you requested and also add a unit test. To do that, I had to adjust the Oh and: CI fails, but the three test failures don't seem to be related to my PR at all. |
src/search.rs
Outdated
| assert_eq!(response.hits.len(), 2); | ||
| dbg!(&index_a.uid); | ||
| dbg!(&index_b.uid); | ||
| dbg!(&response.hits); |
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
91ab096 to
596699f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/search.rs (1)
758-760: Typos & stale TODO inFederationHitInfo
TOOD→TODO- The comment is obsolete; we learnt that
weightedRankingScoreis always returned (see learning note).
Removing the TODO keeps the codebase clean.- // TOOD: not mentioned in the docs, is that optional? + // weightedRankingScore is always present in Meilisearch replies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/search.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/search.rs (1)
Learnt from: LukasKalbertodt
PR: meilisearch/meilisearch-rust#625
File: src/search.rs:758-759
Timestamp: 2025-06-12T12:31:13.041Z
Learning: In Meilisearch federated multi-search responses, `_federation.weightedRankingScore` is always returned, independent of the `showRankingScore` query parameter, so the `weighted_ranking_score` field in `FederationHitInfo` should remain a non-optional `f32`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
src/search.rs (2)
735-743: Makeoffset,limit,estimated_total_hitsresilientThe struct assumes these fields are always present, yet the public docs describe them as “may be omitted”. Deserialization will panic if Meilisearch ever omits one of them (e.g. in future versions).
Consider switching to
Option<usize>(mirrorsSearchResults) or add a comment that the SDK intentionally diverges because the field is guaranteed after server-side merge.
636-646: Great replacement of manual impl with derive + bound hackDeriving
Serializehere and using#[serde(bound(serialize = ""))]eliminates boilerplate while avoiding the extra trait bound onHttp. Nice touch!
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub(crate) federation_options: Option<QueryFederationOptions>, | ||
| } |
There was a problem hiding this comment.
SearchQuery serializes federation_options under the wrong key
#[serde(rename_all = "camelCase")] converts federation_options → federationOptions, but the Meilisearch API expects the field to be named federation.
- #[serde(skip_serializing_if = "Option::is_none")]
- pub(crate) federation_options: Option<QueryFederationOptions>,
+ #[serde(skip_serializing_if = "Option::is_none", rename = "federation")]
+ pub(crate) federation_options: Option<QueryFederationOptions>,Without this fix, per-query weights are silently ignored by the server.
📝 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.
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub(crate) federation_options: Option<QueryFederationOptions>, | |
| } | |
| #[serde(skip_serializing_if = "Option::is_none", rename = "federation")] | |
| pub(crate) federation_options: Option<QueryFederationOptions>, | |
| } |
🤖 Prompt for AI Agents
In src/search.rs around lines 368 to 370, the field federation_options is
serialized with the key federationOptions due to the serde rename_all attribute,
but the Meilisearch API expects the key to be federation. Fix this by adding a
serde rename attribute to federation_options to rename it to "federation" during
serialization, ensuring the API receives the correct field name and per-query
weights are not ignored.
There was a problem hiding this comment.
Incorrect, this is on SearchQuery, where the field is indeed called federationOptions. So all is good. I did now add a test about this, showing that all is good, though. So I guess this bot used Cunningham's law on me to make me improve the unit tests. Interesting use of AI...
There was a problem hiding this comment.
@LukasKalbertodt You’re absolutely right – I mixed up the per-query federationOptions with the top-level federation parameter. Thanks for pointing it out (and for beefing up the tests)! I’ve noted that in SearchQuery the field must stay federationOptions, while the overall multi-search uses federation.
No further changes needed.
Thanks again for the clarification!
✏️ Learnings added
Learnt from: LukasKalbertodt
PR: meilisearch/meilisearch-rust#625
File: src/search.rs:368-370
Timestamp: 2025-06-12T13:28:23.675Z
Learning: In the Meilisearch Rust client, `SearchQuery` serializes its per-query federation settings under the key `federationOptions`; only the top-level multi-search parameter is named `federation`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
596699f to
a826b64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/search.rs (1)
368-370:⚠️ Potential issueWrong JSON key sent –
federation_optionsserialises asfederationOptionsBecause
SearchQueryis under#[serde(rename_all = "camelCase")], the current field name becomesfederationOptions, but the Meilisearch API expects the per-query object to be calledfederation.
Any request usingwith_federation_optionswill therefore be silently ignored by the server.- #[serde(skip_serializing_if = "Option::is_none")] - pub(crate) federation_options: Option<QueryFederationOptions>, + #[serde(skip_serializing_if = "Option::is_none", rename = "federation")] + pub(crate) federation_options: Option<QueryFederationOptions>,
🧹 Nitpick comments (1)
src/search.rs (1)
758-759: Typo and stale TODO
TOOD→TODO.
If the optionality question has already been settled (see learning note), delete the comment to avoid confusion.- // TOOD: not mentioned in the docs, is that optional? + // TODO: verify with future Meilisearch releases whether this ever becomes optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/search.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/search.rs (1)
Learnt from: LukasKalbertodt
PR: meilisearch/meilisearch-rust#625
File: src/search.rs:758-759
Timestamp: 2025-06-12T12:31:13.041Z
Learning: In Meilisearch federated multi-search responses, `_federation.weightedRankingScore` is always returned, independent of the `showRankingScore` query parameter, so the `weighted_ranking_score` field in `FederationHitInfo` should remain a non-optional `f32`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
src/search.rs (1)
735-743:offset,limit,estimated_total_hitsshould likely beOption<usize>The TODO hints the values may be absent. Hard-coding them as non-optional will break deserialization as soon as the server chooses to omit one (e.g. when no hits are found or a future API change).
Switching toOption<usize>keeps the client forward-compatible.
Pull Request
Related issue
Fixes #609
What does this PR do?
This PR adds types and methods to use the federated multi search API. There are multiple ways to add this to this library, depending on how strictly one wants to type everything. I decided to:
FederatedMultiSearchResponse, which also required a newClient::execute_federated_multi_search_query. The response of federated searches is just very different from normal multi searches, and I didn't think having an enum returned byexecute_multi_search_querywas a particularly nice design (and it would have been a breaking change).federation: Option<FederationHitInfo>to eachSearchResult(which isNonefor non-federated searches). I don't think it's worth to have a dedicatedFederatedSearchResult.MultiSearchQuery::with_federationwhich returns a newFederatedMultiSearchQuery. One could also changeMultiSearchQueryto be able to represent federated queries, but I had a slight preference for my solution.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Documentation