Introduce ToXContentObject interface #22387

javanna · 2016-12-30T14:03:56Z

ToXContentObject extends ToXContent without adding new methods to it, while allowing to mark classes that output complete xcontent objects to distinguish them from classes that require starting and ending an anonymous object externally.

Ideally ToXContent would be renamed to ToXContentFragment or something along those lines, but that would be a huge change in our codebase, hence we can simply document the fact that ToXContent outputs fragments with no guarantees that the output is valid per se without an external ancestor.

As part of this PR also some of our response classes are migrated to the new interface. In general all of our response should implement ToXContentObject, but some don't even implement yet ToXContent, while some that do will be migrated step by step as part of the effort made for REST high level client (essentially while writing parsing code, we can also adjust the toXContent methods.

RestToXContentListener, String#toString and XContentHelper#toXContent used to take a boolean argument to indicate whether the output needed to be wrapped into a new object. This decision doesn't have to be made case by case now. We can rely on whether a class implements ToXContent (fragment) or ToXContentObject (complete valid object) instead.

There are many more things that can be done to improve how deal with ToXContent classes, like unifying their toStrings output etc. those improvements will come later, one at a time.

Relates to # 3889
Relates to #16347

nik9000

I left some minor stuff, some questions about naming, etc. But I think this'll be lovely to have.

I wonder if you can also convert the methods in XContentBuilder that take a ToXContent to taking ToXContentObject instead?

nik9000 · 2017-01-01T13:22:41Z

