Add ability to delete documents by filter

[sanders41]

Pull Request

Related issue

Fixes #<issue_number>

What does this PR do?

• Adds a delete_documents_by_filter method

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

[sanders41]

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.

[alallema]

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

[sanders41]

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.

[alallema]

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

[bidoubiwa]

Very nice ✨

If possible, can it be updated following this comment.

[sanders41]

@bidoubiwa continuing our discussion about the version helper here. If I run delete_documents_by_filter on Meilisearch 1.1 I get a MeilisearchApiError: meilisearch.errors.MeilisearchApiError: MeilisearchApiError. 405 Client Error: Method Not Allowed for url: http://127.0.0.1:7700/indexes/indexUID/documents/delete.

[sanders41]

I also want to get your thoughts on combining delete_documents and delete_documents_by_filter. I can do that, but because delete_documents takes an ids parameter I can't think of a way to do it without a breaking change. I would need to change delete_documents(self, ids: List[Union[str, int]]) to something like delete_documents(self, params: DocumentsDeletionQuery | DocumentsIds).

[sanders41]

And FYI, the tests are failing because 1.2.0-rc.1 was just released, but it hasn't finished publishing yet. I'll re-run once it's available.

[brunoocasali]

@sanders41 I've published new specs to be applied in this new version. Can you check it? (it is on the central issue)

[brunoocasali]

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.

[sanders41]

@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?

[brunoocasali]

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".

[sanders41]

@brunoocasali I updated as requested. I made filter a named only parameter so you can use index.delete_documents(filter="genre=action") rather than index.delete_documents(None, "genre=action"). If you don't like that let me know and I'll change it.

[sanders41]

I was in a hurry when writing my reason for using a named parameter for filter and now realize my reason isn't really clear since index.delete_documents(filter="genre=action") will also work without forcing the named parameter.

The first thing I wanted to prevent is with ids being depreciated it would be natural to assume filter is the first parameter so I didn't want people sending a filter in that spot and getting errors. Since we want to prevent breaking current code I couldn't change the order of the parameters.

The second, and more important, is once ids is remove index.delete_documents(None, "genre=action") will break, but index.delete_documents(filter="genre=action") will continue to work so it will break less code. With that I wanted to force this style.

[bidoubiwa]

Hey @sanders41

We discussed a bit to think about all possible options as we did not agree. Below you have the two design solutions we thought of. Which one do you think is best?

Current behavior

These behavior should NOT be breaking in any of the suggested designs

delete_documents([1,2])
delete_documents(["1","2"])
delete_documents("1")
delete_documents(1)
delete_documents(ids=[1,2])
delete_documents(ids=["1","2"])
delete_documents(ids="1")
delete_documents(ids=1)

Option 1: Named variables

delete_documents(ids=[1,2], filter=["id = 1"]) # filter is ignored
delete_documents([1,2], filter=["id = 1"]) # filter is ignored

delete_documents(None, filter=["id = 1"]) # filter
delete_documents(filter=["id = 1"]) # filter

delete_documents(["id = 1"]) # fails as it is considered a document id

Pros:

• ids expect ids, filter expects filter. Intuitive

Cons:

• when both are provided filter is silently ignored
• On first param, without naming, it is always considered document ids.

Option 2: analyse first parameter type

In this option we introduce filter as being par of a dictionary.

{
    "filter": "id = 1"
}

Based on the type of the first parameter

Use old route if first param is:

• array of number/string
• String

Use new route if first param is:

• Dictionnary

delete_documents([1,2]) # document ids
delete_documents(ids=[1,2]) # document ids
delete_documents({ filter: ["id = 1"]}) # filter
delete_documents(ids={ filter: ["id = 1"]}) # filter

Pros:
* Using a dictionnary lets us add possible new parameter easily in the futur, it follows the implementation of the search route params,
* First parameter can be either documents ids or parameters
Cons:
* ids={ filter: [""]} is valid.

Option 3: Both

It is possible to combine both option but implementation gets messy.

[sanders41]

I slightly lean towards option 1 even though I think it is ultimately a confusing option for the user. With this option I don't like the quasi optional parameters. The are listed as optional but really one or the other is required.

With that said I think options 2's ids={ filter: [""]} is even more confusing since ids and filters are two different things so passing a filter as ids seems even more confusing.

[alallema]

Hi @sanders41,
Thank you for your answer.

I slightly lean towards option 1 even though I think it is ultimately a confusing option for the user.

I kind of agree with you on this but I'm not really sure why. Did you find this solution more idiomatic to Python?
I also wonder if many users use this method with the named parameter because the goal would be to rename the field ids param or parameter so this method won't work and will be replaced by the second one:

delete_documents(ids=1)

delete_documents(param=1)

But it will be a breaking change at some point.

[sanders41]

I have a tendency to used named parameters (even when it isn't required) simply because it makes for less breaking changes. The parameters can be moved around, added to, etc without breaking my code. With this being my tendency, I would probably use delete_documents(filter=["id = 1"]) and this had an influence on my decision because I was thinking when ids gets removed my code would still work. If it gets renamed to param that wouldn't be the case though.

Looking through code bases I probably see an equal amount of using named parameters and positional parameters. One isn't really more idiomatic over the other so unfortunately we can't really use that as the decision point.

The one thing I can say here is Python has the option for specifying both position only arguments and named only arguments. def delete_documents(ids, /) is position only and def delete_documents(*, ids) is named only. I see the named only used all the time. I have never seen position only used. This makes me think other people have the same tendency I do to use the name, but I don't have any data to back that up.

[bidoubiwa]

Hey @sanders41

Since everyone seems to agree, let's go for solution 1 if that's okey for you

[alallema]

Since everyone seems to agree, let's go for solution 1 if that's okey for you

I was about to say the same thing. Agreed

[alallema]

Thank you @sanders41 for the implementation of this release feature and the feedback on it 🚀
LGTM!