Skip to content

Conversation

@kumarUjjawal
Copy link
Contributor

@kumarUjjawal kumarUjjawal commented Oct 31, 2025

Pull Request

Related issue

Fixes #662

What does this PR do?

  • Added a method to get the documents by ID
  • also added code samples

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?

Thank you so much for contributing to Meilisearch!

Summary by CodeRabbit

  • New Features

    • Added capability to retrieve documents by their IDs for more targeted queries.
  • Documentation

    • Added code sample demonstrating ID-based document retrieval.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

This PR implements document retrieval by IDs in the Meilisearch Rust SDK. A new ids field and with_ids() builder method are added to DocumentsQuery, the request routing logic is updated to use the fetch-by-IDs endpoint when IDs are provided, and sample code and tests are included.

Changes

Cohort / File(s) Summary
Documentation & Samples
.code-samples.meilisearch.yaml
Added a new code sample get_documents_by_ids_1 demonstrating document retrieval using with_ids() method with example IDs on the "books" index.
Core Feature Implementation
src/documents.rs
Added ids: Option<Vec<&'a str>> field to DocumentsQuery struct; implemented with_ids() builder method for setting IDs; added test cases validating the ID-based retrieval flow.
Request Routing
src/indexes.rs
Modified get_documents_with() conditional logic to route requests to the fetch-by-IDs endpoint when the ids field is populated, in addition to existing filter-based routing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Key areas requiring attention:
    • Verify the new ids field and with_ids() method follow the builder pattern conventions used elsewhere in the codebase
    • Confirm the conditional routing logic in get_documents_with() correctly prioritizes IDs vs. filters vs. default behavior
    • Review test coverage for edge cases (empty IDs, conflicting IDs and filters, etc.)
    • Validate that lifetime parameters ('a) in the new field signature are properly handled

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • curquiza

Poem

🐰 A rabbit hops through document fields,
With IDs now in hand, new joy yields!
with_ids() calls so crisp and clear,
Fetching docs by name, oh so dear!
Meilisearch v1.14 brings delight! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Add method to get documents by ID" directly and clearly captures the main objective of the changeset. It accurately reflects the primary change—introducing functionality to retrieve documents by IDs through a new with_ids() builder method on the DocumentsQuery struct. The title is concise, specific, and provides sufficient clarity for teammates to understand the core contribution when scanning the commit history.
Linked Issues Check ✅ Passed The code changes comprehensively address the requirements specified in linked issue #662. The PR successfully updates the SDK to allow retrieving documents by IDs through the new ids field and with_ids() builder method on DocumentsQuery, aligning with the expected behavior outlined in the issue. The implementation includes code samples demonstrating usage (e.g., DocumentsQuery::with_ids(["1", "2"])), adds appropriate test coverage for the new functionality, and updates the routing logic to handle the IDs parameter correctly.
Out of Scope Changes Check ✅ Passed All changes in the pull request are directly related to implementing the "get documents by ID" feature as specified in issue #662. The modifications to .code-samples.meilisearch.yaml, src/documents.rs, and src/indexes.rs all serve the stated objective: adding the ids field and with_ids() method, including code samples and tests, and updating the internal routing logic. No unrelated changes or improvements to other functionality are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/documents.rs (2)

327-348: Consider documenting interaction with filter parameter.

The with_ids() method implementation is correct and follows the established builder pattern. However, the documentation could clarify what happens when both with_ids() and with_filter() are used together on the same query.

Consider adding a note to the documentation:

/// Specify a list of document IDs to retrieve.
///
/// Note: When combined with `with_filter()`, both conditions may apply.
/// Refer to Meilisearch API documentation for the exact behavior.
///
/// # Example

504-515: Consider verifying the actual document IDs in the test.

The test correctly validates that 2 documents are returned, but could be strengthened by verifying that the returned documents have the expected IDs.

Apply this diff to make the test more thorough:

 #[meilisearch_test]
 async fn test_get_documents_by_ids(client: Client, index: Index) -> Result<(), Error> {
     setup_test_index(&client, &index).await?;

     let documents = DocumentsQuery::new(&index)
         .with_ids(["1", "3"]) // retrieve by IDs
         .execute::<MyObject>()
         .await?;

     assert_eq!(documents.results.len(), 2);
+    // Verify that the correct documents were retrieved
+    let ids: Vec<_> = documents.results.iter().filter_map(|d| d.id).collect();
+    assert!(ids.contains(&1));
+    assert!(ids.contains(&3));
     Ok(())
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e90a3c2 and aa5c74f.

📒 Files selected for processing (3)
  • .code-samples.meilisearch.yaml (1 hunks)
  • src/documents.rs (4 hunks)
  • src/indexes.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/documents.rs (2)
src/indexes.rs (23)
  • client (186-188)
  • client (232-234)
  • client (320-322)
  • client (376-378)
  • client (428-430)
  • client (473-475)
  • client (522-525)
  • client (556-558)
  • client (634-636)
  • client (700-702)
  • client (970-972)
  • client (1038-1040)
  • client (1089-1091)
  • client (1133-1135)
  • client (1186-1188)
  • client (1245-1247)
  • index (2171-2171)
  • index (2188-2189)
  • index (2226-2226)
  • index (2298-2298)
  • index (2326-2326)
  • index (2352-2352)
  • index (2379-2379)
src/search.rs (3)
  • index (728-728)
  • index (2055-2058)
  • setup_test_index (1165-1188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: integration-tests
🔇 Additional comments (4)
.code-samples.meilisearch.yaml (1)

145-151: LGTM! Clear example of the new feature.

The code sample effectively demonstrates the new with_ids() functionality. The usage is consistent with the existing API patterns in this file.

src/indexes.rs (1)

520-520: Verify behavior when both filter and ids are provided.

The routing logic correctly uses the fetch endpoint when either filter or ids is present. However, ensure that the Meilisearch API supports combining both filter and ids in the same request. If not, consider adding validation or documentation about this scenario.

Can you confirm whether the Meilisearch v1.14 API supports using both filter and ids parameters simultaneously in the /documents/fetch endpoint? If only one is supported at a time, the SDK should either validate this or document the behavior clearly.

src/documents.rs (2)

206-213: LGTM! Well-documented field addition.

The ids field is correctly typed and documented. The note about string representation of IDs is particularly helpful for users with numeric IDs.


216-228: LGTM! Constructor correctly initializes the new field.

The ids field is properly initialized to None, consistent with other optional fields in the struct.

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.22%. Comparing base (e90a3c2) to head (aa5c74f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #724      +/-   ##
==========================================
+ Coverage   86.18%   86.22%   +0.03%     
==========================================
  Files          20       20              
  Lines        6263     6280      +17     
==========================================
+ Hits         5398     5415      +17     
  Misses        865      865              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kumarUjjawal
Copy link
Contributor Author

@curquiza Could you review some of the opened PRs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v1.14] Get documents by ID

1 participant