Support sort for documents endpoint
#779
Conversation
WalkthroughAdds DocumentsQuery.sort with setter and toArray output; emits fields as arrays; HTTP client serializes list arrays to comma-separated strings; tests updated for sorting and fields; adds symfony/polyfill-php81 dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant DQ as DocumentsQuery
participant HC as HttpClient
participant API as Meilisearch
App->>DQ: setFields([...])\nsetFilter([...])\nsetSort([...])
DQ-->>App: toArray() -> { fields: [...], filter: ..., sort: [...] }
App->>HC: GET /indexes/{uid}/documents?{query}
note right of HC #E8F6EF: buildQueryString()\n- list arrays -> "a,b"\n- booleans -> "true"/"false"
HC->>API: HTTP request with serialized query
API-->>HC: JSON documents
HC-->>App: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
🧹 Nitpick comments (7)
src/Contracts/DocumentsQuery.php (4)
36-40: Good addition ofsort; align phpdoc with returned shape (non-empty) to avoid confusion.
@var list<non-empty-string>|nullallows empty lists, whiletoArray()advertisesnon-empty-list<string>. Recommend making both consistent and non-empty.Apply this docblock tweak:
- /** - * @var list<non-empty-string>|null - */ + /** + * @var non-empty-list<string>|null + */ private ?array $sort = null;
125-130: Setter looks good; consider guarding against empty/whitespace-only entries.Optional: reject
[]and trim entries to keep the contract tight and avoid sendingsort=or invalid values to the API.- public function setSort(array $sort): self - { - $this->sort = $sort; - - return $this; - } + /** + * @param non-empty-list<string> $sort + * + * @return $this + */ + public function setSort(array $sort): self + { + // Trim and filter out empty strings to preserve "non-empty" semantics + $normalized = array_values(array_filter(array_map( + static fn ($s) => \is_string($s) ? trim($s) : $s, + $sort + ), static fn ($s) => \is_string($s) && $s !== '')); + + // If nothing remains, consider it unset (or throw if you prefer stricter behavior) + $this->sort = $normalized !== [] ? $normalized : null; + + return $this; + }
145-153: Skip empty arrays forfields/sortto avoid sending empty params (fields=/sort=).
array_filteronly removes nulls; empty arrays survive and will be imploded to an empty string in the GET path. Mirror theidshandling by omitting keys when arrays are empty.return array_filter([ 'offset' => $this->offset, 'limit' => $this->limit, - 'fields' => $this->fields, + 'fields' => ($this->fields ?? []) !== [] ? $this->fields : null, 'filter' => $this->filter, 'retrieveVectors' => (null !== $this->retrieveVectors ? ($this->retrieveVectors ? 'true' : 'false') : null), 'ids' => ($this->ids ?? []) !== [] ? implode(',', $this->ids) : null, - 'sort' => $this->sort, + 'sort' => ($this->sort ?? []) !== [] ? $this->sort : null, ], static function ($item) { return null !== $item; });
132-141: UpdatetoArrayphpdoc:fieldsis always an arrayThe
toArray()method only ever emitsfieldsas an array (string normalization happens later in HandlesDocuments). Remove the|non-empty-stringunion from the docblock.• File: src/Contracts/DocumentsQuery.php
• Lines: 132–141/** * @return array{ offset?: non-negative-int, limit?: non-negative-int, - fields?: non-empty-list<string>|non-empty-string, + fields?: non-empty-list<string>, filter?: list<non-empty-string|list<non-empty-string>>, retrieveVectors?: 'true'|'false', ids?: string, sort?: non-empty-list<string>, } */tests/Contracts/DocumentsQueryTest.php (1)
23-23: LGTM — test now reflects array-shapedfieldsintoArray().Consider adding a small test for
setSort()to asserttoArray()['sort']is present and preserves order, e.g.['genre:desc','id:asc'].src/Endpoints/Delegates/HandlesDocuments.php (1)
32-37: Normalize only non-empty arrays (avoidfields=/sort=query params).
isset($query['sort'])/isset($query['fields'])is true for empty arrays, leading toimplode([])→''. Prefer checking non-empty before imploding.Minimal change:
- if (isset($query['sort'])) { - $query['sort'] = implode(',', $query['sort']); - } - if (isset($query['fields'])) { - $query['fields'] = implode(',', $query['fields']); - } + if (!empty($query['sort'])) { + $query['sort'] = implode(',', $query['sort']); + } + if (!empty($query['fields'])) { + $query['fields'] = implode(',', $query['fields']); + }Alternatively, to DRY this up:
- if (!empty($query['sort'])) { - $query['sort'] = implode(',', $query['sort']); - } - if (!empty($query['fields'])) { - $query['fields'] = implode(',', $query['fields']); - } + foreach (['sort', 'fields'] as $key) { + if (!empty($query[$key])) { + $query[$key] = implode(',', $query[$key]); + } + }tests/Endpoints/DocumentsTest.php (1)
684-694: LGTM — asserts array-shapedfieldswhen using filter (POST /documents/fetch path).This guards against accidental GET-style normalization leaking into the POST path. Nice.
Optional: Add a companion test that sets both
fieldsandsortwithout a filter and asserts the returned documents only include the requested fields and are sorted accordingly, to cover the GET normalization path end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Contracts/DocumentsQuery.php(2 hunks)src/Endpoints/Delegates/HandlesDocuments.php(1 hunks)tests/Contracts/DocumentsQueryTest.php(1 hunks)tests/Endpoints/DocumentsTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/Contracts/DocumentsQueryTest.php (1)
src/Contracts/DocumentsQuery.php (1)
toArray(143-154)
src/Contracts/DocumentsQuery.php (1)
src/Contracts/SearchQuery.php (2)
setSort(297-302)toArray(463-494)
tests/Endpoints/DocumentsTest.php (5)
tests/TestCase.php (2)
createEmptyIndex(139-145)safeIndexName(147-150)src/Endpoints/Delegates/HandlesSettings.php (2)
updateSortableAttributes(269-272)updateFilterableAttributes(246-249)src/Endpoints/Delegates/HandlesDocuments.php (2)
addDocuments(47-50)getDocuments(23-45)src/Endpoints/Delegates/HandlesTasks.php (1)
waitForTask(45-48)src/Contracts/DocumentsQuery.php (5)
DocumentsQuery(7-155)setSort(125-130)setFields(70-75)setFilter(84-89)toArray(143-154)
|
CI failure seems unrelated |
norkunas
left a comment
norkunas
left a comment
There was a problem hiding this comment.
IIRC target branch for this PR should be against bump-meilisearch-v1.16.0
|
Sure, just changed that. |
|
Hey there, I reverted the base branch to Apologies, updating the base branch dismissed @norkunas's review. I hope we didn't lose useful feedback 😭 A note on
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Wait before merging: E.g. "for each param as key -> value, if array_is_list(value), then value = implode(',', value) ?" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
- Coverage 89.78% 87.79% -2.00%
==========================================
Files 59 65 +6
Lines 1449 1589 +140
==========================================
+ Hits 1301 1395 +94
- Misses 148 194 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Endpoints/DocumentsTest.php (3)
669-682: Prefer using getResults() over array access on DocumentsResultsAvoid relying on ArrayAccess for DocumentsResults; extract results explicitly for consistency with the rest of this file.
- $response = $index->getDocuments((new DocumentsQuery())->setSort(['genre:desc', 'id:asc'])); - self::assertSame(2, $response[0]['id']); + $response = $index->getDocuments((new DocumentsQuery())->setSort(['genre:desc', 'id:asc'])); + $docs = $response->getResults(); + self::assertSame(2, $docs[0]['id']); - $response = $index->getDocuments((new DocumentsQuery())->setSort(['genre:desc', 'id:desc'])); - self::assertSame(1344, $response[0]['id']); + $response = $index->getDocuments((new DocumentsQuery())->setSort(['genre:desc', 'id:desc'])); + $docs = $response->getResults(); + self::assertSame(1344, $docs[0]['id']);Note: I’m intentionally not asking to wait for settings tasks here; per the retrieved learning on this repo, waiting on addDocuments ensures prerequisite settings tasks are completed.
684-699: Use getResults() and make field-order assertion robust
- Extract results before indexing.
- Comparing exact key order can be brittle; compare sorted keys to ensure only the requested fields are returned.
- $response = $index->getDocuments($query); - self::assertSame(123, $response[0]['id']); - self::assertSame(['id', 'title'], \array_keys($response[0])); + $response = $index->getDocuments($query); + $docs = $response->getResults(); + self::assertSame(123, $docs[0]['id']); + $keys = \array_keys($docs[0]); + sort($keys); + self::assertSame(['id', 'title'], $keys);
701-711: Add a complementary GET-path test for fields normalizationA small integration test would ensure arrays are imploded for GET and only requested fields are returned, covering the symmetric case of your POST-path check above.
Proposed test (add near other documents tests):
public function testGetDocumentsWithFieldsArrayOnGet(): void { $index = $this->createEmptyIndex($this->safeIndexName('movies')); $promise = $index->addDocuments(self::DOCUMENTS); $index->waitForTask($promise['taskUid']); $response = $index->getDocuments((new DocumentsQuery())->setFields(['id', 'title'])); $docs = $response->getResults(); self::assertNotEmpty($docs); // Assert only requested fields are returned (order-independent). $keys = \array_keys($docs[0]); sort($keys); self::assertSame(['id', 'title'], $keys); }I can open a follow-up PR with this test if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/Endpoints/DocumentsTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T05:05:22.383Z
Learnt from: bpolaszek
PR: meilisearch/meilisearch-php#779
File: tests/Endpoints/DocumentsTest.php:669-682
Timestamp: 2025-08-20T05:05:22.383Z
Learning: In Meilisearch, tasks have implicit dependencies where document operations (like addDocuments) won't be processed until prerequisite setting tasks (like updateSortableAttributes, updateFilterableAttributes) have completed. Therefore, waiting for the addDocuments task completion implicitly ensures that all prerequisite setting tasks have also completed.
Applied to files:
tests/Endpoints/DocumentsTest.php
🧬 Code graph analysis (1)
tests/Endpoints/DocumentsTest.php (4)
tests/TestCase.php (2)
createEmptyIndex(139-145)safeIndexName(147-150)src/Endpoints/Delegates/HandlesSettings.php (2)
updateSortableAttributes(269-272)updateFilterableAttributes(246-249)src/Endpoints/Delegates/HandlesDocuments.php (1)
getDocuments(23-45)src/Contracts/DocumentsQuery.php (5)
DocumentsQuery(7-158)setSort(128-133)setFields(70-75)setFilter(84-89)toArray(146-157)
🔇 Additional comments (1)
tests/Endpoints/DocumentsTest.php (1)
701-711: LGTM: asserts DocumentsQuery keeps fields as array when filter is setThis validates the contract that DocumentsQuery stays transport-agnostic (arrays in toArray), leaving GET-vs-POST normalization to HandlesDocuments.
bpolaszek
force-pushed
the
feature/sort-documents
branch
from
6e88f69 to
67d7c1f
Compare
|
hello @bpolaszek thank you for your PR Can you fix the git conflicts with |
bpolaszek
force-pushed
the
feature/sort-documents
branch
from
ef6de8d to
751c3d0
Compare
|
Hi @curquiza, Here you go! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/polyfills.php (1)
19-21: Eliminate unused loop variable ($v) to satisfy PHPMD.Minor cleanup to avoid “Unused local variable” warnings without allocating extra memory.
Apply this diff:
- foreach ($array as $k => $v) { + foreach ($array as $k => $_) {src/Http/Client.php (1)
168-170: Consider RFC 3986 encoding for more predictable URLs.Not required, but using RFC 3986 avoids “+” for spaces and ensures consistent percent-encoding across environments.
Suggested change (outside the selected lines, shown for context):
- return \count($queryParams) > 0 ? '?'.http_build_query($queryParams) : ''; + return \count($queryParams) > 0 ? '?'.http_build_query($queryParams, '', '&', PHP_QUERY_RFC3986) : '';tests/PollyfillsTest.php (3)
11-11: Run the polyfill test on PHP 8.0 too (adjust RequiresPhp).
array_is_listwas introduced in PHP 8.1. With< 8.0, you’re skipping 8.0 where the polyfill is also required. Change to< 8.1.Apply this diff:
- #[RequiresPhp('< 8.0')] + #[RequiresPhp('< 8.1')]
9-9: Typo: rename PollyfillsTest → PolyfillsTest for consistency.Keeps naming aligned with
polyfills.phpand common terminology.If you want, I can propose a follow-up PR commit to rename the file/class.
14-18: Avoid inline PHPStan ignores by bootstrapping Composer in PHPStan.If you adopt the PHPStan bootstrap suggestion in phpstan.dist.neon, these
@phpstan-ignore-linecomments become unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
composer.json(1 hunks)phpstan.dist.neon(1 hunks)src/Contracts/DocumentsQuery.php(2 hunks)src/Http/Client.php(1 hunks)src/polyfills.php(1 hunks)tests/Contracts/DocumentsQueryTest.php(1 hunks)tests/Endpoints/DocumentsTest.php(1 hunks)tests/PollyfillsTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Contracts/DocumentsQueryTest.php
- tests/Endpoints/DocumentsTest.php
- src/Contracts/DocumentsQuery.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/PollyfillsTest.php (2)
tests/TestCase.php (1)
TestCase(16-180)src/polyfills.php (1)
array_is_list(11-26)
🪛 PHPMD (2.15.0)
src/polyfills.php
19-19: Avoid unused local variables such as '$v'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (3)
composer.json (1)
31-33: Autoloading the polyfill via "files" is appropriate.Good choice for globally available helpers while guarding with function_exists in the implementation. No issues spotted.
src/polyfills.php (1)
11-26: Polyfill logic matches PHP 8.1 semantics and is safely guarded.The implementation is correct, fast, and properly namespaced in the global scope with a function_exists guard. The doc reference to Symfony’s polyfill is a nice touch.
src/Http/Client.php (1)
168-170: List normalization for query params is correct and centralizes GET handling.Imploding list-like arrays here keeps
DocumentsQueryindependent of HTTP verb and aligns with Meilisearch’sfields/sortexpectations.
|
Failures are related to Meilisearch 1.19 - tests pass on 1.18. |
|
@Strift is going to open a PR to fix that soon and be able to merge your PR. Sorry for the incovenience |
|
Ty @Strift 👍 |
Co-authored-by: Tomas Norkūnas <[email protected]>
bpolaszek
force-pushed
the
feature/sort-documents
branch
from
a8b1328 to
09b8b9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Endpoints/DocumentsTest.php (2)
669-682: Good coverage for GET /documents sortingThis validates sort ordering via GET (no filter), exercising the implode behavior for list-like arrays. Consider, optionally, asserting a few consecutive items or the full order to strengthen the guarantee beyond the first item.
684-699: Make fields-order assertion robust (don’t rely on key order)JSON/object key order isn’t guaranteed across versions/backends. Sort keys before asserting to avoid brittle failures.
- self::assertSame(['id', 'title'], array_keys($response[0])); + $keys = array_keys($response[0]); + sort($keys); + self::assertSame(['id', 'title'], $keys);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
composer.json(1 hunks)src/Contracts/DocumentsQuery.php(2 hunks)src/Http/Client.php(1 hunks)tests/Contracts/DocumentsQueryTest.php(1 hunks)tests/Endpoints/DocumentsTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Http/Client.php
- tests/Contracts/DocumentsQueryTest.php
- src/Contracts/DocumentsQuery.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T05:05:22.383Z
Learnt from: bpolaszek
PR: meilisearch/meilisearch-php#779
File: tests/Endpoints/DocumentsTest.php:669-682
Timestamp: 2025-08-20T05:05:22.383Z
Learning: In Meilisearch, tasks have implicit dependencies where document operations (like addDocuments) won't be processed until prerequisite setting tasks (like updateSortableAttributes, updateFilterableAttributes) have completed. Therefore, waiting for the addDocuments task completion implicitly ensures that all prerequisite setting tasks have also completed.
Applied to files:
tests/Endpoints/DocumentsTest.php
🧬 Code graph analysis (1)
tests/Endpoints/DocumentsTest.php (4)
tests/TestCase.php (2)
createEmptyIndex(139-145)safeIndexName(147-150)src/Endpoints/Delegates/HandlesSettings.php (2)
updateSortableAttributes(269-272)updateFilterableAttributes(246-249)src/Endpoints/Delegates/HandlesDocuments.php (2)
addDocuments(41-44)getDocuments(23-39)src/Contracts/DocumentsQuery.php (5)
DocumentsQuery(7-158)setSort(128-133)setFields(70-75)setFilter(84-89)toArray(146-157)
🔇 Additional comments (2)
composer.json (1)
25-26: Polyfill addition for PHP 8.1 features looks appropriateIncluding symfony/polyfill-php81 to support array_is_list on PHP 7.4/8.0 aligns with the updated query serialization. No further changes needed here.
tests/Endpoints/DocumentsTest.php (1)
701-711: Fields remain an array in toArray() — LGTMThis ensures DocumentsQuery returns fields as an array (not imploded), matching normalization handled downstream. Looks good.
|
Well, I don't get why linter wants |
|
It's a limited list which functions can be optimized with prefixing |
5 participants
Pull Request
Related issue
Fixes #776
What does this PR do?
sortparameter introduced onGET /documentsandPOST /documents/fetchendpoints in 1.16.sortparameter is an array by design, but should be imploded when passed through GET, similarly asfieldsdoes.DocumentsQuery's responsibility of being aware of what verb will be used for the request. As a consequence, the normalization logic ofsorthas been introduced inHandlesDocuments, and the wayfieldswas normalized has been refactored accordingly, to maintain things consistent.FYI: similar normalization issue in #777, there's room for improvement I guess
Summary by CodeRabbit