Skip to content

Add stricter geohash parsing #30376

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

Merged
merged 5 commits into from
May 7, 2018
Merged

Add stricter geohash parsing #30376

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f48daa
Add stricter geohash parsing
imotov May 3, 2018
e3e5e8f
Add changelog entry
imotov May 4, 2018
fae6635
Merge remote-tracking branch 'elastic/master' into issue-23579-empty-…
imotov May 4, 2018
23ae3d8
Merge remote-tracking branch 'elastic/master' into issue-23579-empty-…
imotov May 7, 2018
ae6105b
Merge remote-tracking branch 'elastic/master' into issue-23579-empty-…
imotov May 7, 2018
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
2 changes: 2 additions & 0 deletions docs/CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ Machine Learning::

* Account for gaps in data counts after job is reopened ({pull}30294[#30294])

Add validation that geohashes are not empty and don't contain unsupported characters ({pull}30376[#30376])

[[release-notes-6.3.1]]
== Elasticsearch version 6.3.1

Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/elasticsearch/common/geo/GeoHashUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,17 @@ public static final String stringEncodeFromMortonLong(long hashedVal, final int
* Encode to a morton long value from a given geohash string
*/
public static final long mortonEncode(final String hash) {
if (hash.isEmpty()) {
throw new IllegalArgumentException("empty geohash");
}
int level = 11;
long b;
long l = 0L;
for(char c : hash.toCharArray()) {
b = (long)(BASE_32_STRING.indexOf(c));
if (b < 0) {
throw new IllegalArgumentException("unsupported symbol [" + c + "] in geohash [" + hash + "]");
}
l |= (b<<((level--*5) + MORTON_OFFSET));
if (level < 0) {
// We cannot handle more than 12 levels
Expand Down
8 changes: 6 additions & 2 deletions server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Strings;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -126,7 +125,12 @@ public GeoPoint resetFromIndexableField(IndexableField field) {
}

public GeoPoint resetFromGeoHash(String geohash) {
final long hash = mortonEncode(geohash);
final long hash;
try {
hash = mortonEncode(geohash);
} catch (IllegalArgumentException ex) {
throw new ElasticsearchParseException(ex.getMessage(), ex);
}
Copy link
Contributor

@jpountz jpountz May 4, 2018

Choose a reason for hiding this comment

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

why don't we just use the IAE?

Copy link
Contributor Author

@imotov imotov May 4, 2018

Choose a reason for hiding this comment

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

All other formatting issues are throwing ElasticsearchParseException at the moment, so I have decided to stick with it for consistency sake. It also makes it easier to ignore in parseGeoPointIgnoringMalformed and parseGeoPointStringIgnoringMalformed later on if mapping has ignore_malformed set to true.

return this.reset(GeoHashUtils.decodeLatitude(hash), GeoHashUtils.decodeLongitude(hash));
}

Expand Down
58 changes: 33 additions & 25 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,14 +299,7 @@ public Mapper parse(ParseContext context) throws IOException {
if (token == XContentParser.Token.START_ARRAY) {
// its an array of array of lon/lat [ [1.2, 1.3], [1.4, 1.5] ]
while (token != XContentParser.Token.END_ARRAY) {
try {
parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse));
} catch (ElasticsearchParseException e) {
if (ignoreMalformed.value() == false) {
throw e;
}
context.addIgnoredField(fieldType.name());
}
parseGeoPointIgnoringMalformed(context, sparse);
token = context.parser().nextToken();
}
} else {
Expand All @@ -326,42 +319,57 @@ public Mapper parse(ParseContext context) throws IOException {
} else {
while (token != XContentParser.Token.END_ARRAY) {
if (token == XContentParser.Token.VALUE_STRING) {
parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value()));
parseGeoPointStringIgnoringMalformed(context, sparse);
} else {
try {
parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse));
} catch (ElasticsearchParseException e) {
if (ignoreMalformed.value() == false) {
throw e;
}
}
parseGeoPointIgnoringMalformed(context, sparse);
}
token = context.parser().nextToken();
}
}
}
} else if (token == XContentParser.Token.VALUE_STRING) {
parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value()));
parseGeoPointStringIgnoringMalformed(context, sparse);
} else if (token == XContentParser.Token.VALUE_NULL) {
if (fieldType.nullValue() != null) {
parse(context, (GeoPoint) fieldType.nullValue());
}
} else {
try {
parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse));
} catch (ElasticsearchParseException e) {
if (ignoreMalformed.value() == false) {
throw e;
}
context.addIgnoredField(fieldType.name());
}
parseGeoPointIgnoringMalformed(context, sparse);
}
}

context.path().remove();
return null;
}

/**
* Parses geopoint represented as an object or an array, ignores malformed geopoints if needed
*/
private void parseGeoPointIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException {
try {
parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse));
} catch (ElasticsearchParseException e) {
if (ignoreMalformed.value() == false) {
throw e;
}
context.addIgnoredField(fieldType.name());
}
}

/**
* Parses geopoint represented as a string and ignores malformed geopoints if needed
*/
private void parseGeoPointStringIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException {
try {
parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value()));
} catch (ElasticsearchParseException e) {
if (ignoreMalformed.value() == false) {
throw e;
}
context.addIgnoredField(fieldType.name());
}
}

@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
Expand Down
13 changes: 12 additions & 1 deletion server/src/test/java/org/elasticsearch/common/geo/GeoHashTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.elasticsearch.common.geo;

import org.apache.lucene.geo.Rectangle;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.test.ESTestCase;

/**
Expand Down Expand Up @@ -95,7 +96,17 @@ public void testLongGeohashes() {
Rectangle expectedBbox = GeoHashUtils.bbox(geohash);
Rectangle actualBbox = GeoHashUtils.bbox(extendedGeohash);
assertEquals("Additional data points above 12 should be ignored [" + extendedGeohash + "]" , expectedBbox, actualBbox);

}
}

public void testInvalidGeohashes() {
IllegalArgumentException ex;

ex = expectThrows(IllegalArgumentException.class, () -> GeoHashUtils.mortonEncode("55.5"));
assertEquals("unsupported symbol [.] in geohash [55.5]", ex.getMessage());

ex = expectThrows(IllegalArgumentException.class, () -> GeoHashUtils.mortonEncode(""));
assertEquals("empty geohash", ex.getMessage());
}

}
47 changes: 47 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {

Expand Down Expand Up @@ -398,4 +399,50 @@ public void testNullValue() throws Exception {
assertThat(defaultValue, not(equalTo(doc.rootDoc().getField("location").binaryValue())));
}

public void testInvalidGeohashIgnored() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties")
.startObject("location")
.field("type", "geo_point")
.field("ignore_malformed", "true")
.endObject()
.endObject().endObject().endObject());

DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser()
.parse("type", new CompressedXContent(mapping));

ParsedDocument doc = defaultMapper.parse(SourceToParse.source("test", "type", "1", BytesReference
.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("location", "1234.333")
.endObject()),
XContentType.JSON));

assertThat(doc.rootDoc().getField("location"), nullValue());
}


public void testInvalidGeohashNotIgnored() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties")
.startObject("location")
.field("type", "geo_point")
.endObject()
.endObject().endObject().endObject());

DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser()
.parse("type", new CompressedXContent(mapping));

MapperParsingException ex = expectThrows(MapperParsingException.class,
() -> defaultMapper.parse(SourceToParse.source("test", "type", "1", BytesReference
.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("location", "1234.333")
.endObject()),
XContentType.JSON)));

assertThat(ex.getMessage(), equalTo("failed to parse"));
assertThat(ex.getRootCause().getMessage(), equalTo("unsupported symbol [.] in geohash [1234.333]"));
}

}
13 changes: 13 additions & 0 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 @@ -175,6 +175,19 @@ public void testInvalidField() throws IOException {
assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]"));
}

public void testInvalidGeoHash() throws IOException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field("geohash", "!!!!");
content.endObject();

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 [!!!!]"));
}

private XContentParser objectLatLon(double lat, double lon) throws IOException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
Expand Down