Begin moving XContent to a separate lib/artifact #29300

dakrone · 2018-03-29T17:34:59Z

This commit moves a large portion of the XContent code from the server project
to the libs/xcontent project. For the pieces that have been moved, some
helpers have been duplicated to allow them to be decoupled from ES helper
classes. In addition, Booleans and CheckedFunction have been moved to the
elasticsearch-core project.

This decoupling is a move so that we can eventually make things like the
high-level REST client not rely on the entire ES jar, only the parts it needs.

There are some pieces that are still not decoupled, in particular some of the
XContent tests still remain in the server project, this is because they test a
large portion of the pluggable xcontent pieces through
XContentElasticsearchException. They may be decoupled in future work.
Additionally, there may be more piecese that we want to move to the xcontent lib
in the future that are not part of this PR, this is a starting point.

Relates to #28504

This commit moves a large portion of the XContent code from the `server` project to the `libs/xcontent` project. For the pieces that have been moved, some helpers have been duplicated to allow them to be decoupled from ES helper classes. In addition, `Booleans` and `CheckedFunction` have been moved to the `elasticsearch-core` project. This decoupling is a move so that we can eventually make things like the high-level REST client not rely on the entire ES jar, only the parts it needs. There are some pieces that are still not decoupled, in particular some of the XContent tests still remain in the server project, this is because they test a large portion of the pluggable xcontent pieces through `XContentElasticsearchException`. They may be decoupled in future work. Additionally, there may be more piecese that we want to move to the xcontent lib in the future that are not part of this PR, this is a starting point. Relates to elastic#28504

elasticmachine · 2018-03-29T17:35:00Z

Pinging @elastic/es-core-infra

nik9000

I left a few things. The biggest one to me is simpleMatch which I think we should keep in :server if we can.

nik9000 · 2018-03-29T18:25:54Z

libs/xcontent/build.gradle

