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

Make Geo Context Parsing More Strict #32412

Closed
wants to merge 5 commits into from
Closed

Make Geo Context Parsing More Strict #32412

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
12e689b
Make Geo Context Parsing More Strict
imotov Jul 24, 2018
26b927e
Add support for no geo context
imotov Jul 25, 2018
0a14711
Remove support for non-geo_point in the geo context
imotov Aug 2, 2018
ce83aac
Merge remote-tracking branch 'elastic/master' into issue-32202-incorr…
imotov Aug 2, 2018
097d93d
Fix NPE when the field is missing.
imotov Aug 2, 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
22 changes: 17 additions & 5 deletions .../src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

package org.elasticsearch.search.suggest.completion.context;

import org.apache.lucene.document.StringField;
import org.apache.lucene.document.LatLonDocValuesField;
import org.apache.lucene.document.LatLonPoint;
import org.apache.lucene.index.DocValuesType;
import org.apache.lucene.index.IndexableField;
import org.elasticsearch.ElasticsearchParseException;
Expand Down Expand Up @@ -200,18 +201,29 @@ public Set<CharSequence> parseContext(Document document) {
geohashes.add(stringEncode(spare.getLon(), spare.getLat(), precision));
}
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

the code above seems useless in 7 (even in 6.x ?) since we have a LatLonPoint and a LatLonDocValuesField ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I understood the code above is if you want to represent lat and lon as an object, but don't want to index it as an actual geopoint you could do that.

// Does this object exist? If it does and we didn't find what we need - we need to warn user
for (IndexableField field : document.getFields()) {
if (field.name().startsWith(fieldName + ".")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the "dots in field names" discussion has been a long running one. Do we not yet have a more general/graceful way of throwing these exceptions? This is more of a question out of my own curiosity and not intended to hold up the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we should do that, can we restrict the geo context to geo fields and validate this assumption from the mapping (context.mapperService) directly ? If the field has the correct type in the mapping we don't have to check why it's missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can definitely remove that and clean this up even more, but this will be much more breaking change, since today you can have this sudo geo object and it works, if we will remove this in 7, then working indices with this, created in 6 are going to break. I am not sure we can do that. We will probably have to disable it for indices in 7 but I don't think we can fully remove it until 8.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd treat this case like the geohash string version, this is not documented nor tested so we should remove it. The documentation is clear, you can use a path that references a geo_point field or set the point directly in the context.

throw new ElasticsearchParseException(
"cannot parse geo context field [{}], expected object with lat/lon fields or geo_point", fieldName);
}
}
}
} else {
for (IndexableField field : fields) {
if (field instanceof StringField) {
spare.resetFromString(field.stringValue());
} else {
if (field instanceof LatLonPoint || field instanceof LatLonDocValuesField){
// todo return this to .stringValue() once LatLonPoint implements it
spare.resetFromIndexableField(field);
geohashes.add(spare.geohash());
}
geohashes.add(spare.geohash());
}
if (geohashes.isEmpty()) {
throw new ElasticsearchParseException(
"cannot parse geo context field [{}], expected object with lat/lon fields or geo_point", fieldName);
}
}

}

Set<CharSequence> locations = new HashSet<>();
Expand Down
108 changes: 105 additions & 3 deletions server/src/test/java/org/elasticsearch/search/suggest/ContextCompletionSuggestSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.carrotsearch.randomizedtesting.generators.RandomStrings;
import org.apache.lucene.util.LuceneTestCase.SuppressCodecs;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.index.IndexResponse;
import org.elasticsearch.action.search.SearchResponse;
Expand Down Expand Up @@ -493,14 +494,24 @@ public void testGeoNeighbours() throws Exception {
}

