feat: Added customMetadata in index apis #1179
Conversation
📝 WalkthroughWalkthroughIntroduces an optional metadata parameter across client, index, and task APIs, threads it into URL construction and request payloads as Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Index
participant HTTP
participant Server
participant TaskModel
Client->>Index: add_documents(..., metadata="Test metadata")
Note right of Index: metadata threaded through\nmethod stack and _build_url
Index->>HTTP: POST /indexes/{uid}/documents?customMetadata=Test%20metadata + body
HTTP->>Server: send request
Server-->>HTTP: 202 Accepted (task JSON including customMetadata)
HTTP->>Index: response
Index->>TaskModel: parse task JSON -> Task.custom_metadata
TaskModel-->>Client: TaskInfo with custom_metadata="Test metadata"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the customMetadata field to index-related APIs in the Python SDK, aligning with recent Meilisearch server changes. The implementation adds an optional metadata parameter to all document manipulation methods (add, update, delete) that gets passed to the Meilisearch server as a query parameter.
Key changes:
- Added
customMetadatafield to theTaskmodel to capture metadata returned by the server - Updated all document methods in the
Indexclass to accept an optionalmetadataparameter - Enhanced test coverage by adding metadata assertions to document operation tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| meilisearch/models/task.py | Added customMetadata field as Optional[str] to Task model |
| meilisearch/index.py | Added metadata parameter to all document methods (add/update/delete variants) and updated URL building logic to include customMetadata as a query parameter |
| tests/index/test_index_document_meilisearch.py | Updated tests to pass metadata parameter and assert that customMetadata is persisted on task objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (4)
meilisearch/index.py (4)
601-634: Inconsistent parameter style:metadatais positional here but keyword-only in similar methods.In
add_documents,add_documents_in_batches, andadd_documents_json, themetadataparameter is keyword-only (after*). However, inadd_documents_csv, it's a regular positional parameter.For API consistency, consider making
metadatakeyword-only:def add_documents_csv( self, str_documents: bytes, primary_key: Optional[str] = None, csv_delimiter: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
636-666: Same inconsistency:metadatashould be keyword-only for consistency.def add_documents_ndjson( self, str_documents: bytes, primary_key: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
751-781: Same inconsistency:metadatashould be keyword-only.def update_documents_ndjson( self, str_documents: str, primary_key: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
820-853: Same inconsistency:metadatashould be keyword-only.def update_documents_csv( self, str_documents: str, primary_key: Optional[str] = None, csv_delimiter: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/index.py(37 hunks)meilisearch/models/task.py(1 hunks)tests/index/test_index_document_meilisearch.py(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index/test_index_document_meilisearch.py (1)
meilisearch/index.py (14)
add_documents(479-514)update(108-152)add_documents_in_batches(516-562)add_documents_json(564-599)update_documents(714-749)update_documents_json(783-818)update_documents_in_batches(901-946)delete_document(948-975)delete_documents(978-1030)delete_all_documents(1032-1055)add_documents_csv(601-634)update_documents_csv(820-853)add_documents_ndjson(636-666)update_documents_ndjson(751-781)
meilisearch/index.py (2)
meilisearch/_httprequests.py (2)
put(120-140)post(98-108)meilisearch/models/task.py (1)
TaskInfo(79-108)
⏰ 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: Agent
🔇 Additional comments (31)
meilisearch/models/task.py (1)
25-25: LGTM -customMetadatafield added to Task model.The field correctly uses
Optional[str]and follows the existing CamelBase naming convention for API compatibility.tests/index/test_index_document_meilisearch.py (18)
31-40: LGTM - Test correctly verifies metadata propagation.
65-74: LGTM - Batch test correctly verifies metadata on each task.
112-120: LGTM - JSON serializer test with metadata verification.
129-140: LGTM - Raw documents test with metadata verification.
149-155: LGTM - Update documents test with metadata.
177-185: LGTM - Update documents JSON test with metadata.
194-205: LGTM - Update documents raw test with metadata.
393-398: LGTM - Update documents test with metadata.
423-432: LGTM - Update documents in batches test with metadata.
440-444: LGTM - Delete document test with metadata.
455-459: LGTM - Delete documents by ID test with metadata.
486-490: LGTM - Delete all documents test with metadata.
499-503: LGTM - Add documents CSV test with metadata.
525-530: LGTM - Update documents CSV test with metadata.
551-556: LGTM - Add documents JSON test with metadata.
563-567: LGTM - Update documents JSON test with metadata.
575-579: LGTM - Add documents NDJSON test with metadata.
587-592: LGTM - Update documents NDJSON test with metadata.meilisearch/index.py (12)
479-514: LGTM -add_documentsmethod correctly supports metadata parameter.The metadata is properly threaded through to
_build_urland documented in the docstring.
516-562: LGTM -add_documents_in_batchescorrectly propagates metadata to each batch.
564-599: LGTM -add_documents_jsoncorrectly propagates metadata.
668-712: LGTM -add_documents_rawcorrectly implements metadata as keyword-only.
714-749: LGTM -update_documentscorrectly implements metadata.
783-818: LGTM -update_documents_jsoncorrectly implements metadata as keyword-only.
855-899: LGTM -update_documents_rawcorrectly implements metadata as keyword-only.
901-946: LGTM -update_documents_in_batchescorrectly propagates metadata.Note: The parameter style follows the existing pattern for this method (positional optional parameters), though it differs from
add_documents_in_batcheswhich uses keyword-only parameters. This is a pre-existing inconsistency.
948-975: LGTM -delete_documentcorrectly implements metadata with URL encoding.
977-1030: LGTM -delete_documentscorrectly handles metadata in both the ids and filter branches.
1032-1055: LGTM -delete_all_documentscorrectly implements metadata.
2430-2445: LGTM -_build_urlcorrectly centralizes the metadata query parameter construction.Good approach to centralize the
metadata→customMetadatamapping in the URL builder.
|
@Strift Tests are failing because sharding require Enterprose addition now. Should we adjust test cases or somehow we can skip this check. |
|
@Strift Please re trigger test cases. |
jawad-khan
force-pushed
the
jawad/update-index-apis
branch
from
690ec39 to
f27c052
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
meilisearch/index.py (1)
901-908: Inconsistent API design:serializerandmetadatashould be keyword-only.Unlike
add_documents_in_batches(line 521-523), this method does not use*,to makeserializerandmetadatakeyword-only. This was flagged in a prior review.Apply this diff for consistency:
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, metadata: Optional[str] = None, ) -> List[TaskInfo]:
🧹 Nitpick comments (4)
meilisearch/index.py (4)
601-634: Inconsistent API design:metadatashould be keyword-only.In
add_documents_csv,metadatais a positional parameter, but in similar methods likeadd_documentsandadd_documents_json,metadatais keyword-only (placed after*,). This inconsistency can lead to confusing API usage.Apply this diff for consistency:
def add_documents_csv( self, str_documents: bytes, primary_key: Optional[str] = None, csv_delimiter: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
636-666: Inconsistent API design:metadatashould be keyword-only.Same issue as
add_documents_csv- for consistency with other methods,metadatashould be keyword-only.Apply this diff:
def add_documents_ndjson( self, str_documents: bytes, primary_key: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
751-781: Inconsistent API design:metadatashould be keyword-only.For consistency with
update_documentsandupdate_documents_json, makemetadatakeyword-only.Apply this diff:
def update_documents_ndjson( self, str_documents: str, primary_key: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
820-853: Inconsistent API design:metadatashould be keyword-only.For consistency, make
metadatakeyword-only.Apply this diff:
def update_documents_csv( self, str_documents: str, primary_key: Optional[str] = None, csv_delimiter: Optional[str] = None, + *, metadata: Optional[str] = None, ) -> TaskInfo:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/index.py(37 hunks)meilisearch/models/task.py(1 hunks)tests/index/test_index_document_meilisearch.py(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/models/task.py
🧰 Additional context used
🧬 Code graph analysis (1)
meilisearch/index.py (3)
meilisearch/_httprequests.py (3)
put(120-140)delete(142-147)post(98-108)meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/client.py (1)
index(231-247)
🔇 Additional comments (30)
tests/index/test_index_document_meilisearch.py (19)
31-40: LGTM - metadata propagation test looks correct.The test properly verifies that the
customMetadataparameter is passed throughadd_documentsand returned on the completed task.
57-76: LGTM - batch metadata propagation test is thorough.The test correctly verifies that
customMetadatais propagated for each batch task inadd_documents_in_batches.
106-120: LGTM - JSON serializer with metadata test is correct.
123-140: LGTM - raw document addition with metadata test is correct.
143-155: LGTM - update documents with metadata test is correct.
171-185: LGTM - update documents JSON with metadata test is correct.
188-205: LGTM - update documents raw with metadata test is correct.
386-408: LGTM - update single document with metadata test is correct.
415-434: LGTM - update documents in batches with metadata test is correct.
437-447: LGTM - delete single document with metadata test is correct.
450-464: LGTM - delete documents by ID with metadata test is correct.
467-480: LGTM - delete documents with filter and metadata test is correct.The previously reported assignment bug on line 475 has been addressed.
483-493: LGTM - delete all documents with metadata test is correct.
496-506: LGTM - add documents CSV with metadata test is correct.
522-531: LGTM - update documents CSV with metadata test is correct.
548-557: LGTM - add documents JSON file with metadata test is correct.
560-569: LGTM - update documents JSON file with metadata test is correct.
572-581: LGTM - add documents NDJSON with metadata test is correct.
584-593: LGTM - update documents NDJSON with metadata test is correct.meilisearch/index.py (11)
948-975: LGTM - delete_document metadata implementation is correct.The inline URL construction with
parse.urlencodeproperly handles thecustomMetadataquery parameter.
1010-1030: LGTM - delete_documents metadata handling is correct.Both the deprecated
idspath and thefilterpath correctly append thecustomMetadataquery parameter when provided.
1032-1055: LGTM - delete_all_documents metadata implementation is correct.
2430-2445: LGTM -_build_urlmetadata handling is correct.The implementation properly adds
customMetadatato the query parameters only whenmetadatais provided, and usesparse.urlencodefor proper URL encoding.
479-514: LGTM -add_documentsmetadata implementation is correct.The
metadataparameter is properly defined as keyword-only and correctly passed to_build_url.
516-562: LGTM -add_documents_in_batchesmetadata propagation is correct.Metadata is properly propagated to each batch call.
564-599: LGTM -add_documents_jsonmetadata propagation is correct.
668-712: LGTM -add_documents_rawmetadata implementation is correct.
714-749: LGTM -update_documentsmetadata implementation is correct.
783-818: LGTM -update_documents_jsonmetadata propagation is correct.
855-899: LGTM -update_documents_rawmetadata implementation is correct.
jawad-khan
force-pushed
the
jawad/update-index-apis
branch
from
f27c052 to
7cbc800
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
meilisearch/index.py (1)
901-908: Inconsistent API design:serializershould be keyword-only.In
add_documents_in_batches(line 521-522),serializerandmetadataare keyword-only arguments (preceded by*,), but here they're positional. This inconsistency could confuse users.🔎 Proposed fix for consistent keyword-only arguments
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, metadata: Optional[str] = None, ) -> List[TaskInfo]:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/index.pymeilisearch/models/task.pytests/index/test_index_document_meilisearch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/models/task.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index/test_index_document_meilisearch.py (3)
meilisearch/index.py (14)
add_documents(479-514)update(108-152)wait_for_task(261-288)add_documents_in_batches(516-562)add_documents_json(564-599)update_documents(714-749)update_documents_json(783-818)update_documents_in_batches(901-946)delete_document(948-975)delete_documents(978-1030)delete_all_documents(1032-1055)add_documents_csv(601-634)update_documents_csv(820-853)update_documents_ndjson(751-781)meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/task.py (1)
wait_for_task(174-212)
meilisearch/index.py (3)
meilisearch/_httprequests.py (3)
put(120-140)delete(142-147)post(98-108)meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/client.py (1)
index(231-247)
🔇 Additional comments (21)
tests/index/test_index_document_meilisearch.py (14)
31-40: LGTM!Test correctly passes metadata and verifies it's returned in the task's
customMetadatafield.
65-74: LGTM!Test verifies metadata propagation for each batch task correctly.
112-120: LGTM!Correct metadata assertion for JSON document addition with custom serializer.
129-140: LGTM!Correct metadata assertion for raw document addition.
149-155: LGTM!Correct metadata assertion for document update with custom serializer.
177-185: LGTM!Correct metadata assertion for JSON document update.
194-205: LGTM!Correct metadata assertion for raw document update.
393-398: LGTM!Correct metadata assertion for document update.
423-432: LGTM!Correct metadata verification for batch document updates.
440-444: LGTM!Correct metadata assertion for single document deletion.
455-459: LGTM!Correct metadata assertion for batch document deletion by ID.
473-476: LGTM!The previously flagged assignment bug has been correctly fixed to use assertion (
==).
486-490: LGTM!Correct metadata assertion for delete all documents operation.
496-593: LGTM!All CSV, JSON, and NDJSON test variants correctly pass metadata and verify
customMetadatain task responses.meilisearch/index.py (7)
2430-2445: LGTM!The
_build_urlhelper correctly includescustomMetadatain query parameters when metadata is provided, using proper URL encoding.
479-562: LGTM!Metadata parameter correctly added to
add_documentsandadd_documents_in_batcheswith proper keyword-only enforcement and propagation to URL building.
564-712: LGTM!All
add_documents_*variants correctly accept and propagate the metadata parameter through to_build_url.
948-975: LGTM!The
delete_documentmethod correctly appendscustomMetadataas a query parameter when metadata is provided.
1015-1029: LGTM!Both the deprecated
idspath and thefilterpath correctly includecustomMetadatain the URL when metadata is provided.
1032-1055: LGTM!The
delete_all_documentsmethod correctly includescustomMetadatain the URL when provided.
751-899: LGTM!All
update_documents_*variants correctly accept and propagate the metadata parameter through to_build_url.
|
@Strift This is ready from my side. |
There was a problem hiding this comment.
Thanks for the update. Here are some finishing touches:
Missing API support
These should be added in meilisearch/client.py:
Client.create_index/Index.create: Missingmetadataparameter.Index.update(used to change theprimaryKey): Missingmetadataparameter.Client.delete_index/Index.delete: Missingmetadataparameter.Client.cancel_tasks: Missingmetadataparameter.Client.delete_tasks: Missingmetadataparameter.Index.update_settings: Missingmetadataparameter.Index.reset_settings: Missingmetadataparameter.
Keyword-Only Arguments (Inconsistency)
The PR adds metadata as a positional/keyword argument in some places and strictly keyword-only in others.
- In
add_documents: It is correctly placed after *, making it keyword-only. - In
add_documents_csvandupdate_documents_in_batches: It is added as a regular optional argument.
Recommendation: For all public methods, metadata (and usually serializer) should be keyword-only.
Reasoning: This is a standard pattern in this SDK to allow adding new parameters in the future without breaking the positional order for users. Copilot actually flagged this in the PR comments as well.
meilisearch/models/task.py
Outdated
| started_at: Optional[datetime] = None | ||
| finished_at: Optional[datetime] = None | ||
| network: Optional[Dict[str, Any]] = None | ||
| customMetadata: Optional[str] = None |
There was a problem hiding this comment.
| customMetadata: Optional[str] = None | |
| custom_metadata: Optional[str] = None |
All other fields in this class (like enqueued_at, index_uid) use snake_case.
You will also need to update the tests from task.customMetadata to task.custom_metadata.
jawad-khan
force-pushed
the
jawad/update-index-apis
branch
from
7cbc800 to
4808612
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @meilisearch/index.py:
- Line 867: The issue is a formatting error: replace the spaced keyword argument
"metadata = metadata" with the correctly formatted "metadata=metadata" where
it's passed (look for the occurrence of metadata = metadata in the function or
call site around line with metadata assignment), ensuring no spaces around the
equals in the keyword argument so Black will pass.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/index.pymeilisearch/models/task.pytests/index/test_index_document_meilisearch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/models/task.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index/test_index_document_meilisearch.py (2)
meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/task.py (1)
wait_for_task(174-212)
meilisearch/index.py (3)
meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/client.py (1)
index(231-247)meilisearch/_httprequests.py (2)
delete(142-147)post(98-108)
🪛 GitHub Actions: Tests
meilisearch/index.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted; 75 files would be left unchanged. Run 'pipenv run black' to fix code style.
🔇 Additional comments (24)
meilisearch/index.py (8)
479-518: LGTM!The
metadataparameter is correctly added toadd_documentswith proper type annotation, docstring, and propagation to_build_url.
520-570: LGTM!The
metadataparameter is correctly propagated to the underlyingadd_documentscall for each batch.
697-745: LGTM!The
add_documents_rawmethod correctly accepts and propagates themetadataparameter to_build_url.
747-786: LGTM!The
update_documentsmethod correctly accepts and propagates themetadataparameter.
1015-1042: LGTM!The
delete_documentmethod correctly appendscustomMetadatato the URL when metadata is provided.
1044-1097: LGTM!Both code paths in
delete_documents(id-based and filter-based) correctly handle the metadata parameter.
1099-1122: LGTM!The
delete_all_documentsmethod correctly handles the optional metadata parameter.
2497-2515: LGTM!The
_build_urlmethod correctly incorporates themetadataparameter ascustomMetadatain the query string, with proper URL encoding viaparse.urlencode.tests/index/test_index_document_meilisearch.py (16)
33-42: LGTM!Test correctly verifies that
customMetadatais propagated through the task response.
59-78: LGTM!Test correctly verifies that
customMetadatais propagated for each batch task.
108-142: LGTM!Tests for
add_documents_jsonandadd_documents_rawwith custom serializers correctly verify metadata propagation.
145-207: LGTM!Tests for
update_documents,update_documents_json, andupdate_documents_rawwith custom serializers correctly verify metadata propagation.
388-410: LGTM!Test for
update_documentscorrectly verifies metadata propagation on document updates.
417-436: LGTM!Test for
update_documents_in_batchescorrectly verifies metadata propagation for each batch.
439-449: LGTM!Test for
delete_documentcorrectly verifies metadata propagation.
452-466: LGTM!Test for
delete_documents(by ID) correctly verifies metadata propagation.
469-482: LGTM!Test for
delete_documents(with filter) correctly verifies metadata propagation.
485-495: LGTM!Test for
delete_all_documentscorrectly verifies metadata propagation.
498-507: LGTM!Test for
add_documents_csvcorrectly verifies metadata propagation.
524-533: LGTM!Test for
update_documents_csvcorrectly verifies metadata propagation.
550-559: LGTM!Test for
add_documents_jsoncorrectly verifies metadata propagation.
562-571: LGTM!Test for
update_documents_jsoncorrectly verifies metadata propagation.
574-583: LGTM!Test for
add_documents_ndjsoncorrectly verifies metadata propagation.
586-595: LGTM!Test for
update_documents_ndjsoncorrectly verifies metadata propagation.
meilisearch/index.py
Outdated
| "application/json", | ||
| serializer=serializer, | ||
| skip_creation=skip_creation, | ||
| metadata = metadata |
There was a problem hiding this comment.
Formatting issue: Incorrect spacing around = in keyword argument.
Line 867 uses metadata = metadata instead of metadata=metadata. This is causing the Black formatting check to fail in the pipeline.
🔧 Proposed fix
return self.update_documents_raw(
str_documents,
primary_key,
"application/json",
serializer=serializer,
skip_creation=skip_creation,
- metadata = metadata
+ metadata=metadata
)🤖 Prompt for AI Agents
In @meilisearch/index.py at line 867, The issue is a formatting error: replace
the spaced keyword argument "metadata = metadata" with the correctly formatted
"metadata=metadata" where it's passed (look for the occurrence of metadata =
metadata in the function or call site around line with metadata assignment),
ensuring no spaces around the equals in the keyword argument so Black will pass.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
meilisearch/task.py (1)
156-184: Stale docstring parameter and minor description inconsistency.
- The docstring at lines 162-163 documents a
config:parameter that doesn't exist in the method signature.- Line 159 says "Delete a list of enqueued or processing tasks" but the route deletes finished tasks (per the referenced docs at line 165).
Proposed docstring fix
def delete_tasks( self, parameters: MutableMapping[str, Any], *, metadata: Optional[str] = None ) -> TaskInfo: - """Delete a list of enqueued or processing tasks. + """Delete a list of finished tasks. Parameters ---------- - config: - Config object containing permission and location of Meilisearch. parameters: parameters accepted by the delete tasks route:https://www.meilisearch.com/docs/reference/api/tasks#delete-task. metadata (optional): Custom metadata string to attach to the task.tests/index/test_index_document_meilisearch.py (2)
173-187: Same type mismatch in update_documents_json test.Similar to
test_add_documents_json_custom_serializer, this test passes a list of dicts instead of the declaredstrtype forstr_documents.
108-122: Type mismatch: add_documents_json parameter type declaration is incorrect.The method signature declares
str_documents: bytes, but the underlying HTTP layer acceptsUnion[Mapping[str, Any], Sequence[Mapping[str, Any]], List[str], bytes, str]. This test passes a list of dicts, which works due to duck typing in the HTTP client, but violates the declared type contract. The method signature should be updated to accurately reflect the accepted parameter types, or the method should handle type conversion internally.
🧹 Nitpick comments (4)
meilisearch/task.py (1)
151-152: Consider handling empty string metadata explicitly.Currently
if metadata:treats empty string""as falsy, which is likely the intended behavior. However, if a user explicitly passesmetadata="", they might expect it to be sent (resulting in an emptycustomMetadataquery param). Consider usingif metadata is not None:if you want to allow empty strings, or document that empty strings are ignored.Also applies to: 181-182
meilisearch/index.py (3)
648-691: Missing keyword-only marker before skip_creation.At line 653,
*,is added but it comes aftercsv_delimiter, makingcsv_delimitera positional parameter whileskip_creationandmetadataare keyword-only. This is inconsistent with similar methods likeadd_documents_ndjson(line 693-731) where there's no*,marker. Consider whethercsv_delimitershould also be keyword-only for API consistency.
693-731: Missing keyword-only marker for metadata parameter.Unlike other methods in this file,
add_documents_ndjsondoesn't have a*,marker before the keyword-only parameters. Theskip_creationandmetadataparameters should be keyword-only for consistency with other methods.Proposed fix
def add_documents_ndjson( self, str_documents: bytes, primary_key: Optional[str] = None, + *, skip_creation: Optional[bool] = None, metadata: Optional[str] = None, ) -> TaskInfo:
827-866: Missing keyword-only marker consistency.
update_documents_ndjson(line 831) andupdate_documents_csv(line 919) add*,markers but at slightly different positions relative to their parameters. For consistency across all methods, ensure the*,marker comes after positional parameters and before optional keyword-only parameters.Also applies to: 914-957
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
meilisearch/client.pymeilisearch/index.pymeilisearch/models/task.pymeilisearch/task.pytests/index/test_index_document_meilisearch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meilisearch/models/task.py
🧰 Additional context used
🧬 Code graph analysis (4)
meilisearch/task.py (3)
meilisearch/client.py (2)
cancel_tasks(732-755)delete_tasks(757-778)meilisearch/models/task.py (1)
TaskInfo(79-108)meilisearch/_httprequests.py (1)
post(98-108)
tests/index/test_index_document_meilisearch.py (2)
meilisearch/client.py (2)
index(246-262)wait_for_task(780-807)meilisearch/index.py (14)
add_documents(505-544)update(116-171)wait_for_task(287-314)add_documents_in_batches(546-600)add_documents_json(602-646)update_documents(786-825)update_documents_json(868-912)update_documents_in_batches(1012-1066)delete_document(1068-1095)delete_documents(1098-1150)delete_all_documents(1152-1175)add_documents_csv(648-691)update_documents_csv(914-957)update_documents_ndjson(827-866)
meilisearch/client.py (3)
meilisearch/index.py (2)
create(198-236)delete(89-114)meilisearch/_httprequests.py (1)
delete(142-147)meilisearch/task.py (2)
cancel_tasks(125-154)delete_tasks(156-184)
meilisearch/index.py (1)
meilisearch/_httprequests.py (4)
delete(142-147)patch(110-118)HttpRequests(19-227)post(98-108)
🪛 GitHub Actions: Tests
meilisearch/task.py
[error] 222-222: MeilisearchTimeoutError: timeout of 5000ms has exceeded on process 1141 when waiting for task to be resolved. (Occurred during pytest run: 'pipenv run pytest --cov-report=xml')
[error] 222-222: MeilisearchTimeoutError: timeout of 5000ms has exceeded on process 1140 when waiting for task to be resolved. (Occurred during pytest run: 'pipenv run pytest --cov-report=xml')
[error] 222-222: MeilisearchTimeoutError: timeout of 5000ms has exceeded on process 1143 when waiting for task to be resolved. (Occurred during pytest run: 'pipenv run pytest --cov-report=xml')
🔇 Additional comments (16)
meilisearch/task.py (1)
125-154: LGTM - metadata parameter correctly added to cancel_tasks.The implementation correctly threads the metadata parameter through to the request URL as
customMetadata. The pattern is consistent with other methods in the PR.meilisearch/client.py (3)
84-115: LGTM - create_index correctly delegates metadata to Index.create.The method signature update and delegation to
Index.createwith themetadataparameter is correctly implemented.
117-144: LGTM - delete_index correctly constructs URL with customMetadata.The URL construction pattern using
parse.urlencodeis consistent with other methods in this PR.
732-778: LGTM - cancel_tasks and delete_tasks correctly pass metadata to task_handler.Both methods correctly delegate the metadata parameter to the underlying
task_handlermethods.tests/index/test_index_document_meilisearch.py (3)
33-42: LGTM - test_add_documents correctly validates metadata propagation.The test verifies that metadata is passed through and reflected in
task.custom_metadataafter task completion.
59-78: LGTM - test_add_documents_in_batches validates metadata across batches.Good coverage ensuring metadata is propagated to each batch task.
388-410: LGTM - comprehensive metadata test coverage for document operations.The tests thoroughly validate metadata propagation across update, delete (single/batch/all), CSV, JSON, and NDJSON document operations.
Also applies to: 417-436, 439-466, 469-482, 485-495, 498-507, 524-533, 550-559, 562-571, 574-583, 586-595
meilisearch/index.py (9)
89-114: LGTM - delete() method correctly adds metadata to URL.The implementation correctly constructs the URL with
customMetadataquery parameter when metadata is provided.
116-171: LGTM - update() method correctly adds metadata to URL.The metadata parameter is correctly threaded through to the PATCH request URL.
197-236: LGTM - create() static method correctly handles metadata.The static method correctly constructs the URL with metadata when provided.
505-544: LGTM - add_documents correctly uses _build_url with metadata.The method correctly delegates URL construction to
_build_url, which handles the metadata parameter.
546-600: LGTM - batch methods correctly propagate metadata to individual operations.Both
add_documents_in_batchesandupdate_documents_in_batchescorrectly pass the metadata parameter to each underlying operation, ensuring all batch tasks carry the same metadata.Also applies to: 1012-1066
1068-1095: LGTM - delete operations correctly handle metadata.All delete methods (
delete_document,delete_documents,delete_all_documents) correctly construct URLs with thecustomMetadataquery parameter when metadata is provided.Also applies to: 1097-1150, 1152-1175
1218-1276: LGTM - update_settings correctly handles metadata.The method correctly constructs the URL with metadata for settings updates.
1278-1304: LGTM - reset_settings correctly handles metadata.The method correctly constructs the URL with metadata for settings reset.
2563-2581: LGTM - _build_url correctly centralizes metadata handling for document operations.The
_build_urlmethod now correctly includescustomMetadatain the query parameters when metadata is provided. This centralizes the URL construction logic for document-related operations.
|
@Strift It's ready now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/index/test_index_document_meilisearch.py (2)
81-93: Consider adding metadata test coverage for consistency.
test_add_documents_custom_serializerandtest_add_documents_in_batches_custom_serializer(lines 81-106) don't test the metadata parameter, while similar serializer tests (test_add_documents_json_custom_serializer,test_update_documents_custom_serializer) do include metadata assertions. Consider adding metadata validation here for complete coverage, or document why it's intentionally excluded.
599-871: Consider combining metadata tests with skip_creation tests.The
skip_creationtests don't validate metadata propagation. Since bothmetadataandskip_creationare optional parameters on the same operations, consider adding metadata assertions to at least one skip_creation test to verify they work together. This would ensure no parameter conflicts exist.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index/test_index_document_meilisearch.py
🔇 Additional comments (19)
tests/index/test_index_document_meilisearch.py (19)
33-42: LGTM!The test correctly validates metadata propagation through
add_documents, asserting thatcustom_metadatais preserved in the completed task.
67-78: LGTM!The test properly validates that metadata is propagated to all batch tasks, verifying
custom_metadatafor each individual batch response.
114-122: LGTM!Metadata propagation through
add_documents_jsonwith custom serializer is properly tested.
131-142: LGTM!Metadata propagation through
add_documents_rawis properly tested.
151-157: LGTM!Metadata propagation through
update_documentswith custom serializer is properly tested.
179-187: LGTM!Metadata propagation through
update_documents_jsonwith custom serializer is properly tested.
196-207: LGTM!Metadata propagation through
update_documents_rawis properly tested.
395-400: Good edge case coverage.Testing with
metadata=""validates that empty string metadata is properly preserved, which is a useful edge case.
425-434: LGTM!Metadata propagation through
update_documents_in_batchesis properly tested for all batch tasks.
442-446: LGTM!Metadata propagation through
delete_documentis properly tested.
457-461: LGTM!Metadata propagation through
delete_documents(ID-based) is properly tested, including for the deprecated path.
475-477: LGTM!Metadata propagation through
delete_documents(filter-based) is properly tested.
488-492: LGTM!Metadata propagation through
delete_all_documentsis properly tested.
501-505: LGTM!Metadata propagation through
add_documents_csvis properly tested.
527-532: LGTM!Metadata propagation through
update_documents_csvis properly tested.
553-558: LGTM!Metadata propagation through
add_documents_jsonis properly tested.
565-569: LGTM!Metadata propagation through
update_documents_jsonis properly tested.
577-581: LGTM!Metadata propagation through
add_documents_ndjsonis properly tested.
589-594: LGTM!Metadata propagation through
update_documents_ndjsonis properly tested.
2 participants
Related issue
Fixes #1168
What does this PR do?
Update Index APIs to Support customMetadata
This PR updates the index-related APIs in the Python SDK to align with recent Meilisearch server changes and adds support for the new customMetadata field.
Key Updates
Added customMetadata support across index creation and update methods.
Synced request/response structures with the latest Meilisearch index API.
...
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.