Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply tasks changes #310

Merged
merged 3 commits into from
Jul 6, 2022
Merged

Apply tasks changes #310

merged 3 commits into from
Jul 6, 2022

Conversation

alallema
Copy link
Contributor

@alallema alallema commented Jun 30, 2022

Pull Request

Related to: meilisearch/integration-guides#205

What does this PR do?

Breaking because enforces the users to use Meilisearch v0.28.0

Changes

  • Remove GET /indexes/:indexUid/tasks. Use GET /tasks?indexUid=:indexUid instead.
  • Remove GET /indexes/:indexUid/tasks/:taskUid. Use GET /tasks/:taskUid instead.
  • Possibility to filter the results:
    • In the endpoint GET /tasks we can filter by: type, status and indexUid.
    • We need to join the array values when filter by status like this: enqueued,processing.
  • Index.GetTask now use GET /tasks/:task_uid
  • Index.GetTasks now use GET/tasks?indexUid=movies
  • Rename task uid field to taskUid.
  • Index.GetTasks accept pagination metadata, added limit (default: 20), from.
  • Client.GetTasks accept pagination metadata, added limit (default: 20), from.

@alallema alallema force-pushed the tasks_changes branch 2 times, most recently from 25b71ff to 4807eec Compare June 30, 2022 12:42
@alallema alallema marked this pull request as draft June 30, 2022 12:45
@alallema alallema requested a review from brunoocasali June 30, 2022 13:22
@alallema alallema marked this pull request as ready for review June 30, 2022 15:24
@@ -62,11 +62,11 @@ search_post_1: |-
get_task_1: |-
client.GetTask(1);
get_all_tasks_1: |-
client.GetTasks();
client.GetTasks(nil);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if this is the best way to handle that? I'm not sure if this will be so "intuitive" to the users.

PS: I know there are no optional args in go, so maybe they have their own way to handle that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use variadic parameters but I don't think it's a good way to handle it, even if it works it's made for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it could work but not for every case unfortunately

client.go Outdated Show resolved Hide resolved
client.go Outdated
acceptedStatusCodes: []int{http.StatusOK},
functionName: "GetTasks",
}
if param != nil && param.Limit != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should allow the user to pass 0 as the limit, since the Meilisearch engine allows it either ":)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but for that, I should declare limit as *int which would force the user to initialize his struct in several lines such as:

    limit = 10
    query: &TasksQuery{
        Limit: *limit,
    }

And I think for this specific case when limit = 0 there returns an empty return array so I'm not sure that the inconvenience is worth it

client.go Outdated Show resolved Hide resolved
index.go Outdated Show resolved Hide resolved
index.go Outdated Show resolved Hide resolved
index.go Outdated
acceptedStatusCodes: []int{http.StatusOK},
functionName: "GetTasks",
}
if param != nil && param.Limit != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to re-use the code from client.go and index.go?

