-
Notifications
You must be signed in to change notification settings - Fork 131
Add support for custom metadata in document operations #907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for custom metadata in document operations #907
Conversation
WalkthroughThis PR adds support for attaching custom metadata to document operations in the Meilisearch Java SDK. New overloaded methods are introduced in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
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 (3)
src/main/java/com/meilisearch/sdk/Index.java (2)
434-450: Deprecated method receives new overload.While technically correct, adding a
customMetadataparameter to the deprecateddeleteDocumentsmethod may encourage continued use. Consider whether this aligns with your deprecation strategy.
239-258: Consider addingcustomMetadatasupport to batch operations.The
addDocumentsInBatchesandupdateDocumentsInBatchesmethods don't support the newcustomMetadataparameter. This means users performing batch operations cannot attach custom metadata to individual batch tasks.If this is intentional (e.g., due to implementation complexity), consider documenting this limitation. Otherwise, add overloads that accept
customMetadata:public TaskInfo[] addDocumentsInBatches(String document, Integer batchSize, String primaryKey, String customMetadata) throws MeilisearchException { // ... existing logic, but pass customMetadata to addDocuments calls arrayResponses.add( this.documents.addDocuments( this.uid, jsonSubArray.toString(), primaryKey, null, customMetadata)); // ... }Also applies to: 353-372
src/test/java/com/meilisearch/integration/DocumentsTest.java (1)
735-867: Consider adding edge case tests for custom metadata.The current tests cover the happy path well, but consider adding tests for edge cases to ensure robustness:
- Special characters in metadata (quotes, Unicode, etc.)
- Very long metadata strings
- Empty string vs null metadata (to ensure they behave as expected)
Example test skeleton:
@Test public void testAddDocumentsWithSpecialCharactersInCustomMetadata() throws Exception { String indexUid = "AddDocumentsSpecialCharsMetadata"; Index index = client.index(indexUid); String specialMetadata = "metadata with \"quotes\" and 日本語"; TaskInfo task = index.addDocuments("[" + singleDocument + "]", null, null, specialMetadata); index.waitForTask(task.getTaskUid()); com.meilisearch.sdk.model.Task taskDetails = index.getTask(task.getTaskUid()); assertThat(taskDetails.getCustomMetadata(), is(equalTo(specialMetadata))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/meilisearch/sdk/Documents.java(6 hunks)src/main/java/com/meilisearch/sdk/Index.java(6 hunks)src/main/java/com/meilisearch/sdk/model/Task.java(1 hunks)src/test/java/com/meilisearch/integration/DocumentsTest.java(1 hunks)
🔇 Additional comments (18)
src/main/java/com/meilisearch/sdk/model/Task.java (1)
23-23: LGTM!The
customMetadatafield addition is appropriate for capturing custom metadata from task responses. The field will be automatically serialized/deserialized via the JSON handler, and Lombok's@Getterwill provide accessor methods.src/main/java/com/meilisearch/sdk/Documents.java (6)
207-241: LGTM! Consistent implementation.The
updateDocumentsmethod follows the same backward-compatible pattern asaddDocuments.
251-271: LGTM! Consistent implementation.The
deleteDocumentmethod follows the established pattern.
281-301: LGTM! Consistent implementation.The
deleteDocumentsbatch method follows the established pattern.
311-337: LGTM! Consistent implementation.The
deleteDocumentsByFiltermethod follows the established pattern.
346-364: LGTM! Consistent implementation.The
deleteAllDocumentsmethod follows the established pattern.
162-196: URL encoding is not currently implemented in URLBuilder; verify this aligns with API design expectations.The
URLBuilder.addParameter()method appends parameter values directly without URL encoding them. This applies to all parameters—includingcustomMetadata,primaryKey, andcsvDelimiter—and is a pre-existing pattern, not specific to this PR. While the implementation is consistent with the existing design, special characters (spaces, quotes, ampersands, etc.) in thecustomMetadatavalue could potentially break URL parsing or cause unexpected behavior if not handled by the caller or HTTP client.No integration tests verify
customMetadatawith special characters, and noURLEncoderusage exists in the codebase for parameter encoding.src/main/java/com/meilisearch/sdk/Index.java (5)
207-225: LGTM! Clean delegation pattern.The new
addDocumentsoverload properly delegates to theDocumentsclass with thecustomMetadataparameter.
321-339: LGTM! Consistent delegation.The new
updateDocumentsoverload follows the same delegation pattern.
402-416: LGTM! Consistent delegation.The new
deleteDocumentoverload follows the established pattern.
467-482: LGTM! Consistent delegation.The new
deleteDocumentsByFilteroverload follows the established pattern.
497-509: LGTM! Consistent delegation.The new
deleteAllDocumentsoverload follows the established pattern.src/test/java/com/meilisearch/integration/DocumentsTest.java (6)
735-753: LGTM! Good test coverage for add operation.The test properly verifies that custom metadata is attached to the add documents task and can be retrieved.
755-776: LGTM! Good test coverage for update operation.The test properly verifies custom metadata handling for update operations.
778-797: LGTM! Good test coverage for single document deletion.The test properly verifies custom metadata handling for single document deletion.
799-819: LGTM! Good test coverage for batch deletion.The test properly verifies custom metadata handling for batch document deletion.
821-843: LGTM! Good test coverage for filter-based deletion.The test properly verifies custom metadata handling for filter-based document deletion.
845-867: LGTM! Good test coverage for delete all operation.The test properly verifies custom metadata handling for deleting all documents.
curquiza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
bors merge
|
Build succeeded:
|
Pull Request
Related issue
Fixes #905
What does this PR do?
This pull request adds support for passing custom metadata when performing document operations in the Meilisearch Java SDK. The main changes include updating the
DocumentsandIndexclasses to accept an optionalcustomMetadataparameter for add, update, and delete operations, storing this metadata in theTaskmodel, and introducing comprehensive integration tests to validate the new functionality.Model changes
Taskmodel (Task.java) to include acustomMetadatafield, ensuring that metadata attached to tasks is stored and accessible.Test coverage
DocumentsTest.javato verify that custom metadata is correctly attached and retrievable for all document operations (add, update, delete single, delete batch, delete by filter, delete all).PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.