Skip to content

Conversation

@matarrese
Copy link
Contributor

Hello guys,
I' m having a look on this problem.
Can I ask which one should be the allowed behaviour?
if the value sent is:
"search.remote.my_cluster.seeds": null
seems that there is a missed match in AbstractScopedSettings:512
if (Regex.simpleMatch(entry, key) && canRemove.test(key)) {
entry value: search.remote.my_cluster.seeds
key value: search.remote.my_cluster.seeds.0
so, the simpleMatch fails and the key is not added as value to be removed.

Which behaviour should be accepted? Error or accepting the straight null value?
I wrote a simple solution that makes the match successful.
I don't know if it can be considered correct, please let me know your thought!
Closes #25953

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna javanna added :Core/Infra/Settings Settings infrastructure and APIs review labels Aug 4, 2017
@rjernst
Copy link
Member

rjernst commented Aug 16, 2017

I don't think this change is correct. If setting "foo": null, it would delete foo.bar if it existed, not just foo.0 etc.

@matarrese
Copy link
Contributor Author

Hi @rjernst , thank you for your answer.
What's about if the second part after the dot must be another digit checked with a specific regular expression?

@rjernst
Copy link
Member

rjernst commented Aug 17, 2017

It would work, but it becomes more costly to do the check. I think the real solution here is to fix the underlying problems with Settings and array value representation, as mentioned in #25953.

@matarrese
Copy link
Contributor Author

matarrese commented Aug 29, 2017

Hi @rjernst , thank you for your update. I have a question
when you say that it should be fixed the underlying problem, is this keySet that should be represented in a different way?
screen shot 2017-08-29 at 12 52 46

@rjernst
Copy link
Member

rjernst commented Aug 29, 2017

is this keySet that should be represented in a different way?

Yes. Right now, the keys of the internal settings map are "special" for arrays. This allows the values to always be String, even for an array setting. The key in this case is appended with ".INDEX" of the array value, so for {"foo":["hello"]}, the map looks like foo.1 -> hello. This is why arrays are so complicated to manipulated in Settings: there are multiple keys that must be adjusted together.

@cbuescher
Copy link
Member

@matarrese I'm just revisiting this while reviewing some older open PRs. Does @rjernst answer makes sense to you? If so, can we close this PR?

@javanna
Copy link
Member

javanna commented Nov 6, 2017

Relates to #26878. I think that we can close this.

@javanna javanna closed this Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Settings Settings infrastructure and APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants