Skip to content

Add serialization test for FieldMappers when include_defaults=true #58235

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

Merged
merged 9 commits into from
Jun 18, 2020
Merged

Add serialization test for FieldMappers when include_defaults=true #58235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
57976bc
Add serialization test for FieldMappers when include_defaults=true
romseygeek Jun 17, 2020
4378509
remove debugging println
romseygeek Jun 17, 2020
26dbc1c
Merge branch 'master' into bug/mapping-404
elasticmachine Jun 17, 2020
16b4137
Merge branch 'master' into bug/mapping-404
elasticmachine Jun 17, 2020
0dad695
Merge remote-tracking branch 'origin/master' into bug/mapping-404
romseygeek Jun 17, 2020
0af547d
Simplify check
romseygeek Jun 17, 2020
82dcb82
Merge remote-tracking branch 'romseygeek/bug/mapping-404' into bug/ma…
romseygeek Jun 17, 2020
eaff992
Add explicit check for index option in base FieldMapper test
romseygeek Jun 18, 2020
6272943
Merge branch 'master' into bug/mapping-404
elasticmachine Jun 18, 2020
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
6 changes: 5 additions & 1 deletion ...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ public class ScaledFloatFieldMapper extends FieldMapper {
public static final String CONTENT_TYPE = "scaled_float";
// use the same default as numbers
private static final Setting<Boolean> COERCE_SETTING = NumberFieldMapper.COERCE_SETTING;
private static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
}

public static class Builder extends FieldMapper.Builder<Builder> {

Expand All @@ -83,7 +87,7 @@ public static class Builder extends FieldMapper.Builder<Builder> {
private Double nullValue;

public Builder(String name) {
super(name, new FieldType());
super(name, FIELD_TYPE);
builder = this;
}

Expand Down
2 changes: 1 addition & 1 deletion ...pper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class RankFeatureFieldMapperTests extends FieldMapperTestCase<RankFeature

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "store", "doc_values");
return Set.of("analyzer", "similarity", "store", "doc_values", "index");
}

@Before
Expand Down
2 changes: 1 addition & 1 deletion ...per-extras/src/test/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class RankFeaturesFieldMapperTests extends FieldMapperTestCase<RankFeatur

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "store", "doc_values");
return Set.of("analyzer", "similarity", "store", "doc_values", "index");
}