public void testGeoField() throws Exception {
// Version version = VersionUtils.randomVersionBetween(random(), Version.V_2_0_0, Version.V_5_0_0_alpha5);
// Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, version).build();
XContentBuilder mapping = jsonBuilder();
mapping.startObject();
mapping.startObject(TYPE);
mapping.startObject("properties");
mapping.startObject("pin");
mapping.field("type", "geo_point");
if (randomBoolean()) {
mapping.field("type", "geo_point");
// Enable store and disable indexing sometimes
if (randomBoolean()) {
mapping.field("store", "true");
}
if (randomBoolean()) {
mapping.field("index", "false");
}
} else {
// Make it object sometimes
mapping.field("type", "object");
}
mapping.endObject();
mapping.startObject(FIELD);
mapping.field("type", "completion");
Expand Down Expand Up @@ -551,6 +562,97 @@ public void testGeoField() throws Exception {
assertEquals("Hotel Amsterdam in Berlin", searchResponse.getSuggest().getSuggestion(suggestionName).iterator().next().getOptions().iterator().next().getText().string());
}

public void testMalformedGeoField() throws Exception {
XContentBuilder mapping = jsonBuilder();
mapping.startObject();
mapping.startObject(TYPE);
mapping.startObject("properties");
mapping.startObject("pin");
String type = randomFrom("text", "keyword", "long", "object");
mapping.field("type", type);
mapping.endObject();
mapping.startObject(FIELD);
mapping.field("type", "completion");
mapping.field("analyzer", "simple");

mapping.startArray("contexts");
mapping.startObject();
mapping.field("name", "st");
mapping.field("type", "geo");
mapping.field("path", "pin");
mapping.field("precision", 5);
mapping.endObject();
mapping.endArray();

mapping.endObject();
mapping.endObject();
mapping.endObject();
mapping.endObject();

assertAcked(prepareCreate(INDEX).addMapping(TYPE, mapping));

if ("object".equals(type)) {
XContentBuilder source1 = jsonBuilder()
.startObject()
.startObject("pin")
.field("type", "shape")
.endObject()
.startObject(FIELD)
.array("input", "Hotel Amsterdam in Berlin")
.endObject()
.endObject();
assertThat(expectThrows(ElasticsearchParseException.class, () ->
client().prepareIndex(INDEX, TYPE, "1").setSource(source1).execute().actionGet()).getMessage(),
equalTo("cannot parse geo context field [pin], expected object with lat/lon fields or geo_point"));

XContentBuilder source2 = jsonBuilder()
.startObject()
.startObject("pin")
.field("lat", 10.0)
.endObject()
.startObject(FIELD)
.array("input", "Hotel Berlin in Amsterdam")
.endObject()
.endObject();
assertThat(expectThrows(ElasticsearchParseException.class, () ->
client().prepareIndex(INDEX, TYPE, "2").setSource(source2).execute().actionGet()).getMessage(),
equalTo("cannot parse geo context field [pin], expected object with lat/lon fields or geo_point"));
} else if ("long".equals(type)) {
XContentBuilder source2 = jsonBuilder()
.startObject()
.field("pin", 1000)
.startObject(FIELD)
.array("input", "Hotel Berlin in Amsterdam")
.endObject()
.endObject();
assertThat(expectThrows(ElasticsearchParseException.class, () ->
client().prepareIndex(INDEX, TYPE, "2").setSource(source2).execute().actionGet()).getMessage(),
equalTo("cannot parse geo context field [pin], expected object with lat/lon fields or geo_point"));
} else {
XContentBuilder source1 = jsonBuilder()
.startObject()
.field("pin", "52.529172, 13.407333")
.startObject(FIELD)
.array("input", "Hotel Amsterdam in Berlin")
.endObject()
.endObject();
assertThat(expectThrows(ElasticsearchParseException.class, () ->
client().prepareIndex(INDEX, TYPE, "1").setSource(source1).execute().actionGet()).getMessage(),
equalTo("cannot parse geo context field [pin], expected object with lat/lon fields or geo_point"));

XContentBuilder source2 = jsonBuilder()
.startObject()
.field("pin", "u173zhryfg5n")
.startObject(FIELD)
.array("input", "Hotel Berlin in Amsterdam")
.endObject()
.endObject();
assertThat(expectThrows(ElasticsearchParseException.class, () ->
client().prepareIndex(INDEX, TYPE, "2").setSource(source2).execute().actionGet()).getMessage(),
equalTo("cannot parse geo context field [pin], expected object with lat/lon fields or geo_point"));
}
}

public void testSkipDuplicatesWithContexts() throws Exception {
LinkedHashMap<String, ContextMapping<?>> map = new LinkedHashMap<>();
map.put("type", ContextBuilder.category("type").field("type").build());
Expand Down