From 980cbed54e09f4fe05df87a739f44001c23e3a01 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 3 Jul 2018 08:47:55 -0400 Subject: [PATCH] Fix handling of points_only with term strategy in geo_shape 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 --- .../index/mapper/GeoShapeFieldMapper.java | 27 ++++++--- .../mapper/GeoShapeFieldMapperTests.java | 60 ++++++++++++++++++- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java index 7b083c2ce9e0f..318d9cfc6fa7e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java @@ -199,6 +199,7 @@ public static class TypeParser implements Mapper.TypeParser { @Override public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { Builder builder = new Builder(name); + Boolean pointsOnly = null; for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); String fieldName = entry.getKey(); @@ -230,13 +231,18 @@ public Mapper.Builder parse(String name, Map 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; } } @@ -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) { @@ -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()); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java index 00b3b7c7f3e73..7ff8c28f6dc63 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java @@ -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 { @@ -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); + } + }