From 8b8cd26a59ece1f42ffb0f688c2ace1b3da6d083 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Tue, 1 Apr 2014 17:30:03 +0200 Subject: [PATCH] Geo: Allow to parse lat/lon as strings and coerce them In order to be more failsafe parsing GeoPoints can support lat/lon as strings and coerce them. Added support and test for this. --- .../elasticsearch/common/geo/GeoUtils.java | 24 ++++++---- .../context/GeolocationContextMapping.java | 24 ++++++---- .../index/search/geo/GeoUtilsTests.java | 16 ++++++- .../GeoLocationContextMappingTest.java | 45 +++++++++++++++++++ 4 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java diff --git a/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 3167c19fb1a..c1b9cca01d3 100644 --- a/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -343,16 +343,24 @@ public class GeoUtils { if(parser.currentToken() == Token.FIELD_NAME) { String field = parser.text(); if(LATITUDE.equals(field)) { - if(parser.nextToken() == Token.VALUE_NUMBER) { - lat = parser.doubleValue(); - } else { - throw new ElasticsearchParseException("latitude must be a number"); + parser.nextToken(); + switch (parser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + lat = parser.doubleValue(true); + break; + default: + throw new ElasticsearchParseException("latitude must be a number"); } } else if (LONGITUDE.equals(field)) { - if(parser.nextToken() == Token.VALUE_NUMBER) { - lon = parser.doubleValue(); - } else { - throw new ElasticsearchParseException("latitude must be a number"); + parser.nextToken(); + switch (parser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + lon = parser.doubleValue(true); + break; + default: + throw new ElasticsearchParseException("longitude must be a number"); } } else if (GEOHASH.equals(field)) { if(parser.nextToken() == Token.VALUE_STRING) { diff --git a/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java b/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java index efdb2cdeba1..7ddbef0f2b2 100644 --- a/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java +++ b/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java @@ -325,20 +325,28 @@ public class GeolocationContextMapping extends ContextMapping { final String fieldName = parser.text(); if("lat".equals(fieldName)) { if(point == null) { - if (parser.nextToken() == Token.VALUE_NUMBER) { - lat = parser.doubleValue(); - } else { - throw new ElasticsearchParseException("latitude must be a number"); + parser.nextToken(); + switch (parser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + lat = parser.doubleValue(true); + break; + default: + throw new ElasticsearchParseException("latitude must be a number"); } } else { throw new ElasticsearchParseException("only lat/lon or [" + FIELD_VALUE + "] is allowed"); } } else if ("lon".equals(fieldName)) { if(point == null) { - if(parser.nextToken() == Token.VALUE_NUMBER) { - lon = parser.doubleValue(); - } else { - throw new ElasticsearchParseException("longitude must be a number"); + parser.nextToken(); + switch (parser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + lon = parser.doubleValue(true); + break; + default: + throw new ElasticsearchParseException("longitude must be a number"); } } else { throw new ElasticsearchParseException("only lat/lon or [" + FIELD_VALUE + "] is allowed"); diff --git a/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java b/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java index 6ac719892a9..5f5796e0da9 100644 --- a/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java +++ b/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java @@ -26,10 +26,13 @@ import org.apache.lucene.spatial.prefix.tree.GeohashPrefixTree; import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.ElasticsearchTestCase; import org.junit.Test; -import static org.hamcrest.MatcherAssert.assertThat; +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.Matchers.*; /** @@ -251,6 +254,17 @@ public class GeoUtilsTests extends ElasticsearchTestCase { } } + @Test + public void testTryParsingLatLonFromString() throws Exception { + XContentBuilder builder = jsonBuilder().startObject().field("lat", "52").field("lon", "4").endObject(); + XContentParser parser = XContentHelper.createParser(builder.bytes()); + parser.nextToken(); + GeoPoint geoPoint = GeoUtils.parseGeoPoint(parser); + assertThat(geoPoint.lat(), is(52.0)); + assertThat(geoPoint.lon(), is(4.0)); + } + + private static void assertNormalizedPoint(GeoPoint input, GeoPoint expected) { GeoUtils.normalizePoint(input); assertThat(input, equalTo(expected)); diff --git a/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java b/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java new file mode 100644 index 00000000000..b15865ad639 --- /dev/null +++ b/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java @@ -0,0 +1,45 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.suggest.context; + +import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.collect.Maps; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.test.ElasticsearchTestCase; +import org.junit.Test; + +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; + +/** + * + */ +public class GeoLocationContextMappingTest extends ElasticsearchTestCase { + + @Test + public void testThatParsingGeoPointsWorksWithCoercion() throws Exception { + XContentBuilder builder = jsonBuilder().startObject().field("lat", "52").field("lon", "4").endObject(); + XContentParser parser = XContentHelper.createParser(builder.bytes()); + parser.nextToken(); + + GeolocationContextMapping mapping = GeolocationContextMapping.load("foo", Maps.newHashMap()); + mapping.parseQuery("foo", parser); + } + +}