client.go Outdated
req.withQueryParams["status"] = param.Status
}
if param != nil && param.Type != "" {
req.withQueryParams["type"] = param.Type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can define the map inline, and after that, remove the nulls or empties like this:

map[string]string{
   "type": len(param.IndexUID) != 0 ? strings.Join(param.IndexUID, ",") : nil, 
   "limit": ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot put logic inline in a struct declaration in go or I don't know how

@brunoocasali
Copy link
Member

@alallema I noticed there is some linter issues to be fixed as well, I'm not sure if you'll do that in this PR or not :)

@alallema alallema requested a review from brunoocasali July 5, 2022 11:22
@alallema alallema merged commit 9ecebb7 into bump-meilisearch-v0.28.0 Jul 6, 2022
@alallema alallema deleted the tasks_changes branch July 6, 2022 15:30
@alallema alallema added the breaking-change The related changes are breaking for the users label Jul 11, 2022
bors bot added a commit that referenced this pull request Jul 11, 2022
304: Changes related to the next Meilisearch release (v0.28.0) r=alallema a=meili-bot

Related to this issue: meilisearch/integration-guides#205

This PR:
- gathers the changes related to the next Meilisearch release (v0.28.0) so that this package is ready when the official release is out.
- should pass the tests against the [latest pre-release of Meilisearch](https://github.com/meilisearch/meilisearch/releases).
- might eventually contain test failures until the Meilisearch v0.28.0 is out.

⚠️ This PR should NOT be merged until the next release of Meilisearch (v0.28.0) is out.

_This PR is auto-generated for the [pre-release week](https://github.com/meilisearch/integration-guides/blob/master/guides/pre-release-week.md) purpose._

Done:
- #306 
- #305
- #310
- #311
- #312
- #313
- #314
- #315 
- #317
- #321 

Co-authored-by: meili-bot <[email protected]>
Co-authored-by: Clémentine Urquizar <[email protected]>
Co-authored-by: Amélie <[email protected]>
Co-authored-by: alallema <[email protected]>
bors bot added a commit that referenced this pull request Jul 11, 2022
324: Update version for the next release (v0.20.0) r=alallema a=alallema

This version makes this package compatible with Meilisearch v0.28.0 🎉
Check out the changelog of [Meilisearch v0.28.0](https://github.com/meilisearch/meilisearch/releases/tag/v0.28.0) for more information on the changes.

## 💥 Breaking changes

- `Index.Search` changes in the response fields: #306
    - `nbHits` replaced with `estimatedTotalHits`
    - `exhaustiveNbHits` is deleted
    - `exhaustiveFacetsCount` is deleted
  - `facetsDistribution` response parameter is renamed `facetDistribution`.
- `Index.Search` changes in the request parameters:  #306
  - `matches` renamed `showMatchesPosition`
  - `facetsDistribution` request parameter is renamed `facets`.
- `Index.GetDocuments()` and `Index.GetDocument()` request parameters: #314
  - `attributesToRetrieve` replaced with `fields`.
- `Index.GetTasks()` has additional parameters for filtering: `type`, `status` and `indexUid`. #310
- All asynchronous methods now return a struct`TaskInfo` instead of 'Task' like `AddDocuments` or `CreateIndex`. #310
- `Index.GetTasks` and `Client.GetTasks` accept pagination metadata, added `limit` (default: 20), `from`. #310
- `Client.GetAllIndexes` and `Client.GetAllRawIndexes` now returns an `IndexesResults` struct containing the following fields: `Results`, `Limit`, `Offset`, `Total`. #312 
- `Client.GetAllIndexes` accept pagination metadata, added `limit` (default: 20) and `offset` (default: 0). #312
- The `IndexUid` field in both `TaskInfo` and `Task` can be nil Update tasks routes #313
- `Index.GetDocuments` and `Client.GetDocuments` now returns an `DocumentsResults` struct containing the following fields: `Results`, `Limit`, `Offset`, `Total`. #314
- `Client.GetDumpStatus` has been removed #311
- `Client.CreateDump()` now returns an `TaskInfo` #321 
- `Client.GenerateTenantToken(APIKeyUID string, SearchRules map[string]interface{}, Options *TenantTokenOptions)` has now a mandatory `APIKeyUID` parameter which should contain the uid of a specific API key. #315
- `Index.GetDocuments` now accepts pagination parameters: `limit` (default: 20) and `offset` (default: 0). #314
- `Client.GetKeys` accept pagination metadata, added `limit` (default: 20) and `offset` (default: 0). #313
- `Client.UpdateKey` now can just update the `Description` and/or the `Name`. #313
- `Key` now has an additional `Name` field. #313

## 🚀 Enhancements


- `Client.GetKeys(param *KeysQuery)` can now also find keys based on their key uid. #313
- `Client.CreateKey(request *Key)` lets you specify a custom uid (optionally) to create a new Key. #313

Thanks again to `@alallema`  ! 🎉

Co-authored-by: alallema <[email protected]>
Co-authored-by: Amélie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants