Add documents API sort #646
Conversation
WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Index as Index#documents
participant API as Meilisearch API
Client->>Index: documents(options)
alt options includes filter
Note right of Index: build POST body with { limit, offset, fields, filter, sort }
Index->>API: POST /indexes/:uid/documents/fetch (body includes sort)
API-->>Index: { hits, limit, offset, ... }
else no filter
Note right of Index: build GET query with { limit, offset, fields, sort }
Index->>API: GET /indexes/:uid/documents?sort=...
API-->>Index: { results, limit, offset, ... }
end
Index-->>Client: documents list
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 |
|
The errors are related to: #645 |
![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 (3)
lib/meilisearch/index.rb (2)
76-76: Clarify the :sort option and note server version support.Improve the inline docs with examples and an explicit version note for when Meilisearch added sort on the documents API.
- # :sort - A list of attributes written as an array or as a comma-separated string (optional) + # :sort - A list of sort rules ("attribute:order"), provided as an Array or a comma-separated String (optional, Meilisearch v1.16+).
82-85: Normalize sort for POST and drop empty sort for GET to avoid sending ambiguous values.Today:
- POST body forwards
sortas-is. If a caller passes a comma-separated String, it may or may not be accepted by the API. Normalizing to an Array makes behavior predictable.- GET query serializes Arrays to a comma-separated String. If the array is empty, we currently send
sort=which can cause confusing behavior on older servers.Proposed refactor: coerce POST
sortto an Array when given as a String, and strip emptysorton both paths. This is backward-compatible and avoids sending meaningless parameters.Can you confirm whether
/documents/fetchacceptssortas a String? If not guaranteed across Meilisearch versions, the normalization below is safer.Utils.version_error_handler(__method__) do - if options.key?(:filter) - http_post "/indexes/#{@uid}/documents/fetch", Utils.filter(options, [:limit, :offset, :fields, :filter, :sort]) - else - http_get "/indexes/#{@uid}/documents", Utils.parse_query(options, [:limit, :offset, :fields, :sort]) - end + if options.key?(:filter) + body = Utils.filter(options, [:limit, :offset, :fields, :filter, :sort]) + # Normalize POST body: accept "title:asc,title2:desc" and turn it into an Array. + if body[:sort].is_a?(String) + body[:sort] = body[:sort].split(',').map!(&:strip) + end + body.delete(:sort) if body[:sort].respond_to?(:empty?) && body[:sort].empty? + http_post "/indexes/#{@uid}/documents/fetch", body + else + query = Utils.parse_query(options, [:limit, :offset, :fields, :sort]) + # Avoid sending sort= for empty arrays on older servers. + if query[:sort].is_a?(String) && query[:sort].empty? + query.delete(:sort) + end + http_get "/indexes/#{@uid}/documents", query + end endspec/meilisearch/index/documents_spec.rb (1)
427-447: Sorting tests look solid; consider adding a GET string-form case (and POST string-form if you adopt normalization).Current tests cover Array form for both GET and POST. To fully capture the public contract described in docs (“array or comma-separated string”), add:
- GET with a comma-separated String.
- Optionally POST with a String if you adopt the normalization suggested in lib/meilisearch/index.rb.
describe 'sorts documents' do before do index.update_sortable_attributes(['title']).await end it 'get' do docs = index.documents(sort: ['title:asc']) expect(docs['results'].first).to include('objectId' => 1, 'title' => 'Alice In Wonderland') expect(docs['results'].last).to include('objectId' => 13, 'title' => 'Zen in the Art of Archery') docs = index.documents(sort: ['title:desc']) expect(docs['results'].first).to include('objectId' => 13, 'title' => 'Zen in the Art of Archery') expect(docs['results'].last).to include('objectId' => 1, 'title' => 'Alice In Wonderland') end + + it 'get accepts comma-separated string' do + docs = index.documents(sort: 'title:asc') + expect(docs['results'].first).to include('objectId' => 1, 'title' => 'Alice In Wonderland') + expect(docs['results'].last).to include('objectId' => 13, 'title' => 'Zen in the Art of Archery') + end it 'post' do docs = index.documents(filter: 'comment NOT EXISTS', sort: ['title:asc']) expect(docs['results'].first).to include('objectId' => 2, 'title' => 'Le Rouge et le Noir') expect(docs['results'].last).to include('objectId' => 13, 'title' => 'Zen in the Art of Archery') end + + # Enable this only if POST normalization to Array is implemented in the SDK: + # it 'post accepts comma-separated string' do + # docs = index.documents(filter: 'comment NOT EXISTS', sort: 'title:asc') + # expect(docs['results'].first).to include('objectId' => 2, 'title' => 'Le Rouge et le Noir') + # end 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 (2)
lib/meilisearch/index.rb(1 hunks)spec/meilisearch/index/documents_spec.rb(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/meilisearch/index.rb (2)
lib/meilisearch/utils.rb (2)
filter(44-46)parse_query(48-56)lib/meilisearch/http_request.rb (2)
http_post(39-51)http_get(26-37)
spec/meilisearch/index/documents_spec.rb (1)
lib/meilisearch/index.rb (3)
update_filterable_attributes(531-534)update_sortable_attributes(549-552)documents(79-87)
🔇 Additional comments (2)
spec/meilisearch/index/documents_spec.rb (2)
13-13: Good addition to exercise NOT EXISTS logic.Adding a document without a
commentfield is a clean way to validatecomment NOT EXISTSbehavior in sorting and filtering tests.
387-387: Enablingcommentas filterable is necessary for the NOT EXISTS scenario.This aligns the test setup with the filter used later and prevents false negatives.
Nymuxyzo
force-pushed
the
feat/documents-api-sort
branch
from
be3dcc8 to
a74c0bd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 806 806
=========================================
Hits 806 806 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Nymuxyzo
force-pushed
the
feat/documents-api-sort
branch
from
a74c0bd to
1630584
Compare
Nymuxyzo
force-pushed
the
feat/documents-api-sort
branch
from
1630584 to
6ba4c1d
Compare
Pull Request
Related issue
Fixes #643
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
New Features
Documentation
Tests