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

Filters parameter can not be empty string #1338

Closed
bidoubiwa opened this issue Apr 15, 2021 · 5 comments
Closed

Filters parameter can not be empty string #1338

bidoubiwa opened this issue Apr 15, 2021 · 5 comments
Labels
bug Something isn't working as expected v0.25.0 PRs/issues solved in v0.25.0
Projects
Milestone

Comments

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Apr 15, 2021

Describe the bug

When trying to do a search with an empty string an error is thrown. The error message is also not really clear.

To Reproduce
Steps to reproduce the behavior:

  1. Make a search
  2. use filters: "" as a search parameter
  3. paf
Error [MeiliSearchApiError]: error parsing filter;  --> 1:1
  |
1 | 
  | ^---
  |
  = expected other

  type: 'MeiliSearchApiError',
  errorCode: 'invalid_filter',
  errorType: 'invalid_request_error',
  errorLink: 'https://docs.meilisearch.com/errors#invalid_filter',
  httpStatus: 400

Expected behavior
Ignore the empty filter as no filter is still a filter.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: IOS
  • meilisearch: v0.20
@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 15, 2021

$ curl -X POST http://localhost:7700/indexes/indexUID/search --data '{ "q": "", "filters": "" }'
{"message":"error parsing filter;  --> 1:1\n  |\n1 | \n  | ^---\n  |\n  = expected other","errorCode":"invalid_filter","errorType":"invalid_request_error","errorLink":"https://docs.meilisearch.com/errors#invalid_filter"}⏎

MeiliSearch version: 0.20.0

@bidoubiwa bidoubiwa reopened this Apr 15, 2021
@curquiza curquiza added the bug Something isn't working as expected label Jul 1, 2021
@curquiza
Copy link
Member

curquiza commented Aug 5, 2021

Checked with @gmourier
It put it as good_first_issue. The expected behavior: not return an error and do not apply any filter.
A test should be added.

This will be fixed in next versions

@curquiza curquiza added good first issue Good for newcomers and removed good first issue Good for newcomers labels Aug 5, 2021
@curquiza curquiza added this to Candidate in Bug triage via automation Aug 10, 2021
@curquiza curquiza moved this from Candidate to Bugs - severity 3 in Bug triage Aug 10, 2021
@curquiza
Copy link
Member

curquiza commented Dec 8, 2021

Fixed in v0.25.0rc0

Test:

  • add:
[
  { "id": 2,    "title": "Pride and Prejudice",                    "author": "Jane Austin",              "genre": "romance",    "price": 3.5 },
  { "id": 456,  "title": "Le Petit Prince",                        "author": "Antoine de Saint-Exupéry", "genre": "adventure" , "price": 10.0 },
  { "id": 1,    "title": "Alice In Wonderland",                    "author": "Lewis Carroll",            "genre": "fantasy",    "price": 25.99 },
  { "id": 1344, "title": "The Hobbit",                             "author": "J. R. R. Tolkien",         "genre": "fantasy" },
  { "id": 4,    "title": "Harry Potter and the Half-Blood Prince", "author": "J. K. Rowling",            "genre": "fantasy" },
  { "id": 42,   "title": "The Hitchhiker's Guide to the Galaxy",   "author": "Douglas Adams" }
]
  • add filterable attributes
  • search
{
    "q": "",
    "filter": ""
}
{
    "hits": [],
    "nbHits": 0,
    "exhaustiveNbHits": false,
    "query": "",
    "limit": 20,
    "offset": 0,
    "processingTimeMs": 2
}

@curquiza curquiza closed this as completed Dec 8, 2021
Bug triage automation moved this from Bugs - severity 3 to Done Dec 8, 2021
@curquiza curquiza added this to the v0.25.0 milestone Dec 8, 2021
@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Dec 8, 2021

An empty filter gives returns no hits? Since there is no filter condition in the string, it should not apply any filter.

With the same logic, an empty query should also return no documents.

Also implementation wise, if you create dynamically your filters you'll have to check if filter === "" and then remove the
filter field of your query parameter.

Maybe I should answer to that in the related specification?

@curquiza
Copy link
Member

curquiza commented Dec 9, 2021

Discussion about what we need to do here meilisearch/milli#422

bors bot added a commit to meilisearch/milli that referenced this issue Dec 9, 2021
422: Prefer returning `None` instead of using an `FilterCondition::Empty` state r=Kerollmops a=Kerollmops

This PR is related to the issue comment meilisearch/meilisearch#1338 (comment) which exhibits the fact that when a filter is known to be empty no results are returned which is wrong, the filter should not apply as no restriction is done on the documents set.

The filter system on the milli side has introduced an Empty state which was used in this kind of situation but I found out that it is not needed and that when we parse a filter and that it is empty we can simply return `None` as the `Filter::from_array` constructor does. So I removed it and added tests!

On the MeiliSearch side, we just need to match on a `None` and completely ignore the filter in such a case.

Co-authored-by: Clément Renault <clement@meilisearch.com>
@curquiza curquiza mentioned this issue Dec 9, 2021
3 tasks
@curquiza curquiza added the v0.25.0 PRs/issues solved in v0.25.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected v0.25.0 PRs/issues solved in v0.25.0
Projects
No open projects
Development

No branches or pull requests

2 participants