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

Add ability to delete documents by filter #756

Merged
merged 7 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions meilisearch/errors.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
from __future__ import annotations

import json
from functools import wraps
from typing import TYPE_CHECKING, Any, Callable

from requests import Response

if TYPE_CHECKING:
from meilisearch.client import Client
from meilisearch.index import Index
from meilisearch.task import TaskHandler


class MeilisearchError(Exception):
"""Generic class for Meilisearch error handling"""
Expand Down Expand Up @@ -54,3 +61,15 @@ class MeilisearchTimeoutError(MeilisearchError):

def __str__(self) -> str:
return f"MeilisearchTimeoutError, {self.message}"


def version_error_hint_message(func: Callable) -> Any:
@wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
try:
return func(*args, **kwargs)
except MeilisearchApiError as exc:
exc.message = f"{exc.message}. Hint: It might not be working because you're not up to date with the Meilisearch version that {func.__name__} call requires."
raise exc

return wrapper
36 changes: 34 additions & 2 deletions meilisearch/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from meilisearch._httprequests import HttpRequests
from meilisearch.config import Config
from meilisearch.errors import version_error_hint_message
from meilisearch.models.document import Document, DocumentsResults
from meilisearch.models.index import Faceting, IndexStats, Pagination, TypoTolerance
from meilisearch.models.task import Task, TaskInfo, TaskResults
Expand Down Expand Up @@ -729,12 +730,15 @@ def delete_document(self, document_id: Union[str, int]) -> TaskInfo:
)
return TaskInfo(**response)

def delete_documents(self, ids: List[Union[str, int]]) -> TaskInfo:
def delete_documents(
self,
ids: List[Union[str, int]],
Copy link
Member

Choose a reason for hiding this comment

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

Following the new approach from the central issue, the idea should be to move the delete_documents_by_filter to the same delete_documents by adding a new argument filter to the method. Also, I don't know how Python handles deprecations, but the ids argument should be deprecated in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brunoocasali I considered this new approach, but think it could be confusing. If a user sends both ids and a filter the filter will be silently ignored. Is this OK? Based on the message this means eventually you will only be able to delete by filter and not id?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct!

I understand that could be strange at first sight, but the idea is to add the functionality and not break the current usage (without creating a new method just for a new use case).

Also, I understand that our SDKs (most of them) are not stable, but that is not the foresee I have for them. I believe breakings are always a bad sign, especially when they are not "expected" please remember the context explained here.

Since those SDKs are unstable, we can eliminate the mixed behavior when we move them to a v1 (not necessarily being directly related to the engine being on v2). Still, the cool thing is that we introduced warnings/deprecations to the users before doing it, so everyone is "ready".

) -> TaskInfo:
"""Delete multiple documents from the index.

Parameters
----------
list:
ids:
List of unique identifiers of documents.

Returns
Expand All @@ -754,6 +758,34 @@ def delete_documents(self, ids: List[Union[str, int]]) -> TaskInfo:
)
return TaskInfo(**response)

@version_error_hint_message
def delete_documents_by_filter(
self, filter: Union[str, List[Union[str, List[str]]]] # pylint: disable=redefined-builtin
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pylint doesn't like the parameter being named filter so I set it to ignore this error. If you would rather call it something else I can do that. If you want to rename do you have any suggestions for a name? I couldn't think of a good one.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, it's ok to keep filter. Is this the same issue with filters in the plural?
I don't think we should give it any other name than the one it has

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pylint would be ok with filters. The reason pylint doesn't like filter is because python has a built in filter function so it is trying to prevent overlap. In this case I can't think of a time where naming it filter would actually cause any issues though so I think either ignoring the pylint error or renaming to filters are both fine options, which ever you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, there is no problem to keep the same name as in the Meilisearch API. I will merge it

) -> TaskInfo:
"""Delete documents from the index by filter.

Parameters
----------
filter:
The filter value information.

Returns
-------
task_info:
TaskInfo instance containing information about a task to track the progress of an asynchronous process.
https://docs.meilisearch.com/reference/api/tasks.html#get-one-task

Raises
------
MeilisearchApiError
An error containing details about why Meilisearch can't process your request. Meilisearch error codes are described here: https://docs.meilisearch.com/errors/#meilisearch-errors
"""
response = self.http.post(
f"{self.config.paths.index}/{self.uid}/{self.config.paths.document}/delete",
body={"filter": filter},
)
return TaskInfo(**response)

def delete_all_documents(self) -> TaskInfo:
"""Delete all documents from the index.

Expand Down
2 changes: 0 additions & 2 deletions meilisearch/models/document.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
from __future__ import annotations

from typing import Any, Dict, Iterator


Expand Down
21 changes: 20 additions & 1 deletion tests/errors/test_api_error_meilisearch.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import requests

import meilisearch
from meilisearch.errors import MeilisearchApiError
from meilisearch.errors import MeilisearchApiError, version_error_hint_message
from tests import BASE_URL, MASTER_KEY


Expand All @@ -33,3 +33,22 @@ def test_meilisearch_api_error_no_code(mock_post):
with pytest.raises(MeilisearchApiError):
client = meilisearch.Client(BASE_URL, MASTER_KEY + "123")
client.create_index("some_index")


def test_version_error_hint_message():
mock_response = requests.models.Response()
mock_response.status_code = 408

class FakeClass:
@version_error_hint_message
def test_method(self):
raise MeilisearchApiError("This is a test", mock_response)

with pytest.raises(MeilisearchApiError) as e:
fake = FakeClass()
fake.test_method()

assert (
"MeilisearchApiError. This is a test. Hint: It might not be working because you're not up to date with the Meilisearch version that test_method call requires."
== str(e.value)
)
14 changes: 14 additions & 0 deletions tests/index/test_index_document_meilisearch.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ def test_delete_documents(index_with_documents):
index.get_document(document)


def test_delete_documents_by_filter(index_with_documents):
index = index_with_documents()
response = index.update_filterable_attributes(["genre"])
index.wait_for_task(response.task_uid)
response = index.get_documents()
assert "action" in ([x.__dict__.get("genre") for x in response.results])
response = index.delete_documents_by_filter("genre=action")
index.wait_for_task(response.task_uid)
response = index.get_documents()
genres = [x.__dict__.get("genre") for x in response.results]
assert "action" not in genres
assert "cartoon" in genres


def test_delete_all_documents(index_with_documents):
"""Tests deleting all the documents in the index."""
index = index_with_documents()
Expand Down