Skip to content

Commit

Permalink
Remove geohash field from GenericPointParser (elastic#104677)
Browse files Browse the repository at this point in the history 
None of the APIs support this field so it is not used. More over, there
is not test exercising this logic except for GeoTileUtilsTest which is
faking the structure, therefore let's remove it.
  • Loading branch information
@iverase
iverase authored Jan 24, 2024
1 parent 9b634a1 commit e177182
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 155 deletions.
44 changes: 10 additions & 34 deletions server/src/main/java/org/elasticsearch/common/geo/GenericPointParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public abstract class GenericPointParser<T> {
private static final String X_FIELD = "x";
private static final String Y_FIELD = "y";
private static final String Z_FIELD = "z";
private static final String GEOHASH = "geohash";
private static final String TYPE = "type";
private static final String COORDINATES = "coordinates";
private final Map<String, FieldParser<?>> fields;
Expand Down Expand Up @@ -112,9 +111,8 @@ List<Double> parseField(XContentSubParser subParser) throws IOException {
* @param mapType whether the parser is for 'geo_point' or 'point'
* @param xField the name of the first coordinate when constructing points (either 'x' or 'lat')
* @param yField the name of the second coordinate when constructing points (either 'y' or 'lon')
* @param supportGeohash whether to support parsing geohash values (only geo_point supports this currently)
*/
public GenericPointParser(String mapType, String xField, String yField, boolean supportGeohash) {
public GenericPointParser(String mapType, String xField, String yField) {
this.mapType = mapType;
this.xField = xField;
this.yField = yField;
Expand All @@ -124,9 +122,6 @@ public GenericPointParser(String mapType, String xField, String yField, boolean
fields.put(Z_FIELD, new DoubleFieldParser(Z_FIELD, Z_FIELD));
fields.put(TYPE, new StringFieldParser(TYPE));
fields.put(COORDINATES, new DoubleArrayFieldParser(COORDINATES));
if (supportGeohash) {
fields.put(GEOHASH, new StringFieldParser(GEOHASH));
}
}

public abstract void assertZValue(boolean ignoreZValue, double zValue);
Expand All @@ -142,11 +137,10 @@ public GenericPointParser(String mapType, String xField, String yField, boolean
* @param ignoreZValue {@link XContentParser} to not throw an error if 3 dimensional data is provided
* @return new Point parsed from the parser
*/
public T parsePoint(XContentParser parser, boolean ignoreZValue, Function<String, T> fromString, Function<String, T> fromGeohash)
throws IOException, ElasticsearchParseException {
public T parsePoint(XContentParser parser, boolean ignoreZValue, Function<String, T> fromString) throws IOException,
ElasticsearchParseException {
double x = Double.NaN;
double y = Double.NaN;
String geohash = null;
String geojsonType = null;
List<Double> coordinates = null;

Expand All @@ -162,7 +156,6 @@ public T parsePoint(XContentParser parser, boolean ignoreZValue, Function<String
case X_FIELD -> x = (Double) fieldParser.parseField(subParser);
case Y_FIELD -> y = (Double) fieldParser.parseField(subParser);
case Z_FIELD -> assertZValue(ignoreZValue, (Double) fieldParser.parseField(subParser));
case GEOHASH -> geohash = (String) fieldParser.parseField(subParser);
case TYPE -> geojsonType = (String) fieldParser.parseField(subParser);
case COORDINATES -> coordinates = ((DoubleArrayFieldParser) fieldParser).parseField(subParser);
}
Expand All @@ -175,16 +168,7 @@ public T parsePoint(XContentParser parser, boolean ignoreZValue, Function<String
}
}
}
assertOnlyOneFormat(
geohash != null,
Double.isNaN(x) == false,
Double.isNaN(y) == false,
coordinates != null,
geojsonType != null
);
if (geohash != null) {
return fromGeohash.apply(geohash);
}
assertOnlyOneFormat(Double.isNaN(x) == false, Double.isNaN(y) == false, coordinates != null, geojsonType != null);
if (coordinates != null) {
if (geojsonType == null || geojsonType.toLowerCase(Locale.ROOT).equals("point") == false) {
throw new ElasticsearchParseException("[type] for {} can only be 'Point'", mapType);
Expand Down Expand Up @@ -241,20 +225,12 @@ private static double parseValidDouble(XContentSubParser subParser, String field
}
}

private void assertOnlyOneFormat(boolean geohash, boolean x, boolean y, boolean coordinates, boolean type) {
boolean xy = x && y;
boolean geojson = coordinates && type;
var found = new ArrayList<String>();
if (geohash) found.add("geohash");
if (xy) found.add(xField + "/" + yField);
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 (x || y || type || coordinates) {
throw new ElasticsearchParseException(fieldError());
}
} else if (found.size() == 0) {
private void assertOnlyOneFormat(boolean x, boolean y, boolean coordinates, boolean type) {
final boolean xy = x && y;
final boolean geojson = coordinates && type;
if (xy && geojson) {
throw new ElasticsearchParseException("fields matching more than one point format");
} else if ((xy || geojson) == false) {
if (x) {
throw new ElasticsearchParseException("Required [{}]", yField);
} else if (y) {
Expand Down
20 changes: 8 additions & 12 deletions server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,18 +410,14 @@ public static GeoPoint parseGeoPoint(XContentParser parser, final boolean ignore
*/
public static GeoPoint parseGeoPoint(XContentParser parser, final boolean ignoreZValue, final EffectivePoint effectivePoint)
throws IOException, ElasticsearchParseException {
return geoPointParser.parsePoint(parser, ignoreZValue, value -> {
GeoPoint point = new GeoPoint();
point.resetFromString(value, ignoreZValue, effectivePoint);
return point;
}, value -> {
GeoPoint point = new GeoPoint();
point.parseGeoHash(value, effectivePoint);
return point;
});
}

private static GenericPointParser<GeoPoint> geoPointParser = new GenericPointParser<>("geo_point", "lon", "lat", true) {
return geoPointParser.parsePoint(
parser,
ignoreZValue,
value -> new GeoPoint().resetFromString(value, ignoreZValue, effectivePoint)
);
}

private static final GenericPointParser<GeoPoint> geoPointParser = new GenericPointParser<>("geo_point", "lon", "lat") {

@Override
public void assertZValue(boolean ignoreZValue, double zValue) {
Expand Down
51 changes: 4 additions & 47 deletions server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
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 @@ -124,40 +123,12 @@ 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 [location] not supported - must be one of: lon, lat, z, type, coordinates, geohash"));
assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, 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 [location] not supported - must be one of: lon, lat, z, type, coordinates, 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"));
}
assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, type, coordinates"));
}
}

Expand All @@ -170,27 +141,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 [test] not supported - must be one of: lon, lat, z, type, coordinates, geohash"));
assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, 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 [test] not supported - must be one of: lon, lat, z, type, coordinates, geohash"));
}
}

public void testInvalidGeoHash() throws IOException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field("geohash", "!!!!");
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("unsupported symbol [!] in geohash [!!!!]"));
assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, type, coordinates"));
}
}

Expand Down
60 changes: 4 additions & 56 deletions server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.geometry.utils.Geohash;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
Expand All @@ -20,11 +19,9 @@
import java.io.IOException;

import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.closeTo;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.hamcrest.Matchers.not;
Expand Down Expand Up @@ -503,45 +500,6 @@ public void testParseGeoPointArrayZValueError() throws IOException {
}
}

public void testParseGeoPointGeohash() throws IOException {
for (int i = 0; i < 100; i++) {
int geoHashLength = randomIntBetween(1, Geohash.PRECISION);
StringBuilder geohashBuilder = new StringBuilder(geoHashLength);
for (int j = 0; j < geoHashLength; j++) {
geohashBuilder.append(BASE_32[randomInt(BASE_32.length - 1)]);
}
XContentBuilder json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject();
try (XContentParser parser = createParser(json)) {
parser.nextToken();
GeoPoint point = GeoUtils.parseGeoPoint(parser);
assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0)));
assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0)));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
}
json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject();
try (XContentParser parser = createParser(json)) {
while (parser.currentToken() != Token.VALUE_STRING) {
parser.nextToken();
}
GeoPoint point = GeoUtils.parseGeoPoint(parser);
assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0)));
assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0)));
}
}
}