core/src/main/java/org/elasticsearch/common/Strings.java

        try {
            XContentBuilder builder = JsonXContent.contentBuilder();
-            if (wrapInObject) {
+            if (toXContent instanceof ToXContentObject == false) {


I'd probably pull the instanceof check into a boolean variable just to make sure it is only done once.

I keep thinking that we should be able to remove the instanceof check by making another method and we could do that for the most part but I don't think it'd be worth it. At least not for now, anyway.

I'd probably pull the instanceof check into a boolean variable just to make sure it is only done once.

will do, makes sense.

I keep thinking that we should be able to remove the instanceof check by making another method and we could do that for the most part but I don't think it'd be worth it. At least not for now, anyway.

agreed, the instanceof checks are not nice. Yet they are an improvement compared to the previous boolean flag.
If we had two methods, the method variant that accepts ToXContent would also accept a ToXContentObject which would throw error when adding the anonymous object as it already has it. Not too sure. As you said it may stay this way for now.

nik9000 · 2017-01-01T13:25:42Z

core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java

@@ -26,6 +26,8 @@

 /**
 * An interface allowing to transfer an object to "XContent" using an {@link XContentBuilder}.
+ * The output may not be valid alone without a valid ancestor, or without starting and ending an


What about "The output may or may not be a value object. Objects implementing {@link ToXContentObject} output a valid value but those that don't may or may not require emitting a startObject and an endObject."

nik9000 · 2017-01-01T13:26:58Z

core/src/main/java/org/elasticsearch/common/xcontent/ToXContentObject.java

+
+/**
+ * An interface allowing to transfer an object to "XContent" using an {@link XContentBuilder}.
+ * The difference between {@link ToXContent} and {@link ToXContentObject} is that the former outputs a fragment that


the former may output a fragment that requires....

nik9000 · 2017-01-01T13:28:06Z

core/src/main/java/org/elasticsearch/common/xcontent/ToXContentObject.java

+ * may require to start and end a new anonymous object externally, while the latter guarantees that what gets printed
+ * out is fully valid syntax without any external addition.
+ */
+public interface ToXContentObject extends ToXContent {


I wonder if ToXContentValue is better. The idea being that objects that implement the interface output valid left hand side "values".

I do not know. When I read value, I don't think of a complete object. But I do see how this is debatable. I would much rather have ToXContent and ToXContentFragment, but that'd require a huge change that I don't want to make at the moment. How about ToXContentValueObject? :D

I would much rather have ToXContent and ToXContentFragment

That would be the best option, but in the meanwhile I like ToXContentObject for the marker interface.

nik9000 · 2017-01-01T13:29:41Z

core/src/main/java/org/elasticsearch/rest/action/RestToXContentListener.java

-    public final RestResponse buildResponse(Response response, XContentBuilder builder) throws Exception {
-        if (wrapInObject()) {
+    protected final void toXContent(Response response, XContentBuilder builder) throws IOException {
+        if (response instanceof ToXContentObject == false) {


It'd be nice to be able to change this to only take ToXContentObject one day....

But that day can wait.

yes it would be, we are not that far from that day, yet some more work is required to migrate response that are reused throughout different apis.

javanna · 2017-01-03T13:46:01Z

@nik9000 thanks for your review, I have addressed your comments. I will wait for some more comments on naming and the approach taken from others before pushing. I think this is an improvement but there are a few things in this PR that are not super nice (e.g. the instanceof checks)

nik9000 · 2017-01-03T17:09:07Z

I was thinking we could consider those instanceof checks fairly temporary. At some point we can remove the versions that take ToXContent - like in the response handler. I figure at some point later we can make a ToXContentFragment for the toString ones and remove the toString version that takes ToXContent and use ToXContentFragment. Eventually. If it looks like a good choice.

tlrx · 2017-01-05T13:58:00Z

core/src/main/java/org/elasticsearch/common/Strings.java

-     * Return a {@link String} that is the json representation of the provided
-     * {@link ToXContent}.
+     * Return a {@link String} that is the json representation of the provided {@link ToXContent}.
+     * Wraps the output into an anonymous object depending on whether the provided argument implements {@link ToXContentObject} or not
     */
    public static String toString(ToXContent toXContent) {


I feel like we could already change this method to

public static String toString(ToXContentObject o) { return toString(o, false); }

and instead change all Strings.toString(ToXContent) calls to Strings.toString(ToXContent, false), in which we can just check that when wrapInObject is true the object does not also implement ToXContentObject.

This would avoid most instanceof checks and it prepares the field for a toString(ToXContentFragment o) method. Both of them would later delegate to a private static toString(ToXContent toXContent, boolean wrapInObject)...

tlrx · 2017-01-05T14:07:07Z

core/src/main/java/org/elasticsearch/common/xcontent/ToXContentObject.java

+ * may require to start and end a new anonymous object externally, while the latter guarantees that what gets printed
+ * out is fully valid syntax without any external addition.
+ */
+public interface ToXContentObject extends ToXContent {


I would much rather have ToXContent and ToXContentFragment

That would be the best option, but in the meanwhile I like ToXContentObject for the marker interface.

tlrx

I like this change as well as the ToXContentObject name. I also left a minor comment.

javanna · 2017-01-05T17:56:06Z

The instanceOf checks are gone now. I followed Tanguy's suggestion in XContentHelper#toXContent and Strings#toString, to have two method variants with either ToXContent or ToXContentObject as an argument. As for RestToXContentListener and RestStatusToXContentListener, I moved over to ToXContentObject all the responses that use such listeners, so that they now require ToXContentObject. This PR is getting big-ish so we should get it in soon-ish :)

javanna · 2017-01-05T23:02:58Z

@s1monw would you mind having a look too please?

s1monw · 2017-01-06T14:11:29Z

I wonder if should rather have a default method on the interface rather than a marker. The problem with a marker is that you have to do instanceof checks to do the right thing which are easily missable and if you cast up you are lost if you miss them?

javanna · 2017-01-06T14:15:14Z

@s1monw good point I agree that the current toString etc. methods will not work for instance if one does

ToXContent toXContent = searchResponse;
Strings.toString(toXContent);

although SearchResponse implements ToXContentObject.

I was considering as well a default method but I am not super sure if a different method helps. Then you would have two ways of printing out the same object which may get confusing. What about looking into not having ToXContentObject extend ToXContent anymore (and copying the same method)? I think that should be possible in the state that this PR is now. I think that would resolve the ambiguity?

s1monw · 2017-01-06T14:18:49Z

I was suggesting this:

diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java b/core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java
index 01111fa..9efdaaa 100644
--- a/core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java
+++ b/core/src/main/java/org/elasticsearch/common/xcontent/ToXContent.java
@@ -126,4 +126,8 @@ public interface ToXContent {
     }
 
     XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException;
+
+    default boolean isFragment() {
+        return true;
+    }
 }

s1monw · 2017-01-06T14:22:34Z

if you don't like this then please make sure you are doing instance of checks where you distinguish. I don't see them yet

javanna · 2017-01-06T14:47:15Z

@s1monw I followed your advice to resolve the ambiguity, and also kept the ToXContentObject marker around. Should look better now.

s1monw

LGTM

`ToXContentObject` extends `ToXContent` without adding new methods to it, while allowing to mark classes that output complete xcontent objects to distinguish them from classes that require starting and ending an anonymous object externally. Ideally ToXContent would be renamed to ToXContentFragment, but that would be a huge change in our codebase, hence we simply document the fact that toXContent outputs fragments with no guarantees that the output is valid per se without an external ancestor. Relates to elastic#16347

This involved changing also index, delete, update so that bulk can print them out in its own format.

…ToXContentObject This also allows to make RestToXContentListener require ToXContentObject rather than ToXContent

Relates to #22387

Related to elastic#22387

Related to #22387

javanna added :Internal >enhancement v5.2.0 v6.0.0-alpha1 labels Dec 30, 2016

nik9000 approved these changes Jan 1, 2017

View reviewed changes

javanna force-pushed the enhancement/toxcontent_object branch from c764688 to d2516ca Compare January 3, 2017 13:39

tlrx reviewed Jan 5, 2017

View reviewed changes

tlrx approved these changes Jan 5, 2017

View reviewed changes

javanna force-pushed the enhancement/toxcontent_object branch 3 times, most recently from cb42070 to f25c498 Compare January 5, 2017 17:06

nik9000 approved these changes Jan 5, 2017

View reviewed changes

s1monw approved these changes Jan 6, 2017

View reviewed changes

javanna force-pushed the enhancement/toxcontent_object branch from 61c9f3a to 1504ba8 Compare January 6, 2017 16:41

javanna added 6 commits January 6, 2017 20:05

Migrate some more responses to ToXContentObject

a6b2cfe

Make TermVectorsResponse a ToXContentObject

6f23d34

Make SearchResponse a ToXContentObject

fdb3bf9

Make DocWriteResponse a ToXContentObject

1ebaec8

This involved changing also index, delete, update so that bulk can print them out in its own format.

Make PercolateResponse a ToXContentObject

87feeda

Make StatusToXContent extend ToXContentObject and rename it to Status…

8d08dc0

…ToXContentObject This also allows to make RestToXContentListener require ToXContentObject rather than ToXContent

javanna force-pushed the enhancement/toxcontent_object branch from 1504ba8 to 8d08dc0 Compare January 6, 2017 19:06

javanna merged commit ded694f into elastic:master Jan 6, 2017

javanna added v5.3.0 and removed v5.2.0 labels Jan 7, 2017

javanna mentioned this pull request Jan 7, 2017

Clarify ToXContent contract #16347

Closed

ywelsch added a commit that referenced this pull request Jan 10, 2017

[TEST] Fix JSON generation of failure in InternalTestCluster

c35277e

Relates to #22387

ywelsch added a commit that referenced this pull request Jan 10, 2017

[TEST] Fix JSON generation of failure in InternalTestCluster

676aa29

Relates to #22387

imotov mentioned this pull request Jan 19, 2017

Fix broken TaskInfo.toString() #22698

Merged

imotov added a commit to imotov/elasticsearch that referenced this pull request Jan 21, 2017

Fix broken TaskInfo.toString()

cfb415d

Related to elastic#22387

imotov added a commit that referenced this pull request Jan 21, 2017

Fix broken TaskInfo.toString()

3d4b45e

Related to #22387

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ToXContentObject interface #22387

Introduce ToXContentObject interface #22387

javanna commented Dec 30, 2016 •

edited

Loading

nik9000 left a comment

nik9000 Jan 1, 2017

javanna Jan 3, 2017

nik9000 Jan 1, 2017

nik9000 Jan 1, 2017

nik9000 Jan 1, 2017

javanna Jan 3, 2017

tlrx Jan 5, 2017

nik9000 Jan 1, 2017

nik9000 Jan 1, 2017

javanna Jan 3, 2017

javanna commented Jan 3, 2017

nik9000 commented Jan 3, 2017

tlrx Jan 5, 2017 •

edited

Loading

tlrx Jan 5, 2017

tlrx left a comment

javanna commented Jan 5, 2017

javanna commented Jan 5, 2017

s1monw commented Jan 6, 2017

javanna commented Jan 6, 2017

s1monw commented Jan 6, 2017

s1monw commented Jan 6, 2017

javanna commented Jan 6, 2017

s1monw left a comment

Introduce ToXContentObject interface #22387

Introduce ToXContentObject interface #22387

Conversation

javanna commented Dec 30, 2016 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jan 3, 2017

nik9000 commented Jan 3, 2017

tlrx Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment

javanna commented Jan 5, 2017

javanna commented Jan 5, 2017

s1monw commented Jan 6, 2017

javanna commented Jan 6, 2017

s1monw commented Jan 6, 2017

s1monw commented Jan 6, 2017

javanna commented Jan 6, 2017

s1monw left a comment

Choose a reason for hiding this comment

javanna commented Dec 30, 2016 •

edited

Loading

tlrx Jan 5, 2017 •

edited

Loading