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

disallow creating geo_shape mappings with deprecated parameters #70850

Merged
merged 13 commits into from
Apr 30, 2021
Merged

disallow creating geo_shape mappings with deprecated parameters #70850

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
edfd032
disallow creating geo_shape mappings with deprecated parameters
iverase Mar 25, 2021
40ad9bc
better error message
iverase Mar 25, 2021
d707cf0
iter
iverase Mar 25, 2021
ab9063e
Merge branch 'master' into LegacyGeoShapeFieldMapper
iverase Apr 29, 2021
82995d1
iter
iverase Apr 29, 2021
4b4a5e3
move check to n GeoShapeFieldMapper's type parser
iverase Apr 29, 2021
44edae5
typo
iverase Apr 29, 2021
1b5ecf8
iter
iverase Apr 29, 2021
ff7fd85
iter
iverase Apr 29, 2021
6d66d6a
make possible to override creation of mapper service with specific ve…
iverase Apr 29, 2021
e93344b
Add test for GeoShapeWithDocValues
iverase Apr 29, 2021
4f3a451
update comment
iverase Apr 29, 2021
75b827d
Merge branch 'master' into LegacyGeoShapeFieldMapper
iverase Apr 30, 2021
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
3 changes: 2 additions & 1 deletion server/src/internalClusterTest/java/org/elasticsearch/search/geo/GeoFilterIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,9 @@ public void testShapeRelations() throws Exception {
.endObject()
.endObject());

final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
CreateIndexRequestBuilder mappingRequest = client().admin().indices().prepareCreate("shapes")
.setMapping(mapping);
.setMapping(mapping).setSettings(settings(version).build());
mappingRequest.get();
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().get();

Expand Down
23 changes: 16 additions & 7 deletions server/src/internalClusterTest/java/org/elasticsearch/search/geo/GeoShapeIntegrationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
package org.elasticsearch.search.geo;

import org.elasticsearch.Version;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.cluster.ClusterState;
Expand All @@ -22,6 +23,7 @@
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.VersionUtils;

import static org.elasticsearch.index.query.QueryBuilders.geoShapeQuery;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
Expand All @@ -32,6 +34,11 @@

public class GeoShapeIntegrationIT extends ESIntegTestCase {

@Override
protected boolean forbidPrivateIndexSettings() {
return false;
}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder()
Expand Down Expand Up @@ -125,25 +132,25 @@ public void testIgnoreMalformed() throws Exception {
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));
}

public void testMappingUpdate() throws Exception {
public void testMappingUpdate() {
// create index
assertAcked(client().admin().indices().prepareCreate("test")
.setMapping("shape", "type=geo_shape").get());
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
assertAcked(client().admin().indices().prepareCreate("test").setSettings(settings(version).build())
.setMapping("shape", "type=geo_shape,strategy=recursive").get());
ensureGreen();

String update ="{\n" +
" \"properties\": {\n" +
" \"shape\": {\n" +
" \"type\": \"geo_shape\",\n" +
" \"strategy\": \"recursive\"\n" +
" \"type\": \"geo_shape\"" +
" }\n" +
" }\n" +
"}";

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().indices()
.preparePutMapping("test")
.setSource(update, XContentType.JSON).get());
assertThat(e.getMessage(), containsString("mapper [shape] of type [geo_shape] cannot change strategy from [BKD] to [recursive]"));
assertThat(e.getMessage(), containsString("mapper [shape] of type [geo_shape] cannot change strategy from [recursive] to [BKD]"));
}

