Skip to content

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

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.

Sign up for GitHub

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 1 commit into from
Jan 15, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 25 additions & 24 deletions server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,32 +773,23 @@ public XContentBuilder field(String name, Object value) throws IOException {
}

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;
Expand All @@ -810,11 +801,11 @@ private void unknownValue(Object value) throws IOException {
//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) {
Expand Down Expand Up @@ -863,18 +854,25 @@ public XContentBuilder field(String name, Map<String, Object> values) throws IOE
}

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;
Expand All @@ -884,7 +882,7 @@ public XContentBuilder field(String name, Iterable<?> values) throws IOException
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();
}
Expand All @@ -895,11 +893,14 @@ private XContentBuilder value(Iterable<?> values) throws IOException {
} 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();
}
Expand Down Expand Up @@ -1076,9 +1077,9 @@ private static void ensureNoSelfReferences(final Object value, final Set<Object>

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 {
Expand Down
1 change: 0 additions & 1 deletion server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,6 @@ public void testObjects() throws Exception {
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());
}
}
Expand Down