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.

Sign up for GitHub

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

REST high-level client: add support for Indices Update Settings API #28892

Merged
merged 13 commits into from
Mar 30, 2018
Merged

REST high-level client: add support for Indices Update Settings API #28892

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
97b0bdc
REST high-level client: add support for Indices Update Settings API
olcbean Mar 3, 2018
a7c44ba
UpdateSettingsRequest : add fromXContent and toXContent
olcbean Mar 7, 2018
874f576
Merge remote-tracking branch 'origin/master' into hlrc_update_index_s…
olcbean Mar 22, 2018
e944714
avoid possible NPE if there are no indices
olcbean Mar 24, 2018
624423d
Merge remote-tracking branch 'origin/master' into hlrc_update_index_s…
olcbean Mar 24, 2018
32454d9
do not compare the indices for the serialization and to/from XContent…
olcbean Mar 25, 2018
4f8109c
remove an unnecessary checkstyle suppression
olcbean Mar 25, 2018
74cbb30
going back to an explicit ToFromXContent test
olcbean Mar 27, 2018
2ac8ddb
Split the UpdateSettingsRequestTests in two
olcbean Mar 27, 2018
a4982a8
extend the equals and cacheCode to cover all object fields
olcbean Mar 28, 2018
c7aa404
complete streamable test; simple XContenet test
olcbean Mar 29, 2018
f95231f
modify the XContent tests to randomly test for enclosing settings in the
olcbean Mar 29, 2018
f9d8914
adding few more comments
olcbean Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .../main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ public String toString() {
public boolean equals(Object o) {
if (super.equals(o)) {
UpdateSettingsRequest that = (UpdateSettingsRequest) o;
// do not include the indices as they are not part of the serialized request
return Objects.equals(settings, that.settings) && Arrays.equals(indices, that.indices);
}
return false;
Expand Down
80 changes: 67 additions & 13 deletions .../java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,34 +19,35 @@

package org.elasticsearch.action.admin.indices.settings.put;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.Settings.Builder;
import org.elasticsearch.common.util.CollectionUtils;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractStreamableXContentTestCase;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.test.AbstractStreamableTestCase;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.StringJoiner;
import java.util.function.Predicate;

public class UpdateSettingsRequestTests extends AbstractStreamableXContentTestCase<UpdateSettingsRequest> {
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
import static org.hamcrest.Matchers.equalTo;

@Override
protected UpdateSettingsRequest doParseInstance(XContentParser parser) throws IOException {
return new UpdateSettingsRequest().fromXContent(parser);
}
public class UpdateSettingsRequestTests extends AbstractStreamableTestCase<UpdateSettingsRequest> {

@Override
protected UpdateSettingsRequest mutateInstance(UpdateSettingsRequest request) {
return new UpdateSettingsRequest(mutateSettings(request.settings()), request.indices());
}

@Override
protected Predicate<String> getRandomFieldsExcludeFilter() {
// do not insert any fields into the request body, as every inserted field will be interpreted as a new setting
return p -> true;
if (randomBoolean()) {
return new UpdateSettingsRequest(mutateSettings(request.settings()), request.indices());
}
return new UpdateSettingsRequest(request.settings(), mutateIndices(request.indices()));
}

@Override
Expand All @@ -67,6 +68,15 @@ protected void assertEqualInstances(UpdateSettingsRequest expectedInstance, Upda
super.assertEqualInstances(expectedInstance, newInstance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now better what I made you do! I am sorry, I think that I don't like it :) Our requests don't fit well with this way of testing. While responses are usually entirely serialized both on the transport layer and on the REST layer, with requests the toXContent is only the request body, but there's also the query_string params that are serialized at transport but not as XContent. This makes for impossible equals/hashcode and complicated testing. To do things right, here we should probably have two different tests, one for the Xcontent part and one for the serialization. I am wondering though if this is becoming besides the scope of this PR. I am totally fine with going back to testing only XContent here. Thanks for trying, and sorry for the back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, it is fun to experiment :) Now we know that it is better to keep the *RequestTests extend ESTestCase ( or maybe AbstractStreamableTestCase but extending from AbstractStreamableXContentTestCase makes the tests tricky to maintain )

I still kept the serialization tests and went back to explicit to/from XContent testing.

}

public void testXContent() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think that it would be possible to have two different test classes, one for the xcontent bits, that doesn't rely on equals/hashcode , and the other one like this on that we have that tests serialization and ordinary equals/hashcode ? Let me know if I am missing something.

doToFromXContentWithEnclosingSettingsField(false);
}

// test that enclosing the setting in "settings" will be correctly parsed
public void testXContentWithEnclosingSettingsField() throws IOException {
doToFromXContentWithEnclosingSettingsField(true);
}

private static Settings mutateSettings(Settings settings) {
if (settings.isEmpty()) {
return randomSettings(1, 5);
Expand All @@ -84,6 +94,15 @@ private static Settings mutateSettings(Settings settings) {
return builder.build();
}

private static String[] mutateIndices(String[] indices) {
if (CollectionUtils.isEmpty(indices)) {
return randomIndicesNames(1, 5);
}
String[] mutated = Arrays.copyOf(indices, indices.length);
Arrays.asList(mutated).replaceAll(i -> i += randomAlphaOfLengthBetween(2, 5));
return mutated;
}

private static Settings randomSettings(int min, int max) {
int num = randomIntBetween(min, max);
Builder builder = Settings.builder();
Expand All @@ -107,4 +126,39 @@ private static String[] randomIndicesNames(int minIndicesNum, int maxIndicesNum)
return indices;
}

private void doToFromXContentWithEnclosingSettingsField(boolean addSettingsField) throws IOException {
final UpdateSettingsRequest request = createTestInstance();
boolean humanReadable = randomBoolean();
final XContentType xContentType = randomFrom(XContentType.values());

BytesReference bytesRef;
if (addSettingsField) {
UpdateSettingsRequest requestWithEnclosingSettings = new UpdateSettingsRequest(request.settings()) {
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.startObject("settings");
this.settings().toXContent(builder, params);
builder.endObject();
builder.endObject();
return builder;
}
};
BytesReference originalBytes = toShuffledXContent(requestWithEnclosingSettings, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
if (randomBoolean()) {
Predicate<String> excludeFilter = (s) -> s.startsWith("settings");
bytesRef = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed something strange in the UpdateSettingsRequest#fromXContent ( maybe not as part of this PR but it is easy to show it here ;) )

If the request is something like :

{
  "field1" : "value1",
  "field2" : "value2",
  "settings" : 
  {
    "field3" : "value3"
  }
}

only "field3" : "value3" is parsed as a setting in the request ( exposed here by the insertRandomFields : inserting a random object at the top level is silently ignored )

Do we want such leniency in the requests ?
Do we want to keep on supporting the possibility to wrap the settings in "settings" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch, I would assume that we want to get rid of this but it's a potentially breaking change. Would you mind opening a new issue for this please?

} else {
bytesRef = originalBytes;
}
} else {
bytesRef = toShuffledXContent(request, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
}

XContentParser parser = createParser(xContentType.xContent(), bytesRef);
UpdateSettingsRequest parsedRequest = new UpdateSettingsRequest().fromXContent(parser);

assertNull(parser.nextToken());
assertThat(parsedRequest.settings(), equalTo(request.settings()));
}

}