Decouple XContentBuilder from BytesReference #28972

dakrone · 2018-03-09T21:51:08Z

This commit removes all mentions of BytesReference from XContentBuilder.
This is needed so that we can completely decouple the XContent code and move it
into its own dependency.

While this change appears large, it is due to two main changes, moving
.bytes() and .string() out of XContentBuilder itself into static methods
BytesReference.bytes and Strings.toString respectively. The rest of the
change is code reacting to these changes (the majority of it in tests).

Relates to #28504

This commit removes all mentions of `BytesReference` from `XContentBuilder`. This is needed so that we can completely decouple the XContent code and move it into its own dependency. While this change appears large, it is due to two main changes, moving `.bytes()` and `.string()` out of XContentBuilder itself into static methods `BytesReference.bytes` and `Strings.toString` respectively. The rest of the change is code reacting to these changes (the majority of it in tests). Relates to elastic#28504

elasticmachine · 2018-03-09T21:51:11Z

Pinging @elastic/es-core-infra

…ields We assert right below it that the XContent equivalent are equal, and the byte array instances are not going to be the same exact thing regardless.

nik9000 · 2018-03-12T14:54:31Z

server/src/main/java/org/elasticsearch/common/bytes/BytesReference.java

@@ -38,6 +41,16 @@

    private Integer hash = null; // we cache the hash of this reference since it can be quite costly to re-calculated it

