diff --git a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 6b2c82a7b67..5c95f3deb30 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -19,8 +19,6 @@ package org.elasticsearch.search.sort; -import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; - import org.apache.lucene.search.SortField; import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; @@ -46,6 +44,8 @@ import org.elasticsearch.search.MultiValueMode; import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.search.sort.NestedSortBuilder.NESTED_FIELD; + /** * A sort builder to sort based on a document field. */ @@ -91,7 +91,9 @@ public class FieldSortBuilder extends SortBuilder { } this.setNestedFilter(template.getNestedFilter()); this.setNestedPath(template.getNestedPath()); - this.setNestedSort(template.getNestedSort()); + if (template.getNestedSort() != null) { + this.setNestedSort(template.getNestedSort()); + }; } /** @@ -203,10 +205,13 @@ public class FieldSortBuilder extends SortBuilder { * Sets the nested filter that the nested objects should match with in order * to be taken into account for sorting. * - * TODO should the above getters and setters be deprecated/ changed in - * favour of real getters and setters? + * @deprecated set nested sort with {@link #setNestedSort(NestedSortBuilder)} and retrieve with {@link #getNestedSort()} */ + @Deprecated public FieldSortBuilder 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; } @@ -214,7 +219,10 @@ public class FieldSortBuilder 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; } @@ -223,8 +231,14 @@ public class FieldSortBuilder 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 FieldSortBuilder 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; } @@ -232,16 +246,30 @@ public class FieldSortBuilder extends SortBuilder { /** * Returns the nested path if sorting occurs in a field that is inside a * nested object. + * @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 FieldSortBuilder 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; } diff --git a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java index 0534123e0f0..683e949d5cd 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -134,9 +134,14 @@ public abstract class AbstractSortTestCase> extends EST assertNotSame(testItem, parsedItem); assertEquals(testItem, parsedItem); assertEquals(testItem.hashCode(), parsedItem.hashCode()); + assertWarnings(testItem); } } + protected void assertWarnings(T testItem) { + // assert potential warnings based on the test sort configuration. Do nothing by default, subtests can overwrite + } + /** * test that build() outputs a {@link SortField} that is similar to the one * we would get when parsing the xContent the sort builder is rendering out diff --git a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index b368ed651ce..7dd3f0f6818 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -19,16 +19,20 @@ x * Licensed to Elasticsearch under one or more contributor package org.elasticsearch.search.sort; +import org.apache.lucene.index.Term; import org.apache.lucene.search.SortField; import org.apache.lucene.search.SortedNumericSelector; import org.apache.lucene.search.SortedNumericSortField; import org.apache.lucene.search.SortedSetSelector; import org.apache.lucene.search.SortedSetSortField; +import org.apache.lucene.search.TermQuery; 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.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.TypeFieldMapper; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.index.query.QueryShardException; @@ -36,6 +40,7 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.MultiValueMode; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -79,11 +84,19 @@ public class FieldSortBuilderTests extends AbstractSortTestCase NestedSortBuilderTests.createRandomNestedSort(3))); + if (original.getNestedPath() == null && original.getNestedFilter() == null) { + mutated.setNestedSort( + randomValueOtherThan(original.getNestedSort(), () -> NestedSortBuilderTests.createRandomNestedSort(3))); + } else { + if (randomBoolean()) { + mutated.setNestedPath(randomValueOtherThan(original.getNestedPath(), () -> randomAlphaOfLengthBetween(1, 10))); + } else { + mutated.setNestedFilter(randomValueOtherThan(original.getNestedFilter(), () -> randomNestedFilter())); + } + } break; case 1: mutated.sortMode(randomValueOtherThan(original.sortMode(), () -> randomFrom(SortMode.values()))); @@ -242,20 +264,34 @@ public class FieldSortBuilderTests extends AbstractSortTestCase 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()); + + } + + @Override + protected void assertWarnings(FieldSortBuilder testItem) { + List expectedWarnings = new ArrayList<>(); + if (testItem.getNestedFilter() != null) { + expectedWarnings.add("[nested_filter] has been deprecated in favour for the [nested] parameter"); + } + if (testItem.getNestedPath() != null) { + expectedWarnings.add("[nested_path] has been deprecated in favor of the [nested] parameter"); + } + if (expectedWarnings.isEmpty() == false) { + assertWarnings(expectedWarnings.toArray(new String[expectedWarnings.size()])); + } + } + @Override protected FieldSortBuilder fromXContent(XContentParser parser, String fieldName) throws IOException { return FieldSortBuilder.fromXContent(parser, fieldName);