Skip to content
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

Begin moving XContent to a separate lib/artifact #29300

Merged
merged 13 commits into from
Apr 2, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* 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.
*/

package org.elasticsearch.common;

/**
* Utility class for glob-like matching
*/
public class Glob {

/**
* Match a String against the given pattern, supporting the following simple
* pattern styles: "xxx*", "*xxx", "*xxx*" and "xxx*yyy" matches (with an
* arbitrary number of pattern parts), as well as direct equality.
*
* @param pattern the pattern to match against
* @param str the String to match
* @return whether the String matches the given pattern
*/
public static boolean globMatch(String pattern, String str) {
Copy link
Member

Choose a reason for hiding this comment

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

++

if (pattern == null || str == null) {
return false;
}
int firstIndex = pattern.indexOf('*');
if (firstIndex == -1) {
return pattern.equals(str);
}
if (firstIndex == 0) {
if (pattern.length() == 1) {
return true;
}
int nextIndex = pattern.indexOf('*', firstIndex + 1);
if (nextIndex == -1) {
return str.endsWith(pattern.substring(1));
} else if (nextIndex == 1) {
// Double wildcard "**" - skipping the first "*"
return globMatch(pattern.substring(1), str);
}
String part = pattern.substring(1, nextIndex);
int partIndex = str.indexOf(part);
while (partIndex != -1) {
if (globMatch(pattern.substring(nextIndex), str.substring(partIndex + part.length()))) {
return true;
}
partIndex = str.indexOf(part, partIndex + 1);
}
return false;
}
return (str.length() >= firstIndex &&
pattern.substring(0, firstIndex).equals(str.substring(0, firstIndex)) &&
globMatch(pattern.substring(firstIndex), str.substring(firstIndex)));
}

}
83 changes: 83 additions & 0 deletions libs/xcontent/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
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.maven-base-publish'
apply plugin: 'nebula.maven-scm'

archivesBaseName = 'elasticsearch-xcontent'

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

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

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:xcontent-tests") {
testCompile("org.elasticsearch.test:framework:${version}") {
exclude group: 'org.elasticsearch', module: 'elasticsearch-xcontent'
}
}

}

forbiddenApisMain {
// xcontent does not depend on server
// 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:xcontent") {
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
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,9 @@ 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;
map(valueMap, ensureNoSelfReferences);
} else if (value instanceof Iterable) {
value((Iterable<?>) value, ensureNoSelfReferences);
} else if (value instanceof Object[]) {
Expand Down Expand Up @@ -799,7 +800,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 +829,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 +938,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