From 78ff4f52d605c109efd60039b9ef600a072c24ff Mon Sep 17 00:00:00 2001 From: Isabel Drost-Fromm Date: Thu, 28 Apr 2016 12:10:56 +0200 Subject: [PATCH] Introduces GeoValidationMethod to GeoDistanceSortBuilder Previously like in other geo related query parsers we were using a combination of two booleans for coerce and ignore_malformed which was error prone and not very clear. Switched to using GeoValidationMethod instead as we already do e.g. in GeoBoundingBoxQueryBuilder. Left support for both, coerce and ignore_malformed in the parser but deprecated the two in favour of validation method. Introduced the same deprecation in geo bounding box query builder. --- .../query/GeoBoundingBoxQueryBuilder.java | 6 +- .../search/sort/GeoDistanceSortBuilder.java | 102 +++++++++--------- .../search/sort/GeoDistanceSortBuilderIT.java | 3 +- .../sort/GeoDistanceSortBuilderTests.java | 74 ++++++++++--- 4 files changed, 114 insertions(+), 71 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java index db743b971c7..7f5d5e2ec8d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java @@ -64,10 +64,12 @@ public class GeoBoundingBoxQueryBuilder extends AbstractQueryBuilder { public static final String NAME = "_geo_distance"; public static final String ALTERNATIVE_NAME = "_geoDistance"; - 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 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"); - 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 GeoValidationMethod DEFAULT_VALIDATION = GeoValidationMethod.DEFAULT; + + private static final ParseField UNIT_FIELD = new ParseField("unit"); + private static final ParseField DISTANCE_TYPE_FIELD = new ParseField("distance_type"); + private static final ParseField VALIDATION_METHOD_FIELD = new ParseField("validation_method"); + private static final ParseField IGNORE_MALFORMED_FIELD = new ParseField("ignore_malformed") + .withAllDeprecated("use validation_method instead"); + private static final ParseField COERCE_FIELD = new ParseField("coerce", "normalize") + .withAllDeprecated("use validation_method instead"); + private static final ParseField SORTMODE_FIELD = new ParseField("mode", "sort_mode"); + private static final ParseField NESTED_PATH_FIELD = new ParseField("nested_path"); + private static final ParseField NESTED_FILTER_FIELD = new ParseField("nested_filter"); private final String fieldName; private final List points = new ArrayList<>(); @@ -87,9 +90,7 @@ public class GeoDistanceSortBuilder extends SortBuilder private QueryBuilder nestedFilter; private String nestedPath; - // TODO switch to GeoValidationMethod enum - private boolean coerce = DEFAULT_COERCE; - private boolean ignoreMalformed = DEFAULT_IGNORE_MALFORMED; + private GeoValidationMethod validation = DEFAULT_VALIDATION; /** * Constructs a new distance based sort on a geo point like field. @@ -144,8 +145,7 @@ public class GeoDistanceSortBuilder extends SortBuilder this.sortMode = original.sortMode; this.nestedFilter = original.nestedFilter; this.nestedPath = original.nestedPath; - this.coerce = original.coerce; - this.ignoreMalformed = original.ignoreMalformed; + this.validation = original.validation; } /** @@ -161,8 +161,7 @@ public class GeoDistanceSortBuilder extends SortBuilder sortMode = in.readOptionalWriteable(SortMode::readFromStream); nestedFilter = in.readOptionalNamedWriteable(QueryBuilder.class); nestedPath = in.readOptionalString(); - coerce = in.readBoolean(); - ignoreMalformed =in.readBoolean(); + validation = GeoValidationMethod.readFromStream(in); } @Override @@ -175,8 +174,7 @@ public class GeoDistanceSortBuilder extends SortBuilder out.writeOptionalWriteable(sortMode); out.writeOptionalNamedWriteable(nestedFilter); out.writeOptionalString(nestedPath); - out.writeBoolean(coerce); - out.writeBoolean(ignoreMalformed); + validation.writeTo(out); } /** @@ -257,6 +255,21 @@ public class GeoDistanceSortBuilder extends SortBuilder return this.unit; } + /** + * Sets validation method for this sort builder. + */ + public GeoDistanceSortBuilder validation(GeoValidationMethod method) { + this.validation = method; + return this; + } + + /** + * Returns the validation method to use for this sort builder. + */ + public GeoValidationMethod validation() { + return validation; + } + /** * Defines which distance to use for sorting in the case a document contains multiple geo points. * Possible values: min and max @@ -309,26 +322,6 @@ public class GeoDistanceSortBuilder extends SortBuilder return this.nestedPath; } - public GeoDistanceSortBuilder coerce(boolean coerce) { - this.coerce = coerce; - return this; - } - - public boolean coerce() { - return this.coerce; - } - - public GeoDistanceSortBuilder ignoreMalformed(boolean ignoreMalformed) { - if (coerce == false) { - this.ignoreMalformed = ignoreMalformed; - } - return this; - } - - public boolean ignoreMalformed() { - return this.ignoreMalformed; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); @@ -354,8 +347,7 @@ public class GeoDistanceSortBuilder extends SortBuilder if (nestedFilter != null) { builder.field(NESTED_FILTER_FIELD.getPreferredName(), nestedFilter, params); } - builder.field(COERCE_FIELD.getPreferredName(), coerce); - builder.field(IGNORE_MALFORMED_FIELD.getPreferredName(), ignoreMalformed); + builder.field(VALIDATION_METHOD_FIELD.getPreferredName(), validation); builder.endObject(); builder.endObject(); @@ -386,14 +378,14 @@ public class GeoDistanceSortBuilder extends SortBuilder Objects.equals(order, other.order) && Objects.equals(nestedFilter, other.nestedFilter) && Objects.equals(nestedPath, other.nestedPath) && - Objects.equals(coerce, other.coerce) && - Objects.equals(ignoreMalformed, other.ignoreMalformed); + Objects.equals(validation, other.validation); } @Override public int hashCode() { return Objects.hash(this.fieldName, this.points, this.geoDistance, - this.unit, this.sortMode, this.order, this.nestedFilter, this.nestedPath, this.coerce, this.ignoreMalformed); + this.unit, this.sortMode, this.order, this.nestedFilter, + this.nestedPath, this.validation); } /** @@ -417,8 +409,9 @@ public class GeoDistanceSortBuilder extends SortBuilder QueryBuilder nestedFilter = null; String nestedPath = null; - boolean coerce = GeoDistanceSortBuilder.DEFAULT_COERCE; - boolean ignoreMalformed = GeoDistanceSortBuilder.DEFAULT_IGNORE_MALFORMED; + boolean coerce = GeoValidationMethod.DEFAULT_LENIENT_PARSING; + boolean ignoreMalformed = GeoValidationMethod.DEFAULT_LENIENT_PARSING; + GeoValidationMethod validation = null; XContentParser.Token token; String currentName = parser.currentName(); @@ -463,6 +456,8 @@ public class GeoDistanceSortBuilder extends SortBuilder if (coerce == false) { ignoreMalformed = ignore_malformed_value; } + } else if (parseFieldMatcher.match(currentName, VALIDATION_METHOD_FIELD)) { + validation = GeoValidationMethod.fromString(parser.text()); } else if (parseFieldMatcher.match(currentName, SORTMODE_FIELD)) { sortMode = SortMode.fromString(parser.text()); } else if (parseFieldMatcher.match(currentName, NESTED_PATH_FIELD)) { @@ -498,8 +493,13 @@ public class GeoDistanceSortBuilder extends SortBuilder } result.setNestedFilter(nestedFilter); result.setNestedPath(nestedPath); - result.coerce(coerce); - result.ignoreMalformed(ignoreMalformed); + if (validation == null) { + // looks like either validation was left unset or we are parsing old validation json + result.validation(GeoValidationMethod.infer(coerce, ignoreMalformed)); + } else { + // ignore deprecated coerce/ignore_malformed + result.validation(validation); + } return result; } @@ -512,7 +512,7 @@ public class GeoDistanceSortBuilder extends SortBuilder localPoints.add(new GeoPoint(geoPoint)); } - if (!indexCreatedBeforeV2_0 && !ignoreMalformed) { + if (!indexCreatedBeforeV2_0 && !GeoValidationMethod.isIgnoreMalformed(validation)) { for (GeoPoint point : localPoints) { if (GeoUtils.isValidLatitude(point.lat()) == false) { throw new ElasticsearchParseException( @@ -529,9 +529,9 @@ public class GeoDistanceSortBuilder extends SortBuilder } } - if (coerce) { + if (GeoValidationMethod.isCoerce(validation)) { for (GeoPoint point : localPoints) { - GeoUtils.normalizePoint(point, coerce, coerce); + GeoUtils.normalizePoint(point, true, true); } } diff --git a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java index 31c1d5793e0..5229c905bf8 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderIT.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.text.Text; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.index.query.GeoValidationMethod; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.test.ESIntegTestCase; @@ -314,7 +315,7 @@ public class GeoDistanceSortBuilderIT extends ESIntegTestCase { .setSource( new SearchSourceBuilder().sort(SortBuilders.geoDistanceSort(LOCATION_FIELD, 2.0, 2.0) .unit(DistanceUnit.KILOMETERS).geoDistance(GeoDistance.PLANE) - .ignoreMalformed(true).coerce(true))).execute().actionGet(); + .validation(GeoValidationMethod.COERCE))).execute().actionGet(); checkCorrectSortOrderForGeoSort(searchResponse); } 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 87fd183f1ce..f65050ca9ce 100644 --- a/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/sort/GeoDistanceSortBuilderTests.java @@ -33,6 +33,7 @@ import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; +import org.elasticsearch.index.query.GeoValidationMethod; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.indices.query.IndicesQueriesRegistry; import org.elasticsearch.test.geo.RandomGeoGenerator; @@ -94,10 +95,7 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase GeoDistanceSortBuilder.fromXContent(context, "")); + assertTrue(e.getMessage().startsWith("Deprecated field ")); + + } + + public void testIgnoreMalformedIsDeprecated() throws IOException { + String json = "{\n" + + " \"testname\" : [ {\n" + + " \"lat\" : -6.046997540714173,\n" + + " \"lon\" : -51.94128329747579\n" + + " } ],\n" + + " \"unit\" : \"m\",\n" + + " \"distance_type\" : \"sloppy_arc\",\n" + + " \"mode\" : \"SUM\",\n" + + " \"ignore_malformed\" : true\n" + + "}"; + XContentParser itemParser = XContentHelper.createParser(new BytesArray(json)); + itemParser.nextToken(); + + QueryParseContext context = new QueryParseContext(indicesQueriesRegistry, itemParser, ParseFieldMatcher.STRICT); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> GeoDistanceSortBuilder.fromXContent(context, "")); + assertTrue(e.getMessage().startsWith("Deprecated field ")); + + } public void testSortModeSumIsRejectedInJSON() throws IOException { String json = "{\n" + @@ -279,9 +322,7 @@ public class GeoDistanceSortBuilderTests extends AbstractSortTestCase