IndexService indexService;
Expand Down
2 changes: 1 addition & 1 deletion ...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ protected void mergeOptions(FieldMapper other, List<String> conflicts) {
@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
if (includeDefaults || (mappedFieldType.isSearchable() && fieldType.indexOptions() != IndexOptions.DOCS)) {
if (fieldType.indexOptions() != IndexOptions.NONE && (includeDefaults || fieldType.indexOptions() != IndexOptions.DOCS)) {
builder.field("index_options", indexOptionToString(fieldType.indexOptions()));
}
if (nullValue != null) {
Expand Down
2 changes: 1 addition & 1 deletion ...murmur3/src/test/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class Murmur3FieldMapperTests extends FieldMapperTestCase<Murmur3FieldMap

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "doc_values");
return Set.of("analyzer", "similarity", "doc_values", "index");
}

@Before
Expand Down
5 changes: 4 additions & 1 deletion .../internalClusterTest/java/org/elasticsearch/indices/mapping/SimpleGetFieldMappingsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,10 @@ public void testGetFieldMappings() throws Exception {
public void testSimpleGetFieldMappingsWithDefaults() throws Exception {
assertAcked(prepareCreate("test").setMapping(getMappingForType()));
client().admin().indices().preparePutMapping("test").setSource("num", "type=long").get();
client().admin().indices().preparePutMapping("test").setSource("field2", "type=text,index=false").get();

GetFieldMappingsResponse response = client().admin().indices().prepareGetFieldMappings()
.setFields("num", "field1", "obj.subfield").includeDefaults(true).get();
.setFields("num", "field1", "field2", "obj.subfield").includeDefaults(true).get();

assertThat((Map<String, Object>) response.fieldMappings("test", "num").sourceAsMap().get("num"),
hasEntry("index", Boolean.TRUE));
Expand All @@ -139,6 +140,8 @@ public void testSimpleGetFieldMappingsWithDefaults() throws Exception {
hasEntry("index", Boolean.TRUE));
assertThat((Map<String, Object>) response.fieldMappings("test", "field1").sourceAsMap().get("field1"),
hasEntry("type", "text"));
assertThat((Map<String, Object>) response.fieldMappings("test", "field2").sourceAsMap().get("field2"),
hasEntry("type", "text"));
assertThat((Map<String, Object>) response.fieldMappings("test", "obj.subfield").sourceAsMap().get("subfield"),
hasEntry("type", "keyword"));
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.elasticsearch.index.mapper;

import org.apache.lucene.document.FieldType;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.DocValuesFieldExistsQuery;
Expand Down Expand Up @@ -61,6 +62,7 @@ public static class Defaults {
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setStored(false);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.freeze();
}
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.lucene.document.LatLonDocValuesField;
import org.apache.lucene.document.LatLonPoint;
import org.apache.lucene.document.StoredField;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.IndexableField;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Explicit;
Expand Down Expand Up @@ -50,6 +51,7 @@ public class GeoPointFieldMapper extends AbstractPointGeometryFieldMapper<List<?
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setStored(false);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.freeze();
}

Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.apache.lucene.document.FieldType;
import org.apache.lucene.document.LatLonShape;
import org.apache.lucene.index.IndexOptions;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.geo.GeometryParser;
import org.elasticsearch.common.geo.builders.ShapeBuilder;
Expand Down Expand Up @@ -55,6 +56,7 @@ public class GeoShapeFieldMapper extends AbstractShapeGeometryFieldMapper<Geomet
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setDimensions(7, 4, Integer.BYTES);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.setOmitNorms(true);
FIELD_TYPE.freeze();
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.lucene.document.InetAddressPoint;
import org.apache.lucene.document.SortedSetDocValuesField;
import org.apache.lucene.document.StoredField;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.SortedSetDocValues;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.DocValuesFieldExistsQuery;
Expand Down Expand Up @@ -64,6 +65,7 @@ public static class Defaults {
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setDimensions(1, Integer.BYTES);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.freeze();
}
}
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,8 @@ protected void mergeOptions(FieldMapper other, List<String> conflicts) {
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);

if (includeDefaults || (mappedFieldType.isSearchable() && fieldType.indexOptions() != IndexOptions.DOCS)) {
if (fieldType.indexOptions() != IndexOptions.NONE
&& (includeDefaults || fieldType.indexOptions() != Defaults.FIELD_TYPE.indexOptions())) {
builder.field("index_options", indexOptionToString(fieldType.indexOptions()));
}
if (nullValue != null) {
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public static class Defaults {
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setStored(false);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.freeze();
}
}
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.index.mapper;

import org.apache.lucene.document.FieldType;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.BoostQuery;
import org.apache.lucene.search.DocValuesFieldExistsQuery;
Expand Down Expand Up @@ -75,6 +76,7 @@ public static class Defaults {
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setStored(false);
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
FIELD_TYPE.freeze();
}
public static final DateFormatter DATE_FORMATTER = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER;
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,8 @@ protected boolean docValuesByDefault() {
@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
if (includeDefaults || (mappedFieldType.isSearchable() && fieldType.indexOptions() != Defaults.FIELD_TYPE.indexOptions())) {
if (fieldType.indexOptions() != IndexOptions.NONE
Copy link
Member

Choose a reason for hiding this comment

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

so we don't want to print out index_options: none ? sorry I don't recall what we were doing before the refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if index is set to false then we don't emit the index_options. The pre-existing method that converts the lucene IndexOptions enum to a string doesn' accept IndexOptions.NONE

Copy link
Member

Choose a reason for hiding this comment

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

thanks for explaining, makes sense

&& (includeDefaults || fieldType.indexOptions() != Defaults.FIELD_TYPE.indexOptions())) {
builder.field("index_options", indexOptionToString(fieldType.indexOptions()));
}
if (includeDefaults || fieldType.storeTermVectors() != Defaults.FIELD_TYPE.storeTermVectors()) {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class BinaryFieldMapperTests extends FieldMapperTestCase<BinaryFieldMappe

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "eager_global_ordinals", "norms", "similarity");
return Set.of("analyzer", "eager_global_ordinals", "norms", "similarity", "index");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/CompletionFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public void addModifiers() {

@Override
protected Set<String> unsupportedProperties() {
return Set.of("doc_values");
return Set.of("doc_values", "index");
}

@Override
Expand Down
27 changes: 17 additions & 10 deletions test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiConsumer;

Expand Down Expand Up @@ -95,6 +96,7 @@ protected Set<String> unsupportedProperties() {
b.docValues(false);
}),
booleanModifier("eager_global_ordinals", true, (a, t) -> a.setEagerGlobalOrdinals(t)),
booleanModifier("index", false, (a, t) -> a.index(t)),
booleanModifier("norms", false, FieldMapper.Builder::omitNorms),
new Modifier("search_analyzer", true, (a, b) -> {
a.searchAnalyzer(new NamedAnalyzer("standard", AnalyzerScope.INDEX, new StandardAnalyzer()));
Expand Down Expand Up @@ -228,22 +230,27 @@ protected void assertSerializes(String indexname, T builder) throws IOException

Mapper.BuilderContext context = new Mapper.BuilderContext(SETTINGS, new ContentPath(1));

XContentBuilder x = JsonXContent.contentBuilder();
x.startObject().startObject("properties");
builder.build(context).toXContent(x, ToXContent.EMPTY_PARAMS);
x.endObject().endObject();
String mappings = Strings.toString(x);
String mappings = mappingsToString(builder.build(context), false);
String mappingsWithDefault = mappingsToString(builder.build(context), true);

mapperService.merge("_doc", new CompressedXContent(mappings), MapperService.MergeReason.MAPPING_UPDATE);

Mapper rebuilt = mapperService.documentMapper().mappers().getMapper(builder.name);
x = JsonXContent.contentBuilder();
x.startObject().startObject("properties");
rebuilt.toXContent(x, ToXContent.EMPTY_PARAMS);
x.endObject().endObject();
String reparsed = Strings.toString(x);
String reparsed = mappingsToString(rebuilt, false);
String reparsedWithDefault = mappingsToString(rebuilt, true);

assertThat(reparsed, equalTo(mappings));
assertThat(reparsedWithDefault, equalTo(mappingsWithDefault));
}

private String mappingsToString(ToXContent builder, boolean includeDefaults) throws IOException {
ToXContent.Params params = includeDefaults ?
new ToXContent.MapParams(Map.of("include_defaults", "true")) : ToXContent.EMPTY_PARAMS;
XContentBuilder x = JsonXContent.contentBuilder();
x.startObject().startObject("properties");
builder.toXContent(x, params);
x.endObject().endObject();
return Strings.toString(x);
}

}
2 changes: 1 addition & 1 deletion ...ics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class HistogramFieldMapperTests extends FieldMapperTestCase<HistogramFiel

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "doc_values", "store");
return Set.of("analyzer", "similarity", "doc_values", "store", "index");
}

public void testParseValue() throws Exception {
Expand Down
2 changes: 2 additions & 0 deletions .../main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ public Builder(String name) {
builder = this;
}

// TODO we should ban setting 'index' on constant keyword

public Builder setValue(String value) {
this.value = value;
return this;
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ protected Collection<Class<? extends Plugin>> getPlugins() {

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "store", "doc_values");
return Set.of("analyzer", "similarity", "store", "doc_values", "index");
}

@Override
Expand Down
3 changes: 2 additions & 1 deletion ...attened/src/main/java/org/elasticsearch/xpack/flattened/mapper/FlatObjectFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
if (includeDefaults || mappedFieldType.isSearchable() && fieldType.indexOptions() != Defaults.FIELD_TYPE.indexOptions()) {
if (fieldType.indexOptions() != IndexOptions.NONE
&& (includeDefaults || fieldType.indexOptions() != Defaults.FIELD_TYPE.indexOptions())) {
Copy link
Member

Choose a reason for hiding this comment

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

this logic is now duplicated in multiple places, I am tempted to ask if we can share it, on the other hand I am afraid it may complicate things and removing this duplication is not a priority at the moment.

builder.field("index_options", indexOptionToString(fieldType.indexOptions()));
}
if (includeDefaults || depthLimit != Defaults.DEPTH_LIMIT) {
Expand Down
7 changes: 6 additions & 1 deletion ...n/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.lucene.document.FieldType;
import org.apache.lucene.document.LatLonShape;
import org.apache.lucene.document.ShapeField;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.Version;
Expand Down Expand Up @@ -61,13 +62,17 @@
*/
public class GeoShapeWithDocValuesFieldMapper extends GeoShapeFieldMapper {
public static final String CONTENT_TYPE = "geo_shape";
public static final FieldType FIELD_TYPE = new FieldType();
static {
FIELD_TYPE.setIndexOptions(IndexOptions.DOCS);
}

public static class Builder extends AbstractShapeGeometryFieldMapper.Builder<Builder, GeoShapeWithDocValuesFieldType> {

private boolean docValuesSet = false;

public Builder(String name) {
super (name, new FieldType());
super (name, FIELD_TYPE);
this.hasDocValues = true;
}

Expand Down
2 changes: 1 addition & 1 deletion ...ors/src/test/java/org/elasticsearch/xpack/vectors/mapper/DenseVectorFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ protected Collection<Class<? extends Plugin>> getPlugins() {

@Override
protected Set<String> unsupportedProperties() {
return Set.of("analyzer", "similarity", "doc_values", "store");
return Set.of("analyzer", "similarity", "doc_values", "store", "index");
}

@Before
Expand Down