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

Delete documents by filter #9

Open
1 task
brunoocasali opened this issue Jun 7, 2023 · 1 comment
Open
1 task

Delete documents by filter #9

brunoocasali opened this issue Jun 7, 2023 · 1 comment
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@brunoocasali
Copy link
Contributor

⚠️ This issue is generated, it means the examples and the namings do not necessarily correspond to the language of this repository.
Also, if you are a maintainer, please add any clarification and instructions about this issue.

Sorry if this is already wholly/partially implemented. Feel free to let me know about the state of this issue in the repo.

Related to meilisearch/integration-guides#261


⚠️ This issue is part of the new Meilisearch release v1.2

New implementation

Related to:

It allows the user to use a filter expression to remove documents.

Since there is already a method called deleteDocuments(uids) in the SDKs, and this method is calling the route POST /indexes/:index_uid/documents/delete-batch, the requirements to implement the new way is to create a conditional internally to check the presence of the new parameter filters and then call the new method POST /indexes/:index_uid/documents/delete.
When the developer uses the old ids argument, it should keep calling the old implementation.

This will avoid any silent failures the users may find.

For example, in the Ruby SDK:

# Public: Delete documents from an index
#
# documents_ids - An array with document ids (deprecated, optional)
# filter - A hash containing a filter that should match documents. Available ONLY with Meilisearch v1.2 and newer (optional)
# 
# Returns a Task object.
def delete_documents(documents_ids = nil, filter: nil)
  if documents_ids
    http_post "/indexes/#{@uid}/documents/delete-batch", documents_ids
  else
    http_post "/indexes/#{@uid}/documents/delete", { filter: filter }
  end
end

Extra: Add inline documentation for the method, explaining the availability of the filter syntax only for Meilisearch v1.2 and newer.

Extra: Mark the document_ids parameter as deprecated. eg. @Deprecated('migration') on Java/Dart, Obsolete on C#, etc. Add a library if necessary.

Extra: Add a try/catch to detect the error and give the user a hint about the possibility of a version mismatch between the SDK version and the instance version. Check this PR for an example in JavaScript: meilisearch/meilisearch-js#1492

Extra: General recommendations:

  • For languages where method overloading is available, use the overloading.
  • For languages where a builder is available, use the builder pattern.

Todo:

  • Implement the deleteDocuments new behavior keeping the old behavior when possible.
@brunoocasali
Copy link
Contributor Author

@BlitzBanana can you add the labels enhancement / good first issue to this issue?

Also, I put the exact same issue as the other repositories here, but I think the implementation will be simpler because the name Meilisearch.Document.delete/3 is free to be used. Just a simple wrapper will close the deal.

@BlitzBanana BlitzBanana added enhancement New feature or request good first issue Good for newcomers labels Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants