Specify the new fetch documents route #234

irevoire · 2023-05-04T12:47:38Z

🤖 API Diff

Implemented in meilisearch/meilisearch#3570
Associated issue: meilisearch/meilisearch#3477

Summary

Specify the new fetch documents route

Changes

The GET /indexes/:uid/documents route has been updated with a new query parameter filter
A new POST /indexes/:uid/documents/fetch route has been created with all the parameters of the GET version above (limit, offset, fields, and filter)
A new error code has been introduced in case an error happens on the new filter fields: invalid_document_filter

Misc

Update OpenAPI specification file
Update telemetry datapoints

github-actions · 2023-05-04T12:54:11Z

🤖 API change detected:

Added (1)

POST /indexes/{indexUid}/documents/fetch

Modified (1)

GET /indexes/{indexUid}/documents
- Query parameter added: filter

Powered by Bump

3570: Get documents by filter r=irevoire a=dureuill # Pull Request ## Related issue Associated spec: meilisearch/specifications#234 None really, this is more of an extension of #3477: since after this issue we'll be able to delete documents by filter, it makes sense to also be able to get documents by filter. ## What does this PR do? ### User standpoint - Add a new `filter` URL parameter to `GET /indexes/{:indexUid}/documents` and a new `POST /indexes/{:indexUid}/documents/fetch` route with the same `offset, limit, fields, filter` ### Implementation standpoint - Add a new `Index::iter_documents` method to iterate on a set of documents rather than return a vector of these documents. - Rewrite the other `Index::*documents` methods to use the new `Index::iter_documents` method. ## Usage <details> <summary> Sample request and response </summary> ``` curl -X POST 'http://localhost:7700/indexes/index-1101/documents/fetch' -H 'Content-Type: application/json' --data-binary '{ "filter": "genres = Comedy", "limit": 3, "offset": 8000}' | jsonxf ``` ```json { "results": [ { "id": 326126, "title": "Bad Exorcists", "overview": "A trio of awkward teens intend to win a horror festival by making their own movie, but wind up getting their actress possessed in the process.", "genres": [ "Horror", "Comedy" ], "poster": "https://image.tmdb.org/t/p/w500/lwd65kPbjFacAw3QSXiwSsW6cFU.jpg", "release_date": 1425081600 }, { "id": 326215, "title": "Ooops! Noah is Gone...", "overview": "It's the end of the world. A flood is coming. Luckily for Dave and his son Finny, a couple of clumsy Nestrians, an Ark has been built to save all animals. But as it turns out, Nestrians aren't allowed. Sneaking on board with the involuntary help of Hazel and her daughter Leah, two Grymps, they think they're safe. Until the curious kids fall off the Ark. Now Finny and Leah struggle to survive the flood and hungry predators and attempt to reach the top of a mountain, while Dave and Hazel must put aside their differences, turn the Ark around and save their kids. It's definitely not going to be smooth sailing.", "genres": [ "Animation", "Adventure", "Comedy", "Family" ], "poster": "https://image.tmdb.org/t/p/w500/gEJXHgpiKh89Vwjc4XUY5CIgUdB.jpg", "release_date": 1427328000 }, { "id": 326241, "title": "For Here or to Go?", "overview": "An aspiring Indian tech entrepreneur in the Silicon Valley finds himself unexpectedly battling the bizarre American immigration system to keep his dream alive or prepare to return home forever.", "genres": [ "Drama", "Comedy" ], "poster": "https://image.tmdb.org/t/p/w500/ff8WaA7ItBgl36kdT232i0d0Fnq.jpg", "release_date": 1490918400 } ], "offset": 8000, "limit": 3, "total": 9331 } ``` <img width="1348" alt="Capture d’écran 2023-03-08 à 10 09 04" src="https://user-images.githubusercontent.com/41078892/223670905-6932b79b-f9b8-4a41-b59e-be2171705b7d.png"> </details> # Draft status - [ ] Route naming: having one route be `GET /indexes/{:indexUid}/documents` and the other `POST /indexes/{:indexUid}/documents/fetch` is suboptimal (also, technically a breaking change for documents with `fetch` as uid?), but `POST /indexes/{:indexUid}/documents` is already used to insert documents. Co-authored-by: Louis Dureuil <louis@meilisearch.com> Co-authored-by: Tamo <tamo@meilisearch.com>

