chore: backport document sorting to v1.x #802
Conversation
WalkthroughAdds sort support to DocumentsQuery, updates query serialization to collapse list-valued parameters into comma-separated strings, adjusts tests to validate sorting and combined sort/filter/fields behavior, and adds a PHP 8.1 polyfill dependency in composer.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant Q as DocumentsQuery
participant C as Http\Client
participant S as Meilisearch Server
U->>Q: setFields(['id','title'])<br/>setSort(['genre:desc','id:asc'])
U->>C: getDocuments(index, Q->toArray())
note over Q,C: Q->toArray() returns arrays for fields/sort
C->>C: buildQueryString(params)
rect rgba(220,240,255,0.4)
note right of C: For each param value
C->>C: if bool → 'true'/'false'
C->>C: if list array → implode with ','
end
C->>S: GET /indexes/{uid}/documents?fields=id,title&sort=genre:desc,id:asc&filter=...
S-->>C: 200 OK JSON
C-->>U: Decoded response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Contracts/DocumentsQuery.php (4)
36-39: Align sort typing with behavior (non-empty list vs possibly empty).Property doc allows any list; toArray advertises non-empty-list. Either (a) normalize empty to null, or (b) relax the toArray phpdoc. Prefer (a) to avoid sending sort=.
Apply this if you choose (a):
- /** - * @var list<non-empty-string>|null - */ - private ?array $sort = null; + /** + * @var non-empty-list<non-empty-string>|null + */ + private ?array $sort = null;
125-133: Normalize and validate sort values; avoid empty payloads.Trim entries, drop empties, and null-out when nothing remains to prevent sending sort="".
- public function setSort(array $sort): self - { - $this->sort = $sort; - - return $this; - } + public function setSort(array $sort): self + { + $sort = array_values(array_filter( + array_map(static fn($s) => \is_string($s) ? trim($s) : (string) $s, $sort), + static fn(string $s) => $s !== '' + )); + $this->sort = $sort !== [] ? $sort : null; + return $this; + }
136-145: PhpDoc return shape for fields is outdated.toArray() now returns fields as an array; the union allowing non-empty-string is misleading.
- * fields?: non-empty-list<string>|non-empty-string, + * fields?: non-empty-list<string>,Also consider documenting ids as “comma-separated string of IDs” for clarity.
151-156: Skip empty arrays for fields/sort in payload.Sending fields=[] or sort=[] yields fields= or sort= after transport coercion. Treat empty as unset.
- 'fields' => $this->fields, + 'fields' => ($this->fields ?? []) !== [] ? $this->fields : null, ... - 'sort' => $this->sort, + 'sort' => ($this->sort ?? []) !== [] ? $this->sort : null,src/Http/Client.php (1)
224-226: Only implode list arrays when all elements are scalars.As written, any list is imploded. If a list contains nested arrays (e.g., complex filters) passed via query, it will flatten incorrectly.
- if (\is_array($value) && array_is_list($value)) { - $queryParams[$key] = implode(',', $value); - } + if (\is_array($value) && array_is_list($value) && self::isListOfScalars($value)) { + $queryParams[$key] = implode(',', array_map('strval', $value)); + }Add this helper in the class (outside this hunk):
private static function isListOfScalars(array $list): bool { foreach ($list as $v) { if (\is_array($v) || \is_object($v)) { return false; } } return true; }tests/Endpoints/DocumentsTest.php (2)
681-694: Sorting test looks good; minor stability tweak optional.You rely on Meilisearch task ordering by waiting only on addDocuments. It’s fine (tasks are FIFO), but explicitly waiting for the settings tasks would make the test more robust to future changes in server semantics.
- $index->updateSortableAttributes(['id', 'genre']); - $index->updateFilterableAttributes(['id', 'genre']); - $promise = $index->addDocuments(self::DOCUMENTS); + $t1 = $index->updateSortableAttributes(['id', 'genre']); + $t2 = $index->updateFilterableAttributes(['id', 'genre']); + $index->waitForTask($t1['taskUid']); + $index->waitForTask($t2['taskUid']); + $promise = $index->addDocuments(self::DOCUMENTS);
696-711: Avoid key-order brittleness in field selection assertion.JSON key order isn’t guaranteed; assert on set equality instead of order.
- self::assertSame(['id', 'title'], array_keys($response[0])); + self::assertSameCanonicalizing(['id', 'title'], array_keys($response[0]));Optionally also assert count($response[0]) === 2 to ensure no extra fields leak.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🔇 Additional comments (2)
composer.json (1)
37-39: Good call adding the PHP 8.1 polyfill for array_is_list.This keeps v1.x working on PHP < 8.1 while using array_is_list in the client. LGTM.
tests/Contracts/DocumentsQueryTest.php (1)
23-23: Assertion updated to array format — matches new API shape.This reflects the toArray() change. LGTM.
Pull Request
What does this PR do?
documentsendpoint #779 tov1.xPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit