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

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
This commit is contained in:
Christoph Büscher 2017-09-05 14:38:10 +02:00 committed by GitHub
parent eeded72b19
commit 760bd6c568
3 changed files with 262 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java
View File

@ -300,10 +300,17 @@ public class GeoDistanceSortBuilder extends SortBuilder<GeoDistanceSortBuilder>
}
/**
* 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<GeoDistanceSortBuilder>
/**
* 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<GeoDistanceSortBuilder>
/**
* 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<GeoDistanceSortBuilder>
/**
* 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<GeoDistanceSortBuilder>
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<GeoDistanceSortBuilder>
} 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) {

215
core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java
View File

@ -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<GeoDistanceSortBuilder> {
@ -86,16 +98,25 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase<GeoDistanc
if (randomBoolean()) {
result.sortMode(randomValueOtherThan(SortMode.SUM, () -> 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<GeoDistanc
() -> 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<GeoDistanc
return GeoDistanceSortBuilder.fromXContent(parser, null);
}
@Override
protected void assertWarnings(GeoDistanceSortBuilder testItem) {
List<String> 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<GeoDistanc
sort = builder.build(context);
assertEquals(SortField.class, sort.field.getClass()); // can't use LatLon optimized sorting with DESC sorting
}
/**
* Test that the sort builder order gets transfered correctly to the SortField
*/
public void testBuildSortFieldOrder() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0);
assertEquals(false, geoDistanceSortBuilder.build(shardContextMock).field.getReverse());
geoDistanceSortBuilder.order(SortOrder.ASC);
assertEquals(false, geoDistanceSortBuilder.build(shardContextMock).field.getReverse());
geoDistanceSortBuilder.order(SortOrder.DESC);
assertEquals(true, geoDistanceSortBuilder.build(shardContextMock).field.getReverse());
}
/**
* Test that the sort builder mode gets transfered correctly to the SortField
*/
public void testMultiValueMode() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0);
geoDistanceSortBuilder.sortMode(SortMode.MAX);
SortField sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.MAX, comparatorSource.sortMode());
// also use MultiValueMode.Max if no Mode set but order is DESC
geoDistanceSortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0);
geoDistanceSortBuilder.order(SortOrder.DESC);
sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.MAX, comparatorSource.sortMode());
// use MultiValueMode.Min if no Mode and order is ASC
geoDistanceSortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0);
// need to use distance unit other than Meters to not get back a LatLonPointSortField
geoDistanceSortBuilder.order(SortOrder.ASC).unit(DistanceUnit.INCH);
sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.MIN, comparatorSource.sortMode());
geoDistanceSortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0);
// need to use distance unit other than Meters to not get back a LatLonPointSortField
geoDistanceSortBuilder.sortMode(SortMode.MIN).unit(DistanceUnit.INCH);
sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.MIN, comparatorSource.sortMode());
geoDistanceSortBuilder.sortMode(SortMode.AVG);
sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.AVG, comparatorSource.sortMode());
geoDistanceSortBuilder.sortMode(SortMode.MEDIAN);
sortField = geoDistanceSortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
assertEquals(MultiValueMode.MEDIAN, comparatorSource.sortMode());
}
/**
* Test that the sort builder nested object gets created in the SortField
*/
public void testBuildNested() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0)
.setNestedSort(new NestedSortBuilder("path").setFilter(QueryBuilders.matchAllQuery()));
SortField sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
Nested nested = comparatorSource.nested();
assertNotNull(nested);
assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery());
sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedPath("path");
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(TypeFieldMapper.NAME, "__path")), nested.getInnerQuery());
sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedPath("path")
.setNestedFilter(QueryBuilders.matchAllQuery());
sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class));
comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource();
nested = comparatorSource.nested();
assertNotNull(nested);
assertEquals(new MatchAllDocsQuery(), nested.getInnerQuery());
// if nested path is missing, we omit any filter and return a regular SortField
// (LatLonSortField)
sortBuilder = new GeoDistanceSortBuilder("fieldName", 1.0, 1.0).setNestedFilter(QueryBuilders.termQuery("fieldName", "value"));
sortField = sortBuilder.build(shardContextMock).field;
assertThat(sortField, instanceOf(SortField.class));
}
/**
* Test that if coercion is used, a point gets normalized but the original values in the builder are unchanged
*/
public void testBuildCoerce() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", -180.0, -360.0);
sortBuilder.validation(GeoValidationMethod.COERCE);
assertEquals(-180.0, sortBuilder.points()[0].getLat(), 0.0);
assertEquals(-360.0, sortBuilder.points()[0].getLon(), 0.0);
SortField sortField = sortBuilder.build(shardContextMock).field;
assertEquals(LatLonDocValuesField.newDistanceSort("fieldName", 0.0, 180.0), sortField);
}
/**
* Test that if validation is strict, invalid points throw an error
*/
public void testBuildInvalidPoints() throws IOException {
QueryShardContext shardContextMock = createMockShardContext();
{
GeoDistanceSortBuilder sortBuilder = new GeoDistanceSortBuilder("fieldName", -180.0, 0.0);
sortBuilder.validation(GeoValidationMethod.STRICT);
ElasticsearchParseException ex = expectThrows(ElasticsearchParseException.class, () -> 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());
}
}

18
core/src/test/java/org/elasticsearch/search/sort/SortBuilderTests.java
View File

@ -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++) {
Set<String >expectedWarningHeaders = new HashSet<>();
List<SortBuilder<?>> 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()]));
}
}
}