/**
Expand Down Expand Up @@ -205,7 +212,9 @@ public void testIndexPolygonDateLine() throws Exception {
assertAcked(client().admin().indices().prepareCreate("vector").setMapping(mappingVector).get());
ensureGreen();

assertAcked(client().admin().indices().prepareCreate("quad").setMapping(mappingQuad).get());
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
assertAcked(client().admin().indices().prepareCreate("quad")
.setSettings(settings(version).build()).setMapping(mappingQuad).get());
ensureGreen();

String source = "{\n" +
Expand Down
25 changes: 18 additions & 7 deletions ...rc/internalClusterTest/java/org/elasticsearch/search/geo/LegacyGeoShapeIntegrationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.search.geo;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.cluster.ClusterState;
Expand All @@ -24,6 +25,7 @@
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.VersionUtils;

import java.io.IOException;

Expand All @@ -35,10 +37,16 @@

public class LegacyGeoShapeIntegrationIT extends ESIntegTestCase {

@Override
protected boolean forbidPrivateIndexSettings() {
return false;
}

/**
* Test that orientation parameter correctly persists across cluster restart
*/
public void testOrientationPersistence() throws Exception {
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
String idxName = "orientation";
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
.startObject("properties").startObject("location")
Expand All @@ -49,7 +57,7 @@ public void testOrientationPersistence() throws Exception {
.endObject().endObject());

// create index
assertAcked(prepareCreate(idxName).setMapping(mapping));
assertAcked(prepareCreate(idxName).setMapping(mapping).setSettings(settings(version).build()));

mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
.startObject("properties").startObject("location")
Expand All @@ -59,7 +67,7 @@ public void testOrientationPersistence() throws Exception {
.endObject()
.endObject().endObject());

assertAcked(prepareCreate(idxName+"2").setMapping(mapping));
assertAcked(prepareCreate(idxName+"2").setMapping(mapping).setSettings(settings(version).build()));
ensureGreen(idxName, idxName+"2");

internalCluster().fullRestart();
Expand Down Expand Up @@ -95,7 +103,8 @@ public void testOrientationPersistence() throws Exception {
*/
public void testIgnoreMalformed() throws Exception {
// create index
assertAcked(client().admin().indices().prepareCreate("test")
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
assertAcked(prepareCreate("test").setSettings(settings(version).build())
.setMapping("shape", "type=geo_shape,tree=quadtree,ignore_malformed=true").get());
ensureGreen();

Expand Down Expand Up @@ -136,9 +145,9 @@ public void testIndexShapeRouting() throws Exception {
" }\n" +
" }}";


final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
// create index
assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping).get());
assertAcked(prepareCreate("test").setSettings(settings(version).build()).setMapping(mapping).get());
ensureGreen();

String source = "{\n" +
Expand All @@ -162,7 +171,8 @@ public void testIndexShapeRouting() throws Exception {
*/
public void testLegacyCircle() throws Exception {
// create index
assertAcked(client().admin().indices().prepareCreate("test")
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
assertAcked(prepareCreate("test").setSettings(settings(version).build())
.setMapping("shape", "type=geo_shape,strategy=recursive,tree=geohash").get());
ensureGreen();

Expand All @@ -181,9 +191,10 @@ public void testLegacyCircle() throws Exception {
}

public void testDisallowExpensiveQueries() throws InterruptedException, IOException {
final Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
try {
// create index
assertAcked(client().admin().indices().prepareCreate("test")
assertAcked(client().admin().indices().prepareCreate("test").setSettings(settings(version).build())
.setMapping("shape", "type=geo_shape,strategy=recursive,tree=geohash").get());
ensureGreen();

Expand Down
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,10 @@ public LegacyGeoShapeFieldMapper(String simpleName, MappedFieldType mappedFieldT
super(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), Lucene.KEYWORD_ANALYZER),
builder.ignoreMalformed.get(), builder.coerce.get(), builder.ignoreZValue.get(), builder.orientation.get(),
multiFields, copyTo, indexer, parser);
if (builder.indexCreatedVersion.onOrAfter(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this into the Builder constructor? The earlier we throw the better, in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried but I have issues with this test:

elasticsearch/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

Line 270 in 91c700b

public final void testDeprecatedBoost() throws IOException {

It seems that checking of unknown parameters happen after the builder.

throw new IllegalArgumentException("mapper [" + name()
+ "] of type [geo_shape] with deprecated parameters is no longer allowed");
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to be a bit clearer about what is causing the error here. Maybe instead of having the version check here we should move it to containsDeprecatedParameter so that the error message can explicitly contain the parameters that were used and are no longer allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I have done is to collect the used deprecated parameters in the builder so we can report back to the user. wdyt?

}
this.indexCreatedVersion = builder.indexCreatedVersion;
this.builder = builder;
}
Expand Down
4 changes: 1 addition & 3 deletions server/src/test/java/org/elasticsearch/index/mapper/FieldFilterMapperPluginTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,9 +297,7 @@ public Function<String, Predicate<String>> getFieldFilter() {
" \"type\": \"geo_point\"\n" +
" },\n" +
" \"area_visible\": {\n" +
" \"type\": \"geo_shape\", \n" +
" \"tree\": \"quadtree\",\n" +
" \"precision\": \"1m\"\n" +
" \"type\": \"geo_shape\"" +
" }\n" +
" }\n" +
" },\n" +
Expand Down
17 changes: 17 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.apache.lucene.spatial.prefix.tree.GeohashPrefixTree;
import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.common.CheckedConsumer;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.geo.GeoUtils;
Expand All @@ -25,6 +26,7 @@
import org.elasticsearch.index.query.SearchExecutionContext;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin;
import org.elasticsearch.test.VersionUtils;

import java.io.IOException;
import java.util.Collection;
Expand Down Expand Up @@ -98,6 +100,21 @@ protected boolean supportsMeta() {
return false;
}

@Override
protected MapperService createMapperService(XContentBuilder mappings) throws IOException {
Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
return createMapperService(version, mappings);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we also override createMapperService(Version, XContentBuilder) and do an assumeFalse that the version is less than Version.V_8_0_0? It's final in the base class at the moment but I think it's fine to make it overrideable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works too. Implemented.


public void testInvalidCurrentVersion() {
MapperParsingException e =
expectThrows(MapperParsingException.class,
() -> createMapperService(Version.CURRENT, fieldMapping(this::minimalMapping)));
assertThat(e.getMessage(),
containsString("mapper [field] of type [geo_shape] with deprecated parameters is no longer allowed"));
assertFieldWarnings("strategy");
}

public void testLegacySwitches() throws IOException {
// if one of the legacy parameters is added to a 'type':'geo_shape' config then
// that will select the legacy field mapper
Expand Down
15 changes: 13 additions & 2 deletions server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@
import org.elasticsearch.Version;
import org.elasticsearch.common.geo.SpatialStrategy;
import org.elasticsearch.index.mapper.LegacyGeoShapeFieldMapper.GeoShapeFieldType;
import org.elasticsearch.test.VersionUtils;

import java.io.IOException;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;

public class LegacyGeoShapeFieldTypeTests extends FieldTypeTestCase {

/**
Expand All @@ -30,9 +33,17 @@ public void testSetStrategyName() {
assertTrue(fieldType.pointsOnly());
}

public void testFetchSourceValue() throws IOException {
public void testInvalidCurrentVersion() {
iverase marked this conversation as resolved.
Show resolved Hide resolved
IllegalArgumentException e =
expectThrows(IllegalArgumentException.class,
() -> new LegacyGeoShapeFieldMapper.Builder("field", Version.CURRENT, false, true).build(new ContentPath()));
assertThat(e.getMessage(),
containsString("mapper [field] of type [geo_shape] with deprecated parameters is no longer allowed"));
}

MappedFieldType mapper = new LegacyGeoShapeFieldMapper.Builder("field", Version.CURRENT, false, true)
public void testFetchSourceValue() throws IOException {
Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
MappedFieldType mapper = new LegacyGeoShapeFieldMapper.Builder("field", version, false, true)
.build(new ContentPath()).fieldType();

Map<String, Object> jsonLineString = Map.of("type", "LineString", "coordinates",
Expand Down
Loading