From 9e1059be2bdb26af9b0b8875f8a92f9f2f1a36ca Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Wed, 23 Mar 2016 13:37:32 +0100 Subject: [PATCH 1/6] Remove deprecated reverse option from sorting Relates to #17047 --- .../org/elasticsearch/search/sort/FieldSortBuilder.java | 6 ------ .../elasticsearch/search/sort/GeoDistanceSortBuilder.java | 5 +---- .../org/elasticsearch/search/sort/ScoreSortBuilder.java | 8 +------- .../search/sort/GeoDistanceSortBuilderTests.java | 1 - 4 files changed, 2 insertions(+), 18 deletions(-) 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 fe946521ea4..8ff9ac0b754 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -50,7 +50,6 @@ public class FieldSortBuilder extends SortBuilder { public static final ParseField NESTED_FILTER = new ParseField("nested_filter"); public static final ParseField MISSING = new ParseField("missing"); public static final ParseField ORDER = new ParseField("order"); - public static final ParseField REVERSE = new ParseField("reverse"); public static final ParseField SORT_MODE = new ParseField("mode"); public static final ParseField UNMAPPED_TYPE = new ParseField("unmapped_type"); @@ -377,11 +376,6 @@ public class FieldSortBuilder extends SortBuilder { nestedPath = parser.text(); } else if (context.parseFieldMatcher().match(currentFieldName, MISSING)) { missing = parser.objectBytes(); - } else if (context.parseFieldMatcher().match(currentFieldName, REVERSE)) { - if (parser.booleanValue()) { - order = SortOrder.DESC; - } - // else we keep the default ASC } else if (context.parseFieldMatcher().match(currentFieldName, ORDER)) { String sortOrder = parser.text(); if ("asc".equals(sortOrder)) { 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 df8eb1f2d7c..0b1e52c0aa6 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -66,7 +66,6 @@ public class GeoDistanceSortBuilder extends SortBuilder public static final boolean DEFAULT_COERCE = false; public static final boolean DEFAULT_IGNORE_MALFORMED = false; public static final ParseField UNIT_FIELD = new ParseField("unit"); - public static final ParseField REVERSE_FIELD = new ParseField("reverse"); public static final ParseField DISTANCE_TYPE_FIELD = new ParseField("distance_type"); public static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize"); public static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed"); @@ -444,9 +443,7 @@ public class GeoDistanceSortBuilder extends SortBuilder geoPoints.add(point); } } else if (token.isValue()) { - if (parseFieldMatcher.match(currentName, REVERSE_FIELD)) { - order = parser.booleanValue() ? SortOrder.DESC : SortOrder.ASC; - } else if (parseFieldMatcher.match(currentName, ORDER_FIELD)) { + if (parseFieldMatcher.match(currentName, ORDER_FIELD)) { order = SortOrder.fromString(parser.text()); } else if (parseFieldMatcher.match(currentName, UNIT_FIELD)) { unit = DistanceUnit.fromString(parser.text()); diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index b35b9b94164..3d273f6fe24 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -40,7 +40,6 @@ public class ScoreSortBuilder extends SortBuilder { public static final String NAME = "_score"; static final ScoreSortBuilder PROTOTYPE = new ScoreSortBuilder(); - public static final ParseField REVERSE_FIELD = new ParseField("reverse"); public static final ParseField ORDER_FIELD = new ParseField("order"); private static final SortField SORT_SCORE = new SortField(null, SortField.Type.SCORE); private static final SortField SORT_SCORE_REVERSE = new SortField(null, SortField.Type.SCORE, true); @@ -71,12 +70,7 @@ public class ScoreSortBuilder extends SortBuilder { if (token == XContentParser.Token.FIELD_NAME) { currentName = parser.currentName(); } else if (token.isValue()) { - if (matcher.match(currentName, REVERSE_FIELD)) { - if (parser.booleanValue()) { - result.order(SortOrder.ASC); - } - // else we keep the default DESC - } else if (matcher.match(currentName, ORDER_FIELD)) { + if (matcher.match(currentName, ORDER_FIELD)) { result.order(SortOrder.fromString(parser.text())); } else { throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] failed to parse field [" + currentName + "]"); 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 abd91eb6f14..5eddaf9b34a 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -227,7 +227,6 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase Date: Thu, 24 Mar 2016 12:04:31 +0100 Subject: [PATCH 2/6] Remove mention of reverse in docs and add to migration doc --- docs/reference/migration/migrate_5_0/search.asciidoc | 5 +++++ docs/reference/search/request/sort.asciidoc | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/reference/migration/migrate_5_0/search.asciidoc b/docs/reference/migration/migrate_5_0/search.asciidoc index 595cf2e3fe9..418486d600e 100644 --- a/docs/reference/migration/migrate_5_0/search.asciidoc +++ b/docs/reference/migration/migrate_5_0/search.asciidoc @@ -141,3 +141,8 @@ The term vectors APIs no longer persist unmapped fields in the mappings. The `dfs` parameter to the term vectors API has been removed completely. Term vectors don't support distributed document frequencies anymore. + +==== Sort + +The `reverse` parameter has been removed, in favour of explicitly +specifying the sort order with the `order` option. diff --git a/docs/reference/search/request/sort.asciidoc b/docs/reference/search/request/sort.asciidoc index 14ab207c301..dfe4b586655 100644 --- a/docs/reference/search/request/sort.asciidoc +++ b/docs/reference/search/request/sort.asciidoc @@ -346,7 +346,7 @@ When sorting on a field, scores are not computed. By setting { "track_scores": true, "sort" : [ - { "post_date" : {"reverse" : true} }, + { "post_date" : {"order" : "desc"} }, { "name" : "desc" }, { "age" : "desc" } ], From 5dd481bfe3ef81a59217f7809ca51dc4f31b893e Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Thu, 24 Mar 2016 15:55:21 +0100 Subject: [PATCH 3/6] Disallow reverse option and check it throws an exception This adds tests to check that using reverse as sort option is disallowed and throws an exception. --- .../search/sort/FieldSortBuilder.java | 4 +-- .../search/sort/GeoDistanceSortBuilder.java | 10 +++++++ .../search/sort/ScoreSortBuilder.java | 1 + .../search/sort/FieldSortBuilderTests.java | 28 +++++++++++++++++++ .../sort/GeoDistanceSortBuilderTests.java | 24 +++++++++++++++- .../search/sort/ScoreSortBuilderTests.java | 21 ++++++++++++++ 6 files changed, 85 insertions(+), 3 deletions(-) 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 f75ea37f854..414062c0cd2 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -372,14 +372,14 @@ public class FieldSortBuilder extends SortBuilder { } else if ("desc".equals(sortOrder)) { order = SortOrder.DESC; } else { - throw new IllegalStateException("Sort order " + sortOrder + " not supported."); + throw new ParsingException(parser.getTokenLocation(), "Sort order [{}] not supported.", sortOrder); } } else if (context.parseFieldMatcher().match(currentFieldName, SORT_MODE)) { sortMode = SortMode.fromString(parser.text()); } else if (context.parseFieldMatcher().match(currentFieldName, UNMAPPED_TYPE)) { unmappedType = parser.text(); } else { - throw new IllegalArgumentException("Option " + currentFieldName + " not supported."); + throw new ParsingException(parser.getTokenLocation(), "Option [{}] not supported.", currentFieldName); } } } 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 4712799c658..c6a63d5f085 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -29,6 +29,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoDistance.FixedSourceDistance; import org.elasticsearch.common.geo.GeoPoint; @@ -72,6 +73,7 @@ public class GeoDistanceSortBuilder extends SortBuilder public static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode"); public static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); public static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); + public static final ParseField REVERSE_FORBIDDEN = new ParseField("reverse"); public static final GeoDistanceSortBuilder PROTOTYPE = new GeoDistanceSortBuilder("_na_", -1, -1); @@ -465,6 +467,14 @@ public class GeoDistanceSortBuilder extends SortBuilder sortMode = SortMode.fromString(parser.text()); } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) { nestedPath = parser.text(); + } else if (parseFieldMatcher.match(currentName, REVERSE_FORBIDDEN)) { + // explicitly filter for reverse in json: used to be a valid sort option, forbidden now, + // if not filtered here it will be treated as a reference to a field in the index with + // geo coordinates given as geohash string + throw new ParsingException( + parser.getTokenLocation(), + "Sort option [{}] no longer supported.", + REVERSE_FORBIDDEN.getPreferredName()); } else { GeoPoint point = new GeoPoint(); point.resetFromString(parser.text()); diff --git a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index 9124540f881..c222634ca0c 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -41,6 +41,7 @@ public class ScoreSortBuilder extends SortBuilder { public static final String NAME = "_score"; public static final ScoreSortBuilder PROTOTYPE = new ScoreSortBuilder(); public static final ParseField ORDER_FIELD = new ParseField("order"); + private static final ParseField REVERSE_FORBIDDEN = new ParseField("reverse"); private static final SortField SORT_SCORE = new SortField(null, SortField.Type.SCORE); private static final SortField SORT_SCORE_REVERSE = new SortField(null, SortField.Type.SCORE, true); 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 2c1a4a65c7b..f28ec8797c2 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -20,6 +20,12 @@ x * Licensed to Elasticsearch under one or more contributor package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; +import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.index.query.QueryParseContext; import java.io.IOException; @@ -103,4 +109,26 @@ public class FieldSortBuilderTests extends AbstractSortTestCase Date: Tue, 29 Mar 2016 11:26:41 +0200 Subject: [PATCH 4/6] Fix build errors after last merge. --- .../org/elasticsearch/search/sort/FieldSortBuilderTests.java | 2 +- .../elasticsearch/search/sort/GeoDistanceSortBuilderTests.java | 2 +- .../org/elasticsearch/search/sort/ScoreSortBuilderTests.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 f4d960aab0f..1ea2180f5e0 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -124,7 +124,7 @@ public class FieldSortBuilderTests extends AbstractSortTestCase Date: Tue, 29 Mar 2016 13:16:33 +0200 Subject: [PATCH 5/6] Refine dealing with removed reverse sort option. For geo distance sort parsing: Disallow anything but VALUE_STRING as geo hash, disallow resetting field name for geo fields. Also make error message for wrong lat/lon values more verbose by including the affected field name. --- .../search/sort/GeoDistanceSortBuilder.java | 41 +++++++++++---- .../search/sort/AbstractSortTestCase.java | 2 +- .../sort/GeoDistanceSortBuilderTests.java | 50 +++++++++++++++++-- 3 files changed, 78 insertions(+), 15 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 1f5dccbdf49..e5c03890d74 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -39,6 +39,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentParser.Token; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; @@ -433,6 +434,13 @@ public class GeoDistanceSortBuilder extends SortBuilder nestedFilter = context.parseInnerQueryBuilder(); } else { // the json in the format of -> field : { lat : 30, lon : 12 } + if (fieldName != null) { + throw new ParsingException( + parser.getTokenLocation(), + "Trying to reset fieldName to [{}], already set to [{}].", + currentName, + fieldName); + } fieldName = currentName; GeoPoint point = new GeoPoint(); GeoUtils.parseGeoPoint(parser, point); @@ -459,19 +467,24 @@ public class GeoDistanceSortBuilder extends SortBuilder sortMode = SortMode.fromString(parser.text()); } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) { nestedPath = parser.text(); - } else if (parseFieldMatcher.match(currentName, REVERSE_FORBIDDEN)) { - // explicitly filter for reverse in json: used to be a valid sort option, forbidden now, - // if not filtered here it will be treated as a reference to a field in the index with - // geo coordinates given as geohash string - throw new ParsingException( - parser.getTokenLocation(), - "Sort option [{}] no longer supported.", - REVERSE_FORBIDDEN.getPreferredName()); - } else { + } else if (token == Token.VALUE_STRING){ + if (fieldName != null) { + throw new ParsingException( + parser.getTokenLocation(), + "Trying to reset fieldName to [{}], already set to [{}].", + currentName, + fieldName); + } + GeoPoint point = new GeoPoint(); point.resetFromString(parser.text()); geoPoints.add(point); fieldName = currentName; + } else { + throw new ParsingException( + parser.getTokenLocation(), + "Only geohashes of type string supported for field [{}]", + currentName); } } } @@ -502,10 +515,16 @@ public class GeoDistanceSortBuilder extends SortBuilder if (!indexCreatedBeforeV2_0 && !ignoreMalformed) { for (GeoPoint point : localPoints) { if (GeoUtils.isValidLatitude(point.lat()) == false) { - throw new ElasticsearchParseException("illegal latitude value [{}] for [GeoDistanceSort]", point.lat()); + throw new ElasticsearchParseException( + "illegal latitude value [{}] for [GeoDistanceSort] for field [{}].", + point.lat(), + fieldName); } if (GeoUtils.isValidLongitude(point.lon()) == false) { - throw new ElasticsearchParseException("illegal longitude value [{}] for [GeoDistanceSort]", point.lon()); + throw new ElasticsearchParseException( + "illegal longitude value [{}] for [GeoDistanceSort] for field [{}].", + point.lon(), + fieldName); } } } 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 7cc44f22a2f..e3aff9ac1be 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/sort/AbstractSortTestCase.java @@ -207,7 +207,7 @@ public abstract class AbstractSortTestCase> extends EST } } - private QueryShardContext createMockShardContext() { + protected QueryShardContext createMockShardContext() { Index index = new Index(randomAsciiOfLengthBetween(1, 10), "_na_"); IndexSettings idxSettings = IndexSettingsModule.newIndexSettings(index, Settings.EMPTY); IndicesFieldDataCache cache = new IndicesFieldDataCache(Settings.EMPTY, null); 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 15842ddef10..378a6a4536e 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -21,6 +21,7 @@ package org.elasticsearch.search.sort; import org.apache.lucene.search.SortField; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.geo.GeoDistance; @@ -203,7 +204,7 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase Date: Tue, 29 Mar 2016 14:38:48 +0200 Subject: [PATCH 6/6] Do not fail if providing coordinates for same field name --- .../org/elasticsearch/search/sort/GeoDistanceSortBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 e5c03890d74..345fa77c18c 100644 --- a/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -434,7 +434,7 @@ public class GeoDistanceSortBuilder extends SortBuilder nestedFilter = context.parseInnerQueryBuilder(); } else { // the json in the format of -> field : { lat : 30, lon : 12 } - if (fieldName != null) { + if (fieldName != null && fieldName.equals(currentName) == false) { throw new ParsingException( parser.getTokenLocation(), "Trying to reset fieldName to [{}], already set to [{}].", @@ -468,7 +468,7 @@ public class GeoDistanceSortBuilder extends SortBuilder } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) { nestedPath = parser.text(); } else if (token == Token.VALUE_STRING){ - if (fieldName != null) { + if (fieldName != null && fieldName.equals(currentName) == false) { throw new ParsingException( parser.getTokenLocation(), "Trying to reset fieldName to [{}], already set to [{}].",