+    public static BytesReference bytes(XContentBuilder xContentBuilder) {


Probably worth Javadoc.

Including the warning that it closes the builder.

nik9000 · 2018-03-12T14:56:40Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -173,6 +170,13 @@ public XContentType contentType() {
        return generator.contentType();
    }

+    /**
+     * @return the output stream to which the built object is being written


I think it'd be worth warning that it is dangerous to modify the steam and that it might not be complete unless the builder is closed. Actually, I wonder if you can have this close the builder too?

I mean, that is kind of weird, but maybe not too weird. Because this is only useful for byte array style output streams. Maybe if this doesn't close the stream it can assert that it is closed? I'm not sure how hard that is.

There isn't a way that I see to assert that the OutputStream is already closed (other than closing it). I'll add documentation about the modification though.

nik9000 · 2018-03-12T15:07:18Z

server/src/test/java/org/elasticsearch/action/get/GetResponseTests.java

@@ -81,7 +81,6 @@ private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws
            parsedGetResponse = GetResponse.fromXContent(parser);
            assertNull(parser.nextToken());
        }
-        assertEquals(expectedGetResponse, parsedGetResponse);


Can you explain why we need this line?

Or, rather, need to remove it?

That was a mistake, I will re-add this.

nik9000 · 2018-03-12T15:15:55Z

server/src/test/java/org/elasticsearch/common/xcontent/builder/XContentBuilderTests.java

            xContentBuilder.field("test1", "value1");
            xContentBuilder.endObject();
-            assertThat(xContentBuilder.bytes().utf8ToString(), equalTo("{\"test\":\"value\",\"foo\":{\"test\":\"value\"},\"test1\":\"value1\"}"));
+            assertThat(BytesReference.bytes(xContentBuilder).utf8ToString(), equalTo("{\"test\":\"value\",\"foo\":{\"test\":\"value\"},\"test1\":\"value1\"}"));


These should probably move into xcontent's tests rather than server's tests so it won't be able to use this.

But that can be another PR :).

It does make me wonder if string should be implemented in the xcontent artifact without Lucene.

I think these should use Strings.toString(xContentBuilder) at least.

nik9000 · 2018-03-12T15:19:45Z

...a/org/elasticsearch/common/xcontent/support/filtering/AbstractXContentFilteringTestCase.java

@@ -69,18 +70,18 @@ public void testSingleFieldObject() throws IOException {
    }

    static void assertXContentBuilderAsString(final XContentBuilder expected, final XContentBuilder actual) {
-        assertThat(actual.bytes().utf8ToString(), is(expected.bytes().utf8ToString()));
+        assertThat(BytesReference.bytes(actual).utf8ToString(), is(BytesReference.bytes(expected).utf8ToString()));


I think these should use Strings.toString(actual)

dakrone · 2018-03-13T20:58:40Z

Thanks for taking a look @nik9000, I pushed commits addressing your comments

…contentbuilder

* Decouple XContentBuilder from BytesReference This commit removes all mentions of `BytesReference` from `XContentBuilder`. This is needed so that we can completely decouple the XContent code and move it into its own dependency. While this change appears large, it is due to two main changes, moving `.bytes()` and `.string()` out of XContentBuilder itself into static methods `BytesReference.bytes` and `Strings.toString` respectively. The rest of the change is code reacting to these changes (the majority of it in tests). Relates to #28504

Changes made in elastic#28972 seems to have changed some assumptions about how SMILE and CBOR write byte[] values and how this is tested. This changes the generation of the randomized DocumentField values back to BytesArray while expecting the JSON and YAML deserialisation to produce Base64 encoded strings and SMILE and CBOR to parse back BytesArray instances. Closes elastic#29080

Changes made in #28972 seems to have changed some assumptions about how SMILE and CBOR write byte[] values and how this is tested. This changes the generation of the randomized DocumentField values back to BytesArray while expecting the JSON and YAML deserialisation to produce Base64 encoded strings and SMILE and CBOR to parse back BytesArray instances. Closes #29080

* es/6.x: (89 commits) Clarify requirements of strict date formats. (#29090) Clarify that dates are always rendered as strings. (#29093) [Docs] Fix link to Grok patterns (#29088) Fix starting on Windows from another drive (#29086) Use removeTask instead of finishTask in PersistentTasksClusterService (#29055) Added minimal docs for reindex api in java-api docs Allow overriding JVM options in Windows service (#29044) Clarify how to set compiler and runtime JDKs (#29101) Fix Parsing Bug with Update By Query for Stored Scripts (#29039) TEST: write ops should execute under shard permit (#28966) [DOCS] Add X-Pack upgrade details (#29038) Revert "Improve error message for installing plugin (#28298)" Docs: HighLevelRestClient#exists (#29073) Validate regular expressions in dynamic templates. (#29013) [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083) Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063) Mute failing GetResultTests and DocumentFieldTests [Docs] Fix Java Api index administration usage (#28260) Improve error message for installing plugin (#28298) Decouple XContentBuilder from BytesReference (#28972) ...

dakrone added >non-issue :Core/Infra/Core Core issues without another label v7.0.0 v6.3.0 labels Mar 9, 2018

dakrone added 2 commits March 9, 2018 16:07

Remove equality check in GetResponseTests.testXContentTestWithRandomF…

9609a20

…ields We assert right below it that the XContent equivalent are equal, and the byte array instances are not going to be the same exact thing regardless.

Add xcontent type-specific original values

5450963

nik9000 reviewed Mar 12, 2018

View reviewed changes

dakrone added 4 commits March 13, 2018 13:50

Re-add deleted assertion

c509ecc

Fix the things

0a28473

Add/enhance javadocs

c240bbf

Use Strings.toString instead of BytesReference.bytes(..).utf8ToString()

82df012

dakrone added 2 commits March 13, 2018 16:34

Merge remote-tracking branch 'origin/master' into de-bytesreference-x…

259a0f8

…contentbuilder

Merge remote-tracking branch 'origin/master' into de-bytesreference-x…

39e5ffa

…contentbuilder

nik9000 approved these changes Mar 14, 2018

View reviewed changes

dakrone merged commit 8e8fdc4 into elastic:master Mar 14, 2018

cbuescher mentioned this pull request Mar 15, 2018

[CI] GetResultTests testToAndFromXContent() failure #29080

Closed

cbuescher mentioned this pull request Mar 15, 2018

[Tests] Fix GetResultTests and DocumentFieldTests failures #29083

Merged

dakrone deleted the de-bytesreference-xcontentbuilder branch April 19, 2018 14:45

javanna mentioned this pull request Jun 14, 2018

6.3 XContentBuilder.string() removed #31326

Closed

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

cbuescher mentioned this pull request Sep 4, 2019

fix a wrong guide tip #46164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple XContentBuilder from BytesReference #28972

Decouple XContentBuilder from BytesReference #28972

dakrone commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

dakrone Mar 13, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

dakrone Mar 13, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

nik9000 Mar 12, 2018

dakrone commented Mar 13, 2018

		@@ -38,6 +41,16 @@

		private Integer hash = null; // we cache the hash of this reference since it can be quite costly to re-calculated it

		public static BytesReference bytes(XContentBuilder xContentBuilder) {

Decouple XContentBuilder from BytesReference #28972

Decouple XContentBuilder from BytesReference #28972

Conversation

dakrone commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

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

dakrone commented Mar 13, 2018