Using ObjectParser in UpdateRequest #29293

liketic · 2018-03-29T09:26:50Z

Reject unknown field and switch fromXContent to ObjectParser.

elasticmachine · 2018-03-29T09:26:52Z

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?

elasticmachine · 2018-03-29T09:26:52Z

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?

elasticmachine · 2018-03-30T15:18:05Z

Pinging @elastic/es-distributed

nik9000

Could you add a unit test with the example that you hit in addition to the one you already have?

nik9000 · 2018-03-30T15:21:52Z

Sorry, I mean the example in the issue.

nik9000

I left a minor thing. Otherwise I think this looks great!

Do you mind dropping _fields? I believe you'd need to add a breaking changes note somewhere in the docs starting at migrate_7_0.asciidoc. Maybe in a new crud.asciidoc file. Are you ok with making that change as part of this?

nik9000 · 2018-03-30T15:29:25Z

server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

+                    fields = Collections.singletonList(parser.text());
+                }
+                return fields;
+            }, FIELDS_FIELD, ObjectParser.ValueType.STRING_ARRAY);


I think you can use PARSER.declareStringArray(UpdateRequest::fields, FIELDS_FIELD). It doens't reject non-array things but I think that is fine. Another choice would be to simply pitch this. It has been deprecated for a year and a half (since 1764ec5) and we are only going to merge this to the master branch anyway so it is going to be released during a major version so we can make breaking changes like dropping a deprecated field.

liketic · 2018-04-02T10:30:07Z

@nik9000 Thank you so much! I updated the PR and hope that's what we need. And added a note in migration/api , not sure if that's appropriate.

And another problem is I don't know how to avoid the failure when executing task :qa:mixed-cluster:v6.3.0-SNAPSHOT#mixedClusterTestRunner, because I removed fields = in.readOptionalStringArray(); from UpdateRequest.readFrom , which seems caused a bwc issue. Could you help me please? Thanks in advance!

nik9000

I left a few suggestions and one small thing.

I think you can fix the failing mixed cluster version test with something like:
You can use something like this:

    - skip:
        version: " - 6.99.99"
        reason:  fields dropped in 7.0

nik9000 · 2018-04-02T13:55:34Z

server/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

