From 7db5538ebd548c6821b8d930c3759459e3564d05 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Fri, 2 Nov 2018 12:03:09 +0100 Subject: [PATCH 1/4] Fix UpdateRequest.fromXContent Closes #34069 --- .../action/update/UpdateRequest.java | 22 +++++++++++- .../action/update/UpdateRequestTests.java | 36 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index 2dcd35dfb36b9..336ac53c03675 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -726,7 +726,22 @@ public UpdateRequest fromXContent(XContentParser parser) throws IOException { return this; } String currentFieldName = null; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + int depth = 0; + while (token != null) { + token = parser.nextToken(); + // have to track depth to avoid premature end of parsing due to nested objects as + // update request could have item ( script / scripted_upsert / upsert / doc etc ) at any nested level + if (token == XContentParser.Token.START_OBJECT) { + depth++; + } else if (token == XContentParser.Token.END_OBJECT) { + currentFieldName = null; + if (depth == 0) { + break; + } + depth--; + continue; + } + if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); } else if ("script".equals(currentFieldName)) { @@ -758,6 +773,11 @@ public UpdateRequest fromXContent(XContentParser parser) throws IOException { } else if ("_source".equals(currentFieldName)) { fetchSourceContext = FetchSourceContext.fromXContent(parser); } + + // copyCurrentStructure / SomeObject.fromXContent moves current token to END_OBJECT + if (parser.currentToken() == XContentParser.Token.END_OBJECT) { + depth--; + } } if (script != null) { this.script = script; diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java index 1dca992c24d15..63ad902ccb358 100644 --- a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.action.update; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.delete.DeleteRequest; import org.elasticsearch.action.index.IndexRequest; @@ -62,8 +63,10 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; public class UpdateRequestTests extends ESTestCase { @@ -275,6 +278,39 @@ public void testFromXContent() throws Exception { assertThat(((Map) doc.get("compound")).get("field2").toString(), equalTo("value2")); } + public void testFromXContentBothScriptAndNestedDocs() throws Exception { + // related to https://github.com/elastic/elasticsearch/issues/34069 + UpdateRequest request = new UpdateRequest("test", "type1", "1") + .fromXContent( + createParser(JsonXContent.jsonXContent, + new BytesArray("{\"any_odd_name_for_update\":{\"doc\":{\"message\":\"set by update:doc\"}}," + + "\"script\":{\"source\":\"ctx._source.message = 'set by script'\"}}"))); + + assertThat(request.doc(), notNullValue()); + assertThat(request.script(), notNullValue()); + + ActionRequestValidationException validate = request.validate(); + assertThat(validate, notNullValue()); + assertThat(validate.validationErrors(), not(empty())); + assertThat(String.valueOf(validate.validationErrors()), + validate.validationErrors(), contains("can't provide both script and doc")); + + request = new UpdateRequest("test", "type1", "1") + .fromXContent( + createParser(JsonXContent.jsonXContent, + new BytesArray("{\"whatever\": {\"script\":{\"source\":\"ctx._source.message = 'set by script'\"}}," + + "\"nested_update1\":{\"nested_update2\":{\"doc\":{\"message\":\"set by update:doc\"}}}}"))); + + assertThat(request.doc(), notNullValue()); + assertThat(request.script(), notNullValue()); + + validate = request.validate(); + assertThat(validate, notNullValue()); + assertThat(validate.validationErrors(), not(empty())); + assertThat(String.valueOf(validate.validationErrors()), + validate.validationErrors(), contains("can't provide both script and doc")); + } + // Related to issue 15338 public void testFieldsParsing() throws Exception { UpdateRequest request = new UpdateRequest("test", "type1", "1").fromXContent( From 55e4de0c9f04277679fafd2ff21e3b36b32824cc Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 5 Nov 2018 15:02:09 +0100 Subject: [PATCH 2/4] add deprecation message for unknown fields --- .../org/elasticsearch/action/update/UpdateRequest.java | 9 +++++++++ .../action/update/UpdateRequestTests.java | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index 336ac53c03675..8d4e3d9aae24a 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -30,6 +30,8 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.lucene.uid.Versions; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -41,6 +43,7 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.VersionType; import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.rest.action.document.RestUpdateAction; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -56,6 +59,9 @@ public class UpdateRequest extends InstanceShardOperationRequest implements DocWriteRequest, WriteRequest, ToXContentObject { + private static final DeprecationLogger DEPRECATION_LOGGER = + new DeprecationLogger(Loggers.getLogger(RestUpdateAction.class)); + private String type; private String id; @Nullable @@ -772,6 +778,9 @@ public UpdateRequest fromXContent(XContentParser parser) throws IOException { } } else if ("_source".equals(currentFieldName)) { fetchSourceContext = FetchSourceContext.fromXContent(parser); + } else { + DEPRECATION_LOGGER.deprecated("Unknown field [{}] used in {} which has no value and will not be accepted in future", + currentFieldName, UpdateRequest.class.getSimpleName()); } // copyCurrentStructure / SomeObject.fromXContent moves current token to END_OBJECT diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java index 63ad902ccb358..e0320c8ed14a1 100644 --- a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java @@ -286,6 +286,9 @@ public void testFromXContentBothScriptAndNestedDocs() throws Exception { new BytesArray("{\"any_odd_name_for_update\":{\"doc\":{\"message\":\"set by update:doc\"}}," + "\"script\":{\"source\":\"ctx._source.message = 'set by script'\"}}"))); + assertWarnings("Unknown field [any_odd_name_for_update] used in UpdateRequest" + + " which has no value and will not be accepted in future"); + assertThat(request.doc(), notNullValue()); assertThat(request.script(), notNullValue()); @@ -301,6 +304,13 @@ public void testFromXContentBothScriptAndNestedDocs() throws Exception { new BytesArray("{\"whatever\": {\"script\":{\"source\":\"ctx._source.message = 'set by script'\"}}," + "\"nested_update1\":{\"nested_update2\":{\"doc\":{\"message\":\"set by update:doc\"}}}}"))); + assertWarnings("Unknown field [whatever] used in UpdateRequest" + + " which has no value and will not be accepted in future", + "Unknown field [nested_update1] used in UpdateRequest" + + " which has no value and will not be accepted in future", + "Unknown field [nested_update2] used in UpdateRequest" + + " which has no value and will not be accepted in future"); + assertThat(request.doc(), notNullValue()); assertThat(request.script(), notNullValue()); From e64fcdb5a7c82235332da4f452601c6b95fd1ccc Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 5 Nov 2018 15:02:09 +0100 Subject: [PATCH 3/4] add deprecation message for field `fields` --- .../java/org/elasticsearch/action/update/UpdateRequest.java | 1 + .../org/elasticsearch/action/update/UpdateRequestTests.java | 2 ++ 2 files changed, 3 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index 8d4e3d9aae24a..c7db445c944eb 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -767,6 +767,7 @@ public UpdateRequest fromXContent(XContentParser parser) throws IOException { } else if ("detect_noop".equals(currentFieldName)) { detectNoop(parser.booleanValue()); } else if ("fields".equals(currentFieldName)) { + DEPRECATION_LOGGER.deprecated("Deprecated field [fields] used, expected [_source] instead"); List fields = null; if (token == XContentParser.Token.START_ARRAY) { fields = (List) parser.list(); diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java index e0320c8ed14a1..a318bfad830e2 100644 --- a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java @@ -327,11 +327,13 @@ public void testFieldsParsing() throws Exception { createParser(JsonXContent.jsonXContent, new BytesArray("{\"doc\": {\"field1\": \"value1\"}, \"fields\": \"_source\"}"))); assertThat(request.doc().sourceAsMap().get("field1").toString(), equalTo("value1")); assertThat(request.fields(), arrayContaining("_source")); + assertWarnings("Deprecated field [fields] used, expected [_source] instead"); request = new UpdateRequest("test", "type2", "2").fromXContent(createParser(JsonXContent.jsonXContent, new BytesArray("{\"doc\": {\"field2\": \"value2\"}, \"fields\": [\"field1\", \"field2\"]}"))); assertThat(request.doc().sourceAsMap().get("field2").toString(), equalTo("value2")); assertThat(request.fields(), arrayContaining("field1", "field2")); + assertWarnings("Deprecated field [fields] used, expected [_source] instead"); } public void testFetchSourceParsing() throws Exception { From e7441ae319b49f7137d94ce60a3b3384c051c1b0 Mon Sep 17 00:00:00 2001 From: Vladimir Dolzhenko Date: Mon, 5 Nov 2018 15:02:09 +0100 Subject: [PATCH 4/4] fixed deprecation message test and log class --- .../org/elasticsearch/action/update/UpdateRequest.java | 3 +-- .../elasticsearch/action/update/UpdateRequestBuilder.java | 3 +-- .../elasticsearch/action/update/UpdateRequestTests.java | 8 +++++++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index c7db445c944eb..744eae211be36 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -43,7 +43,6 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.VersionType; import org.elasticsearch.index.shard.ShardId; -import org.elasticsearch.rest.action.document.RestUpdateAction; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -60,7 +59,7 @@ public class UpdateRequest extends InstanceShardOperationRequest implements DocWriteRequest, WriteRequest, ToXContentObject { private static final DeprecationLogger DEPRECATION_LOGGER = - new DeprecationLogger(Loggers.getLogger(RestUpdateAction.class)); + new DeprecationLogger(Loggers.getLogger(UpdateRequest.class)); private String type; private String id; diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequestBuilder.java index 5ba187013e79f..c2b4222d86ce2 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequestBuilder.java @@ -31,7 +31,6 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.VersionType; -import org.elasticsearch.rest.action.document.RestUpdateAction; import org.elasticsearch.script.Script; import java.util.Map; @@ -39,7 +38,7 @@ public class UpdateRequestBuilder extends InstanceShardOperationRequestBuilder implements WriteRequestBuilder { private static final DeprecationLogger DEPRECATION_LOGGER = - new DeprecationLogger(Loggers.getLogger(RestUpdateAction.class)); + new DeprecationLogger(Loggers.getLogger(UpdateRequestBuilder.class)); public UpdateRequestBuilder(ElasticsearchClient client, UpdateAction action) { super(client, action, new UpdateRequest()); diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java index a318bfad830e2..b65ffbe070684 100644 --- a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java @@ -490,7 +490,9 @@ public void testToAndFromXContent() throws IOException { BytesReference source = RandomObjects.randomSource(random(), xContentType); updateRequest.upsert(new IndexRequest().source(source, xContentType)); } - if (randomBoolean()) { + + final boolean fieldsAreUsed = randomBoolean(); + if (fieldsAreUsed) { String[] fields = new String[randomIntBetween(0, 5)]; for (int i = 0; i < fields.length; i++) { fields[i] = randomAlphaOfLength(5); @@ -541,6 +543,10 @@ public void testToAndFromXContent() throws IOException { BytesReference finalBytes = toXContent(parsedUpdateRequest, xContentType, humanReadable); assertToXContentEquivalent(originalBytes, finalBytes, xContentType); + + if (fieldsAreUsed) { + assertWarnings("Deprecated field [fields] used, expected [_source] instead"); + } } public void testToValidateUpsertRequestAndVersion() {