Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Reset Stop-Words and Synonyms Settings With A Null Value #47

Merged
merged 6 commits into from
Jun 24, 2021

Conversation

gmourier
Copy link
Member

@gmourier gmourier commented Jun 10, 2021

Summary

All Sub Settings POST endpoint accepts a null value in the http request body to reset the engine configuration to the default values. This is not the case for Stop-words and Synonyms settings endpoints.

Motivation

This specification is written to make the above mentioned routes consistent with the others.

@gmourier gmourier added the Ready For Review Feature specification must be reviewed. label Jun 10, 2021
gmourier and others added 3 commits June 10, 2021 11:38
@ManyTheFish
Copy link
Member

I don't like the idea, I don't think that null is considered equivalent to [] in everybody's mind. It could be interpreted as the absence of the field.

I would prefer that the user explicitly set the field to [] if he wants to reset it.

@curquiza
Copy link
Member

curquiza commented Jun 10, 2021

The other settings (for example seachableAttributes and displayedAttribtutes) already accept null as value to reset the settings, so to fill the settings to [].
Even if this possibility is not documented, we know it is already used by some users: we saw it in the integration issues we had. So for me, this spec is a good idea:

  • the users that don't have the habit to use null are still not impacted and the feature is still not documented.
  • the users who used null to reset can now do it consistently for synonyms and stop-words.
  • put [] is still available to reset the settings, and the DELETE routes as well.

@gmourier
Copy link
Member Author

gmourier commented Jun 11, 2021

Sorry @ManyTheFish, I forgot to answer you I got caught elsewhere. Your proposal goes in the direction of an API that can be fully documented. Personally, I'm not a fan of having a hidden workaround that is not documented. The API format specification will solve this kind of case. However, it is important to keep in mind that sometimes we have to make deviations to keep a good DX.

@curquiza, it's true for the 0.21 release, we cut in the fat to advance towards the release. However, your answer makes me wonder about two questions that we can deal with in future releases.

  1. Integrations could accept a null value in the parameters of the update sub-setting method and transform it into [] or {} or even call the DELETE method in this case. So the update method will be a facade in this particular case.
  2. If I understand correctly people don't call the reset methods from integrations? So do we need to keep these? Analytics on the feature level will tell us that to make a decision about that.

@curquiza
Copy link
Member

curquiza commented Jun 11, 2021

  1. If this feature is a problem from your POV, moving it to the SDKs is just move the "problem" into 8 different repos (so duplicate the code) instead of one. Even in the SDKs, it will not be documented to avoid making the README and the docs heavier. So, from my POV, either we keep it everywhere, or not.
  2. The majority call the reset method. But some of them use the update method as well. That's why I thought it's a nice feature to have for people noticing it because it's intuitive for them, without disturbing people that classically use the reset route. So it is really worthy to remove this feature? But if you would rather remove this possibility, I'm following 🙂

However I would add I've never seen someone reporting they have been confused about accepting the null value at the moment. So re-doing all this settings behavior instead of just making it consistent might be a lot of work in the transplant size for nothing (again, regarding the absence of current complaint).

@gmourier
Copy link
Member Author

gmourier commented Jun 11, 2021

Agreed on the fact that the first proposal will shift the effort to the X integration libs. Probably not a good idea, as you pointed out this is something that is important to keep in mind. I don't really see a problem with having this null value in the POST even though it's something that seems unusual and might surprise some developers as well. As I said earlier, making some deviations to keep a good DX is sometimes necessary.

We need factual stats on the use of these routes to talk about this again. The fact is that we have two routes that accomplish the same thing. Is this a problem at the moment? You tell me no from the user feedback you get! The advantage is that we will have a consistent behavior on the whole scope of the sub-settings routes with this specification.

It's not an immediate priority anyway if we have no complaint about it and no quantitative statistics. Let's move forward on this specification as such 😉

@curquiza
Copy link
Member

curquiza commented Jun 11, 2021

Even if your decision is made, I want to share this smart way of usage 😊 It might be intersting.
Some users (sorry I cannot find where their feedback was, since it was several months ago) use the null value when calling the general update route (POST /indexes/:uid/settings ) in order to update and reset setting at the same time!

Example:
Calling POST /indexes/:uid/settings with

{
    "searchableAttributes": null,
    "displayedAttributes": [
        "title"
    ]
}

will reset the searchable attributes and will update the displayed attributes at the same HTTP call.

If we remove the possibility to use the null value to reset, it will remove this possibility.

@ManyTheFish
Copy link
Member

Even if your decision is made, I want to share this smart way of usage 😊 It might be intersting.
Some users (sorry I cannot find where their feedback was, since it was several months ago) use the null value when calling the general update route (POST /indexes/:uid/settings ) in order to update and reset setting at the same time!

Example:
Calling POST /indexes/:uid/settings with

{
    "searchableAttributes": null,
    "displayedAttributes": [
        "title"
    ]
}

will reset the searchable attributes and will update the displayed attributes at the same HTTP call.

If we remove the possibility to use the null value to reset, it will remove this possibility.

What about:

Calling POST /indexes/:uid/settings with

{
    "searchableAttributes": [],
    "displayedAttributes": [
        "title"
    ]
}

This would be similar to your example.

@curquiza
Copy link
Member

Calling POST /indexes/:uid/settings with

{
    "searchableAttributes": [],
    "displayedAttributes": [
        "title"
    ]
}

This would be similar to your example.

From my POV, this means "I don't want any searchable attributes, and only title as displayed attributes". It's not a reset.

@ManyTheFish
Copy link
Member

My bad @curquiza, I miss understood the meaning of reset. 😅
You're right.

gmourier and others added 2 commits June 21, 2021 16:10
Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
@gmourier gmourier merged commit 49a4c65 into main Jun 24, 2021
@gmourier gmourier deleted the reset-stop-words-synonyms-settings-with-null branch June 24, 2021 14:15
@gmourier gmourier added Ready To Be Implemented Feature specification is ready to be implemented. and removed Ready For Review Feature specification must be reviewed. labels Jun 24, 2021
@gmourier gmourier added Implemented Feature specification has been implemented. and removed Ready To Be Implemented Feature specification is ready to be implemented. labels Jul 7, 2021
gmourier added a commit that referenced this pull request Aug 17, 2021
* Initialize specification

* Apply suggestions from code review

Co-authored-by: Quentin de Quelen <quentin@meilisearch.com>

* add an example to reset with a null value

* change subtitle

* Update text/0047-reset-stop-words-synonyms-settings-with-null.md

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>

* add part for SDKS

Co-authored-by: Quentin de Quelen <quentin@meilisearch.com>
Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Implemented Feature specification has been implemented. Q2:2021 v0.21
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants