Skip to content

Commit

Permalink
Fix handling of points_only with term strategy in geo_shape (#31766)
Browse files Browse the repository at this point in the history 
Fixes 2 issues that together cause errors during index creation
with geo_shapes that use the term strategy. The term strategy changes
the default for points_only parameter, but this wasn't taken into
account during serialization. So, setting the term strategy would add
`"points_only": true` to serialization. At the same time if the term
strategy would also cause the `points_only` setting to be not marked as
a processed element during parsing, which would cause index creation to
fail with the error: `Mapping definition for [location] has unsupported`
`parameters:  [points_only : true]`.

Fixes #31707
  • Loading branch information
@imotov
imotov authored Jul 5, 2018
1 parent 09e8ac8 commit 94d3ddd
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 9 deletions.
27 changes: 20 additions & 7 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ public static class TypeParser implements Mapper.TypeParser {
@Override
public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException {
Builder builder = new Builder(name);
Boolean pointsOnly = null;
for (Iterator<Map.Entry<String, Object>> iterator = node.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
String fieldName = entry.getKey();
Expand Down Expand Up @@ -230,13 +231,18 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
} else if (GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName().equals(fieldName)) {
builder.ignoreZValue(XContentMapValues.nodeBooleanValue(fieldNode, name + "." + GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName()));
iterator.remove();
} else if (Names.STRATEGY_POINTS_ONLY.equals(fieldName)
&& builder.fieldType().strategyName.equals(SpatialStrategy.TERM.getStrategyName()) == false) {
boolean pointsOnly = XContentMapValues.nodeBooleanValue(fieldNode, name + "." + Names.STRATEGY_POINTS_ONLY);
builder.fieldType().setPointsOnly(pointsOnly);
} else if (Names.STRATEGY_POINTS_ONLY.equals(fieldName)) {
pointsOnly = XContentMapValues.nodeBooleanValue(fieldNode, name + "." + Names.STRATEGY_POINTS_ONLY);
iterator.remove();
}
}
if (pointsOnly != null) {
if (builder.fieldType().strategyName.equals(SpatialStrategy.TERM.getStrategyName()) && pointsOnly == false) {
throw new IllegalArgumentException("points_only cannot be set to false for term strategy");
} else {
builder.fieldType().setPointsOnly(pointsOnly);
}
}
return builder;
}
}
Expand Down Expand Up @@ -565,7 +571,7 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
} else if (includeDefaults && fieldType().treeLevels() == 0) { // defaults only make sense if tree levels are not specified
builder.field(Names.TREE_PRESISION, DistanceUnit.METERS.toString(50));
}
if (includeDefaults || fieldType().strategyName() != Defaults.STRATEGY) {
if (includeDefaults || fieldType().strategyName().equals(Defaults.STRATEGY) == false) {
builder.field(Names.STRATEGY, fieldType().strategyName());
}
if (includeDefaults || fieldType().distanceErrorPct() != fieldType().defaultDistanceErrorPct) {
Expand All @@ -574,8 +580,15 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
if (includeDefaults || fieldType().orientation() != Defaults.ORIENTATION) {
builder.field(Names.ORIENTATION, fieldType().orientation());
}
if (includeDefaults || fieldType().pointsOnly() != GeoShapeFieldMapper.Defaults.POINTS_ONLY) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
if (fieldType().strategyName().equals(SpatialStrategy.TERM.getStrategyName())) {
// For TERMs strategy the defaults for points only change to true
if (includeDefaults || fieldType().pointsOnly() != true) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
}
} else {
if (includeDefaults || fieldType().pointsOnly() != GeoShapeFieldMapper.Defaults.POINTS_ONLY) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
}
}
if (includeDefaults || coerce.explicit()) {
builder.field(Names.COERCE, coerce.value());
Expand Down
60 changes: 58 additions & 2 deletions server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;

public class GeoShapeFieldMapperTests extends ESSingleNodeTestCase {

Expand Down Expand Up @@ -588,10 +589,65 @@ public void testSerializeDefaults() throws Exception {
}
}

public String toXContentString(GeoShapeFieldMapper mapper) throws IOException {
public void testPointsOnlyDefaultsWithTermStrategy() throws IOException {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", "quadtree")
.field("precision", "10m")
.field("strategy", "term")
.endObject().endObject()
.endObject().endObject());

DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser().parse("type1", new CompressedXContent(mapping));
FieldMapper fieldMapper = defaultMapper.mappers().getMapper("location");
assertThat(fieldMapper, instanceOf(GeoShapeFieldMapper.class));

GeoShapeFieldMapper geoShapeFieldMapper = (GeoShapeFieldMapper) fieldMapper;
PrefixTreeStrategy strategy = geoShapeFieldMapper.fieldType().defaultStrategy();

assertThat(strategy.getDistErrPct(), equalTo(0.0));
assertThat(strategy.getGrid(), instanceOf(QuadPrefixTree.class));
assertThat(strategy.getGrid().getMaxLevels(), equalTo(23));
assertThat(strategy.isPointsOnly(), equalTo(true));
// term strategy changes the default for points_only, check that we handle it correctly
assertThat(toXContentString(geoShapeFieldMapper, false), not(containsString("points_only")));
}


public void testPointsOnlyFalseWithTermStrategy() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", "quadtree")
.field("precision", "10m")
.field("strategy", "term")
.field("points_only", false)
.endObject().endObject()
.endObject().endObject());

DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser();

IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> parser.parse("type1", new CompressedXContent(mapping))
);
assertThat(e.getMessage(), containsString("points_only cannot be set to false for term strategy"));
}

public String toXContentString(GeoShapeFieldMapper mapper, boolean includeDefaults) throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject();
mapper.doXContentBody(builder, true, new ToXContent.MapParams(Collections.singletonMap("include_defaults", "true")));
ToXContent.Params params;
if (includeDefaults) {
params = new ToXContent.MapParams(Collections.singletonMap("include_defaults", "true"));
} else {
params = ToXContent.EMPTY_PARAMS;
}
mapper.doXContentBody(builder, includeDefaults, params);
return Strings.toString(builder.endObject());
}

public String toXContentString(GeoShapeFieldMapper mapper) throws IOException {
return toXContentString(mapper, true);
}

}

0 comments on commit 94d3ddd

Please sign in to comment.