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

Close xcontent parsers (partial) #31513

Merged
merged 5 commits into from
Jun 24, 2018
Merged

Close xcontent parsers (partial) #31513

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9675849
Partial pass at closing XContentParsers in server
tomcallahan Jun 20, 2018
b61ab2a
fix checkstyle
tomcallahan Jun 20, 2018
169b2a0
fix test failures
tomcallahan Jun 20, 2018
14b0b5f
address reviewer comments
tomcallahan Jun 22, 2018
c774e10
address reviewer comment
tomcallahan Jun 22, 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
7 changes: 4 additions & 3 deletions ...ain/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,9 +558,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.startObject("mappings");
for (Map.Entry<String, String> entry : mappings.entrySet()) {
builder.field(entry.getKey());
XContentParser parser = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
DeprecationHandler.THROW_UNSUPPORTED_OPERATION, entry.getValue());
builder.copyCurrentStructure(parser);
try (XContentParser parser = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
DeprecationHandler.THROW_UNSUPPORTED_OPERATION, entry.getValue())) {
builder.copyCurrentStructure(parser);
}
}
builder.endObject();

Expand Down
11 changes: 6 additions & 5 deletions ...va/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,13 @@ private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws
assertThat(iae.getMessage(),
containsString("[cluster_update_settings_request] unknown field [" + unsupportedField + "], parser not found"));
} else {
XContentParser parser = createParser(xContentType.xContent(), originalBytes);
ClusterUpdateSettingsRequest parsedRequest = ClusterUpdateSettingsRequest.fromXContent(parser);
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
ClusterUpdateSettingsRequest parsedRequest = ClusterUpdateSettingsRequest.fromXContent(parser);

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

Expand Down
9 changes: 5 additions & 4 deletions .../src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,12 @@ public static void assertMappingsEqual(Map<String, String> expected, Map<String,
for (Map.Entry<String, String> expectedEntry : expected.entrySet()) {
String expectedValue = expectedEntry.getValue();
String actualValue = actual.get(expectedEntry.getKey());
XContentParser expectedJson = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
try (XContentParser expectedJson = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, expectedValue);
XContentParser actualJson = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, actualValue);
assertEquals(expectedJson.map(), actualJson.map());
XContentParser actualJson = JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, actualValue)){
assertEquals(expectedJson.map(), actualJson.map());
}
}
}

Expand Down
7 changes: 4 additions & 3 deletions .../test/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,10 @@ public void testToAndFromXContent() throws IOException {

private void assertMappingsEqual(String expected, String actual) throws IOException {

XContentParser expectedJson = createParser(XContentType.JSON.xContent(), expected);
XContentParser actualJson = createParser(XContentType.JSON.xContent(), actual);
assertEquals(expectedJson.mapOrdered(), actualJson.mapOrdered());
try (XContentParser expectedJson = createParser(XContentType.JSON.xContent(), expected);
XContentParser actualJson = createParser(XContentType.JSON.xContent(), actual)) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: please align the second line of the try with the first line

assertEquals(expectedJson.mapOrdered(), actualJson.mapOrdered());
}
}

