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

[BREAKING] Tasks Parity #425

Merged
merged 9 commits into from
Oct 11, 2023
Merged

Conversation

Sherlouk
Copy link
Collaborator

@Sherlouk Sherlouk commented Sep 27, 2023

Closes #363
Closes #366
Closes #368
Closes #254

User/Client Facing

  • Introduces Cancel Tasks API
  • Introduces Delete Tasks API
  • Provides compiler-safe access to task types
    • Utilising a future-proof solution avoiding errors should new task types be added without Swift support
  • Provides compiler-safe access to task result details
    • Utilising a future-proof solution avoiding errors should new details be added without Swift support
  • Grouped task related models into own directory
  • Uses Date objects instead of String for dates within task models
  • Added missing fields to existing task models
  • Fixed error caused by global tasks
  • Uses compiler-safe types in task query to improve safety
  • Adds total to task results response (replacing Add total to TasksResult #412)
  • Removed all instances of try! (force try) to prevent crashes in test runner

Breaking Changes

⚠️ This is a breaking change. Even with something as simple as moving to a enum for TaskType requires changes to clients. This PR looks to bring all task changes together meaning that clients can upgrade to this new and safe API design in one go.

  • Task.Status has a new value .canceled (handle in any switch cases)
  • TaskType replaces a previous String based API (use .description to access String)
  • TasksQuery uses compiled types instead of String APIs (example: "indexUpdate" is now .indexUpdate)
  • enqueuedAt now uses Date instead of ISO-8601 String

Parity

Reviewing documentation available on MeiliSearch's website, this PR brings the Tasks API up to 100% parity with existing features. It resolves many errors caused by the previous design (both architecturally but also instability in the current release), and provides future-proofness against further scope within the tasks API.

Personal

As I am developing my own Swift based application for monitoring MeiliSearch (and it's tasks), this branch has been extremely helpful for me in exploring jobs and being able to cancel them and performing my own housekeeping.

The type safety has been delightful and actually detected a bug in my original implementation which had a typo in a task name.

@Sherlouk
Copy link
Collaborator Author

Sherlouk commented Sep 27, 2023

There is likely to be some code samples which I need to update/add as part of this PR too (relating partially to #424)

This will also need a rebase too. However, I'd love a review once time permits as I do not plan on making any further changes to the API unless called upon to.

No functionality change, just an improvement to tests to prevent the test runner from crashing (and less complaints from SwiftLint)
Sometimes it would fail stating the task was still there, I believe this was because it hadn't finished deleting. Hopefully it'll now wait.

Passed 50 iterations locally.
@Sherlouk
Copy link
Collaborator Author

The sole test which is failing is testDeleteAllDocuments - this is not failing due to changes made in this PR. I can reproduce it locally and is caused by an extra "test movie" sitting in the index. This is added by other integration tests, and so depending on the order in which the tests are ran can occasionally fail.

@curquiza
Copy link
Member

bors try

meili-bors bot added a commit that referenced this pull request Sep 28, 2023
@@ -133,7 +133,7 @@ async_guide_filter_by_ids_1: |-
}
}
async_guide_filter_by_statuses_1: |-
client.getTasks(params: TasksQuery(statuses: ["failed", "canceled"])) { result in
client.getTasks(params: TasksQuery(statuses: [.failed, .canceled])) { result in
Copy link
Member

Choose a reason for hiding this comment

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

Is this PR breaking so?
Not an issue for me to break, we are not stable yet. It's to apply the right versioning and provide a clear changelog

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a breaking change, yes. The changes have been documented in the initial PR description, in case you want to copy them into a changelog 😀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For what it's worth, and looking at tickets in the backlog, I'm intentionally looking to prioritise breaking changes now in order to get us to a stable position quicker.

Copy link
Member

Choose a reason for hiding this comment

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

Ok let's merge it with breaking then! thank you for the help of the changelog ❤️

@meili-bors
Copy link
Contributor

meili-bors bot commented Sep 28, 2023

try

Build failed:

@Sherlouk Sherlouk changed the title MeiliSearch Tasks Parity [BREAKING] Tasks Parity Sep 28, 2023
Other solutions include:

- fetching all documents and asserting total count
- deleting all documents before adding again (expect 8)
- create a new index just for this one test
@curquiza curquiza added breaking-change The related changes are breaking for the users enhancement New feature or request labels Oct 11, 2023
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Oct 11, 2023

Build succeeded:

@meili-bors meili-bors bot merged commit 978cd07 into meilisearch:main Oct 11, 2023
6 checks passed
@Sherlouk Sherlouk mentioned this pull request Nov 3, 2023
1 task
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 enhancement New feature or request
Projects
None yet
2 participants