Allow skipCreation when adding/replacing documents#751
Allow skipCreation when adding/replacing documents#751kumarUjjawal wants to merge 3 commits intomeilisearch:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded an optional skip_creation parameter (and preserved primary_key) to document-related public APIs; centralized documents URL construction via a new private build_documents_url helper that appends optional query params. Examples and tests updated to exercise skip_creation and propagate it to HTTP requests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant IndexModule as src/indexes.rs
participant HTTP as HTTP Client
participant Meili as Meilisearch Server
Client->>IndexModule: add_or_replace(docs, primary_key?, skip_creation?)
IndexModule->>IndexModule: build_documents_url(host, uid, primary_key, skip_creation)
IndexModule->>HTTP: POST /indexes/{uid}/documents?primaryKey=...&skipCreation=...
HTTP->>Meili: forward HTTP request
Meili-->>HTTP: response (TaskInfo)
HTTP-->>IndexModule: return response
IndexModule-->>Client: return TaskInfo
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)examples/cli-app/src/main.rs (1)
⏰ 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)
🔇 Additional comments (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.code-samples.meilisearch.yaml (1)
163-163: Consider adding examples that demonstrate skip_creation usage.The documentation currently shows the default behavior (
None, None). Consider adding additional examples demonstrating theskip_creationparameter in action to help users understand when and how to use it.Suggested additional example
add_or_replace_documents_skip_creation_example: |- // Only update existing documents, skip creating new ones let task: TaskInfo = client .index("movies") .add_or_replace(&[ Movie { id: 287947, title: "Shazam ⚡️ - Updated".to_string(), // ... other fields } ], None, Some(true)) // primary_key, skip_creation=true .await .unwrap();Also applies to: 183-183
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.code-samples.meilisearch.yamlsrc/indexes.rs
⏰ 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 (5)
src/indexes.rs (5)
645-663: LGTM: Breaking change properly implemented.The addition of the
skip_creationparameter is a breaking change for users calling this method with positional arguments. However, this is the idiomatic Rust approach for adding optional parameters. Ensure this breaking change is clearly documented in the release notes and that the SDK version is appropriately bumped (likely a minor or major version increment per semantic versioning).
969-987: LGTM: Consistent implementation.The
add_or_updatemethod follows the same pattern asadd_or_replace. The implementation is consistent and correct.
727-733: LGTM: Wrapper methods maintain backward compatibility.All wrapper methods correctly pass
Nonefor theskip_creationparameter, ensuring existing code paths continue to work as before.Also applies to: 772-779, 818-825, 863-870, 908-915, 1722-1725
2293-2366: LGTM: Comprehensive test coverage for skip_creation.Both test cases validate the expected behavior:
test_add_or_replace_with_skip_creationconfirms existing documents are updated while new documents are skippedtest_add_or_update_with_skip_creationconfirms partial updates work correctly with skip_creation, preserving fields not in the update payload
2270-2270: LGTM: Test updated for new signature.The existing test has been correctly updated to work with the new method signature.
| /// Builds the URL for document addition/update endpoints with optional query parameters. | ||
| fn build_documents_url( | ||
| host: &str, | ||
| index_uid: &str, | ||
| primary_key: Option<&str>, | ||
| skip_creation: Option<bool>, | ||
| ) -> String { | ||
| let base_url = format!("{}/indexes/{}/documents", host, index_uid); | ||
| let mut query_params = Vec::new(); | ||
|
|
||
| if let Some(pk) = primary_key { | ||
| query_params.push(format!("primaryKey={}", pk)); | ||
| } | ||
| if skip_creation == Some(true) { | ||
| query_params.push("skipCreation=true".to_string()); | ||
| } | ||
|
|
||
| if query_params.is_empty() { | ||
| base_url | ||
| } else { | ||
| format!("{}?{}", base_url, query_params.join("&")) | ||
| } | ||
| } |
There was a problem hiding this comment.
URL-encode the primary_key parameter.
The primary_key value is inserted directly into the query string without URL encoding. If a primary key contains special characters (spaces, &, =, etc.), this will produce a malformed URL.
🔎 Proposed fix using URL encoding
fn build_documents_url(
host: &str,
index_uid: &str,
primary_key: Option<&str>,
skip_creation: Option<bool>,
) -> String {
let base_url = format!("{}/indexes/{}/documents", host, index_uid);
let mut query_params = Vec::new();
if let Some(pk) = primary_key {
- query_params.push(format!("primaryKey={}", pk));
+ query_params.push(format!("primaryKey={}", urlencoding::encode(pk)));
}
if skip_creation == Some(true) {
query_params.push("skipCreation=true".to_string());
}
if query_params.is_empty() {
base_url
} else {
format!("{}?{}", base_url, query_params.join("&"))
}
}Note: You may need to add the urlencoding crate as a dependency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/indexes.rs around lines 16 to 38, the primary_key is being interpolated
directly into the query string which can break URLs when it contains reserved
characters; update the function to percent-encode the primary_key before
appending it as a query parameter (e.g., use a URL-encoding helper such as
urlencoding::encode or the percent-encoding utilities from the url crate), add
the chosen crate to Cargo.toml, and replace the direct insertion with the
encoded value so the generated URL is always valid.
There was a problem hiding this comment.
@kumarUjjawal, understood. I've noted your decision.
|
@kumarUjjawal can you fix the tests in the CI? |
Fixed it. |
2 participants
Pull Request
Related issue
Fixes #747
What does this PR do?
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.