/**
Expand Down
5 changes: 4 additions & 1 deletion server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.RandomCreateIndexGenerator;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -93,7 +94,9 @@ public void testToAndFromXContent() throws IOException {

ResizeRequest parsedResizeRequest = new ResizeRequest(resizeRequest.getTargetIndexRequest().index(),
resizeRequest.getSourceIndex());
parsedResizeRequest.fromXContent(createParser(xContentType.xContent(), originalBytes));
try (XContentParser xParser = createParser(xContentType.xContent(), originalBytes)) {
parsedResizeRequest.fromXContent(xParser);
}

assertEquals(resizeRequest.getSourceIndex(), parsedResizeRequest.getSourceIndex());
assertEquals(resizeRequest.getTargetIndexRequest().index(), parsedResizeRequest.getTargetIndexRequest().index());
Expand Down
30 changes: 16 additions & 14 deletions server/src/test/java/org/elasticsearch/action/get/MultiGetRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,20 @@ public void testAddWithInvalidKey() throws IOException {
builder.endArray();
}
builder.endObject();
final XContentParser parser = createParser(builder);
final MultiGetRequest mgr = new MultiGetRequest();
final ParsingException e = expectThrows(
try (XContentParser parser = createParser(builder)) {
final MultiGetRequest mgr = new MultiGetRequest();
final ParsingException e = expectThrows(
ParsingException.class,
() -> {
final String defaultIndex = randomAlphaOfLength(5);
final String defaultType = randomAlphaOfLength(3);
final FetchSourceContext fetchSource = FetchSourceContext.FETCH_SOURCE;
mgr.add(defaultIndex, defaultType, null, fetchSource, null, parser, true);
});
assertThat(
assertThat(
e.toString(),
containsString("unknown key [doc] for a START_ARRAY, expected [docs] or [ids]"));
}
}

public void testUnexpectedField() throws IOException {
Expand Down Expand Up @@ -141,16 +142,17 @@ public void testXContentSerialization() throws IOException {
MultiGetRequest expected = createTestInstance();
XContentType xContentType = randomFrom(XContentType.values());
BytesReference shuffled = toShuffledXContent(expected, xContentType, ToXContent.EMPTY_PARAMS, false);
XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled);
MultiGetRequest actual = new MultiGetRequest();
actual.add(null, null, null, null, null, parser, true);
assertThat(parser.nextToken(), nullValue());

assertThat(actual.items.size(), equalTo(expected.items.size()));
for (int i = 0; i < expected.items.size(); i++) {
MultiGetRequest.Item expectedItem = expected.items.get(i);
MultiGetRequest.Item actualItem = actual.items.get(i);
assertThat(actualItem, equalTo(expectedItem));
try (XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled)) {
MultiGetRequest actual = new MultiGetRequest();
actual.add(null, null, null, null, null, parser, true);
assertThat(parser.nextToken(), nullValue());

assertThat(actual.items.size(), equalTo(expected.items.size()));
for (int i = 0; i < expected.items.size(); i++) {
MultiGetRequest.Item expectedItem = expected.items.get(i);
MultiGetRequest.Item actualItem = actual.items.get(i);
assertThat(actualItem, equalTo(expectedItem));
}
}
}
}
Expand Down
10 changes: 6 additions & 4 deletions server/src/test/java/org/elasticsearch/action/get/MultiGetResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ public void testFromXContent() throws IOException {
MultiGetResponse expected = createTestInstance();
XContentType xContentType = randomFrom(XContentType.values());
BytesReference shuffled = toShuffledXContent(expected, xContentType, ToXContent.EMPTY_PARAMS, false);

XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled);
MultiGetResponse parsed = MultiGetResponse.fromXContent(parser);
assertNull(parser.nextToken());
MultiGetResponse parsed;
try (XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled)) {
parsed = MultiGetResponse.fromXContent(parser);
assertNull(parser.nextToken());
}
assertNotSame(expected, parsed);

assertThat(parsed.getResponses().length, equalTo(expected.getResponses().length));
Expand All @@ -60,6 +61,7 @@ public void testFromXContent() throws IOException {
assertThat(actualItem.getResponse(), equalTo(expectedItem.getResponse()));
}
}

}
}

Expand Down
8 changes: 5 additions & 3 deletions server/src/test/java/org/elasticsearch/action/search/MultiSearchResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ public void testFromXContent() throws IOException {
MultiSearchResponse expected = createTestInstance();
XContentType xContentType = randomFrom(XContentType.values());
BytesReference shuffled = toShuffledXContent(expected, xContentType, ToXContent.EMPTY_PARAMS, false);
XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled);
MultiSearchResponse actual = MultiSearchResponse.fromXContext(parser);
assertThat(parser.nextToken(), nullValue());
MultiSearchResponse actual;
try (XContentParser parser = createParser(XContentFactory.xContent(xContentType), shuffled)) {
actual = MultiSearchResponse.fromXContext(parser);
assertThat(parser.nextToken(), nullValue());
}

assertThat(actual.getTook(), equalTo(expected.getTook()));
assertThat(actual.getResponses().length, equalTo(expected.getResponses().length));
Expand Down
33 changes: 18 additions & 15 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,13 @@ public void testUnknownFieldClusterMetaData() throws IOException {
.field("random", "value")
.endObject()
.endObject());
XContentParser parser = createParser(JsonXContent.jsonXContent, metadata);
try {
MetaData.Builder.fromXContent(parser);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Unexpected field [random]", e.getMessage());
try (XContentParser parser = createParser(JsonXContent.jsonXContent, metadata)) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about removing the nesting of trys here ?

try (XContentParser parser = createParser(JsonXContent.jsonXContent, metadata)) {
    MetaData.Builder.fromXContent(parser);
    fail();
} catch (IllegalArgumentException e) {
    assertEquals("Unexpected field [random]", e.getMessage());
}

MetaData.Builder.fromXContent(parser);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Unexpected field [random]", e.getMessage());
}
}
}

Expand All @@ -197,12 +198,13 @@ public void testUnknownFieldIndexMetaData() throws IOException {
.field("random", "value")
.endObject()
.endObject());
XContentParser parser = createParser(JsonXContent.jsonXContent, metadata);
try {
IndexMetaData.Builder.fromXContent(parser);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Unexpected field [random]", e.getMessage());
try (XContentParser parser = createParser(JsonXContent.jsonXContent, metadata)) {
try {
IndexMetaData.Builder.fromXContent(parser);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Unexpected field [random]", e.getMessage());
}
}
}

Expand All @@ -225,9 +227,10 @@ public void testXContentWithIndexGraveyard() throws IOException {
builder.startObject();
originalMeta.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));
final MetaData fromXContentMeta = MetaData.fromXContent(parser);
assertThat(fromXContentMeta.indexGraveyard(), equalTo(originalMeta.indexGraveyard()));
try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder))) {
final MetaData fromXContentMeta = MetaData.fromXContent(parser);
assertThat(fromXContentMeta.indexGraveyard(), equalTo(originalMeta.indexGraveyard()));
}
}

public void testSerializationWithIndexGraveyard() throws IOException {
Expand Down
14 changes: 8 additions & 6 deletions server/src/test/java/org/elasticsearch/common/geo/BaseGeoParsingTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,17 @@ abstract class BaseGeoParsingTestCase extends ESTestCase {
public abstract void testParseGeometryCollection() throws IOException;

protected void assertValidException(XContentBuilder builder, Class expectedException) throws IOException {
XContentParser parser = createParser(builder);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, expectedException);
try (XContentParser parser = createParser(builder)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, expectedException);
}
}

protected void assertGeometryEquals(Shape expected, XContentBuilder geoJson) throws IOException {
XContentParser parser = createParser(geoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertEquals(expected, ShapeParser.parse(parser).build());
try (XContentParser parser = createParser(geoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertEquals(expected, ShapeParser.parse(parser).build());
}
}

protected ShapeCollection<Shape> shapeCollection(Shape... shapes) {
Expand Down
Loading