-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
bug fix about elasticsearch.common.settings.Settings.processSetting #44047
Conversation
Pinging @elastic/es-core-infra |
@kkewwei Thanks for finding a subtle bug and submitting a solution. We need to add some unit tests before we merge this PR, but first I'm going to describe what seems to me to be going on here in order to try to pin down what the exact problem is and make sure that the rest of the team agrees with us. DiscussionThe It looks to me like
…was being added as:
The change to line 147 fixes this bug. The ClassCast exception in the original bug report occurs when the map is already in this bad state, with settings like the following:
…was resulting in:
In this case, it drops a value. That's no good! The change in line 153 fixes this case. Unit Test SuggestionsIt's pretty easy to reproduce this bug by copying and adjusting code from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great catch and a great fix. We need to add at least two unit tests before merging - see the comment I left on the ticket.
It's my pleasure to add some unit tests, and I am very grateful for your sharding code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for my delay in reviewing this code. I have been traveling.
The idea here is correct, but we can't access the processSetting
method the way you've done it in this code. The processSetting
method is private
, and I don't believe we should change that. However, we can correctly test the code changes if we build our settings as follows:
public void testProcessSetting() throws IOException {
Settings test = Settings.builder()
.put("ant", "value1")
.put("ant.bee.cat", "value2")
.put("bee.cat", "value3")
.build();
XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent());
builder.startObject();
test.toXContent(builder, new ToXContent.MapParams(Collections.emptyMap()));
builder.endObject();
assertEquals("{\"ant.bee\":{\"cat\":\"value2\"},\"ant\":\"value1\",\"bee\":{\"cat\":\"value3\"}}", Strings.toString(builder));
test = Settings.builder()
.put("ant", "value1")
.put("ant.bee.cat", "value2")
.put("ant.bee.cat.dog.ewe", "value3")
.build();
builder = XContentBuilder.builder(XContentType.JSON.xContent());
builder.startObject();
test.toXContent(builder, new ToXContent.MapParams(Collections.emptyMap()));
builder.endObject();
assertEquals("{\"ant.bee\":{\"cat.dog\":{\"ewe\":\"value3\"},\"cat\":\"value2\"},\"ant\":\"value1\"}", Strings.toString(builder));
}
This is the approach that is used in the TestSettings#testToXContent
method.
@elasticmachine test this please |
Thank you for your sharind code which I use in unit test, It is a good idea to test it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good. Thank you for your prompt revisions, and for taking the time to improve Elasticsearch!
Relates #43791