-                                DEPRECATION_LOGGER.deprecated("Deprecated field [fields] used, expected [_source] instead");
-                                List<Object> values = parser.list();
-                                fields = values.toArray(new String[values.size()]);
-                            } else {


Removing the stuff under the else actually makes us start parsing requests with arrays very strangely. I think it is worth keeping the throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected a simple value for field [" + currentFieldName + "] but found [" + token + "]"); part in an if statement that checks for START_ARRAY. And probably adding a test with an array.

Thanks! I put the throw back and add a test.

nik9000 · 2018-04-02T14:02:46Z

server/src/test/java/org/elasticsearch/update/UpdateIT.java

@@ -225,7 +225,7 @@ public void testUpsertDoc() throws Exception {
        UpdateResponse updateResponse = client().prepareUpdate(indexOrAlias(), "type1", "1")
                .setDoc(XContentFactory.jsonBuilder().startObject().field("bar", "baz").endObject())
                .setDocAsUpsert(true)
-                .setFields("_source")
+                .setFetchSource(true)


Hmmm. I think this is true by default. Can you check what happens if you drop the .setFetchSource(true) part?

By default the field fetchSource is null in the class UpdateRequest:

private FetchSourceContext fetchSourceContext;

If we drop this, we'll get a AssertionError.

I see it. I was tired when I was reading it. You are right to do what you did.

nik9000 · 2018-04-02T14:02:54Z

server/src/test/java/org/elasticsearch/update/UpdateIT.java

-        } catch (DocumentMissingException e) {
-            // all is well
-        }
+        expectThrows(DocumentMissingException.class,


Usually I do

Exception e = expectThrows(DocumentMissingException.class, () -> the thing); assertEquals("whatever", e.getMessage());

Just to make sure it was the right exception. In this case there is only one kind of message for DocumentMissingException so it is probably safe to do what you are doing, but it still might be nice anyway.

nik9000 · 2018-04-02T14:07:04Z

And thanks so much for doing this!

@jimczi, I believe you deprecated fields in updates. Are you ok with removing for 7.0 like I've suggested here? I just realized I might want to confirm with you.

jimczi · 2018-04-02T14:29:34Z

+1 to remove `fields. Thanks @liketic @nik9000 !

liketic · 2018-04-03T03:39:01Z

Thanks @nik9000 , I pushed 2024751

nik9000

So I'm genuinely sorry I suggested dropping the fields parameter in this PR. It has made it much larger than your original contribution. Thanks so much for following through with this.

I left a few things. I believe you should remove your "skip"s in the rest tests. I didn't understand what you meant by the REST tests failing when I told you about the skip. It is an option, but not the right one in this case.

We need to make sure that update requests work in a mixed version cluster and those tests were telling you that they were not. I left some line comments on the line that I'm fairly sure need to be changed to fix the tests.

Again, thanks for all of your work on this. I think this is super close!

nik9000 · 2018-04-03T15:14:49Z

server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

@@ -800,7 +772,6 @@ public void readFrom(StreamInput in) throws IOException {
            doc = new IndexRequest();
            doc.readFrom(in);
        }
-        fields = in.readOptionalStringArray();


So removing this line is going to cause problems in mixed clusters. Elasticsearch versions before 7.0.0 will continue to send this array. You have to read it and decide what to do with it. Something like:

if (false == in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { String[] fields = in.readOptionalStringArray(); if (fields != null) { throw new IllegalArgumentException("[fields] is no longer supported"); } }

is probably fine to be honest.

nik9000 · 2018-04-03T15:16:11Z

server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

@@ -838,7 +809,6 @@ public void writeTo(StreamOutput out) throws IOException {
            doc.id(id);
            doc.writeTo(out);
        }
-        out.writeOptionalStringArray(fields);


You'll want to do something like:

if (false == out.getVersion().onOrAfter(Version.V_7_0_0_alpha_1) { out.writeOptionalStringArray(null); }

That way 6.x can parse the request. We know there aren't any fields anyway because we couldn't have parsed them.

nik9000 · 2018-04-03T15:17:21Z

docs/reference/migration/migrate_7_0/api.asciidoc

@@ -22,3 +22,6 @@ The following parameters starting with underscore have been removed:
 Instead of these removed parameters, use their non camel case equivalents without
 starting underscore, e.g. use `version_type` instead of `_version_type` or `versionType`.

+
+==== The parameter `fields` deprecated in 6.x has been removed from Bulk request 
+and Update request.


Can you also add a note about the change you update request?

I'm not sure this is the right file for them but you may as well put them here for now and we'll move them if we decide there is a better spot.

liketic · 2018-04-04T14:59:53Z

Thanks @nik9000 That's much better! I pushed fae7908

nik9000

This looks good to me! I'll merge it when I get clean builds on the intake jobs.

nik9000 · 2018-04-11T16:54:16Z

Or maybe there are conflicts. Nuts!

If you are around @liketic, can you merge master into this branch and resolve the conflicts? I can do it if you'd prefer.

liketic · 2018-04-12T02:16:28Z

Thanks @nik9000 , the conflict was resolved.

nik9000 · 2018-04-12T19:01:18Z

Thanks @nik9000 , the conflict was resolved.

❤️

nik9000

@elasticmachine, test this please

nik9000 · 2018-04-13T13:54:25Z

Damn, wrong power words!

@elasticmachine, run all tests!

nik9000 · 2018-04-13T13:54:32Z

That did it.

nik9000 · 2018-04-13T16:42:57Z

The PR build failed but it looks like that is because this needs master merged into it again. I've done so locally and am re-running the build locally.

nik9000 · 2018-04-13T21:35:23Z

OK! I got a passing build locally. I'll merge this when the intake build is green.

nik9000 · 2018-04-16T12:41:34Z

Merged to master! Finally! Thanks @liketic! Thanks for sticking with it! I've learned my lesson. Next time I won't ask about dropping a deprecated thing when converting to ObjectParser. That more than doubled the complexity of this. But thanks for finishing it out!

For posterity: I rewrote the commit message but accidentally didn't rewrite the subject line. Oops.

* master: [TEST] REST client request without leading '/' (#29471) Using ObjectParser in UpdateRequest (#29293) Prevent accidental changes of default values (#29528) [Docs] Add definitions to glossary (#29127) Avoid self-deadlock in the translog (#29520) Minor cleanup in NodeInfo.groovy Lazy configure build tasks that require older JDKs (#29519) Simplify snapshot check in root build file Make NodeInfo#nodeVersion strongly-typed as Version (#29515) Enable license header exclusions (#29379) Use proper Java version for BWC builds (#29493) Mute TranslogTests#testFatalIOExceptionsWhileWritingConcurrently Enable skipping fetching latest for BWC builds (#29497)

* master: (96 commits) TEST: Mute testEnsureWeReconnect Mute full cluster restart test recovery test REST high-level client: add support for Indices Update Settings API [take 2] (elastic#29327) Plugins: Fix native controller confirmation for non-meta plugin (elastic#29434) Remove PipelineExecutionService#executeIndexRequest (elastic#29537) Fix overflow error in parsing of long geohashes (elastic#29418) Remove unused index.ttl.disable_purge setting (elastic#29527) FullClusterRestartIT.testRecovery should wait for all initializing shards Build: Fail if any libs depend on non-core libs (elastic#29336) [TEST] REST client request without leading '/' (elastic#29471) Using ObjectParser in UpdateRequest (elastic#29293) Prevent accidental changes of default values (elastic#29528) [Docs] Add definitions to glossary (elastic#29127) Avoid self-deadlock in the translog (elastic#29520) Minor cleanup in NodeInfo.groovy Lazy configure build tasks that require older JDKs (elastic#29519) Simplify snapshot check in root build file Make NodeInfo#nodeVersion strongly-typed as Version (elastic#29515) Enable license header exclusions (elastic#29379) Use proper Java version for BWC builds (elastic#29493) ...

This change ensures that when null is passed as the value to one of its Nullable parameters, it is not wrapped in a String array. That would in turn cause a NPE when attempting to serialize FetchSourceContext as its constructor checks for null values only. In master, the problematic behavior was corrected as part of elastic#29293

This change ensures that when null is passed as the value to one of UpdateRequest#fetchSource @nullable parameters, it is not wrapped in a String array. That would in turn cause a NPE when attempting to serialize FetchSourceContext as its constructor checks explicitly for Null and not for arrays of Null objects. In master, the problematic behavior was corrected as part of #29293

Using ObjectParser in UpdateRequest (elastic#28740)

ce966d0

nik9000 self-requested a review March 30, 2018 15:17

nik9000 added review :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.0.0 labels Mar 30, 2018

nik9000 added the >feature label Mar 30, 2018

nik9000 reviewed Mar 30, 2018

View reviewed changes

nik9000 requested changes Mar 30, 2018

View reviewed changes

liketic added 3 commits April 2, 2018 11:46

Merge branch 'master' into fix-issues/28740

0d3fd89

Address comments

a3a8087

Drop parameter fields

d456759

nik9000 requested changes Apr 2, 2018

View reviewed changes

Fix test bwc issue

2024751

nik9000 requested changes Apr 3, 2018

View reviewed changes

liketic added 2 commits April 4, 2018 22:56

Fix bwc issue caused by removing fields

fae7908

Merge branch 'master' into fix-issues/28740

553bf99

nik9000 approved these changes Apr 11, 2018

View reviewed changes

Resolve conflicts

28622b4

nik9000 approved these changes Apr 12, 2018

View reviewed changes

nik9000 merged commit 0bfb59d into elastic:master Apr 16, 2018

liketic deleted the fix-issues/28740 branch April 22, 2018 10:18

vladimirdolzhenko added the >breaking label Nov 1, 2018

This was referenced Nov 1, 2018

update fails silently if script and doc update are both in one line #34069

Closed

Fix UpdateRequest.fromXContent #35257

Merged

jkakavas mentioned this pull request Dec 19, 2018

Handle Null in FetchSourceContext#fetchSource #36839

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

lcawl mentioned this pull request Apr 9, 2019

Add 7.0.0 Release Notes #41029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using ObjectParser in UpdateRequest #29293

Using ObjectParser in UpdateRequest #29293

liketic commented Mar 29, 2018

elasticmachine commented Mar 29, 2018

elasticmachine commented Mar 29, 2018

elasticmachine commented Mar 30, 2018

nik9000 left a comment

nik9000 commented Mar 30, 2018

nik9000 left a comment

nik9000 Mar 30, 2018

liketic commented Apr 2, 2018 •

edited

Loading

nik9000 left a comment

nik9000 Apr 2, 2018

liketic Apr 3, 2018

nik9000 Apr 2, 2018

liketic Apr 2, 2018 •

edited

Loading

nik9000 Apr 2, 2018

nik9000 Apr 2, 2018

nik9000 Apr 2, 2018

nik9000 commented Apr 2, 2018

jimczi commented Apr 2, 2018

liketic commented Apr 3, 2018

nik9000 left a comment

nik9000 Apr 3, 2018

nik9000 Apr 3, 2018

nik9000 Apr 3, 2018

liketic commented Apr 4, 2018

nik9000 left a comment

nik9000 commented Apr 11, 2018

liketic commented Apr 12, 2018

nik9000 commented Apr 12, 2018

nik9000 left a comment

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 16, 2018

Using ObjectParser in UpdateRequest #29293

Using ObjectParser in UpdateRequest #29293

Conversation

liketic commented Mar 29, 2018

elasticmachine commented Mar 29, 2018

elasticmachine commented Mar 29, 2018

elasticmachine commented Mar 30, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Mar 30, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liketic commented Apr 2, 2018 • edited Loading

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liketic Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Apr 2, 2018

jimczi commented Apr 2, 2018

liketic commented Apr 3, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liketic commented Apr 4, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Apr 11, 2018

liketic commented Apr 12, 2018

nik9000 commented Apr 12, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 13, 2018

nik9000 commented Apr 16, 2018

liketic commented Apr 2, 2018 •

edited

Loading

liketic Apr 2, 2018 •

edited

Loading