From 99390499522b732f2377f242df36388982a87d10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 9 Dec 2016 10:28:00 +0100 Subject: [PATCH 1/6] Factor out sort values from InternalSearchHit This adds fromXContent method and unit test for sort values that are part of InternalSearchHit. In order to centralize serialisation and xContent parsing and rendering code, move all relevant parts to a new class which can be unit tested much better in isolation.This is part of the preparation for parsing search responses on the client side. --- .../search/internal/InternalSearchHit.java | 93 +--------- .../search/internal/SearchSortValues.java | 172 ++++++++++++++++++ .../internal/SearchSortValuesTests.java | 127 +++++++++++++ 3 files changed, 305 insertions(+), 87 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java create mode 100644 core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java diff --git a/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java b/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java index 82324e3edf78b..4c149b127ab2a 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java +++ b/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java @@ -20,7 +20,6 @@ package org.elasticsearch.search.internal; import org.apache.lucene.search.Explanation; -import org.apache.lucene.util.BytesRef; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; @@ -48,7 +47,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -67,8 +65,6 @@ public class InternalSearchHit implements SearchHit { - private static final Object[] EMPTY_SORT_VALUES = new Object[0]; - private transient int docId; private float score = Float.NEGATIVE_INFINITY; @@ -86,7 +82,7 @@ public class InternalSearchHit implements SearchHit { private Map highlightFields = null; - private Object[] sortValues = EMPTY_SORT_VALUES; + private SearchSortValues sortValues = new SearchSortValues(new Object[0]); private String[] matchedQueries = Strings.EMPTY_ARRAY; @@ -343,17 +339,12 @@ public void highlightFields(Map highlightFields) { } public void sortValues(Object[] sortValues, DocValueFormat[] sortValueFormats) { - this.sortValues = Arrays.copyOf(sortValues, sortValues.length); - for (int i = 0; i < sortValues.length; ++i) { - if (this.sortValues[i] instanceof BytesRef) { - this.sortValues[i] = sortValueFormats[i].format((BytesRef) sortValues[i]); - } - } + this.sortValues = new SearchSortValues(sortValues, sortValueFormats); } @Override public Object[] sortValues() { - return sortValues; + return sortValues.sortValues(); } @Override @@ -499,13 +490,7 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t } builder.endObject(); } - if (sortValues != null && sortValues.length > 0) { - builder.startArray(Fields.SORT); - for (Object sortValue : sortValues) { - builder.value(sortValue); - } - builder.endArray(); - } + sortValues.toXContent(builder, params); if (matchedQueries.length > 0) { builder.startArray(Fields.MATCHED_QUERIES); for (String matchedFilter : matchedQueries) { @@ -603,34 +588,7 @@ public void readFrom(StreamInput in) throws IOException { this.highlightFields = unmodifiableMap(highlightFields); } - size = in.readVInt(); - if (size > 0) { - sortValues = new Object[size]; - for (int i = 0; i < sortValues.length; i++) { - byte type = in.readByte(); - if (type == 0) { - sortValues[i] = null; - } else if (type == 1) { - sortValues[i] = in.readString(); - } else if (type == 2) { - sortValues[i] = in.readInt(); - } else if (type == 3) { - sortValues[i] = in.readLong(); - } else if (type == 4) { - sortValues[i] = in.readFloat(); - } else if (type == 5) { - sortValues[i] = in.readDouble(); - } else if (type == 6) { - sortValues[i] = in.readByte(); - } else if (type == 7) { - sortValues[i] = in.readShort(); - } else if (type == 8) { - sortValues[i] = in.readBoolean(); - } else { - throw new IOException("Can't match type [" + type + "]"); - } - } - } + sortValues = new SearchSortValues(in); size = in.readVInt(); if (size > 0) { @@ -681,46 +639,7 @@ public void writeTo(StreamOutput out) throws IOException { highlightField.writeTo(out); } } - - if (sortValues.length == 0) { - out.writeVInt(0); - } else { - out.writeVInt(sortValues.length); - for (Object sortValue : sortValues) { - if (sortValue == null) { - out.writeByte((byte) 0); - } else { - Class type = sortValue.getClass(); - if (type == String.class) { - out.writeByte((byte) 1); - out.writeString((String) sortValue); - } else if (type == Integer.class) { - out.writeByte((byte) 2); - out.writeInt((Integer) sortValue); - } else if (type == Long.class) { - out.writeByte((byte) 3); - out.writeLong((Long) sortValue); - } else if (type == Float.class) { - out.writeByte((byte) 4); - out.writeFloat((Float) sortValue); - } else if (type == Double.class) { - out.writeByte((byte) 5); - out.writeDouble((Double) sortValue); - } else if (type == Byte.class) { - out.writeByte((byte) 6); - out.writeByte((Byte) sortValue); - } else if (type == Short.class) { - out.writeByte((byte) 7); - out.writeShort((Short) sortValue); - } else if (type == Boolean.class) { - out.writeByte((byte) 8); - out.writeBoolean((Boolean) sortValue); - } else { - throw new IOException("Can't handle sort field value of type [" + type + "]"); - } - } - } - } + sortValues.writeTo(out); if (matchedQueries.length == 0) { out.writeVInt(0); diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java new file mode 100644 index 0000000000000..a24d0cfe83c8c --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java @@ -0,0 +1,172 @@ +/* + * 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.search.internal; + +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.action.support.ToXContentToBytes; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.internal.InternalSearchHit.Fields; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Objects; + +public class SearchSortValues extends ToXContentToBytes implements ToXContent, Writeable { + + private final Object[] sortValues; + + SearchSortValues(Object[] sortValues) { + this.sortValues = Objects.requireNonNull(sortValues, "sort values must not be empty"); + } + + public SearchSortValues(Object[] sortValues, DocValueFormat[] sortValueFormats) { + this.sortValues = Arrays.copyOf(sortValues, sortValues.length); + for (int i = 0; i < sortValues.length; ++i) { + if (this.sortValues[i] instanceof BytesRef) { + this.sortValues[i] = sortValueFormats[i].format((BytesRef) sortValues[i]); + } + } + } + + public SearchSortValues(StreamInput in) throws IOException { + int size = in.readVInt(); + if (size > 0) { + sortValues = new Object[size]; + for (int i = 0; i < sortValues.length; i++) { + byte type = in.readByte(); + if (type == 0) { + sortValues[i] = null; + } else if (type == 1) { + sortValues[i] = in.readString(); + } else if (type == 2) { + sortValues[i] = in.readInt(); + } else if (type == 3) { + sortValues[i] = in.readLong(); + } else if (type == 4) { + sortValues[i] = in.readFloat(); + } else if (type == 5) { + sortValues[i] = in.readDouble(); + } else if (type == 6) { + sortValues[i] = in.readByte(); + } else if (type == 7) { + sortValues[i] = in.readShort(); + } else if (type == 8) { + sortValues[i] = in.readBoolean(); + } else { + throw new IOException("Can't match type [" + type + "]"); + } + } + } else { + sortValues = new Object[0]; + } + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + if (sortValues.length > 0) { + builder.startArray(Fields.SORT); + for (Object sortValue : sortValues) { + builder.value(sortValue); + } + builder.endArray(); + } + return builder; + } + + public static SearchSortValues fromXContent(XContentParser parser) throws IOException { + assert parser.currentToken() == XContentParser.Token.FIELD_NAME; + assert parser.currentName().equals(Fields.SORT); + XContentParser.Token token = parser.nextToken(); + assert token == XContentParser.Token.START_ARRAY; + return new SearchSortValues(parser.list().toArray()); + } + + public Object[] sortValues() { + return sortValues; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVInt(sortValues.length); + if (sortValues.length > 0) { + for (Object sortValue : sortValues) { + if (sortValue == null) { + out.writeByte((byte) 0); + } else { + Class type = sortValue.getClass(); + if (type == String.class) { + out.writeByte((byte) 1); + out.writeString((String) sortValue); + } else if (type == Integer.class) { + out.writeByte((byte) 2); + out.writeInt((Integer) sortValue); + } else if (type == Long.class) { + out.writeByte((byte) 3); + out.writeLong((Long) sortValue); + } else if (type == Float.class) { + out.writeByte((byte) 4); + out.writeFloat((Float) sortValue); + } else if (type == Double.class) { + out.writeByte((byte) 5); + out.writeDouble((Double) sortValue); + } else if (type == Byte.class) { + out.writeByte((byte) 6); + out.writeByte((Byte) sortValue); + } else if (type == Short.class) { + out.writeByte((byte) 7); + out.writeShort((Short) sortValue); + } else if (type == Boolean.class) { + out.writeByte((byte) 8); + out.writeBoolean((Boolean) sortValue); + } else { + throw new IOException("Can't handle sort field value of type [" + type + "]"); + } + } + } + } + } + + public boolean xContentEquals(ToXContentToBytes other) { + return this.toString().equals(other.toString()); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + SearchSortValues other = (SearchSortValues) obj; + return Arrays.equals(sortValues, other.sortValues); + } + + @Override + public int hashCode() { + return Arrays.hashCode(sortValues); + } +} diff --git a/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java new file mode 100644 index 0000000000000..ae0cea6be76d9 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java @@ -0,0 +1,127 @@ +/* + * 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.search.internal; + +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.function.Supplier; + +import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; + +public class SearchSortValuesTests extends ESTestCase { + + public static SearchSortValues createTestItem() { + List> valueSuppliers = new ArrayList<>(); + // this should reflect all values that are allowed to go through the transport layer + valueSuppliers.add(() -> null); + valueSuppliers.add(() -> randomInt()); + valueSuppliers.add(() -> randomLong()); + valueSuppliers.add(() -> randomDouble()); + valueSuppliers.add(() -> randomFloat()); + valueSuppliers.add(() -> randomByte()); + valueSuppliers.add(() -> randomShort()); + valueSuppliers.add(() -> randomBoolean()); + valueSuppliers.add(() -> frequently() ? randomAsciiOfLengthBetween(1, 30) : randomRealisticUnicodeOfCodepointLength(30)); + + int size = randomInt(20); + Object[] values = new Object[size]; + for (int i = 0; i < size; i++) { + Supplier supplier = randomFrom(valueSuppliers); + values[i] = supplier.get(); + } + return new SearchSortValues(values); + } + + public void testFromXContent() throws IOException { + SearchSortValues sortValues = createTestItem(); + XContentType xcontentType = randomFrom(XContentType.values()); + XContentBuilder builder = XContentFactory.contentBuilder(xcontentType); + if (randomBoolean()) { + builder.prettyPrint(); + } + builder.startObject(); // we need to wrap xContent output in proper object to create a parser for it + builder = sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + + XContentParser parser = xcontentType.xContent().createParser(builder.bytes()); + parser.nextToken(); // skip to the elements field name token, fromXContent advances from there if called from ourside + parser.nextToken(); + if (sortValues.sortValues().length > 0) { + SearchSortValues parsed = SearchSortValues.fromXContent(parser); + assertTrue(sortValues.xContentEquals(parsed)); + parser.nextToken(); + } + assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken()); + assertNull(parser.nextToken()); + } + + public void testToXContent() throws IOException { + SearchSortValues sortValues = new SearchSortValues(new Object[]{ 1, "foo", 3.0}); + XContentBuilder builder = JsonXContent.contentBuilder(); + builder.startObject(); + sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + assertEquals("{\"sort\":[1,\"foo\",3.0]}", builder.string()); + } + + /** + * Test equality and hashCode properties + */ + public void testEqualsAndHashcode() { + checkEqualsAndHashCode(createTestItem(), SearchSortValuesTests::copy, SearchSortValuesTests::mutate); + } + + public void testSerialization() throws IOException { + SearchSortValues sortValues = createTestItem(); + try (BytesStreamOutput output = new BytesStreamOutput()) { + sortValues.writeTo(output); + try (StreamInput in = output.bytes().streamInput()) { + SearchSortValues deserializedCopy = new SearchSortValues(in); + assertEquals(sortValues, deserializedCopy); + assertEquals(sortValues.hashCode(), deserializedCopy.hashCode()); + assertNotSame(sortValues, deserializedCopy); + } + } + } + + private static SearchSortValues mutate(SearchSortValues original) { + Object[] sortValues = original.sortValues(); + if (sortValues.length == 0) { + return new SearchSortValues(new Object[] { 1 }); + } + return new SearchSortValues(Arrays.copyOf(sortValues, sortValues.length + 1)); + } + + private static SearchSortValues copy(SearchSortValues original) { + return new SearchSortValues(Arrays.copyOf(original.sortValues(), original.sortValues().length)); + } +} From a7ae785e063428a3d8e2a529aeba599fb8fa9faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 12 Dec 2016 19:02:13 +0100 Subject: [PATCH 2/6] review comments --- .../search/internal/InternalSearchHit.java | 2 +- .../search/internal/SearchSortValues.java | 98 +++++++++---------- .../internal/SearchSortValuesTests.java | 45 ++++++++- 3 files changed, 91 insertions(+), 54 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java b/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java index 4c149b127ab2a..76de9740c8e62 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java +++ b/core/src/main/java/org/elasticsearch/search/internal/InternalSearchHit.java @@ -82,7 +82,7 @@ public class InternalSearchHit implements SearchHit { private Map highlightFields = null; - private SearchSortValues sortValues = new SearchSortValues(new Object[0]); + private SearchSortValues sortValues = SearchSortValues.EMPTY; private String[] matchedQueries = Strings.EMPTY_ARRAY; diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java index a24d0cfe83c8c..f00f180baa4dd 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java @@ -20,13 +20,13 @@ package org.elasticsearch.search.internal; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.action.support.ToXContentToBytes; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.internal.InternalSearchHit.Fields; @@ -34,8 +34,9 @@ import java.util.Arrays; import java.util.Objects; -public class SearchSortValues extends ToXContentToBytes implements ToXContent, Writeable { +public class SearchSortValues implements ToXContent, Writeable { + static final SearchSortValues EMPTY = new SearchSortValues(new Object[0]); private final Object[] sortValues; SearchSortValues(Object[] sortValues) { @@ -43,6 +44,8 @@ public class SearchSortValues extends ToXContentToBytes implements ToXContent, W } public SearchSortValues(Object[] sortValues, DocValueFormat[] sortValueFormats) { + Objects.requireNonNull(sortValues); + Objects.requireNonNull(sortValueFormats); this.sortValues = Arrays.copyOf(sortValues, sortValues.length); for (int i = 0; i < sortValues.length; ++i) { if (this.sortValues[i] instanceof BytesRef) { @@ -84,6 +87,45 @@ public SearchSortValues(StreamInput in) throws IOException { } } + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeVInt(sortValues.length); + for (Object sortValue : sortValues) { + if (sortValue == null) { + out.writeByte((byte) 0); + } else { + Class type = sortValue.getClass(); + if (type == String.class) { + out.writeByte((byte) 1); + out.writeString((String) sortValue); + } else if (type == Integer.class) { + out.writeByte((byte) 2); + out.writeInt((Integer) sortValue); + } else if (type == Long.class) { + out.writeByte((byte) 3); + out.writeLong((Long) sortValue); + } else if (type == Float.class) { + out.writeByte((byte) 4); + out.writeFloat((Float) sortValue); + } else if (type == Double.class) { + out.writeByte((byte) 5); + out.writeDouble((Double) sortValue); + } else if (type == Byte.class) { + out.writeByte((byte) 6); + out.writeByte((Byte) sortValue); + } else if (type == Short.class) { + out.writeByte((byte) 7); + out.writeShort((Short) sortValue); + } else if (type == Boolean.class) { + out.writeByte((byte) 8); + out.writeBoolean((Boolean) sortValue); + } else { + throw new IOException("Can't handle sort field value of type [" + type + "]"); + } + } + } + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { if (sortValues.length > 0) { @@ -97,10 +139,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } public static SearchSortValues fromXContent(XContentParser parser) throws IOException { - assert parser.currentToken() == XContentParser.Token.FIELD_NAME; - assert parser.currentName().equals(Fields.SORT); - XContentParser.Token token = parser.nextToken(); - assert token == XContentParser.Token.START_ARRAY; + XContentParserUtils.ensureFieldName(parser, parser.currentToken(), Fields.SORT); + parser.nextToken(); + XContentParserUtils.ensureType(XContentParser.Token.START_ARRAY, parser.currentToken(), () -> parser.getTokenLocation()); return new SearchSortValues(parser.list().toArray()); } @@ -108,51 +149,6 @@ public Object[] sortValues() { return sortValues; } - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeVInt(sortValues.length); - if (sortValues.length > 0) { - for (Object sortValue : sortValues) { - if (sortValue == null) { - out.writeByte((byte) 0); - } else { - Class type = sortValue.getClass(); - if (type == String.class) { - out.writeByte((byte) 1); - out.writeString((String) sortValue); - } else if (type == Integer.class) { - out.writeByte((byte) 2); - out.writeInt((Integer) sortValue); - } else if (type == Long.class) { - out.writeByte((byte) 3); - out.writeLong((Long) sortValue); - } else if (type == Float.class) { - out.writeByte((byte) 4); - out.writeFloat((Float) sortValue); - } else if (type == Double.class) { - out.writeByte((byte) 5); - out.writeDouble((Double) sortValue); - } else if (type == Byte.class) { - out.writeByte((byte) 6); - out.writeByte((Byte) sortValue); - } else if (type == Short.class) { - out.writeByte((byte) 7); - out.writeShort((Short) sortValue); - } else if (type == Boolean.class) { - out.writeByte((byte) 8); - out.writeBoolean((Boolean) sortValue); - } else { - throw new IOException("Can't handle sort field value of type [" + type + "]"); - } - } - } - } - } - - public boolean xContentEquals(ToXContentToBytes other) { - return this.toString().equals(other.toString()); - } - @Override public boolean equals(Object obj) { if (this == obj) { diff --git a/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java index ae0cea6be76d9..c92bd782c58d5 100644 --- a/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java +++ b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.internal; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ToXContent; @@ -33,6 +34,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.function.Supplier; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; @@ -72,12 +74,13 @@ public void testFromXContent() throws IOException { builder = sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); - XContentParser parser = xcontentType.xContent().createParser(builder.bytes()); + XContentParser parser = createParser(builder); parser.nextToken(); // skip to the elements field name token, fromXContent advances from there if called from ourside parser.nextToken(); if (sortValues.sortValues().length > 0) { SearchSortValues parsed = SearchSortValues.fromXContent(parser); - assertTrue(sortValues.xContentEquals(parsed)); + + assertEquivalent(builder.bytes(), toXContent(parsed, xcontentType), xcontentType); parser.nextToken(); } assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken()); @@ -124,4 +127,42 @@ private static SearchSortValues mutate(SearchSortValues original) { private static SearchSortValues copy(SearchSortValues original) { return new SearchSortValues(Arrays.copyOf(original.sortValues(), original.sortValues().length)); } + + /** + * Asserts that the provided {@link BytesReference}s hold the same content. + * The comparison is done between the map representation of the provided + * objects. + */ + // TODO move this to a common place + public static void assertEquivalent(BytesReference expected, BytesReference actual, XContentType xContentType) throws IOException { + // we tried comparing byte per byte, but that didn't fly for a couple of + // reasons: + // 1) whenever anything goes through a map while parsing, ordering is + // not preserved, which is perfectly ok + // 2) Jackson SMILE parser parses floats as double, which then get + // printed out as double (with double precision) + try (XContentParser actualParser = xContentType.xContent().createParser(actual)) { + Map actualMap = actualParser.map(); + try (XContentParser expectedParser = xContentType.xContent().createParser(expected)) { + Map expectedMap = expectedParser.map(); + assertEquals(expectedMap, actualMap); + } + } + } + + /** + * Returns the bytes that represent the XContent output of the provided + * {@link ToXContent} object, using the provided {@link XContentType}. Wraps + * the output into a new anonymous object depending on the value of the + * wrapInObject argument. + */ + // TODO move this to a common place + private static BytesReference toXContent(ToXContent toXContent, XContentType xContentType) throws IOException { + try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) { + builder.startObject(); + toXContent.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + return builder.bytes(); + } + } } From 2c8f26c04c64482ff07979c9a00e1bd62d957aa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 19 Dec 2016 15:44:46 +0100 Subject: [PATCH 3/6] Addressing review comment --- .../org/elasticsearch/search/internal/SearchSortValues.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java index f00f180baa4dd..e9cd344a1ad59 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java @@ -140,8 +140,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws public static SearchSortValues fromXContent(XContentParser parser) throws IOException { XContentParserUtils.ensureFieldName(parser, parser.currentToken(), Fields.SORT); - parser.nextToken(); - XContentParserUtils.ensureType(XContentParser.Token.START_ARRAY, parser.currentToken(), () -> parser.getTokenLocation()); + XContentParser.Token token = parser.nextToken(); + XContentParserUtils.ensureType(XContentParser.Token.START_ARRAY, token, () -> parser.getTokenLocation()); return new SearchSortValues(parser.list().toArray()); } From 6e9a5839e83526950b9e95c4d6d8417ef99c8327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 19 Dec 2016 22:03:55 +0100 Subject: [PATCH 4/6] Using common helper methods --- .../search/internal/SearchSortValues.java | 10 ++++- .../internal/SearchSortValuesTests.java | 45 ++----------------- 2 files changed, 11 insertions(+), 44 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java index e9cd344a1ad59..5446994eee166 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.internal; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -32,6 +33,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Locale; import java.util.Objects; public class SearchSortValues implements ToXContent, Writeable { @@ -139,9 +141,13 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } public static SearchSortValues fromXContent(XContentParser parser) throws IOException { - XContentParserUtils.ensureFieldName(parser, parser.currentToken(), Fields.SORT); + XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser::getTokenLocation); + if (parser.currentName().equals(Fields.SORT) == false) { + String message = "Failed to parse object: expecting field with name [%s] but found [%s]"; + throw new ParsingException(parser.getTokenLocation(), String.format(Locale.ROOT, message, Fields.SORT, parser.currentName())); + } XContentParser.Token token = parser.nextToken(); - XContentParserUtils.ensureType(XContentParser.Token.START_ARRAY, token, () -> parser.getTokenLocation()); + XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_ARRAY, token, parser::getTokenLocation); return new SearchSortValues(parser.list().toArray()); } diff --git a/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java index c92bd782c58d5..e67c14a9d7dfe 100644 --- a/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java +++ b/core/src/test/java/org/elasticsearch/search/internal/SearchSortValuesTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.search.internal; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ToXContent; @@ -34,10 +33,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.Map; import java.util.function.Supplier; +import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; public class SearchSortValuesTests extends ESTestCase { @@ -79,8 +79,7 @@ public void testFromXContent() throws IOException { parser.nextToken(); if (sortValues.sortValues().length > 0) { SearchSortValues parsed = SearchSortValues.fromXContent(parser); - - assertEquivalent(builder.bytes(), toXContent(parsed, xcontentType), xcontentType); + assertToXContentEquivalent(builder.bytes(), toXContent(parsed, xcontentType, true), xcontentType); parser.nextToken(); } assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken()); @@ -127,42 +126,4 @@ private static SearchSortValues mutate(SearchSortValues original) { private static SearchSortValues copy(SearchSortValues original) { return new SearchSortValues(Arrays.copyOf(original.sortValues(), original.sortValues().length)); } - - /** - * Asserts that the provided {@link BytesReference}s hold the same content. - * The comparison is done between the map representation of the provided - * objects. - */ - // TODO move this to a common place - public static void assertEquivalent(BytesReference expected, BytesReference actual, XContentType xContentType) throws IOException { - // we tried comparing byte per byte, but that didn't fly for a couple of - // reasons: - // 1) whenever anything goes through a map while parsing, ordering is - // not preserved, which is perfectly ok - // 2) Jackson SMILE parser parses floats as double, which then get - // printed out as double (with double precision) - try (XContentParser actualParser = xContentType.xContent().createParser(actual)) { - Map actualMap = actualParser.map(); - try (XContentParser expectedParser = xContentType.xContent().createParser(expected)) { - Map expectedMap = expectedParser.map(); - assertEquals(expectedMap, actualMap); - } - } - } - - /** - * Returns the bytes that represent the XContent output of the provided - * {@link ToXContent} object, using the provided {@link XContentType}. Wraps - * the output into a new anonymous object depending on the value of the - * wrapInObject argument. - */ - // TODO move this to a common place - private static BytesReference toXContent(ToXContent toXContent, XContentType xContentType) throws IOException { - try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) { - builder.startObject(); - toXContent.toXContent(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - return builder.bytes(); - } - } } From 5ac00543cbfa3297b560c25f1097ab816991011a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 20 Dec 2016 12:47:45 +0100 Subject: [PATCH 5/6] Re-adding XContentParserUtils.ensureFieldName helper --- .../common/xcontent/XContentParserUtils.java | 14 ++++++++++++++ .../search/internal/SearchSortValues.java | 8 +------- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java index 846582fa5f42c..2483938816d8d 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java @@ -22,6 +22,7 @@ import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.XContentParser.Token; +import java.io.IOException; import java.util.Locale; import java.util.function.Supplier; @@ -34,6 +35,19 @@ public final class XContentParserUtils { private XContentParserUtils() { } + /** + * Makes sure that current token is of type {@link XContentParser.Token#FIELD_NAME} and the the field name is equal to the provided one + * @throws ParsingException if the token is not of type {@link XContentParser.Token#FIELD_NAME} or is not equal to the given field name + */ + public static void ensureFieldName(XContentParser parser, Token token, String fieldName) throws IOException { + ensureExpectedToken(Token.FIELD_NAME, token, parser::getTokenLocation); + String current = parser.currentName() != null ? parser.currentName() : ""; + if (current.equals(fieldName) == false) { + String message = "Failed to parse object: expecting field with name [%s] but found [%s]"; + throw new ParsingException(parser.getTokenLocation(), String.format(Locale.ROOT, message, fieldName, current)); + } + } + /** * @throws ParsingException with a "unknown field found" reason */ diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java index 5446994eee166..a4fcb18f828e4 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchSortValues.java @@ -20,7 +20,6 @@ package org.elasticsearch.search.internal; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -33,7 +32,6 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Locale; import java.util.Objects; public class SearchSortValues implements ToXContent, Writeable { @@ -141,11 +139,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } public static SearchSortValues fromXContent(XContentParser parser) throws IOException { - XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser::getTokenLocation); - if (parser.currentName().equals(Fields.SORT) == false) { - String message = "Failed to parse object: expecting field with name [%s] but found [%s]"; - throw new ParsingException(parser.getTokenLocation(), String.format(Locale.ROOT, message, Fields.SORT, parser.currentName())); - } + XContentParserUtils.ensureFieldName(parser, parser.currentToken(), Fields.SORT); XContentParser.Token token = parser.nextToken(); XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_ARRAY, token, parser::getTokenLocation); return new SearchSortValues(parser.list().toArray()); From 5b0d3f1253b5d9047111f5d070ad7769433cf00e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 20 Dec 2016 15:51:50 +0100 Subject: [PATCH 6/6] No null checks in ensureFieldName --- .../elasticsearch/common/xcontent/XContentParserUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java b/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java index 2483938816d8d..956a0d85de816 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/XContentParserUtils.java @@ -41,10 +41,10 @@ private XContentParserUtils() { */ public static void ensureFieldName(XContentParser parser, Token token, String fieldName) throws IOException { ensureExpectedToken(Token.FIELD_NAME, token, parser::getTokenLocation); - String current = parser.currentName() != null ? parser.currentName() : ""; - if (current.equals(fieldName) == false) { + String currentName = parser.currentName(); + if (currentName.equals(fieldName) == false) { String message = "Failed to parse object: expecting field with name [%s] but found [%s]"; - throw new ParsingException(parser.getTokenLocation(), String.format(Locale.ROOT, message, fieldName, current)); + throw new ParsingException(parser.getTokenLocation(), String.format(Locale.ROOT, message, fieldName, currentName)); } }