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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some settings can recieve null as a value while they shouln't #2929

Closed
bidoubiwa opened this issue Oct 18, 2022 · 11 comments
Closed

Some settings can recieve null as a value while they shouln't #2929

bidoubiwa opened this issue Oct 18, 2022 · 11 comments
Assignees

Comments

@bidoubiwa
Copy link
Contributor

Describe the bug
Some settings, I only have two examples but there are probably more, can receive the value null during an update while it should raise an error.

Example: See spec

馃敶 Sending a value with a different type than Boolean for the enabled field returns an bad_request error.
馃敶 Sending a value with a different type than Array of String for the disableOnAttributes field returns an bad_request error.

Upon receiving null as a value, they are reset to their default value

To Reproduce
Steps to reproduce the behavior:

  1. Create an index
  2. Update the settings { typoTolerance: { enabled: false }}
  3. Check if is indeed has false as a value
  4. Update the settings { typoTolerance: { enabled: null }}
  5. Check the settings, enabled is now true

Expected behavior
Should follow the spec, either the spec should change or the behavior.

Meilisearch version: v0.29.1

@bidoubiwa bidoubiwa changed the title Some settings are set upon recieving null as a value while they shouln't Some settings can recieve null as a value while they shouln't Oct 18, 2022
@curquiza curquiza added this to the v0.30.0 milestone Oct 20, 2022
@curquiza curquiza added the bug Something isn't working as expected label Oct 20, 2022
@curquiza
Copy link
Member

Thanks for the report @bidoubiwa
Will try to fix this for v0.30.0 (during pre-release) or for v1 if too risky to fix this during a pre-release phase

@guimachiavelli guimachiavelli added the impacts docs This issue involves changes in the Meilisearch's documentation label Oct 24, 2022
@guimachiavelli
Copy link
Member

guimachiavelli commented Oct 24, 2022

We will probably want to clarify the behaviour when assigning null to settings in the docs. I took the liberty of adding the impacts-docs myself, if that's alright.

@curquiza
Copy link
Member

discussed with @gmourier -> the spec should be followed, not modified.

@Pranav-yadav
Copy link
Contributor

@curquiza I can work on this if I know which settings are affected?
maybe a list with permalinks to all of such settings would be appreciated :)

@curquiza
Copy link
Member

curquiza commented Nov 2, 2022

Hello @Pranav-yadav

We don't know exactly which ones are concerned, this issue involves also a small investigation by testing all the settings
The list of all the settings present in Meilisearch are described in the docs here

@Pranav-yadav
Copy link
Contributor

Ohh! Then, I shall come to this after tackling easier ones... hahaha

@curquiza curquiza removed good first issue Good for newcomers hacktoberfest labels Nov 3, 2022
@ManyTheFish
Copy link
Member

ManyTheFish commented Nov 8, 2022

Are we sure about this? Considering it as a bug and fixing this issue would forbid the user to reset any settings under typoTolerance settings. This means that the only way to reset a part of the typoTolerance settings is to:

  1. call DELETE /indexes/:uid/settings/typo-tolerance to reset the whole typoTolerance settings
  2. set the settings that we didn't want to reset

poke @gmourier

@dureuill
Copy link
Contributor

dureuill commented Nov 9, 2022

Discussed with @Kerollmops, @ManyTheFish and @gmourier:

  • The current implementation behavior of resetting to the default value when sending null is the desired behavior, so I will send a PR so that the specification reflects the implementation behavior
  • I will also check that the behavior is consistent in the implementation at any level of settings (top level, when setting eg typoTolerance to null, when setting minWordSizeForTypos to null, and also for faceting and pagination).

@curquiza
Copy link
Member

curquiza commented Nov 9, 2022

Ok, thanks for the return @dureuill

I close this issue then!

@curquiza curquiza closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2022
@curquiza curquiza removed bug Something isn't working as expected impacts docs This issue involves changes in the Meilisearch's documentation labels Nov 9, 2022
@curquiza
Copy link
Member

curquiza commented Nov 9, 2022

Sorry, I reopen because investigation needed before closing, sorry!

@dureuill
Copy link
Contributor

  • I will also check that the behavior is consistent in the implementation at any level of settings (top level, when setting eg typoTolerance to null, when setting minWordSizeForTypos to null, and also for faceting and pagination).

Checked the behavior. It is consistent at any level of settings, with a null value always resetting the value to the default.

We can close this task.

@curquiza curquiza removed this from the v0.30.0 milestone Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants