Skip to content

Begin moving XContent to a separate lib/artifact #29300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 2, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"org.elasticsearch:elasticsearch-secure-sm:${version}": ':libs:secure-sm',
"org.elasticsearch.client:elasticsearch-rest-client:${version}": ':client:rest',
"org.elasticsearch.client:elasticsearch-rest-client-sniffer:${version}": ':client:sniffer',
Expand Down
2 changes: 0 additions & 2 deletions buildSrc/src/main/resources/checkstyle_suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,7 @@
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]util[/\\]concurrent[/\\]EsExecutors.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]util[/\\]concurrent[/\\]ThreadBarrier.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]util[/\\]concurrent[/\\]ThreadContext.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]xcontent[/\\]XContentFactory.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]xcontent[/\\]XContentHelper.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]common[/\\]xcontent[/\\]smile[/\\]SmileXContent.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]discovery[/\\]Discovery.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]discovery[/\\]DiscoverySettings.java" checks="LineLength" />
<suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]discovery[/\\]zen[/\\]ZenDiscovery.java" checks="LineLength" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,34 @@ public static boolean parseBoolean(String value) {
throw new IllegalArgumentException("Failed to parse value [" + value + "] as only [true] or [false] are allowed.");
}

private static boolean hasText(CharSequence str) {
if (str == null || str.length() == 0) {
return false;
}
int strLen = str.length();
for (int i = 0; i < strLen; i++) {
if (!Character.isWhitespace(str.charAt(i))) {
return true;
}
}
return false;
}

/**
*
* @param value text to parse.
* @param defaultValue The default value to return if the provided value is <code>null</code>.
* @return see {@link #parseBoolean(String)}
*/
public static boolean parseBoolean(String value, boolean defaultValue) {
if (Strings.hasText(value)) {
if (hasText(value)) {
return parseBoolean(value);
}
return defaultValue;
}

public static Boolean parseBoolean(String value, Boolean defaultValue) {
if (Strings.hasText(value)) {
if (hasText(value)) {
return parseBoolean(value);
}
return defaultValue;
Expand Down
85 changes: 85 additions & 0 deletions libs/xcontent/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import org.elasticsearch.gradle.precommit.PrecommitTasks

/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

apply plugin: 'elasticsearch.build'
apply plugin: 'nebula.optional-base'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

apply plugin: 'nebula.maven-base-publish'
apply plugin: 'nebula.maven-scm'

archivesBaseName = 'elasticsearch-xcontent'

publishing {
publications {
nebula {
artifactId = archivesBaseName
}
}
}

dependencies {
compile "org.elasticsearch:elasticsearch-core:${version}"

// json and yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

compile "org.yaml:snakeyaml:${versions.snakeyaml}"
compile "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"
compile "com.fasterxml.jackson.dataformat:jackson-dataformat-smile:${versions.jackson}"
compile "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:${versions.jackson}"
compile "com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:${versions.jackson}"

testCompile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
testCompile "junit:junit:${versions.junit}"
testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"

if (isEclipse == false || project.path == ":libs:elasticsearch-xcontent-tests") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:libs:xcontent-tests maybe?

testCompile("org.elasticsearch.test:framework:${version}") {
exclude group: 'org.elasticsearch', module: 'elasticsearch-xcontent'
}
}

}

forbiddenApisMain {
// elasticsearch-xcontent does not depend on server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/elasticsearch-xcontent/xcontent/ i think.

// TODO: Need to decide how we want to handle for forbidden signatures with the changes to core
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt')]
}

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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this needs the new name as well.

main.java.srcDirs = ['java']
main.resources.srcDirs = ['resources']
} else {
test.java.srcDirs = ['java']
test.resources.srcDirs = ['resources']
}
}
}

thirdPartyAudit.excludes = [
// from com.fasterxml.jackson.dataformat.yaml.YAMLMapper (jackson-dataformat-yaml)
'com.fasterxml.jackson.databind.ObjectMapper',
]

dependencyLicenses {
mapping from: /jackson-.*/, to: 'jackson'
}
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public class ParseField {
private String allReplacedWith = null;
private final String[] allNames;

private static final String[] EMPTY = new String[0];

/**
* @param name
* the primary name for this field. This will be returned by
Expand All @@ -46,7 +48,7 @@ public class ParseField {
public ParseField(String name, String... deprecatedNames) {
this.name = name;
if (deprecatedNames == null || deprecatedNames.length == 0) {
this.deprecatedNames = Strings.EMPTY_ARRAY;
this.deprecatedNames = EMPTY;
} else {
final HashSet<String> set = new HashSet<>();
Collections.addAll(set, deprecatedNames);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.common.xcontent;

import org.elasticsearch.common.Booleans;

import java.io.IOException;
import java.util.Map;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.common.xcontent;

import org.elasticsearch.common.Booleans;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -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)}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be in the XContentGenerator class then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

*/
static void copyCurrentStructure(XContentGenerator destination, XContentParser parser) throws IOException {
XContentParser.Token token = parser.currentToken();

// Let's handle field-name separately first
if (token == XContentParser.Token.FIELD_NAME) {
destination.writeFieldName(parser.currentName());
token = parser.nextToken();
// fall-through to copy the associated value
}

switch (token) {
case START_ARRAY:
destination.writeStartArray();
while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
copyCurrentStructure(destination, parser);
}
destination.writeEndArray();
break;
case START_OBJECT:
destination.writeStartObject();
while (parser.nextToken() != XContentParser.Token.END_OBJECT) {
copyCurrentStructure(destination, parser);
}
destination.writeEndObject();
break;
default: // others are simple:
copyCurrentEvent(destination, parser);
}
}

static void copyCurrentEvent(XContentGenerator generator, XContentParser parser) throws IOException {
switch (parser.currentToken()) {
case START_OBJECT:
generator.writeStartObject();
break;
case END_OBJECT:
generator.writeEndObject();
break;
case START_ARRAY:
generator.writeStartArray();
break;
case END_ARRAY:
generator.writeEndArray();
break;
case FIELD_NAME:
generator.writeFieldName(parser.currentName());
break;
case VALUE_STRING:
if (parser.hasTextCharacters()) {
generator.writeString(parser.textCharacters(), parser.textOffset(), parser.textLength());
} else {
generator.writeString(parser.text());
}
break;
case VALUE_NUMBER:
switch (parser.numberType()) {
case INT:
generator.writeNumber(parser.intValue());
break;
case LONG:
generator.writeNumber(parser.longValue());
break;
case FLOAT:
generator.writeNumber(parser.floatValue());
break;
case DOUBLE:
generator.writeNumber(parser.doubleValue());
break;
}
break;
case VALUE_BOOLEAN:
generator.writeBoolean(parser.booleanValue());
break;
case VALUE_NULL:
generator.writeNull();
break;
case VALUE_EMBEDDED_OBJECT:
generator.writeBinary(parser.binaryValue());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

package org.elasticsearch.common.xcontent;

import org.elasticsearch.common.util.CollectionUtils;

import java.io.ByteArrayOutputStream;
import java.io.Closeable;
import java.io.Flushable;
Expand All @@ -35,6 +33,7 @@
import java.util.Date;
import java.util.GregorianCalendar;
import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

map(valueMap, ensureNoSelfReferences);
} else if (value instanceof Iterable) {
value((Iterable<?>) value, ensureNoSelfReferences);
} else if (value instanceof Object[]) {
Expand Down Expand Up @@ -799,7 +799,7 @@ private XContentBuilder map(Map<String, ?> values, boolean ensureNoSelfReference
// checks that the map does not contain references to itself because
// iterating over map entries will cause a stackoverflow error
if (ensureNoSelfReferences) {
CollectionUtils.ensureNoSelfReferences(values);
ensureNoSelfReferences(values);
}

startObject();
Expand Down Expand Up @@ -828,7 +828,7 @@ private XContentBuilder value(Iterable<?> values, boolean ensureNoSelfReferences
// checks that the iterable does not contain references to itself because
// iterating over entries will cause a stackoverflow error
if (ensureNoSelfReferences) {
CollectionUtils.ensureNoSelfReferences(values);
ensureNoSelfReferences(values);
}
startArray();
for (Object value : values) {
Expand Down Expand Up @@ -937,4 +937,39 @@ static void ensureNotNull(Object value, String message) {
throw new IllegalArgumentException(message);
}
}

private static void ensureNoSelfReferences(Object value) {
Iterable<?> it = convert(value);
if (it != null) {
ensureNoSelfReferences(it, value, Collections.newSetFromMap(new IdentityHashMap<>()));
}
}

private static Iterable<?> convert(Object value) {
if (value == null) {
return null;
}
if (value instanceof Map) {
return ((Map<?,?>) value).values();
} else if ((value instanceof Iterable) && (value instanceof Path == false)) {
return (Iterable<?>) value;
} else if (value instanceof Object[]) {
return Arrays.asList((Object[]) value);
} else {
return null;
}
}

private static void ensureNoSelfReferences(final Iterable<?> value, Object originalReference, final Set<Object> ancestors) {
if (value != null) {
if (ancestors.add(originalReference) == false) {
throw new IllegalArgumentException("Iterable object is self-referencing itself");
}
for (Object o : value) {
ensureNoSelfReferences(convert(o), o, ancestors);
}
ancestors.remove(originalReference);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import com.fasterxml.jackson.dataformat.cbor.CBORConstants;
import com.fasterxml.jackson.dataformat.smile.SmileConstants;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.xcontent.cbor.CborXContent;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.common.xcontent.smile.SmileXContent;
Expand Down Expand Up @@ -154,7 +153,8 @@ public static XContentType xContentType(CharSequence content) {
return XContentType.JSON;
}
// Should we throw a failure here? Smile idea is to use it in bytes....
if (length > 2 && first == SmileConstants.HEADER_BYTE_1 && content.charAt(1) == SmileConstants.HEADER_BYTE_2 && content.charAt(2) == SmileConstants.HEADER_BYTE_3) {
if (length > 2 && first == SmileConstants.HEADER_BYTE_1 && content.charAt(1) == SmileConstants.HEADER_BYTE_2 &&
content.charAt(2) == SmileConstants.HEADER_BYTE_3) {
return XContentType.SMILE;
}
if (length > 2 && first == '-' && content.charAt(1) == '-' && content.charAt(2) == '-') {
Expand Down Expand Up @@ -186,7 +186,7 @@ public static XContentType xContentType(CharSequence content) {
public static XContent xContent(CharSequence content) {
XContentType type = xContentType(content);
if (type == null) {
throw new ElasticsearchParseException("Failed to derive xcontent");
throw new XContentParseException("Failed to derive xcontent");
}
return xContent(type);
}
Expand All @@ -213,7 +213,7 @@ public static XContent xContent(byte[] data) {
public static XContent xContent(byte[] data, int offset, int length) {
XContentType type = xContentType(data, offset, length);
if (type == null) {
throw new ElasticsearchParseException("Failed to derive xcontent");
throw new XContentParseException("Failed to derive xcontent");
}
return xContent(type);
}
Expand Down Expand Up @@ -278,7 +278,8 @@ public static XContentType xContentType(byte[] bytes, int offset, int length) {
if (first == '{') {
return XContentType.JSON;
}
if (length > 2 && first == SmileConstants.HEADER_BYTE_1 && bytes[offset + 1] == SmileConstants.HEADER_BYTE_2 && bytes[offset + 2] == SmileConstants.HEADER_BYTE_3) {
if (length > 2 && first == SmileConstants.HEADER_BYTE_1 && bytes[offset + 1] == SmileConstants.HEADER_BYTE_2 &&
bytes[offset + 2] == SmileConstants.HEADER_BYTE_3) {
return XContentType.SMILE;
}
if (length > 2 && first == '-' && bytes[offset + 1] == '-' && bytes[offset + 2] == '-') {
Expand Down
Loading