feat: add getTaskDocuments method for Meilisearch v1.13#956
Open
ashishvwl wants to merge 3 commits intomeilisearch:mainfrom
Open
feat: add getTaskDocuments method for Meilisearch v1.13#956ashishvwl wants to merge 3 commits intomeilisearch:mainfrom
ashishvwl wants to merge 3 commits intomeilisearch:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Client.getTaskDocuments(int) API, a TasksHandler.getTaskDocuments(...) implementation, a documentation code sample entry, and multiple integration tests validating NDJSON task-document retrieval from GET /tasks/{taskUid}/documents. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as TasksHandler
participant Server as Meilisearch Server
Client->>Handler: getTaskDocuments(uid)
Handler->>Server: GET /tasks/{uid}/documents
Server-->>Handler: 200 OK (NDJSON payload)
Handler-->>Client: return NDJSON String
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🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/meilisearch/integration/TasksTest.java (2)
417-419: Several tests are assertion-light and can miss wrong payloads.The current checks (non-null/non-blank only) can still pass if the endpoint returns unexpected content. Please assert task-specific evidence (expected IDs/fields or expected empty result).
Example improvements for stronger assertions
@@ - // Create index task should not have documents - assertThat(taskDocuments, is(notNullValue())); + // Create-index task should not have task documents + List<String> lines = + Arrays.stream(taskDocuments.split("\\R")) + .filter(line -> !line.trim().isEmpty()) + .collect(Collectors.toList()); + assertThat(lines.size(), is(equalTo(0))); @@ - assertThat(taskDocuments, is(notNullValue())); - assertThat(taskDocuments, not(blankOrNullString())); + assertThat(taskDocuments, is(notNullValue())); + assertThat(taskDocuments, not(blankOrNullString())); + assertThat(taskDocuments, containsString("\"id\": 1")); + assertThat(taskDocuments, containsString("\"id\": 2")); + assertThat(taskDocuments, containsString("\"id\": 3"));Also applies to: 438-440, 455-457, 507-509, 537-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/meilisearch/integration/TasksTest.java` around lines 417 - 419, The tests in TasksTest use weak assertions like assertThat(taskDocuments, is(notNullValue())) which can miss incorrect payloads; update the checks (e.g., in the blocks handling taskDocuments and similar variables) to assert task-specific evidence such as expected task IDs, status fields, or that the documents list is empty when appropriate (for example assert expected id equals the created task id, assert status equals "succeeded"/"enqueued"/"failed" as applicable, or assert that documents.size() == 0) so each test validates concrete fields/values rather than just non-nullity; apply the same stronger assertions to the other mentioned locations (around the checks at 438-440, 455-457, 507-509, 537-539).
395-405: NDJSON contract check is too permissive.At Line 402 and Line 403, checking only for
{and}can pass non-NDJSON payloads. Please assert non-empty line count and per-line object shape more strictly.Proposed tightening for NDJSON assertions
- // NDJSON format has each object on a separate line - String[] lines = taskDocuments.split("\n"); - assertThat(lines.length, is(greaterThanOrEqualTo(1))); - - // Each line should be a valid JSON object - for (String line : lines) { - if (!line.trim().isEmpty()) { - assertThat(line, containsString("{")); - assertThat(line, containsString("}")); - } - } + // NDJSON format has one JSON object per non-empty line + List<String> lines = + Arrays.stream(taskDocuments.split("\\R")) + .filter(line -> !line.trim().isEmpty()) + .collect(Collectors.toList()); + assertThat(lines.size(), is(equalTo(2))); + + for (String line : lines) { + String trimmed = line.trim(); + assertTrue(trimmed.startsWith("{") && trimmed.endsWith("}")); + assertThat(trimmed, containsString("\"id\"")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/meilisearch/integration/TasksTest.java` around lines 395 - 405, The current NDJSON check in TasksTest uses loose string checks on taskDocuments lines; instead parse and validate each non-blank line as JSON and assert it's an object. Update the loop that iterates over String[] lines (from taskDocuments) to trim and skip empty lines, then use your JSON parser (e.g., Jackson's ObjectMapper.readTree) to parse each line and assert the resulting JsonNode.isObject() (and optionally check for expected keys like "taskUid" or "indexUid"); also ensure the top-level lines.length assertion verifies at least one non-empty line. This replaces the containsString("{")/("}") checks with strict JSON-object validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/com/meilisearch/integration/TasksTest.java`:
- Line 515: Replace the hardcoded 999999 with a deterministic impossible UID
constant to avoid flakiness: add a private static final long INVALID_TASK_UID =
Long.MAX_VALUE (or similarly impossible value) in TasksTest and change the
assertion to assertThrows(MeilisearchException.class, () ->
client.getTaskDocuments(INVALID_TASK_UID)); this keeps the failing UID stable
across environments and clearly identifies the intent in the test.
---
Nitpick comments:
In `@src/test/java/com/meilisearch/integration/TasksTest.java`:
- Around line 417-419: The tests in TasksTest use weak assertions like
assertThat(taskDocuments, is(notNullValue())) which can miss incorrect payloads;
update the checks (e.g., in the blocks handling taskDocuments and similar
variables) to assert task-specific evidence such as expected task IDs, status
fields, or that the documents list is empty when appropriate (for example assert
expected id equals the created task id, assert status equals
"succeeded"/"enqueued"/"failed" as applicable, or assert that documents.size()
== 0) so each test validates concrete fields/values rather than just
non-nullity; apply the same stronger assertions to the other mentioned locations
(around the checks at 438-440, 455-457, 507-509, 537-539).
- Around line 395-405: The current NDJSON check in TasksTest uses loose string
checks on taskDocuments lines; instead parse and validate each non-blank line as
JSON and assert it's an object. Update the loop that iterates over String[]
lines (from taskDocuments) to trim and skip empty lines, then use your JSON
parser (e.g., Jackson's ObjectMapper.readTree) to parse each line and assert the
resulting JsonNode.isObject() (and optionally check for expected keys like
"taskUid" or "indexUid"); also ensure the top-level lines.length assertion
verifies at least one non-empty line. This replaces the
containsString("{")/("}") checks with strict JSON-object validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f88d4aa7-ec60-4ad3-9ce0-02ee531f5d6c
📒 Files selected for processing (1)
src/test/java/com/meilisearch/integration/TasksTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Related issue
Fixes #948
PR check List:
Update API method to allow retrieving a tasks documents
Add test cases for the new method
Add an example under the get_task_documents_1 YAML key in .code-samples.meilisearch.yaml. It should match the example in the documentation.
@Strift can you review this PR