feat: Adds an optional skipCreation. When set to true, documents that… #1189
Conversation
… don't exist in the index are silently ignored rather than created.
|
Warning Rate limit exceeded@awais786 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded an optional skip_creation parameter to Index document add/update methods. When True, requests include skipCreation=true to silently ignore non-existent documents; when False or None, behavior is unchanged. The flag is propagated into URL construction and per-batch request paths; tests for skip_creation were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Index
participant URLBuilder as _build_url
participant HTTP as "MeiliSearch API"
Client->>Index: add/update documents(..., skip_creation=bool)
Index->>URLBuilder: _build_url(primary_key, csv_delimiter, skip_creation)
URLBuilder-->>Index: url (append ?skipCreation=true when True)
Index->>HTTP: POST url + payload
HTTP-->>Index: TaskInfo
Index-->>Client: TaskInfo
note right of Index: For batching, Index iterates batches and repeats URL build + POST per batch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meilisearch/index.py (1)
1-1: CRITICAL: Fix Black formatting before merge.The pipeline shows a Black formatting failure. Run
black meilisearch/index.pylocally to fix formatting issues.
🧹 Nitpick comments (1)
meilisearch/index.py (1)
870-877: Consider making parameters keyword-only for consistency.In
update_documents_in_batches, theserializerandskip_creationparameters are not keyword-only, whereas inadd_documents_in_batches(line 494-496) they are keyword-only (after the*separator).For API consistency, consider adding the
*separator:def update_documents_in_batches( self, documents: Sequence[Mapping[str, Any]], batch_size: int = 1000, primary_key: Optional[str] = None, + *, serializer: Optional[Type[JSONEncoder]] = None, skip_creation: Optional[bool] = None, ) -> List[TaskInfo]:This matches the signature pattern used in
add_documents_in_batchesand other methods.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
meilisearch/index.py
🧰 Additional context used
🧬 Code graph analysis (1)
meilisearch/index.py (2)
meilisearch/_httprequests.py (1)
put(120-140)meilisearch/models/task.py (1)
TaskInfo(78-107)
🪛 GitHub Actions: Tests
meilisearch/index.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted; run 'black' to format the code.
🔇 Additional comments (3)
meilisearch/index.py (3)
531-531: LGTM: Parameter propagation in batch methods.The
skip_creationparameter is correctly propagated through the batch processing methods to their underlying single-operation methods. This ensures consistent behavior across batch and non-batch operations.Also applies to: 911-911
2389-2390: LGTM: Defensive check for boolean True.The explicit
if skip_creation is True:check is good defensive programming. It ensures only the boolean valueTrueadds the query parameter, preventing truthy non-boolean values from inadvertently triggering the behavior. The string value"true"is correct for URL query parameters.
2378-2393: The implementation is correct. Verification confirms:
- Meilisearch API supports the
skipCreationparameter on document endpoints (introduced in v1.31.0)- The string value
"true"is the correct format for URL query parameters- The conditional
if skip_creation is True:correctly ensures the parameter is only sent when explicitly set toTrue, preserving existing behavior (creation) whenFalseorNone
… don't exist in the index are silently ignored rather than created.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
meilisearch/index.py (1)
470-472: Docstring inaccuracy previously flagged.The docstring still states "Default is False" when the actual default is
None. This issue was already identified in a previous review for all 12 methods.
🧹 Nitpick comments (1)
meilisearch/index.py (1)
892-898: Consider makingserializerandskip_creationkeyword-only for consistency.The signature of
update_documents_in_batchesdiffers fromadd_documents_in_batches(line 489-496). Inadd_documents_in_batches, bothserializerandskip_creationare keyword-only parameters (separated by*), but inupdate_documents_in_batchesthey are positional. While this maintains consistency with the pre-existing pattern in each method, it creates an inconsistency between the two related batch methods.Consider aligning both methods by making these parameters keyword-only:
def update_documents_in_batches( self, documents: Sequence[Mapping[str, Any]], batch_size: int = 1000, primary_key: Optional[str] = None, *, serializer: Optional[Type[JSONEncoder]] = None, skip_creation: Optional[bool] = None, ) -> List[TaskInfo]:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
meilisearch/index.py
🧰 Additional context used
🧬 Code graph analysis (1)
meilisearch/index.py (2)
meilisearch/_httprequests.py (1)
put(120-140)meilisearch/models/task.py (1)
TaskInfo(78-107)
🔇 Additional comments (1)
meilisearch/index.py (1)
2402-2417: TheskipCreationparameter is supported by the Meilisearch API (added in v1.31.0) for document add/update endpoints. The implementation correctly constructs the URL withskipCreation=truewhenskip_creationisTrue, and the string value format is correct for URL query parameters.
… don't exist in the index are silently ignored rather than created.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index/test_index_document_meilisearch.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/index/test_index_document_meilisearch.py (1)
meilisearch/index.py (7)
add_documents(451-487)update_documents(696-732)add_documents_json(538-578)update_documents_json(767-807)add_documents_csv(580-614)update_documents_csv(809-843)add_documents_ndjson(616-647)
🪛 GitHub Actions: Tests
tests/index/test_index_document_meilisearch.py
[error] 759-759: Test failure in test_update_documents_json_with_skip_creation: MeilisearchApiError due to malformed payload when adding documents with skip_creation.
[error] 788-788: Test failure in test_add_documents_csv_with_skip_creation: Expected 1 document after skip_creation, but got 0.
[error] 806-806: Test failure in test_update_documents_csv_with_skip_creation: Expected 1 document after skip_creation, but got 0.
🪛 Ruff (0.14.10)
tests/index/test_index_document_meilisearch.py
584-584: Do not assert blind exception: Exception
(B017)
650-650: Do not assert blind exception: Exception
(B017)
🔇 Additional comments (7)
tests/index/test_index_document_meilisearch.py (7)
592-606: LGTM!This test correctly verifies that
skip_creation=Falsepreserves default behavior, allowing document creation.
608-629: LGTM!This test correctly verifies the important distinction that
skip_creation=Trueblocks creation of new documents while still allowing updates to existing ones.
658-679: LGTM!This test correctly verifies that
update_documentswithskip_creation=Truecan still update existing documents.
681-702: LGTM!This test correctly verifies that
skip_creationworks with batch operations, preventing creation of new documents while respecting existing ones.
704-725: LGTM!This test correctly verifies that
skip_creationworks withupdate_documents_in_batches.
727-748: LGTM!This test correctly verifies that
skip_creationworks with JSON document format.
809-824: LGTM!This test correctly verifies that
skip_creationworks with NDJSON format. Unlike CSV, NDJSON has one document per line with no headers, so taking the first line is correct.
… don't exist in the index are silently ignored rather than created.
… don't exist in the index are silently ignored rather than created.
… don't exist in the index are silently ignored rather than created.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/index/test_index_document_meilisearch.py (2)
584-585: Use a specific exception type instead of bareException.This issue was already identified in a previous review. Catching bare
Exceptionis too broad and may hide unexpected errors. Use a specific exception type likeMeilisearchApiErrorbased on the imports and patterns used elsewhere in this file.
650-651: Use a specific exception type instead of bareException.This issue was already identified in a previous review. Replace the bare
Exceptionwith a specific exception type likeMeilisearchApiError.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index/test_index_document_meilisearch.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/index/test_index_document_meilisearch.py (1)
meilisearch/index.py (9)
add_documents(451-487)wait_for_task(233-260)update_documents(696-732)add_documents_in_batches(489-536)add_documents_json(538-578)update_documents_json(767-807)add_documents_csv(580-614)update_documents_csv(809-843)add_documents_ndjson(616-647)
🪛 GitHub Actions: Tests
tests/index/test_index_document_meilisearch.py
[error] 729-729: pylint: Import outside toplevel (json) (import-outside-toplevel)
[error] 752-752: pylint: Import outside toplevel (json) (import-outside-toplevel)
🪛 Ruff (0.14.10)
tests/index/test_index_document_meilisearch.py
584-584: Do not assert blind exception: Exception
(B017)
650-650: Do not assert blind exception: Exception
(B017)
… don't exist in the index are silently ignored rather than created.
… don't exist in the index are silently ignored rather than created.
|
@Strift You can review this. |
1 participant
#1186
Adds an optional
skip_creationflag across document ingestion/update APIs to ignore creation of non-existent documents when true, and threads it viaskipCreation=truequery param.