Avoid doing redundant work when checking for self references. (#26927)

Currently we test all maps, arrays or iterables. However, in the case that maps
contain sub maps for instance, we will test the sub maps again even though the
work has already been done for the top-level map.

Relates #26907
This commit is contained in:
Adrien Grand 2018-01-15 18:36:32 +01:00 committed by GitHub
parent a16f80a832
commit 0a92e43f62
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 25 additions and 25 deletions

View File

@ -773,32 +773,23 @@ public final class XContentBuilder implements Releasable, Flushable {
}
public XContentBuilder array(String name, Object... values) throws IOException {
return field(name).values(values);
return field(name).values(values, true);
}
XContentBuilder values(Object[] values) throws IOException {
private XContentBuilder values(Object[] values, boolean ensureNoSelfReferences) throws IOException {
if (values == null) {
return nullValue();
}
// checks that the array of object does not contain references to itself because
// iterating over entries will cause a stackoverflow error
ensureNoSelfReferences(values);
startArray();
for (Object o : values) {
value(o);
}
endArray();
return this;
return value(Arrays.asList(values), ensureNoSelfReferences);
}
public XContentBuilder value(Object value) throws IOException {
unknownValue(value);
unknownValue(value, true);
return this;
}
private void unknownValue(Object value) throws IOException {
private void unknownValue(Object value, boolean ensureNoSelfReferences) throws IOException {
if (value == null) {
nullValue();
return;
@ -810,11 +801,11 @@ public final class XContentBuilder implements Releasable, Flushable {
//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) value);
map((Map<String,?>) value, ensureNoSelfReferences);
} else if (value instanceof Iterable) {
value((Iterable<?>) value);
value((Iterable<?>) value, ensureNoSelfReferences);
} else if (value instanceof Object[]) {
values((Object[]) value);
values((Object[]) value, ensureNoSelfReferences);
} else if (value instanceof Calendar) {
value((Calendar) value);
} else if (value instanceof ReadableInstant) {
@ -863,18 +854,25 @@ public final class XContentBuilder implements Releasable, Flushable {
}
public XContentBuilder map(Map<String, ?> values) throws IOException {
return map(values, true);
}
private XContentBuilder map(Map<String, ?> values, boolean ensureNoSelfReferences) throws IOException {
if (values == null) {
return nullValue();
}
// checks that the map does not contain references to itself because
// iterating over map entries will cause a stackoverflow error
ensureNoSelfReferences(values);
if (ensureNoSelfReferences) {
ensureNoSelfReferences(values);
}
startObject();
for (Map.Entry<String, ?> value : values.entrySet()) {
field(value.getKey());
unknownValue(value.getValue());
// pass ensureNoSelfReferences=false as we already performed the check at a higher level
unknownValue(value.getValue(), false);
}
endObject();
return this;
@ -884,7 +882,7 @@ public final class XContentBuilder implements Releasable, Flushable {
return field(name).value(values);
}
private XContentBuilder value(Iterable<?> values) throws IOException {
private XContentBuilder value(Iterable<?> values, boolean ensureNoSelfReferences) throws IOException {
if (values == null) {
return nullValue();
}
@ -895,11 +893,14 @@ public final class XContentBuilder implements Releasable, Flushable {
} else {
// checks that the iterable does not contain references to itself because
// iterating over entries will cause a stackoverflow error
ensureNoSelfReferences(values);
if (ensureNoSelfReferences) {
ensureNoSelfReferences(values);
}
startArray();
for (Object value : values) {
unknownValue(value);
// pass ensureNoSelfReferences=false as we already performed the check at a higher level
unknownValue(value, false);
}
endArray();
}
@ -1076,9 +1077,9 @@ public final class XContentBuilder implements Releasable, Flushable {
Iterable<?> it;
if (value instanceof Map) {
it = ((Map) value).values();
it = ((Map<?,?>) value).values();
} else if ((value instanceof Iterable) && (value instanceof Path == false)) {
it = (Iterable) value;
it = (Iterable<?>) value;
} else if (value instanceof Object[]) {
it = Arrays.asList((Object[]) value);
} else {

View File

@ -534,7 +534,6 @@ public abstract class BaseXContentTestCase extends ESTestCase {
final String expected = o.getKey();
assertResult(expected, () -> builder().startObject().field("objects", o.getValue()).endObject());
assertResult(expected, () -> builder().startObject().field("objects").value(o.getValue()).endObject());
assertResult(expected, () -> builder().startObject().field("objects").values(o.getValue()).endObject());
assertResult(expected, () -> builder().startObject().array("objects", o.getValue()).endObject());
}
}