Conversation
Adds getTaskDocuments() for the GET /tasks/{task_id}/documents
endpoint introduced in Meilisearch v1.13.
Fixes meilisearch#890
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a Tasks endpoint method to fetch a task's documents, a delegate method exposing it on the client surface, a unit test exercising the client call, and a documentation code-sample entry demonstrating usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Delegate as HandlesTasks
participant Tasks as Tasks Endpoint
participant HTTP as HTTP Client
participant Server as Meilisearch Server
Client->>Delegate: getTaskDocuments(taskUid)
Delegate->>Tasks: getDocuments(taskUid)
Tasks->>HTTP: GET /tasks/{taskUid}/documents
HTTP->>Server: HTTP GET /tasks/{taskUid}/documents
Server-->>HTTP: 200 OK + documents payload
HTTP-->>Tasks: response payload
Tasks-->>Delegate: return array payload
Delegate-->>Client: return array payload
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
==========================================
- Coverage 89.78% 87.14% -2.64%
==========================================
Files 59 82 +23
Lines 1449 1821 +372
==========================================
+ Hits 1301 1587 +286
- Misses 148 234 +86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Missing tests :) |
![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
🤖 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/Endpoints/Tasks.php`:
- Around line 25-28: Add integration tests for the new Tasks::getDocuments(int
$taskUid): array method by creating a test method (e.g., testGetDocuments) in
the existing TasksTest class that follows the pattern of other task tests (like
testGetOneTaskFromWaitTask): arrange a stubbed HTTP response for the GET to the
"/{taskUid}/documents" endpoint, call $tasks->getDocuments($taskUid), and assert
the returned array matches the stubbed payload and that the HTTP client was
invoked with the correct path; ensure the test covers at least one successful
response and a case for an empty document list to mirror existing test styles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9f3112e-ad70-4983-a276-0f019086ea6c
📒 Files selected for processing (3)
.code-samples.meilisearch.yamlsrc/Endpoints/Delegates/HandlesTasks.phpsrc/Endpoints/Tasks.php
Verify that getTaskDocuments returns an array for a completed document addition task. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Added test in 2173521: verifies |
tests/Endpoints/TasksTest.php
Outdated
|
|
||
| $documents = $this->client->getTaskDocuments($completedTask->getTaskUid()); | ||
|
|
||
| self::assertIsArray($documents); |
There was a problem hiding this comment.
Maybe asserting that array is not empty is better to prove that we really get something?
Addresses review feedback to verify we actually receive data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/Endpoints/TasksTest.php (1)
57-57:⚠️ Potential issue | 🟡 MinorRemove redundant
assertIsArraycall.The
getTaskDocuments()method has an explicit: arrayreturn type annotation, making the type check at line 57 redundant from a static analysis perspective. KeepassertNotEmpty()to validate that the response contains meaningful data.Proposed fix
- self::assertIsArray($documents); self::assertNotEmpty($documents);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Endpoints/TasksTest.php` at line 57, Remove the redundant type assertion: in the TasksTest test that calls getTaskDocuments() (which already has a : array return type), delete the self::assertIsArray($documents) assertion and keep the self::assertNotEmpty($documents) check to ensure the response contains meaningful data; update only the assertion call in the TasksTest test method that invokes getTaskDocuments().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Endpoints/TasksTest.php`:
- Line 53: The destructured assignment "[$seedTask, $completedTask] =
$this->seedIndex();" declares $seedTask which is unused; change it to ignore the
first slot and only capture the used variable by replacing $seedTask with an
unused/ignored placeholder (e.g. $_ or a blank slot) in the destructuring so
only $completedTask from seedIndex() is assigned, eliminating the PHPMD
unused-variable warning while leaving the seedIndex() call intact.
- Line 55: The test calls getTaskDocuments on $this->client before the
experimental endpoint is enabled; enable the experimental feature flag named
getTaskDocumentsRoute using the HTTP client pattern used in other tests (call
the test HTTP client on $this->client->http or the equivalent test request
helper to set the experimental flag to true) prior to invoking
getTaskDocuments($completedTask->getTaskUid()), then proceed with the existing
call; confirm the feature key matches your Meilisearch version.
---
Duplicate comments:
In `@tests/Endpoints/TasksTest.php`:
- Line 57: Remove the redundant type assertion: in the TasksTest test that calls
getTaskDocuments() (which already has a : array return type), delete the
self::assertIsArray($documents) assertion and keep the
self::assertNotEmpty($documents) check to ensure the response contains
meaningful data; update only the assertion call in the TasksTest test method
that invokes getTaskDocuments().
🪄 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: e39fd884-c5bc-4cef-a48e-5fdc8d6c1f97
📒 Files selected for processing (1)
tests/Endpoints/TasksTest.php
|
Fixed in 980d58b: removed the redundant |
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
Tests are failing:
1) Tests\Endpoints\TasksTest::testGetTaskDocumentsClient
Meilisearch\Exceptions\ApiException: Getting the documents of an enqueued task requires enabling the `get task documents route` experimental feature. See https://github.com/orgs/meilisearch/discussions/808
/home/runner/work/meilisearch-php/meilisearch-php/src/Http/Client.php:248
/home/runner/work/meilisearch-php/meilisearch-php/src/Http/Client.php:171
/home/runner/work/meilisearch-php/meilisearch-php/src/Http/Client.php:76
/home/runner/work/meilisearch-php/meilisearch-php/src/Endpoints/Tasks.php:27
/home/runner/work/meilisearch-php/meilisearch-php/src/Endpoints/Delegates/HandlesTasks.php:25
/home/runner/work/meilisearch-php/meilisearch-php/tests/Endpoints/TasksTest.php:55
Signed-off-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
|
Enabled the experimental feature flag before the test call in 5f38a80 -- follows the same |
| $http = new Client($this->host, getenv('MEILISEARCH_API_KEY')); | ||
| $http->patch('/experimental-features', ['getTaskDocumentsRoute' => true]); |
There was a problem hiding this comment.
@Strift as experimental features are the public/documented api, i think it should be available on $this->client like $this->client->enableExperimentaFeature(...), so the tests wouldn't need to instantiate new http client, wdyt?
There was a problem hiding this comment.
Yes, that would be better. But the enableExperimentaFeature method would be unstable, because the accepted set of features would vary as versions change.
We do it in the JS package, and I personally think that's fine.
There was a problem hiding this comment.
well we could just accept any string, so would not matter if list changes
The API only returns documents for enqueued or processing tasks. The test was calling getTaskDocuments on a completed task.
|
Fixed the test in 96d3851. The issue was calling Re: the experimental feature enablement discussion -- happy to switch to |
|
@Strift this is somewhat unexpected that this endpoint returns ndjson 🙂 |
|
Saw the NDJSON discussion -- should I update |
|
Hey there, I would like to align this feature with how it's being handled in our other SDKs (specifically the JavaScript one). The I prefer to align with what the Meilisearch team designed. Since it's an experimental method, we can still iterate if this doesn't work well. To achieve this, I recommend the following changes:
For reference, you can look at the equivalent implementation in the JavaScript SDK: meilisearch-js PR #2156. |
Align with JS SDK implementation per maintainer request. The
/tasks/{task_id}/documents endpoint returns NDJSON, so streaming
via StreamInterface is the correct approach rather than parsing
into a JSON array.
- Add getStream() to Http contract and Client implementation
- Update Tasks::getDocuments() to use getStream()
- Update HandlesTasks::getTaskDocuments() return type
- Update test to assert StreamInterface and non-empty content
|
Implemented in 902ee12:
Followed the pattern from the existing |
tests/Endpoints/TasksTest.php
Outdated
|
|
||
| self::assertInstanceOf(StreamInterface::class, $stream); | ||
| $content = (string) $stream; | ||
| self::assertNotEmpty($content); |
There was a problem hiding this comment.
well i'd like that the test would show how to access documents, because now it's unclear for me
There was a problem hiding this comment.
Updated in 81d91b5 - the test now parses the NDJSON stream line by line, decodes each document, and asserts the structure. Also fixes the phpstan alreadyNarrowedType error from the removed assertInstanceOf.
Show how to access individual documents from the stream by splitting NDJSON lines, decoding each, and asserting structure. Fixes phpstan alreadyNarrowedType error by removing redundant assertInstanceOf check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The testGetTaskDocumentsClient test was calling getTaskDocuments on a task that hadn't completed yet, causing a JsonException when trying to parse the NDJSON response. Wait for the task to finish first. Also removes the redundant assertNotEmpty($documents) that phpstan flagged as alreadyNarrowedType since the array is guaranteed non-empty by the assertNotEmpty($lines) above. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Fixed in f289c8c: the test was calling |
3 participants
Summary
Adds
getTaskDocuments()for theGET /tasks/{task_id}/documentsendpoint introduced in Meilisearch v1.13.Changes
src/Endpoints/Tasks.php: AddedgetDocuments(int $taskUid)that callsGET /tasks/{id}/documentssrc/Endpoints/Delegates/HandlesTasks.php: AddedgetTaskDocuments(int $uid)delegate method.code-samples.meilisearch.yaml: Addedget_task_documents_1code sampleTesting
getTask()/get()Fixes #890
This contribution was developed with AI assistance (Claude Code).
Summary by CodeRabbit
New Features
Documentation
Tests