From 760bd6c5688d2120f98d87ec983f8cce4b524872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 5 Sep 2017 14:38:10 +0200 Subject: [PATCH] Extend testing of build method in GeoDistanceSortBuilder (#26498) Improve testing around the GeoDistanceSortBuilder#build method, adding checks for correct transfers of the sort order, mode, nested sorts and points validation and coercion. Also changing the behaviour around the nested_path, nested_filter vs. nested parameter in a similar way as in #26490 and deprecating the setters/getters for the old syntax. Relates to #17286 --- .../search/sort/GeoDistanceSortBuilder.java | 45 +++- .../sort/GeoDistanceSortBuilderTests.java | 215 +++++++++++++++++- .../search/sort/SortBuilderTests.java | 18 +- 3 files changed, 262 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index da7864b80ae..bf3979121df 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -300,10 +300,17 @@ public class GeoDistanceSortBuilder extends SortBuilder } /** - * Sets the nested filter that the nested objects should match with in order to be taken into account - * for sorting. - */ + * Sets the nested filter that the nested objects should match with in order to + * be taken into account for sorting. + * + * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} + * and retrieve with {@link #getNestedSort()} + **/ + @Deprecated public GeoDistanceSortBuilder setNestedFilter(QueryBuilder nestedFilter) { + if (this.nestedSort != null) { + throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed"); + } this.nestedFilter = nestedFilter; return this; } @@ -311,7 +318,10 @@ public class GeoDistanceSortBuilder extends SortBuilder /** * Returns the nested filter that the nested objects should match with in order to be taken into account * for sorting. + * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} + * and retrieve with {@link #getNestedSort()} **/ + @Deprecated public QueryBuilder getNestedFilter() { return this.nestedFilter; } @@ -319,8 +329,14 @@ public class GeoDistanceSortBuilder extends SortBuilder /** * Sets the nested path if sorting occurs on a field that is inside a nested object. By default when sorting on a * field inside a nested object, the nearest upper nested object is selected as nested path. - */ + * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} + * and retrieve with {@link #getNestedSort()} + **/ + @Deprecated public GeoDistanceSortBuilder setNestedPath(String nestedPath) { + if (this.nestedSort != null) { + throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed"); + } this.nestedPath = nestedPath; return this; } @@ -328,16 +344,31 @@ public class GeoDistanceSortBuilder extends SortBuilder /** * Returns the nested path if sorting occurs on a field that is inside a nested object. By default when sorting on a * field inside a nested object, the nearest upper nested object is selected as nested path. - */ + * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} + * and retrieve with {@link #getNestedSort()} + **/ + @Deprecated public String getNestedPath() { return this.nestedPath; } + /** + * Returns the {@link NestedSortBuilder} + */ public NestedSortBuilder getNestedSort() { return this.nestedSort; } + /** + * Sets the {@link NestedSortBuilder} to be used for fields that are inside a nested + * object. The {@link NestedSortBuilder} takes a `path` argument and an optional + * nested filter that the nested objects should match with in + * order to be taken into account for sorting. + */ public GeoDistanceSortBuilder setNestedSort(final NestedSortBuilder nestedSort) { + if (this.nestedFilter != null || this.nestedPath != null) { + throw new IllegalArgumentException("Setting both nested_path/nested_filter and nested not allowed"); + } this.nestedSort = nestedSort; return this; } @@ -445,7 +476,7 @@ public class GeoDistanceSortBuilder extends SortBuilder fieldName = currentName; } else if (token == XContentParser.Token.START_OBJECT) { if (NESTED_FILTER_FIELD.match(currentName)) { - DEPRECATION_LOGGER.deprecated("[nested_filter] has been deprecated in favour for the [nested] parameter"); + DEPRECATION_LOGGER.deprecated("[nested_filter] has been deprecated in favour of the [nested] parameter"); nestedFilter = parseInnerQueryBuilder(parser); } else if (NESTED_FIELD.match(currentName)) { nestedSort = NestedSortBuilder.fromXContent(parser); @@ -475,7 +506,7 @@ public class GeoDistanceSortBuilder extends SortBuilder } else if (SORTMODE_FIELD.match(currentName)) { sortMode = SortMode.fromString(parser.text()); } else if (NESTED_PATH_FIELD.match(currentName)) { - DEPRECATION_LOGGER.deprecated("[nested_path] has been deprecated in favor of the [nested] parameter"); + DEPRECATION_LOGGER.deprecated("[nested_path] has been deprecated in favour of the [nested] parameter"); nestedPath = parser.text(); } else if (token == Token.VALUE_STRING){ if (fieldName != null && fieldName.equals(currentName) == false) { diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java index dc2e4ec17ea..c84242d21a2 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -21,25 +21,37 @@ package org.elasticsearch.search.sort; import org.apache.lucene.document.LatLonDocValuesField; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.SortField; +import org.apache.lucene.search.TermQuery; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource; +import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; import org.elasticsearch.index.mapper.GeoPointFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.TypeFieldMapper; import org.elasticsearch.index.query.GeoValidationMethod; import org.elasticsearch.index.query.MatchAllQueryBuilder; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.MultiValueMode; import org.elasticsearch.test.geo.RandomGeoGenerator; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.instanceOf; public class GeoDistanceSortBuilderTests extends AbstractSortTestCase { @@ -86,16 +98,25 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase randomFrom(SortMode.values()))); } - if (randomBoolean()) { - // don't fully randomize here, GeoDistanceSort is picky about the filters that are allowed - NestedSortBuilder nestedSort = new NestedSortBuilder(randomAlphaOfLengthBetween(3, 10)); - nestedSort.setFilter(new MatchAllQueryBuilder()); - result.setNestedSort(nestedSort); - } if (randomBoolean()) { result.validation(randomValueOtherThan(result.validation(), () -> randomFrom(GeoValidationMethod.values()))); } - + if (randomBoolean()) { + if (randomBoolean()) { + // don't fully randomize here, GeoDistanceSort is picky about the filters that are allowed + NestedSortBuilder nestedSort = new NestedSortBuilder(randomAlphaOfLengthBetween(3, 10)); + nestedSort.setFilter(new MatchAllQueryBuilder()); + result.setNestedSort(nestedSort); + } else { + // the following are alternative ways to setNestedSort for nested sorting + if (randomBoolean()) { + result.setNestedFilter(new MatchAllQueryBuilder()); + } + if (randomBoolean()) { + result.setNestedPath(randomAlphaOfLengthBetween(1, 10)); + } + } + } return result; } @@ -155,7 +176,16 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase randomFrom(SortMode.values()))); break; case 6: - result.setNestedSort(randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); + if (original.getNestedPath() == null && original.getNestedFilter() == null) { + result.setNestedSort( + randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); + } else { + if (randomBoolean()) { + result.setNestedPath(randomValueOtherThan(original.getNestedPath(), () -> randomAlphaOfLengthBetween(1, 10))); + } else { + result.setNestedFilter(randomValueOtherThan(original.getNestedFilter(), () -> randomNestedFilter())); + } + } break; case 7: result.validation(randomValueOtherThan(result.validation(), () -> randomFrom(GeoValidationMethod.values()))); @@ -345,6 +375,21 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase expectedWarnings = new ArrayList<>(); + if (testItem.getNestedFilter() != null) { + expectedWarnings.add("[nested_filter] has been deprecated in favour of the [nested] parameter"); + } + if (testItem.getNestedPath() != null) { + expectedWarnings.add("[nested_path] has been deprecated in favour of the [nested] parameter"); + } + if (expectedWarnings.isEmpty() == false) { + assertWarnings(expectedWarnings.toArray(new String[expectedWarnings.size()])); + } + } + + @Override protected GeoDistanceSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException { return GeoDistanceSortBuilder.fromXContent(parser, fieldName); @@ -385,4 +430,158 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase sortBuilder.build(shardContextMock)); + assertEquals("illegal latitude value [-180.0] for [GeoDistanceSort] for field [fieldName].", ex.getMessage()); + } + { + GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 0.0, -360.0); + sortBuilder.validation(GeoValidationMethod.STRICT); + ElasticsearchParseException ex = expectThrows(ElasticsearchParseException.class, () -> sortBuilder.build(shardContextMock)); + assertEquals("illegal longitude value [-360.0] for [GeoDistanceSort] for field [fieldName].", ex.getMessage()); + } + } + + /** + * Test we can either set nested sort via path/filter or via nested sort builder, not both + */ + public void testNestedSortBothThrows() throws IOException { + GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 0.0, 0.0); + IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, + () -> sortBuilder.setNestedPath("nestedPath").setNestedSort(new NestedSortBuilder("otherPath"))); + assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage()); + iae = expectThrows(IllegalArgumentException.class, + () -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedPath("nestedPath")); + assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage()); + iae = expectThrows(IllegalArgumentException.class, + () -> sortBuilder.setNestedSort(new NestedSortBuilder("otherPath")).setNestedFilter(QueryBuilders.matchAllQuery())); + assertEquals("Setting both nested_path/nested_filter and nested not allowed", iae.getMessage()); + + } + } diff --git a/core/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java index 252c55d8413..06f5ccf696c 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java @@ -34,8 +34,10 @@ import org.junit.BeforeClass; import java.io.IOException; import java.util.ArrayList; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Set; import static java.util.Collections.emptyList; @@ -126,10 +128,11 @@ public class SortBuilderTests extends ESTestCase { } /** - * test random syntax variations + * test parsing random syntax variations */ public void testRandomSortBuilders() throws IOException { for (int runs = 0; runs < NUMBER_OF_RUNS; runs++) { + SetexpectedWarningHeaders = new HashSet<>(); List> testBuilders = randomSortBuilderList(); XContentBuilder xContentBuilder = XContentFactory.jsonBuilder(); xContentBuilder.startObject(); @@ -139,6 +142,16 @@ public class SortBuilderTests extends ESTestCase { xContentBuilder.field("sort"); } for (SortBuilder builder : testBuilders) { + if (builder instanceof GeoDistanceSortBuilder) { + GeoDistanceSortBuilder gdsb = (GeoDistanceSortBuilder) builder; + if (gdsb.getNestedFilter() != null) { + expectedWarningHeaders.add("[nested_filter] has been deprecated in favour of the [nested] parameter"); + } + if (gdsb.getNestedPath() != null) { + expectedWarningHeaders.add("[nested_path] has been deprecated in favour of the [nested] parameter"); + } + } + if (builder instanceof ScoreSortBuilder || builder instanceof FieldSortBuilder) { switch (randomIntBetween(0, 2)) { case 0: @@ -176,6 +189,9 @@ public class SortBuilderTests extends ESTestCase { for (SortBuilder parsedBuilder : parsedSort) { assertEquals(iterator.next(), parsedBuilder); } + if (expectedWarningHeaders.size() > 0) { + assertWarnings(expectedWarningHeaders.toArray(new String[expectedWarningHeaders.size()])); + } } }