macraig

LGTM! I'll add @gmourier as reviewer in case I've missed something.

text/0124-documents-api.md

gmourier · 2023-05-05T08:43:36Z

There is no telemetry added with this feature. Is this the case? (Asking for double checking, it's easy to forget from my experience 😮‍💨)

irevoire · 2023-05-05T14:20:33Z

There is no telemetry added with this feature. Is this the case? (Asking for double checking, it's easy to forget from my experience face_exhaling)

Yes! We didn't have the time to implement it (especially since there was no telemetry initially on the GET route either).
Either we wait, and I can add it here at some point during the next week, or I'll do a second spec later.

guimachiavelli · 2023-05-09T10:08:02Z

text/0124-documents-api.md

 - [3.1.2. `GET` - `indexes/:index_uid/documents/:document_id`](#312-get---indexesindexuiddocumentsdocumentid)
 - [3.1.3. `POST` - `indexes/:index_uid/documents`](#313-post---indexesindexuiddocuments)
 - [3.1.4. `PUT` - `indexes/:index_uid/documents`](#314-put---indexesindexuiddocuments)
 - [3.1.5. `DELETE` - `indexes/:index_uid/documents`](#315-delete---indexesindexuiddocuments)
 - [3.1.6. `DELETE` - `indexes/:index_uid/documents/:document_id`](#316-delete---indexesindexuiddocumentsdocumentid)
 - [3.1.7. `POST` - `indexes/:index_uid/documents/delete-batch`](#317-post---indexesindexuiddocumentsdelete-batch)

-#### 3.1.1. `GET` - `indexes/:index_uid/documents`
+#### 3.1.1. `GET` - `indexes/:index_uid/documents` and `POST` - `indexes/:index_uid/documents/fetch`


An unsolicited comment/feedback/question: is it wise to have two endpoints that do almost exactly the same thing, but in slightly different ways?

From a documentation perspective, it makes it harder for us to provide clear guidance on recommended practices and best use cases: "you can use A to do X, but you can also use B. B is slightly better for Y, but A works fine as well." This often results in users having to spend more time than they would want to on making a decision that might not be that important.

Yeah, personally, I think we should deprecate the GET version of the route, same for the search.
To me, basically;

The get route can be slightly easier to use with curl

The post route is better on absolutely everything else (like sending big filters or using the array syntax)

Deprecating GET would be fine for us. In case this is not possible (e.g. API stability and versioning), the second best solution would be to get clear confirmation from product and/or engine teams that we can recommend users to prefer POST and discourage GET usage.

IMHO, with this change GET becomes a net negative. Being slightly easier to use in a relatively small use-case is outweighed by the effort required in choosing between similar options.

@guimachiavelli totally fine on our side to recommend users to use POST instead of GET. We can even go so far as to warn users that GET will be deprecated in the near future.

Brilliant, @macraig, thanks! We'll keep this in mind when documenting these changes and add you as a one of the reviewers to ensure our recommendations are aligned with the API design's intent 👍

For what it's worth, we'd also be very happy if GET /search were to be deprecated.

So, just to be sure are we going to deprecate the GET index/{index_uid}/documents right away or are we waiting until the v1.3?

Cause, I can already mark those getDocuments occurrences as deprecated and point the users to the new fetchDocuments method.

@brunoocasali We won't mark it as deprecated for v1.2, we are waiting for v1.3

Should we (integrations) follow that? Cause I don't see a world where waiting til v1.3 will make much difference for the users, a deprecation notice is not the same as "this method does not work anymore. Take this exception in your face instead".

Once we deprecate it, it'll continue to work until the v2 of meilisearch, right?
The user will never get an exception, I think? (but maybe you could deprecate it in the integration and get rid of the method before meilisearch v2 🤔)

text/0034-telemetry-policies.md

open-api.yaml

text/0124-documents-api.md

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

3738: Add analytics on the get documents resource r=irevoire a=irevoire # Pull Request ## Related issue Fixes #3737 Related spec meilisearch/specifications#234 ## What does this PR do? Add the analytics for the following routes: - `GET` - `/indexes/:uid/documents` - `GET` - `/indexes/:uid/documents/:doc_id` - `POST` - `/indexes/:uid/documents/fetch` These analytics are aggregated between two events: - `Documents Fetched GET` - `Documents Fetched POST` That shares the same payload: Property name | Description | Example | |---------------|-------------|---------| | `requests.total_received` | Total number of request received in this batch | 325 | | `per_document_id` | `false` | false | | `per_filter` | `true` if `POST /indexes/:indexUid/documents/fetch` endpoint was used with a filter in this batch, otherwise `false` | false | | `pagination.max_limit` | Highest value given for the `limit` parameter in this batch | 60 | | `pagination.max_offset` | Highest value given for the `offset` parameter in this batch | 1000 | Co-authored-by: Tamo <tamo@meilisearch.com>

3738: Add analytics on the get documents resource r=dureuill a=irevoire # Pull Request ## Related issue Fixes #3737 Related spec meilisearch/specifications#234 ## What does this PR do? Add the analytics for the following routes: - `GET` - `/indexes/:uid/documents` - `GET` - `/indexes/:uid/documents/:doc_id` - `POST` - `/indexes/:uid/documents/fetch` These analytics are aggregated between two events: - `Documents Fetched GET` - `Documents Fetched POST` That shares the same payload: Property name | Description | Example | |---------------|-------------|---------| | `requests.total_received` | Total number of request received in this batch | 325 | | `per_document_id` | `false` | false | | `per_filter` | `true` if `POST /indexes/:indexUid/documents/fetch` endpoint was used with a filter in this batch, otherwise `false` | false | | `pagination.max_limit` | Highest value given for the `limit` parameter in this batch | 60 | | `pagination.max_offset` | Highest value given for the `offset` parameter in this batch | 1000 | Co-authored-by: Tamo <tamo@meilisearch.com>

gmourier

Thanks @irevoire !

irevoire · 2023-05-22T10:01:09Z

Seen it with @gmourier.
Instead of merging the get and the delete PR, we’re going to close this PR and only merge #236, which contains the specification for both features.

From what I see, all comments have been addressed on this PR. If I missed something or you want to add something, please do it on #236.

1493: Add the filter field in getDocuments for Meilisearch v1.2 r=bidoubiwa a=bidoubiwa as per the specification: meilisearch/specifications#234 - Add the `filter` field on the `getDocuments` route (TS) The `filter` field works precisely like the `filter` field used on the `search` method. See [the docs on how to use filters](https://www.meilisearch.com/docs/learn/advanced/filtering#filter-basics). Co-authored-by: Charlotte Vermandel <charlottevermandel@gmail.com>

irevoire added Ready For Review Feature specification must be reviewed. Implemented Feature specification has been implemented. OpenAPI Update OpenAPI specification. labels May 4, 2023

irevoire requested review from gmourier and macraig and removed request for gmourier May 4, 2023 12:47

irevoire mentioned this pull request May 4, 2023

Get documents by filter meilisearch/meilisearch#3570

Merged

1 task

irevoire force-pushed the fetch-documents branch from d97203f to 6ef42de Compare May 4, 2023 12:53

This was referenced May 4, 2023

Get and delete documents by filter #236

Merged

Delete and get documents by filter meilisearch/meilisearch#3477

Closed

irevoire added 3 commits May 4, 2023 16:27

specify the new fetch documents route

174cafd

update open-api

6ef42de

try to be nice with bumpsh

5e2c6d8

macraig approved these changes May 4, 2023

View reviewed changes

macraig requested a review from gmourier May 4, 2023 15:22

gmourier suggested changes May 5, 2023

View reviewed changes

text/0124-documents-api.md Outdated Show resolved Hide resolved

text/0124-documents-api.md Show resolved Hide resolved

text/0124-documents-api.md Show resolved Hide resolved

text/0124-documents-api.md Outdated Show resolved Hide resolved

guimachiavelli reviewed May 9, 2023

View reviewed changes

brunoocasali mentioned this pull request May 10, 2023

Changes regarding Meilisearch v1.2.0 meilisearch/integration-guides#261

Closed

specify the new analytics of the get/fetch documents route

3141e26

maryamsulemani97 mentioned this pull request May 10, 2023

v1.2: Delete and get documents by filter meilisearch/documentation#2411

Closed

4 tasks

gmourier reviewed May 10, 2023

View reviewed changes

text/0034-telemetry-policies.md Show resolved Hide resolved

text/0034-telemetry-policies.md Outdated Show resolved Hide resolved

text/0034-telemetry-policies.md Outdated Show resolved Hide resolved

gmourier reviewed May 10, 2023

View reviewed changes

text/0034-telemetry-policies.md Outdated Show resolved Hide resolved

gmourier added the Telemetry Update the telemetry collect. label May 10, 2023

This was referenced May 10, 2023

Add analytics on the get documents resource meilisearch/meilisearch#3738

Merged

Add analytics on the get / fetch document route meilisearch/meilisearch#3737

Closed

group the analytics behind already used groups

52ef6c6

irevoire added 2 commits May 10, 2023 15:35

fix a placeholder text

f50255b

group the get and fetch route under a common section

9af7b72

maryamsulemani97 mentioned this pull request May 11, 2023

v1.2: Telemetry table updates meilisearch/documentation#2414

Closed

3 tasks

gmourier mentioned this pull request May 15, 2023

Release v1.2.0 #227

Merged

1 task

gmourier reviewed May 15, 2023

View reviewed changes

open-api.yaml Show resolved Hide resolved

gmourier added v1.2.0 Q2:2023 labels May 15, 2023

irevoire added 4 commits May 16, 2023 11:08

rename the section to contain the name of both routes

8173e1a

clarify how to send the parameter to both routes

826f693

fix the type of the parameters

f03e4ff

add the new fetch route to the api keys actions

af875c7

bidoubiwa reviewed May 16, 2023

View reviewed changes

text/0124-documents-api.md Outdated Show resolved Hide resolved

bidoubiwa reviewed May 16, 2023

View reviewed changes

text/0124-documents-api.md Outdated Show resolved Hide resolved

bidoubiwa reviewed May 16, 2023

View reviewed changes

text/0124-documents-api.md Outdated Show resolved Hide resolved

capitalize json

c63508f

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

bidoubiwa mentioned this pull request May 16, 2023

Add the filter field in getDocuments for Meilisearch v1.2 meilisearch/meilisearch-js#1493

Merged

gmourier approved these changes May 17, 2023

View reviewed changes

irevoire closed this May 22, 2023

irevoire deleted the fetch-documents branch May 22, 2023 10:01

bidoubiwa mentioned this pull request May 23, 2023

Add get documents filter field for ms v1.2 meilisearch/meilisearch-rust#473

Merged

alallema mentioned this pull request May 23, 2023

Adds filter to the parameters for GetDocuments - Meilisearch v1.2 meilisearch/meilisearch-go#439

Merged

This was referenced Jun 5, 2023

Get documents by filter meilisearch/meilisearch-dotnet#432

Closed

Get documents by filter meilisearch/meilisearch-java#613

Closed

Get documents by filter meilisearch/meilisearch-swift#385

Open

Get documents by filter nutshell-lab/meilisearch-ex#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify the new fetch documents route #234

Specify the new fetch documents route #234

irevoire commented May 4, 2023 •

edited by gmourier

Loading

github-actions bot commented May 4, 2023 •

edited

Loading

macraig left a comment

gmourier commented May 5, 2023 •

edited

Loading

irevoire commented May 5, 2023

guimachiavelli May 9, 2023

irevoire May 9, 2023

guimachiavelli May 9, 2023 •

edited

Loading

macraig May 9, 2023

guimachiavelli May 10, 2023

guimachiavelli May 11, 2023

brunoocasali May 12, 2023 •

edited

Loading

gmourier May 15, 2023

brunoocasali May 15, 2023

irevoire May 16, 2023

gmourier left a comment

irevoire commented May 22, 2023

Specify the new fetch documents route #234

Specify the new fetch documents route #234

Conversation

irevoire commented May 4, 2023 • edited by gmourier Loading

Summary

Changes

Misc

github-actions bot commented May 4, 2023 • edited Loading

Added (1)

Modified (1)

macraig left a comment

Choose a reason for hiding this comment

gmourier commented May 5, 2023 • edited Loading

irevoire commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guimachiavelli May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoocasali May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmourier left a comment

Choose a reason for hiding this comment

irevoire commented May 22, 2023

irevoire commented May 4, 2023 •

edited by gmourier

Loading

github-actions bot commented May 4, 2023 •

edited

Loading

gmourier commented May 5, 2023 •

edited

Loading

guimachiavelli May 9, 2023 •

edited

Loading

brunoocasali May 12, 2023 •

edited

Loading