Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support GeoJSON for geo_point #85120

Merged
merged 8 commits into from
Mar 23, 2022
Merged

Support GeoJSON for geo_point #85120

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bbc61bd
Support GeoJSON for geo_point
craigtaverner Mar 18, 2022
a4cb6de
Better error handling for GeoJSON parsing
craigtaverner Mar 21, 2022
318850e
Fixed failing tests
craigtaverner Mar 21, 2022
ee6f1b0
Generalized test with only test data specialized
craigtaverner Mar 22, 2022
f254ccd
More point parsing tests
craigtaverner Mar 22, 2022
f654224
Remove outdated TODOs
craigtaverner Mar 22, 2022
425f34b
Update docs/changelog/85120.yaml
craigtaverner Mar 22, 2022
e6e9573
Simplify exception handling for parsing doubles
craigtaverner Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 111 additions & 46 deletions server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
import org.elasticsearch.xcontent.support.MapXContentParser;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Locale;

public class GeoUtils {

Expand All @@ -42,6 +44,8 @@ public class GeoUtils {
public static final String LATITUDE = "lat";
public static final String LONGITUDE = "lon";
public static final String GEOHASH = "geohash";
public static final String COORDINATES = "coordinates";
public static final String TYPE = "type";

/** Earth ellipsoid major axis defined by WGS 84 in meters */
public static final double EARTH_SEMI_MAJOR_AXIS = 6378137.0; // meters (WGS 84)
Expand Down Expand Up @@ -437,75 +441,107 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
double lat = Double.NaN;
double lon = Double.NaN;
String geohash = null;
NumberFormatException numberFormatException = null;
String geojsonType = null;
ArrayList<Double> coordinates = null;
class NumberFormatExceptionHandler {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class exists only to keep the old behaviour that NumberFormatException is only handled at the end of parsing. However, there is no obvious reason why we would need that. It seems to me perfectly sufficient to throw the exception at the point that NumberFormatException arises, right? We have a number of other exceptions being thrown immediately during parsing, so why should this particular exception be treated differently. If we allow it to be thrown during parsing, this inner class can get replaced by a simple utility method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On theory I have is that it could be that when throwing exceptions during parsing, it is impossible to continue parsing (because Objects are not closed, or fully parsed), so the entire import (or entire query) is failed. This can be fixed by catching exceptions, finishing parsing the inner object and then re-throw the exception (which is what we see happening with NumberFormatException) and then the outer code can decide whether to catch and continue, or re-throw. I remember seeing some comment somewhere about this case. So perhaps NumberFormatException is considered a case we want to just log and continue, while other exceptions are considered serious enough to break the entire import (or query)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, after discussions with @iverase we decided to remove this. The work done in #40447 should cover the requirements to finish parsing objects, so we don't need to capture exceptions anymore.

NumberFormatException numberFormatException = null;
String field = null;

public double doubleValue(XContentSubParser subParser, String field) throws IOException {
try {
return switch (subParser.currentToken()) {
case VALUE_NUMBER, VALUE_STRING -> subParser.doubleValue(true);
default -> throw new ElasticsearchParseException("{} must be a number", field);
};
} catch (NumberFormatException e) {
numberFormatException = e;
this.field = field;
return Double.NaN;
}
}

// TODO: Re-evaluate why this exception is not thrown earlier
public void assertNoException() {
if (numberFormatException != null) {
throw new ElasticsearchParseException("[{}] must be a valid double value", numberFormatException, field);
}
}
}
NumberFormatExceptionHandler numberFormatExceptionHandler = new NumberFormatExceptionHandler();

if (parser.currentToken() == Token.START_OBJECT) {
try (XContentSubParser subParser = new XContentSubParser(parser)) {
while (subParser.nextToken() != Token.END_OBJECT) {
if (subParser.currentToken() == Token.FIELD_NAME) {
String field = subParser.currentName();
subParser.nextToken();
if (LATITUDE.equals(field)) {
subParser.nextToken();
switch (subParser.currentToken()) {
case VALUE_NUMBER:
case VALUE_STRING:
try {
lat = subParser.doubleValue(true);
} catch (NumberFormatException e) {
numberFormatException = e;
}
break;
default:
throw new ElasticsearchParseException("latitude must be a number");
}
lat = numberFormatExceptionHandler.doubleValue(subParser, "latitude");
} else if (LONGITUDE.equals(field)) {
subParser.nextToken();
switch (subParser.currentToken()) {
case VALUE_NUMBER:
case VALUE_STRING:
try {
lon = subParser.doubleValue(true);
} catch (NumberFormatException e) {
numberFormatException = e;
}
break;
default:
throw new ElasticsearchParseException("longitude must be a number");
}
lon = numberFormatExceptionHandler.doubleValue(subParser, "longitude");
} else if (GEOHASH.equals(field)) {
if (subParser.nextToken() == Token.VALUE_STRING) {
if (subParser.currentToken() == Token.VALUE_STRING) {
geohash = subParser.text();
} else {
throw new ElasticsearchParseException("geohash must be a string");
}
} else if (COORDINATES.equals(field)) {
if (subParser.currentToken() == Token.START_ARRAY) {
coordinates = new ArrayList<>();
while (subParser.nextToken() != Token.END_ARRAY) {
coordinates.add(numberFormatExceptionHandler.doubleValue(subParser, field));
}
} else {
throw new ElasticsearchParseException("GeoJSON 'coordinates' must be an array");
}
} else if (TYPE.equals(field)) {
if (subParser.currentToken() == Token.VALUE_STRING) {
geojsonType = subParser.text();
} else {
throw new ElasticsearchParseException("GeoJSON 'type' must be a string");
}
} else {
throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH);
throw new ElasticsearchParseException(
"field must be either [{}], [{}], [{}], [{}] or [{}]",
LATITUDE,
LONGITUDE,
GEOHASH,
COORDINATES,
TYPE
);
}
} else {
throw new ElasticsearchParseException("token [{}] not allowed", subParser.currentToken());
}
}
}
numberFormatExceptionHandler.assertNoException();
assertOnlyOneFormat(
geohash != null,
Double.isNaN(lat) == false,
Double.isNaN(lon) == false,
coordinates != null,
geojsonType != null
);
if (geohash != null) {
if (Double.isNaN(lat) == false || Double.isNaN(lon) == false) {
throw new ElasticsearchParseException("field must be either lat/lon or geohash");
} else {
return point.parseGeoHash(geohash, effectivePoint);
return point.parseGeoHash(geohash, effectivePoint);
}
if (coordinates != null) {
if (geojsonType == null || geojsonType.toLowerCase(Locale.ROOT).equals("point") == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check that we have no other elements in GeoJson (lat, lon or geohash)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've generalized the validation now to cover all types.

throw new ElasticsearchParseException("GeoJSON 'type' for geo_point can only be 'Point'");
}
} else if (numberFormatException != null) {
throw new ElasticsearchParseException(
"[{}] and [{}] must be valid double values",
numberFormatException,
LATITUDE,
LONGITUDE
);
} else if (Double.isNaN(lat)) {
throw new ElasticsearchParseException("field [{}] missing", LATITUDE);
} else if (Double.isNaN(lon)) {
throw new ElasticsearchParseException("field [{}] missing", LONGITUDE);
} else {
return point.reset(lat, lon);
if (coordinates.size() < 2) {
throw new ElasticsearchParseException("GeoJSON 'coordinates' must contain at least two values");
}
if (coordinates.size() == 3) {
GeoPoint.assertZValue(ignoreZValue, coordinates.get(2));
}
if (coordinates.size() > 3) {
throw new ElasticsearchParseException("[geo_point] field type does not accept > 3 dimensions");
}
return point.reset(coordinates.get(1), coordinates.get(0));
}
return point.reset(lat, lon);

} else if (parser.currentToken() == Token.START_ARRAY) {
try (XContentSubParser subParser = new XContentSubParser(parser)) {
Expand Down Expand Up @@ -536,6 +572,35 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
}
}

private static void assertOnlyOneFormat(boolean geohash, boolean lat, boolean lon, boolean coordinates, boolean type) {
String invalidFieldsMessage = "field must be either lat/lon, geohash string or type/coordinates";
boolean latlon = lat && lon;
boolean geojson = coordinates && type;
var found = new ArrayList<String>();
if (geohash) found.add("geohash");
if (latlon) found.add("lat/lon");
if (geojson) found.add("GeoJSON");
if (found.size() > 1) {
throw new ElasticsearchParseException("fields matching more than one point format found: {}", found);
} else if (geohash) {
if (lat || lon || type || coordinates) {
throw new ElasticsearchParseException(invalidFieldsMessage);
}
} else if (found.size() == 0) {
if (lat) {
throw new ElasticsearchParseException("field [{}] missing", LONGITUDE);
} else if (lon) {
throw new ElasticsearchParseException("field [{}] missing", LATITUDE);
} else if (coordinates) {
throw new ElasticsearchParseException("field [{}] missing", TYPE);
} else if (type) {
throw new ElasticsearchParseException("field [{}] missing", COORDINATES);
} else {
throw new ElasticsearchParseException(invalidFieldsMessage);
}
}
}

/**
* Parse a {@link GeoPoint} from a string. The string must have one of the following forms:
*
Expand Down
10 changes: 10 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ public void testFetchSourceValue() throws IOException {
assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, null));
assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, "wkt"));
}

// test single point in GeoJSON format
sourceValue = jsonPoint;
assertEquals(List.of(jsonPoint), fetchSourceValue(mapper, sourceValue, null));
assertEquals(List.of(wktPoint), fetchSourceValue(mapper, sourceValue, "wkt"));

// Test a list of points in GeoJSON format
sourceValue = List.of(jsonPoint, otherJsonPoint);
assertEquals(List.of(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null));
assertEquals(List.of(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt"));
}

public void testFetchVectorTile() throws IOException {
Expand Down
81 changes: 39 additions & 42 deletions server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.geo.GeoJson;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.geometry.Point;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.geo.RandomGeoGenerator;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.json.JsonXContent;

import java.io.IOException;
import java.util.HashMap;
import java.util.function.DoubleSupplier;

import static org.elasticsearch.geometry.utils.Geohash.stringEncode;
Expand Down Expand Up @@ -84,25 +87,28 @@ public void testGeoPointParsing() throws IOException {
GeoPoint point = GeoUtils.parseGeoPoint(objectLatLon(randomPt.lat(), randomPt.lon()));
assertPointsEqual(point, randomPt);

GeoUtils.parseGeoPoint(toObject(objectLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
point = GeoUtils.parseGeoPoint(toObject(objectLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
assertPointsEqual(point, randomPt);

GeoUtils.parseGeoPoint(arrayLatLon(randomPt.lat(), randomPt.lon()), point);
assertPointsEqual(point, randomPt);

GeoUtils.parseGeoPoint(toObject(arrayLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
point = GeoUtils.parseGeoPoint(toObject(arrayLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
assertPointsEqual(point, randomPt);

GeoUtils.parseGeoPoint(geohash(randomPt.lat(), randomPt.lon()), point);
assertCloseTo(point, randomPt.lat(), randomPt.lon());

GeoUtils.parseGeoPoint(toObject(geohash(randomPt.lat(), randomPt.lon())), randomBoolean());
point = GeoUtils.parseGeoPoint(toObject(geohash(randomPt.lat(), randomPt.lon())), randomBoolean());
assertCloseTo(point, randomPt.lat(), randomPt.lon());

GeoUtils.parseGeoPoint(stringLatLon(randomPt.lat(), randomPt.lon()), point);
assertCloseTo(point, randomPt.lat(), randomPt.lon());

GeoUtils.parseGeoPoint(toObject(stringLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
point = GeoUtils.parseGeoPoint(toObject(stringLatLon(randomPt.lat(), randomPt.lon())), randomBoolean());
assertCloseTo(point, randomPt.lat(), randomPt.lon());

point = GeoUtils.parseGeoPoint(GeoJson.toMap(new Point(randomPt.lon(), randomPt.lat())), randomBoolean());
assertCloseTo(point, randomPt.lat(), randomPt.lon());
}

Expand All @@ -118,49 +124,40 @@ public void testInvalidPointEmbeddedObject() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]"));
}
try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser2.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean()));
assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]"));
}
}

public void testInvalidPointLatHashMix() throws IOException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field("lat", 0).field("geohash", stringEncode(0d, 0d));
content.endObject();

try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field must be either lat/lon or geohash"));
assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]"));
}
try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser2.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean()));
assertThat(e.getMessage(), is("field must be either lat/lon or geohash"));
assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]"));
}
}

public void testInvalidPointLonHashMix() throws IOException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field("lon", 0).field("geohash", stringEncode(0d, 0d));
content.endObject();

try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser.nextToken();

Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field must be either lat/lon or geohash"));
}
try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser2.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean()));
assertThat(e.getMessage(), is("field must be either lat/lon or geohash"));
public void testInvalidPointHashMix() throws IOException {
HashMap<String, Object> otherFields = new HashMap<>();
otherFields.put("lat", 0);
otherFields.put("lon", 0);
otherFields.put("type", "Point");
otherFields.put("coordinates", new double[] { 0.0, 0.0 });
for (String other : otherFields.keySet()) {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field(other, otherFields.get(other)).field("geohash", stringEncode(0d, 0d));
content.endObject();

try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates"));
}
try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser2.nextToken();
Exception e = expectThrows(
ElasticsearchParseException.class,
() -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())
);
assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates"));
}
}
}

Expand All @@ -173,13 +170,13 @@ public void testInvalidField() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]"));
assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]"));
}

try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) {
parser2.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean()));
assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]"));
assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]"));
}
}

Expand Down
Loading