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

Only shows the full message when the user have configured the settings for it #2140

Closed
4 tasks done
brunoocasali opened this issue Feb 3, 2022 · 8 comments · Fixed by meilisearch/milli#461 or meilisearch/milli#468
Assignees
Labels
enhancement New feature or improvement milli Related to the milli workspace v0.27.0 PRs/issues solved in v0.27.0
Milestone

Comments

@brunoocasali
Copy link
Member

brunoocasali commented Feb 3, 2022

Describe the bug
When I provide a search request with a sort option, I should receive the error with the available sortable attributes but I'm getting an empty list:

{
    "message": "Attribute `name` is not sortable. Available sortable attributes are: ``.",
    "code": "invalid_sort",
    "type": "invalid_request",
    "link": "https://docs.meilisearch.com/errors#invalid_sort"
}

My request: GET http://localhost:7700/indexes/rails_e661e74611d4d7b8_Color_development/search?sort=name:asc
My index settings:

{
    "displayedAttributes": [
        "*"
    ],
    "searchableAttributes": [
        "name"
    ],
    "filterableAttributes": [
        "short_name"
    ],
    "sortableAttributes": [],
    "rankingRules": [
        "words",
        "typo",
        "proximity",
        "attribute",
        "sort",
        "exactness",
        "hex:asc"
    ],
    "stopWords": [],
    "synonyms": {},
    "distinctAttribute": null
}

I know that I didn’t configure the sortableAttributes, so the question I make is, should we receive a different message? Or is ok to keep the current behavior? Because when I read it the first time, I really thought that something was wrong on my end.

An idea could be having a conditional to show up when the user doesn't have any sortable attributes configured.

{
    "message": "Attribute `name` is not sortable. This index doesn't have configured sortable attributes",
    "code": "invalid_sort",
    ...
}

I've spotted the code here: https://github.com/meilisearch/milli/blob/8dff08d772db7ee2bccfed4a75b943ec8be455a2/milli/src/error.rs#L254 but I wasn’t sure if I need to post the issue here or in milli repo, please let me know :D

Meilisearch version: v0.25.2

Additional context
Running in the dockerized environment (using the official docker image)


TODO

@gmourier
Copy link
Member

gmourier commented Feb 7, 2022

Hi @brunoocasali, I find this type of message in this case also clearer!

@MarinPostma
Copy link
Contributor

MarinPostma commented Feb 7, 2022

@gmourier So you like the new message idea?
@brunoocasali yup, that's right there, how would you like a first core contribution? ;)

@gmourier
Copy link
Member

gmourier commented Feb 7, 2022

@MarinPostma Yes! I think it guides the user better in this case. 👌

@curquiza
Copy link
Member

curquiza commented Mar 8, 2022

Re-opening since it's still not fixed on Meilisearch side.
I updated the issue description accordingly with the next steps

@curquiza curquiza reopened this Mar 8, 2022
@gmourier
Copy link
Member

gmourier commented Mar 8, 2022

Don't forget to add the same variant on filter when the filterableAttributes are empty 🤓 I will spec the 2 new variants for v0.27.0

@gmourier
Copy link
Member

gmourier commented Mar 9, 2022

Specified here meilisearch/specifications#125

bors bot added a commit to meilisearch/milli that referenced this issue Mar 16, 2022
468: Add a new error message when the filterableAttributes are empty r=Kerollmops a=brunoocasali

Fixes meilisearch/meilisearch#2140

Is there a good way to reduce de duplication here? Maybe adding a shared function? I don't know the best and idiomatic way to do that, I appreciate any tip!

Another doubt is related to the duplication of the calling:

```rs
// filter.rs:373
FilterError::AttributeNotFilterable {
    attribute,
    filterable: filterable_fields.into_iter().collect::<Vec<_>>().join(" "),
},
```

and

```rs
// filter.rs:424
return Err(point[0].as_external_error(FilterError::AttributeNotFilterable {
    attribute: "_geo",
    filterable: filterable_fields.into_iter().collect::<Vec<_>>().join(" "),
}))?;
```

I think we could make the `filterable_fields.into_iter().collect::<Vec<_>>().join(" ")` directly into the error handling like the sortable error. I made it into the last commit, if this is something to avoid, let me know and I can remove it :)

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
@curquiza
Copy link
Member

curquiza commented Mar 16, 2022

Reopening since it's not fixed on meilisearch's side but only on milli's side

@curquiza curquiza reopened this Mar 16, 2022
@curquiza curquiza added the milli Related to the milli workspace label Mar 16, 2022
@curquiza
Copy link
Member

Milli was bumped in #2244, with milli v0.24.0 containing the required change for the new error message. We can close this issue. The change will be effective in v0.27.0.

@curquiza curquiza added the v0.27.0 PRs/issues solved in v0.27.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement milli Related to the milli workspace v0.27.0 PRs/issues solved in v0.27.0
Projects
None yet
4 participants