From bdb4f8b19e23d9c9863b2d66f53818fa3c49f09a Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Thu, 24 Sep 2020 11:53:08 -0700 Subject: [PATCH 1/2] Make sure that IdFieldType#isAggregatable is accurate. Before, it always returned 'true' even when the setting "indices.id_field_data.enabled" was false. --- .../index/mapper/IdFieldMapper.java | 26 +++++++++++-------- .../index/mapper/MapperService.java | 4 +-- .../index/mapper/IdFieldMapperTests.java | 2 ++ .../index/mapper/IdFieldTypeTests.java | 12 +++++++-- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index 8471567c1ce24..487d2ae7b163a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -54,6 +54,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.function.BooleanSupplier; import java.util.function.Supplier; /** @@ -95,15 +96,18 @@ public static class Defaults { } } - public static final TypeParser PARSER = new FixedTypeParser(c -> new IdFieldMapper()); + public static final TypeParser PARSER = new FixedTypeParser(c -> { + BooleanSupplier fieldDataEnabled = c.mapperService().isIdFieldDataEnabled(); + return new IdFieldMapper(fieldDataEnabled); + }); static final class IdFieldType extends TermBasedFieldType { + private final BooleanSupplier fieldDataEnabled; - public static final IdFieldType INSTANCE = new IdFieldType(); - - private IdFieldType() { + IdFieldType(BooleanSupplier fieldDataEnabled) { super(NAME, true, true, false, TextSearchInfo.SIMPLE_MATCH_ONLY, Collections.emptyMap()); setIndexAnalyzer(Lucene.KEYWORD_ANALYZER); + this.fieldDataEnabled = fieldDataEnabled; } @Override @@ -143,6 +147,11 @@ public Query termsQuery(List values, QueryShardContext context) { @Override public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, Supplier searchLookup) { + if (fieldDataEnabled.getAsBoolean() == false) { + throw new IllegalArgumentException("Fielddata access on the _id field is disallowed, " + + "you can re-enable it by updating the dynamic cluster setting: " + + IndicesService.INDICES_ID_FIELD_DATA_ENABLED_SETTING.getKey()); + } final IndexFieldData.Builder fieldDataBuilder = new PagedBytesIndexFieldData.Builder( name(), TextFieldMapper.Defaults.FIELDDATA_MIN_FREQUENCY, @@ -156,11 +165,6 @@ public IndexFieldData build( CircuitBreakerService breakerService, MapperService mapperService ) { - if (mapperService.isIdFieldDataEnabled() == false) { - throw new IllegalArgumentException("Fielddata access on the _id field is disallowed, " - + "you can re-enable it by updating the dynamic cluster setting: " - + IndicesService.INDICES_ID_FIELD_DATA_ENABLED_SETTING.getKey()); - } deprecationLogger.deprecate("id_field_data", ID_FIELD_DATA_DEPRECATION_MESSAGE); final IndexFieldData fieldData = fieldDataBuilder.build(cache, breakerService, mapperService); @@ -251,8 +255,8 @@ public boolean advanceExact(int doc) throws IOException { }; } - private IdFieldMapper() { - super(new IdFieldType()); + private IdFieldMapper(BooleanSupplier fieldDataEnabled) { + super(new IdFieldType(fieldDataEnabled)); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 750b217059027..1432d76fb9108 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -464,8 +464,8 @@ public Analyzer searchQuoteAnalyzer() { /** * Returns true if fielddata is enabled for the {@link IdFieldMapper} field, false otherwise. */ - public boolean isIdFieldDataEnabled() { - return idFieldDataEnabled.getAsBoolean(); + public BooleanSupplier isIdFieldDataEnabled() { + return idFieldDataEnabled; } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java index fbf593df5fc5b..621eff32736d2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java @@ -88,6 +88,7 @@ public void testEnableFieldData() throws IOException { throw new UnsupportedOperationException(); }).build(null, null, mapperService)); assertThat(exc.getMessage(), containsString(IndicesService.INDICES_ID_FIELD_DATA_ENABLED_SETTING.getKey())); + assertFalse(ft.isAggregatable()); client().admin().cluster().prepareUpdateSettings() .setTransientSettings(Settings.builder().put(IndicesService.INDICES_ID_FIELD_DATA_ENABLED_SETTING.getKey(), true)) @@ -97,6 +98,7 @@ public void testEnableFieldData() throws IOException { throw new UnsupportedOperationException(); }).build(null, null, mapperService); assertWarnings(ID_FIELD_DATA_DEPRECATION_MESSAGE); + assertTrue(ft.isAggregatable()); } finally { // unset cluster setting client().admin().cluster().prepareUpdateSettings() diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldTypeTests.java index 738e7acfdf6c6..b706afaa25780 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldTypeTests.java @@ -32,7 +32,7 @@ public class IdFieldTypeTests extends ESTestCase { public void testRangeQuery() { - MappedFieldType ft = IdFieldMapper.IdFieldType.INSTANCE; + MappedFieldType ft = new IdFieldMapper.IdFieldType(() -> false); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ft.rangeQuery(null, null, randomBoolean(), randomBoolean(), null, null, null, null)); assertEquals("Field [_id] of type [_id] does not support range queries", e.getMessage()); @@ -53,8 +53,16 @@ public void testTermsQuery() throws Exception { MapperService mapperService = Mockito.mock(MapperService.class); Mockito.when(context.getMapperService()).thenReturn(mapperService); - MappedFieldType ft = IdFieldMapper.IdFieldType.INSTANCE; + MappedFieldType ft = new IdFieldMapper.IdFieldType(() -> false); Query query = ft.termQuery("id", context); assertEquals(new TermInSetQuery("_id", Uid.encodeId("id")), query); } + + public void testIsAggregatable() { + MappedFieldType ft = new IdFieldMapper.IdFieldType(() -> false); + assertFalse(ft.isAggregatable()); + + ft = new IdFieldMapper.IdFieldType(() -> true); + assertTrue(ft.isAggregatable()); + } } From 045a9bcf4258835215891fbe12a53ca5434467f6 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 5 Oct 2020 09:25:37 -0700 Subject: [PATCH 2/2] Fix leftover changes from merge. --- .../java/org/elasticsearch/index/mapper/IdFieldMapper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index de68a55e083ec..3605f7c3a3e27 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -54,7 +54,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.function.BooleanSupplier; import java.util.function.Supplier; /** @@ -99,12 +98,13 @@ public static class Defaults { public static final TypeParser PARSER = new FixedTypeParser(c -> new IdFieldMapper(() -> c.mapperService().isIdFieldDataEnabled())); static final class IdFieldType extends TermBasedFieldType { + private final Supplier fieldDataEnabled; IdFieldType(Supplier fieldDataEnabled) { super(NAME, true, true, false, TextSearchInfo.SIMPLE_MATCH_ONLY, Collections.emptyMap()); - setIndexAnalyzer(Lucene.KEYWORD_ANALYZER); this.fieldDataEnabled = fieldDataEnabled; + setIndexAnalyzer(Lucene.KEYWORD_ANALYZER); } @Override