public void testParseGeoPointGeohashWrongType() throws IOException {
XContentBuilder json = jsonBuilder().startObject().field("geohash", 1.0).endObject();
try (XContentParser parser = createParser(json)) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), containsString("[geohash] must be a string"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
}
}

public void testParseGeoPointLatNoLon() throws IOException {
double lat = 0.0;
XContentBuilder json = jsonBuilder().startObject().field("lat", lat).endObject();
Expand Down Expand Up @@ -691,19 +649,7 @@ public void testParseGeoPointExtraField() throws IOException {
try (XContentParser parser = createParser(json)) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field [foo] not supported - must be one of: lon, lat, z, type, coordinates, geohash"));
}
}

public void testParseGeoPointLonLatGeoHash() throws IOException {
double lat = 0.0;
double lon = 0.0;
String geohash = "abcd";
XContentBuilder json = jsonBuilder().startObject().field("lat", lat).field("lon", lon).field("geohash", geohash).endObject();
try (XContentParser parser = createParser(json)) {
parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), containsString("fields matching more than one point format found"));
assertThat(e.getMessage(), is("field [foo] not supported - must be one of: lon, lat, z, type, coordinates"));
}
}

Expand Down Expand Up @@ -772,7 +718,9 @@ public void testParseGeoPointGeohashPositions() throws IOException {
}

private GeoPoint parseGeohash(String geohash, GeoUtils.EffectivePoint effectivePoint) throws IOException {
try (XContentParser parser = createParser(jsonBuilder().startObject().field("geohash", geohash).endObject())) {
try (XContentParser parser = createParser(jsonBuilder().startObject().field("location", geohash).endObject())) {
parser.nextToken();
parser.nextToken();
parser.nextToken();
return GeoUtils.parseGeoPoint(parser, randomBoolean(), effectivePoint);
}
Expand Down
8 changes: 2 additions & 6 deletions ...k/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/common/CartesianPoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
*/
public static CartesianPoint parsePoint(XContentParser parser, final boolean ignoreZValue) throws IOException,
ElasticsearchParseException {
return cartesianPointParser.parsePoint(parser, ignoreZValue, value -> {
CartesianPoint point = new CartesianPoint();
point.resetFromString(value, ignoreZValue);
return point;
}, value -> null);
return cartesianPointParser.parsePoint(parser, ignoreZValue, value -> new CartesianPoint().resetFromString(value, ignoreZValue));
}

public static CartesianPoint parsePoint(Object value, boolean ignoreZValue) throws ElasticsearchParseException {
Expand Down Expand Up @@ -244,7 +240,7 @@ public static void assertZValue(final boolean ignoreZValue, double zValue) {
}
}

private static GenericPointParser<CartesianPoint> cartesianPointParser = new GenericPointParser<>("point", "x", "y", false) {
private static final GenericPointParser<CartesianPoint> cartesianPointParser = new GenericPointParser<>("point", "x", "y") {

@Override
public void assertZValue(boolean ignoreZValue, double zValue) {
Expand Down

0 comments on commit e177182

Please sign in to comment.