Pass InputStream when creating XContent parser #28754

dakrone · 2018-02-20T23:18:18Z

Rather than passing the raw BytesReference in when creating the xcontent
parser, this passes the StreamInput (which is an InputStream), this allows us to
decouple XContent from BytesReference.

This also removes the use of commons.Booleans so it doesn't require more
external commons classes.

Related to #28504

Rather than passing the raw `BytesReference` in when creating the xcontent parser, this passes the StreamInput (which is an InputStream), this allows us to decouple XContent from BytesReference. This also removes the use of `commons.Booleans` so it doesn't require more external commons classes. Related to elastic#28504

jasontedor

Left one comment, no need for another pass.

jasontedor · 2018-02-21T01:52:53Z

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

    static boolean isStrictDuplicateDetectionEnabled() {
        // Don't allow duplicate keys in JSON content by default but let the user opt out
-        return Booleans.parseBoolean(System.getProperty("es.xcontent.strict_duplicate_detection", "true"));
+        String dupProp = System.getProperty("es.xcontent.strict_duplicate_detection", "true");


Can you make this one separate?

A separate PR?

Yes please, the change has nothing to do with the title (it does with the goal, but not the title).

Certainly, thanks for clarifying

javanna · 2018-02-21T10:11:58Z

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

    /**
     * Creates a parser over the provided bytes.
+     * @deprecated use {@link #createParser(NamedXContentRegistry, DeprecationHandler, InputStream)}
+     * instead, the BytesReference will be removed it a future commit


s/it/in , also not sure what you mean with "the BytesReference will be removed in a future commit", maybe the sentence needs to be clarified.

This removes the use of the `common.Boolean` class from two of the XContent classes, so they can be decoupled from the ES code as much as possible. Related to elastic#28754, elastic#28504

* Pass InputStream when creating XContent parser Rather than passing the raw `BytesReference` in when creating the xcontent parser, this passes the StreamInput (which is an InputStream), this allows us to decouple XContent from BytesReference. This also removes the use of `commons.Booleans` so it doesn't require more external commons classes. Related to #28504 * Undo boolean removal * Enhance deprecation javadoc

* es/master: (143 commits) Revert "Disable BWC tests for build issues" Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724) Build: Consolidate archives and packages configuration (#28760) Skip some plugins service tests on Windows Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749) Disable BWC tests for build issues Ensure that azure stream has socket privileges (#28751) [DOCS] Fixed broken link. Pass InputStream when creating XContent parser (#28754) [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677 Delay path expansion on Windows [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween [Tests] Extract the testing logic for Google Cloud Storage (#28576) [Docs] Update links to java9 docs (#28750) version set in ingest pipeline (#27573) Revert "Add startup logging for standalone tests" Tests: don't wait for completion while trying to get completed task Add 5.6.9 snapshot version [Docs] Java high-level REST client : clean up (#28703) Updated distribution outputs in contributing docs ...

* es/6.x: (140 commits) Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724) Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749) Revert "Disable BWC tests for build issues" Skip some plugins service tests on Windows Build: Consolidate archives and packages configuration (#28760) Ensure that azure stream has socket privileges (#28773) Disable BWC tests for build issues Pass InputStream when creating XContent parser (#28754) [DOCS] Fixed broken link. [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677 Delay path expansion on Windows [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween REST high-level client: add support for Rollover Index API (#28698) [Tests] Extract the testing logic for Google Cloud Storage (#28576) Moved Grok helper code to a separate Gradle module and let ingest-common module depend on it. version set in ingest pipeline (#27573) [Docs] Update links to java9 docs (#28750) Revert "Add startup logging for standalone tests" Tests: don't wait for completion while trying to get completed task Add 5.6.9 snapshot version ...

jasontedor · 2018-02-27T19:55:01Z

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/JsonProcessor.java

        BytesReference bytesRef = (fieldValue == null) ? new BytesArray("null") : new BytesArray(fieldValue.toString());
        try (XContentParser parser = JsonXContent.jsonXContent
-                .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, bytesRef)) {
+                .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, bytesRef.streamInput())) {


These input streams need to be closed.

I believe closing the XContentParser closes the internal JsonParser, which closes the stream (similar to other wrapping chains with streams/readers).

That’s correct but it’s not enough. An exception could be thrown constructing the parser in which case the stream will not be closed.

* Remove Booleans use from XContent and ToXContent This removes the use of the `common.Boolean` class from two of the XContent classes, so they can be decoupled from the ES code as much as possible. Related to #28754, #28504

* Pass InputStream when creating XContent parser Rather than passing the raw `BytesReference` in when creating the xcontent parser, this passes the StreamInput (which is an InputStream), this allows us to decouple XContent from BytesReference. This also removes the use of `commons.Booleans` so it doesn't require more external commons classes. Related to elastic#28504 * Undo boolean removal * Enhance deprecation javadoc

* Remove Booleans use from XContent and ToXContent This removes the use of the `common.Boolean` class from two of the XContent classes, so they can be decoupled from the ES code as much as possible. Related to elastic#28754, elastic#28504

dakrone added >non-issue v7.0.0 v6.3.0 labels Feb 20, 2018

dakrone requested a review from nik9000 February 20, 2018 23:18

jasontedor approved these changes Feb 21, 2018

View reviewed changes

javanna reviewed Feb 21, 2018

View reviewed changes

dakrone added 2 commits February 21, 2018 07:55

Undo boolean removal

56cd6ed

Enhance deprecation javadoc

535490a

dakrone mentioned this pull request Feb 21, 2018

Remove Booleans use from XContent and ToXContent #28768

Merged

dakrone merged commit d7eae4b into elastic:master Feb 21, 2018

dakrone deleted the cp-remove-bytesreference branch February 22, 2018 21:52

dakrone restored the cp-remove-bytesreference branch February 22, 2018 21:54

dakrone deleted the cp-remove-bytesreference branch February 22, 2018 21:54

dakrone restored the cp-remove-bytesreference branch February 22, 2018 21:54

jasontedor reviewed Feb 27, 2018

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pass InputStream when creating XContent parser #28754

Pass InputStream when creating XContent parser #28754

Uh oh!

dakrone commented Feb 20, 2018

Uh oh!

jasontedor left a comment

Uh oh!

jasontedor Feb 21, 2018

Uh oh!

dakrone Feb 21, 2018

Uh oh!

jasontedor Feb 21, 2018

Uh oh!

dakrone Feb 21, 2018

Uh oh!

javanna Feb 21, 2018

Uh oh!

jasontedor Feb 27, 2018

Uh oh!

rjernst Feb 27, 2018 •

edited

Loading

Uh oh!

jasontedor Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Pass InputStream when creating XContent parser #28754

Pass InputStream when creating XContent parser #28754

Uh oh!

Conversation

dakrone commented Feb 20, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

dakrone Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

jasontedor Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

dakrone Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

javanna Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

jasontedor Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

rjernst Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Feb 27, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rjernst Feb 27, 2018 •

edited

Loading