+dependencies {
+    compile "org.elasticsearch:elasticsearch-core:${version}"
+
+    // json and yaml


This comment doesn't help me to understand the file better. Maybe remove it?

I see. It came from :server's javadoc. I think it isn't worth keeping.

nik9000 · 2018-03-29T18:26:15Z

libs/xcontent/build.gradle

+    testCompile "junit:junit:${versions.junit}"
+    testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"
+
+    if (isEclipse == false || project.path == ":libs:elasticsearch-xcontent-tests") {


:libs:xcontent-tests maybe?

nik9000 · 2018-03-29T18:27:20Z

libs/xcontent/build.gradle

+}
+
+forbiddenApisMain {
+    // elasticsearch-xcontent does not depend on server


s/elasticsearch-xcontent/xcontent/ i think.

nik9000 · 2018-03-29T18:28:17Z

libs/xcontent/build.gradle

+
+forbiddenApisMain {
+    // elasticsearch-xcontent does not depend on server
+    // TODO: Need to decide how we want to handle for forbidden signatures with the changes to core


I think you mean that we should have more forbidden signatures here than just what is in jdk-signatures but less than everything in the normal elasticsearch signatures.

If so, then that makes sense to me and I'm happy to think of that in a followup.

nik9000 · 2018-03-29T18:28:26Z

libs/xcontent/build.gradle

+if (isEclipse) {
+    // in eclipse the project is under a fake root, we need to change around the source sets
+    sourceSets {
+        if (project.path == ":libs:elasticsearch-xcontent") {


I believe this needs the new name as well.

nik9000 · 2018-03-29T18:29:37Z

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

@@ -106,4 +108,88 @@ XContentParser createParser(NamedXContentRegistry xContentRegistry,
     */
    XContentParser createParser(NamedXContentRegistry xContentRegistry,
            DeprecationHandler deprecationHandler, Reader reader) throws IOException;
+
+    /**
+     * Low level implementation detail of {@link XContentGenerator#copyCurrentStructure(XContentParser)}.


Should it be in the XContentGenerator class then?

That way it can be private. Which just feels better to me.

This can't be private (it's used elsewhere here and in x-pack), I can move it if you want but I'm not sure it should go in Generator since it deals with both the Generator and a Parser (which is why I picked XContent)

Can all the call sites call the method on XContentBuilder? I think the difference is that that one might use jackson APIs sometimes but I'm not sure why we wouldn't want that.

Can all the call sites call the method on XContentBuilder?

No, this version of copyCurrentStructure supports copying across different types of XContent (so you can copy from a CborParser into a JsonGenerator), that's the difference between using the XContentType-specific one and this generic one

Oh I think I understand more what you're saying now, let me look into it and see if it can be made protected

Okay, I was able to do this, it's private now.

nik9000 · 2018-03-29T18:31:02Z

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

@@ -740,7 +739,8 @@ private void unknownValue(Object value, boolean ensureNoSelfReferences) throws I
            //Path implements Iterable<Path> and causes endless recursion and a StackOverFlow if treated as an Iterable here
            value((Path) value);
        } else if (value instanceof Map) {
-            map((Map<String,?>) value, ensureNoSelfReferences);
+            @SuppressWarnings("unchecked") final Map<String, ?> valueMap = (Map<String, ?>) value;


I tend to put a line break after the annotation. Force of habit, really.

nik9000 · 2018-03-29T18:38:41Z

libs/xcontent/src/main/java/org/elasticsearch/common/xcontent/support/filtering/FilterPath.java

            return next;
        }
        return null;
    }

+    /**


I don't really want to leak our "funny" simpleMatch behavior into different places. I wonder if can accept two nullable TokenFilters and the caller can use FilterPath if it wants.

nik9000 · 2018-03-29T18:40:43Z

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java

@@ -349,7 +349,7 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
                try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY,
                        LoggingDeprecationHandler.INSTANCE, document)) {
                    parser.nextToken();
-                    XContentHelper.copyCurrentStructure(builder.generator(), parser);
+                    XContent.copyCurrentStructure(builder.generator(), parser);


can we call builder.copyCurrentStructure(parser) instead?

Yep, this is done now

rjernst

I left a few thoughts.

rjernst · 2018-03-29T18:31:01Z

libs/xcontent/build.gradle

+ */
+
+apply plugin: 'elasticsearch.build'
+apply plugin: 'nebula.optional-base'


Is this needed? I don't see the optional configuration used here?

This is copied from libs/elasticsearch-core, I don't actually know what it does, should I remove it?

Yes, remove (it's not needed anywhere except in server).

I believe it makes it easy to declare dependencies as optional. https://github.com/nebula-plugins/gradle-extra-configurations-plugin

If we aren't using it we may as well remove it so we no one gets confused.

rjernst · 2018-03-29T18:35:46Z

libs/xcontent/build.gradle

+
+forbiddenApisMain {
+    // elasticsearch-xcontent does not depend on server
+    // TODO: Need to decide how we want to handle for forbidden signatures with the changes to core


We should have a followup item to separate out forbidden signatures that are truly server specific (eg log4j apis that require server deps) and those that should really be shared by all. We already have an "es-all-signatures", that should probably be used here as a start (in addition to jdk signatures)?

This is copied from the elasticsearch-core build.gradle file, is there already a followup item for this then?

Can you explain what you mean by using the "es-all-signatures" here? (like I said, I copied it, so I'm not that familiar yet with configuring it)

We have a number of different signature files. In particular, in addition to the jdk signatures provided by forbiddenapis itself, we have an "es-all" signature file that is supposed to apply across main and test code. Then we have "es-server" and "es-test". We should at least be using the es-all here I think (it only contains jdk methods, I believe). es-server is a little more convoluted, because it has mostly jdk methods, but also some ES server specific code (PathUtils.get), or ES server specific dependency code (log4j methods). So here I am suggesting starting by adding the es-all signatures file. I don't think we have an existing issue to split the signature files more, but it is worth a discussion on where the breakdown should be (how fine grained we should get).

I checked into adding the es-all-signatures here, but it fails because it is checking for a Lucene forbiddenapi, which isn't on the classpath for xcontent (since no lucene dependency)

rjernst · 2018-03-29T18:42:18Z

libs/xcontent/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java

+     * @return the number of bytes copied
+     * @throws IOException in case of I/O errors
+     */
+    private static long copyStream(InputStream in, OutputStream out) throws IOException {


Just a note: java 9 provides this for us, we may want to copy Streams (at least this method) to core and then make core a MR JAR so we can make use of the java 9 method:

https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream-

++, though maybe it'd be simpler to do that in a followup?

Sounds good, I can do that in a follow up (just to keep this a bit smaller) if that's okay with you

rjernst · 2018-03-29T18:50:25Z

libs/xcontent/src/main/java/org/elasticsearch/common/xcontent/support/filtering/FilterPath.java

+     * @param str     the String to match
+     * @return whether the String matches the given pattern
+     */
+    private static boolean simpleMatch(String pattern, String str) {


I think we should have the discussion for each of these copied methods whether it is worthwhile to move these into core. While I'm not a fan of this method (or the naming, since it is really a glob), there seem to be enough uses at a glance (at least, some that will also likely move into libs, like ingest) that it could warrant moving there.

Yeah, if we're already going to want it for ingest, maybe we should move it there.

Okay, I'll move this to core and rename it to have a better name (probably globMatch)

nik9000 · 2018-03-30T14:52:28Z

libs/elasticsearch-core/src/main/java/org/elasticsearch/common/Glob.java

+     * @param str     the String to match
+     * @return whether the String matches the given pattern
+     */
+    public static boolean globMatch(String pattern, String str) {


jasontedor · 2018-03-30T16:21:25Z

build.gradle

@@ -196,6 +196,7 @@ subprojects {
    "org.elasticsearch:elasticsearch-cli:${version}": ':server:cli',
    "org.elasticsearch:elasticsearch-core:${version}": ':libs:elasticsearch-core',
    "org.elasticsearch:elasticsearch-nio:${version}": ':libs:elasticsearch-nio',
+    "org.elasticsearch:elasticsearch-xcontent:${version}": ':libs:xcontent',


Please make this x-content. This is consistent with established naming conventions. Sorry.

jasontedor

LGTM. Thank you! ❤️

dakrone · 2018-03-30T16:27:11Z

@elasticmachine please run packaging tests

jasontedor · 2018-03-30T16:30:41Z

run packaging tests

This moves the method `Streams.copy(InputStream in, OutputStream out)` into the `elasticsearch-core` project (inside the `o.e.core.internal.io` package). It also makes this class into a multi-release class where the Java 9 equivalent uses `InputStream#transferTo`. This is a followup from elastic#29300 (comment)

* Begin moving XContent to a separate lib/artifact This commit moves a large portion of the XContent code from the `server` project to the `libs/xcontent` project. For the pieces that have been moved, some helpers have been duplicated to allow them to be decoupled from ES helper classes. In addition, `Booleans` and `CheckedFunction` have been moved to the `elasticsearch-core` project. This decoupling is a move so that we can eventually make things like the high-level REST client not rely on the entire ES jar, only the parts it needs. There are some pieces that are still not decoupled, in particular some of the XContent tests still remain in the server project, this is because they test a large portion of the pluggable xcontent pieces through `XContentElasticsearchException`. They may be decoupled in future work. Additionally, there may be more piecese that we want to move to the xcontent lib in the future that are not part of this PR, this is a starting point. Relates to #28504

Relates #29300

* master: (80 commits) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300) Java versions for ci (elastic#29320) Minor cleanup in the InternalEngine (elastic#29241) Clarify expectations of false positives/negatives (elastic#27964) Update docs on vertex ordering (elastic#27963) Revert "REST high-level client: add support for Indices Update Settings API (elastic#28892)" (elastic#29323) [test] remove Streamable serde assertions (elastic#29307) Improve query string docs (elastic#28882) fix query string example for boolean query (elastic#28881) Resolve unchecked cast warnings introduced with elastic#28892 REST high-level client: add support for Indices Update Settings API (elastic#28892) Search: Validate script query is run with a single script (elastic#29304) [DOCS] Added info on WGS-84. Closes issue elastic#3590 (elastic#29305) Increase timeout on Netty client latch for tests Build: Use branch specific refspec sysprop for bwc builds (elastic#29299) TEST: trim unsafe commits before opening engine Move trimming unsafe commits from engine ctor to store (elastic#29260) Fix incorrect geohash for lat 90, lon 180 (elastic#29256) Do not load global state when deleting a snapshot (elastic#29278) ...

* master: Build: Fix Java9 MR build (elastic#29312) Reindex: Fix error in delete-by-query rest spec (elastic#29318) Improve similarity integration. (elastic#29187) Fix some query extraction bugs. (elastic#29283) [Docs] Correct experimental note formatting Move Nullable into core (elastic#29341) [Docs] Update getting-started.asciidoc (elastic#29294) Elasticsearch 6.3.0 is now on Lucene 7.3. [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677) Fix missing comma in ingest-node.asciidoc (elastic#29343) Improve exception handling on TransportMasterNodeAction (elastic#29314) Don't break allocation if resize source index is missing (elastic#29311) Use fixture to test repository-s3 plugin (elastic#29296) Fix NDCG for empty search results (elastic#29267) Pass through script params in scripted metric agg (elastic#29154) Fix Eclipse build. Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298) Painless: Remove extraneous INLINE constant. (elastic#29340) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300)

#29322) * Move Streams.copy into elasticsearch-core and make a multi-release jar This moves the method `Streams.copy(InputStream in, OutputStream out)` into the `elasticsearch-core` project (inside the `o.e.core.internal.io` package). It also makes this class into a multi-release class where the Java 9 equivalent uses `InputStream#transferTo`. This is a followup from #29300 (comment)

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

nik9000 requested changes Mar 29, 2018

View reviewed changes

rjernst reviewed Mar 29, 2018

View reviewed changes

dakrone added 6 commits March 29, 2018 13:23

Remove nebula.optional-base plugin

af79dfb

Remove json and yaml comment

c7bda43

Rename some gradle things from elasticsearch-xcontent to xcontent

4bc0b97

Add a newline for suppression

f84637f

Use the generator's version of copyCurrentStructure

7d21a9c

Move the copied Regex.simpleMatch into new Glob.globMatch in core

00a3870

nik9000 approved these changes Mar 30, 2018

View reviewed changes

jasontedor requested changes Mar 30, 2018

View reviewed changes

dakrone added 2 commits March 30, 2018 10:25

Rename xcontent to x-content

544cfde

Merge remote-tracking branch 'origin/master' into start-xcontent-moving

491784b

jasontedor approved these changes Mar 30, 2018

View reviewed changes

dakrone and others added 2 commits March 30, 2018 10:53

ಠ_ಠ

d55d4ff

:hug:

f62f536

dakrone mentioned this pull request Mar 30, 2018

Move Streams.copy into elasticsearch-core and make a multi-release jar #29322

Merged

dakrone added 2 commits March 30, 2018 16:03

Merge remote-tracking branch 'origin/master' into start-xcontent-moving

1f03587

Merge remote-tracking branch 'origin/master' into start-xcontent-moving

ddcaa9f

dakrone merged commit 6b2167f into elastic:master Apr 2, 2018

jpountz added a commit that referenced this pull request Apr 3, 2018

Fix Eclipse build.

f8602b1

Relates #29300

jpountz added a commit that referenced this pull request Apr 3, 2018

Fix Eclipse build.

0f4dfc2

Relates #29300

dakrone deleted the start-xcontent-moving branch April 19, 2018 14:44

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

Begin moving XContent to a separate lib/artifact #29300

Begin moving XContent to a separate lib/artifact #29300

Conversation

dakrone commented Mar 29, 2018

elasticmachine commented Mar 29, 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

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

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

rjernst 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

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

jasontedor left a comment

Choose a reason for hiding this comment

dakrone commented Mar 30, 2018

jasontedor commented Mar 30, 2018