honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Prohibit using `nested_filter`, `nested_path` and new `nested` Option at the same time in FieldSortBuilder (#26490) 

Currently we allow both "old" and "new" way of setting nested sorts on the
FieldSortBuilder at the same time. This should throw an error, instead the user
should choose one of the two possible options.

Also adding testing for the now deprecated nestedPath/nestedFilter parameters,
inlcuding checks that they emmit warnings on parsing and that the new
NestetedSortBuilder overwrites the deprecated parameters when building the
SortField.

Relates to #17286
This commit is contained in:
Christoph Büscher 2017-09-04 17:19:52 +02:00 committed by GitHub
parent 2fd4af82e4
commit 8f0369296f
3 changed files with 114 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
View File

@ -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<FieldSortBuilder> {
}
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<FieldSortBuilder> {
* 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<FieldSortBuilder> {
/**
* 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<FieldSortBuilder> {
* 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<FieldSortBuilder> {
/**
* 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;
}

5
core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java
View File

@ -134,9 +134,14 @@ public abstract class AbstractSortTestCase<T extends SortBuilder<T>> 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

85
core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
View File

@ -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<FieldSortBuilder
if (randomBoolean()) {
builder.sortMode(randomFrom(SortMode.values()));
}
if (randomBoolean()) {
builder.setNestedSort(createRandomNestedSort(3));
if (randomBoolean()) {
builder.setNestedSort(createRandomNestedSort(3));
} else {
// the following are alternative ways to setNestedSort for nested sorting
if (randomBoolean()) {
builder.setNestedFilter(randomNestedFilter());
}
if (randomBoolean()) {
builder.setNestedPath(randomAlphaOfLengthBetween(1, 10));
}
}
}
return builder;
}
@ -93,7 +106,16 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
int parameter = randomIntBetween(0, 4);
switch (parameter) {
case 0:
mutated.setNestedSort(randomValueOtherThan(original.getNestedSort(), () -> 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<FieldSortBuilder
public void testBuildNested() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
FieldSortBuilder sortBuilder = new FieldSortBuilder("value").setNestedPath("path");
FieldSortBuilder sortBuilder = new FieldSortBuilder("fieldName")
.setNestedSort(new NestedSortBuilder("path").setFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value")));
SortField sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertNotNull(comparatorSource.nested());
Nested nested = comparatorSource.nested();
assertNotNull(nested);
assertEquals(new TermQuery(new Term(MAPPED_STRING_FIELDNAME, "value")), nested.getInnerQuery());
sortBuilder = new FieldSortBuilder("value").setNestedPath("path").setNestedFilter(QueryBuilders.termQuery("field", 10.0));
sortBuilder = new FieldSortBuilder("fieldName").setNestedPath("path");
sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertNotNull(comparatorSource.nested());
nested = comparatorSource.nested();
assertNotNull(nested);
assertEquals(new TermQuery(new Term(TypeFieldMapper.NAME, "__path")), nested.getInnerQuery());
sortBuilder = new FieldSortBuilder("fieldName").setNestedPath("path")
.setNestedFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value"));
sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
nested = comparatorSource.nested();
assertNotNull(nested);
assertEquals(new TermQuery(new Term(MAPPED_STRING_FIELDNAME, "value")), nested.getInnerQuery());
// if nested path is missing, we omit any filter and return a SortedNumericSortField
sortBuilder = new FieldSortBuilder("value").setNestedFilter(QueryBuilders.termQuery("field", 10.0));
sortBuilder = new FieldSortBuilder("fieldName").setNestedFilter(QueryBuilders.termQuery(MAPPED_STRING_FIELDNAME, "value"));
sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField, instanceOf(SortedNumericSortField.class));
}
@ -315,6 +351,37 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
assertEquals(expectedError, e.getMessage());
}
/**
* Test we can either set nested sort via path/filter or via nested sort builder, not both
*/
public void testNestedSortBothThrows() throws IOException {
FieldSortBuilder sortBuilder = new FieldSortBuilder(MAPPED_STRING_FIELDNAME);
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());
}
@Override
protected void assertWarnings(FieldSortBuilder testItem) {
List<String> 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);