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

Fix error when requesting _type from fields option. #68413

Closed
wants to merge 2 commits into from
Closed

Fix error when requesting _type from fields option. #68413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d03516f
Fix error when requesting _type from fields option.
jtibshirani Feb 2, 2021
dba10e3
Merge branch 'master' into type-field
elasticmachine Feb 3, 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
14 changes: 10 additions & 4 deletions server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.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.index.mapper;

import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.regex.Regex;

import java.util.Collection;
Expand All @@ -33,16 +34,21 @@ final class FieldTypeLookup {
* For convenience, the set of copied fields includes the field itself.
*/
private final Map<String, Set<String>> fieldToCopiedFields = new HashMap<>();
private final String type;
private final DynamicKeyFieldTypeLookup dynamicKeyLookup;

/**
* A field type representing the document type. This will be null for 8.0 indices,
* which don't have a type and do not support using the _type field in searches.
*/
@Nullable private final TypeFieldType typeFieldType;
Copy link
Contributor Author

@jtibshirani jtibshirani Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@romseygeek I was wondering why we still have this logic on 'master' to handle searches on _type? Should it instead be removed completely? My approach here is to allow using _type against 7.x indices, but for 8.x indices act as though the field doesn't exist. I'm not sure this makes sense -- maybe we should just remove this compatibility logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to removing it entirely in master. I think that was actually my original intention and I just never got round to it after I made the latest round of changes to TypeFieldType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, then I'll close this PR and open a new one to remove TypeFieldType entirely.

For context, what is the plan for handling 7.x API compatibility, where I assume _type will be allowed in searches? (I think I've asked you this 3+ times, I just always seem to forget...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it has changed a few times, @pgomulka will have a better idea of what the current plan is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtibshirani We plan to support _type fields when a request was made with compatible API. if it was on a request, or part of a path it would be parsed but the value ignored. If there will be a need to return a _type field on a response, we will return _doc.
It is great you are asking, I am planning to create few example Compatible API for types removal and ask for feedback (I have few stale PRs with examples, can share offline if you like)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pgomulka -- I will ping you offline to understand the plan. My question is if I can remove support for _type in searches entirely from master, or if I need to leave it in to coordinate with your plans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed offline and agreed that the compatibility layer should continue to support _type within searches. However @pgomulka was fine with us removing this for now, and then later figuring out the best strategy for retaining compatibility (which may involve restoring some pieces).


FieldTypeLookup(
String type,
@Nullable TypeFieldType typeFieldType,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers,
Collection<RuntimeFieldType> runtimeFieldTypes
) {
this.type = type;
this.typeFieldType = typeFieldType;
Map<String, DynamicKeyFieldMapper> dynamicKeyMappers = new HashMap<>();

for (FieldMapper fieldMapper : fieldMappers) {
Expand Down Expand Up @@ -85,7 +91,7 @@ final class FieldTypeLookup {
*/
MappedFieldType get(String field) {
if (field.equals(TypeFieldType.NAME)) {
return new TypeFieldType(type);
return typeFieldType;
}

MappedFieldType fieldType = fullNameToFieldType.get(field);
Expand Down
8 changes: 7 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.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.index.mapper;

import org.elasticsearch.Version;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.analysis.IndexAnalyzers;
import org.elasticsearch.index.analysis.NamedAnalyzer;
Expand Down Expand Up @@ -148,7 +149,12 @@ public MappingLookup(Mapping mapping,
}
}

this.fieldTypeLookup = new FieldTypeLookup(mapping.root().name(), mappers, aliasMappers, mapping.root().runtimeFieldTypes());
TypeFieldType typeFieldType = null;
if (mapping != Mapping.EMPTY && indexSettings.getIndexVersionCreated().before(Version.V_8_0_0)) {
typeFieldType = new TypeFieldType(mapping.root().typeName());
}

this.fieldTypeLookup = new FieldTypeLookup(typeFieldType, mappers, aliasMappers, mapping.root().runtimeFieldTypes());
this.fieldMappers = Collections.unmodifiableMap(fieldMappers);
this.objectMappers = Collections.unmodifiableMap(objects);
}
Expand Down
25 changes: 13 additions & 12 deletions server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
public class FieldTypeLookupTests extends ESTestCase {

public void testEmpty() {
FieldTypeLookup lookup = new FieldTypeLookup("_doc", Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
assertNull(lookup.get("foo"));
Collection<String> names = lookup.simpleMatchToFullName("foo");
assertNotNull(names);
Expand All @@ -37,7 +37,7 @@ public void testFilter() {
Collection<FieldAliasMapper> fieldAliases = singletonList(new FieldAliasMapper("alias", "alias", "test"));
Collection<RuntimeFieldType> runtimeFields = List.of(
new TestRuntimeField("runtime", "type"), new TestRuntimeField("field", "type"));
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup("_doc", fieldMappers, fieldAliases, runtimeFields);
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(null, fieldMappers, fieldAliases, runtimeFields);
assertEquals(3, size(fieldTypeLookup.filter(ft -> true)));
for (MappedFieldType fieldType : fieldTypeLookup.filter(ft -> true)) {
if (fieldType.name().equals("test")) {
Expand Down Expand Up @@ -66,7 +66,7 @@ public void testFilter() {

public void testAddNewField() {
MockFieldMapper f = new MockFieldMapper("foo");
FieldTypeLookup lookup = new FieldTypeLookup("_doc", Collections.singletonList(f), emptyList(), Collections.emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, Collections.singletonList(f), emptyList(), Collections.emptyList());
assertNull(lookup.get("bar"));
assertEquals(f.fieldType(), lookup.get("foo"));
assertEquals(1, size(lookup.filter(ft -> true)));
Expand All @@ -76,7 +76,7 @@ public void testAddFieldAlias() {
MockFieldMapper field = new MockFieldMapper("foo");
FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo");

FieldTypeLookup lookup = new FieldTypeLookup("_doc", Collections.singletonList(field), Collections.singletonList(alias),
FieldTypeLookup lookup = new FieldTypeLookup(null, Collections.singletonList(field), Collections.singletonList(alias),
Collections.emptyList());

MappedFieldType aliasType = lookup.get("alias");
Expand All @@ -90,7 +90,7 @@ public void testSimpleMatchToFullName() {
FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "foo");
FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "bar");

FieldTypeLookup lookup = new FieldTypeLookup("_doc", List.of(field1, field2), List.of(alias1, alias2), List.of());
FieldTypeLookup lookup = new FieldTypeLookup(null, List.of(field1, field2), List.of(alias1, alias2), List.of());

Collection<String> names = lookup.simpleMatchToFullName("b*");

Expand All @@ -107,7 +107,7 @@ public void testSourcePathWithMultiFields() {
.addMultiField(new MockFieldMapper.Builder("field.subfield2"))
.build(new ContentPath());

FieldTypeLookup lookup = new FieldTypeLookup("_doc", singletonList(field), emptyList(), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, singletonList(field), emptyList(), emptyList());

assertEquals(Set.of("field"), lookup.sourcePaths("field"));
assertEquals(Set.of("field"), lookup.sourcePaths("field.subfield1"));
Expand All @@ -123,16 +123,17 @@ public void testSourcePathsWithCopyTo() {
.copyTo("field")
.build(new ContentPath());

FieldTypeLookup lookup = new FieldTypeLookup("_doc", Arrays.asList(field, otherField), emptyList(), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, Arrays.asList(field, otherField), emptyList(), emptyList());

assertEquals(Set.of("other_field", "field"), lookup.sourcePaths("field"));
assertEquals(Set.of("other_field", "field"), lookup.sourcePaths("field.subfield1"));
}

public void testTypeLookup() {
String type = randomAlphaOfLength(4);
TypeFieldType fieldType = new TypeFieldType(type);
assertThat(
((TypeFieldType) new FieldTypeLookup(type, List.of(), List.of(), List.of()).get(TypeFieldType.NAME)).getType(),
((TypeFieldType) new FieldTypeLookup(fieldType, List.of(), List.of(), List.of()).get(TypeFieldType.NAME)).getType(),
equalTo(type)
);
}
Expand All @@ -141,7 +142,7 @@ public void testRuntimeFieldsLookup() {
MockFieldMapper concrete = new MockFieldMapper("concrete");
TestRuntimeField runtime = new TestRuntimeField("runtime", "type");

FieldTypeLookup fieldTypeLookup = new FieldTypeLookup("_doc", List.of(concrete), emptyList(), List.of(runtime));
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(null, List.of(concrete), emptyList(), List.of(runtime));
assertThat(fieldTypeLookup.get("concrete"), instanceOf(MockFieldMapper.FakeFieldType.class));
assertThat(fieldTypeLookup.get("runtime"), instanceOf(TestRuntimeField.class));
assertEquals(2, size(fieldTypeLookup.filter(ft -> true)));
Expand All @@ -155,7 +156,7 @@ public void testRuntimeFieldOverrides() {
TestRuntimeField subfieldOverride = new TestRuntimeField("object.subfield", "type");
TestRuntimeField runtime = new TestRuntimeField("runtime", "type");

FieldTypeLookup fieldTypeLookup = new FieldTypeLookup("_doc", List.of(field, concrete, subfield), emptyList(),
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(null, List.of(field, concrete, subfield), emptyList(),
List.of(fieldOverride, runtime, subfieldOverride));
assertThat(fieldTypeLookup.get("field"), instanceOf(TestRuntimeField.class));
assertThat(fieldTypeLookup.get("object.subfield"), instanceOf(TestRuntimeField.class));
Expand All @@ -170,7 +171,7 @@ public void testRuntimeFieldsSimpleMatchToFullName() {
TestRuntimeField field2 = new TestRuntimeField("field2", "type");
TestRuntimeField subfield = new TestRuntimeField("object.subfield", "type");

FieldTypeLookup fieldTypeLookup = new FieldTypeLookup("_doc", List.of(field1, concrete), emptyList(), List.of(field2, subfield));
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(null, List.of(field1, concrete), emptyList(), List.of(field2, subfield));
{
Set<String> matches = fieldTypeLookup.simpleMatchToFullName("fie*");
assertEquals(2, matches.size());
Expand All @@ -192,7 +193,7 @@ public void testRuntimeFieldsSourcePaths() {
TestRuntimeField field2 = new TestRuntimeField("field2", "type");
TestRuntimeField subfield = new TestRuntimeField("object.subfield", "type");

FieldTypeLookup fieldTypeLookup = new FieldTypeLookup("_doc", List.of(field1, concrete), emptyList(), List.of(field2, subfield));
FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(null, List.of(field1, concrete), emptyList(), List.of(field2, subfield));
{
Set<String> sourcePaths = fieldTypeLookup.sourcePaths("field1");
assertEquals(1, sourcePaths.size());
Expand Down
14 changes: 13 additions & 1 deletion server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public void testNonExistentField() throws IOException {
.endObject();

Map<String, DocumentField> fields = fetchFields(mapperService, source, "non-existent");
assertThat(fields.size(), equalTo(0));
assertTrue(fields.isEmpty());
}

public void testMetadataFields() throws IOException {
Expand All @@ -120,6 +120,18 @@ public void testMetadataFields() throws IOException {
assertTrue(fields.isEmpty());
}

public void testTypeMetadataField() throws IOException {
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
.field("field", "value")
.endObject();

// The _type field was deprecated in 7.x and is not supported in 8.0. So the behavior
// should be the same as if the field didn't exist.
Map<String, DocumentField> fields = fetchFields(mapperService, source, "_type");
assertTrue(fields.isEmpty());
}

public void testFetchAllFields() throws IOException {
MapperService mapperService = createMapperService();
XContentBuilder source = XContentFactory.jsonBuilder().startObject()
Expand Down
10 changes: 5 additions & 5 deletions ...per-flattened/src/test/java/org/elasticsearch/index/mapper/FlattenedFieldLookupTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void testFieldTypeLookup() {
String fieldName = "object1.object2.field";
FlattenedFieldMapper mapper = createFlattenedMapper(fieldName);

FieldTypeLookup lookup = new FieldTypeLookup("_doc", singletonList(mapper), emptyList(), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, singletonList(mapper), emptyList(), emptyList());
assertEquals(mapper.fieldType(), lookup.get(fieldName));

String objectKey = "key1.key2";
Expand All @@ -61,7 +61,7 @@ public void testFieldTypeLookupWithAlias() {
String aliasName = "alias";
FieldAliasMapper alias = new FieldAliasMapper(aliasName, aliasName, fieldName);

FieldTypeLookup lookup = new FieldTypeLookup("_doc", singletonList(mapper), singletonList(alias), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, singletonList(mapper), singletonList(alias), emptyList());
assertEquals(mapper.fieldType(), lookup.get(aliasName));

String objectKey = "key1.key2";
Expand All @@ -84,11 +84,11 @@ public void testFieldTypeLookupWithMultipleFields() {
FlattenedFieldMapper mapper2 = createFlattenedMapper(field2);
FlattenedFieldMapper mapper3 = createFlattenedMapper(field3);

FieldTypeLookup lookup = new FieldTypeLookup("_doc", Arrays.asList(mapper1, mapper2), emptyList(), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, Arrays.asList(mapper1, mapper2), emptyList(), emptyList());
assertNotNull(lookup.get(field1 + ".some.key"));
assertNotNull(lookup.get(field2 + ".some.key"));

lookup = new FieldTypeLookup("_doc", Arrays.asList(mapper1, mapper2, mapper3), emptyList(), emptyList());
lookup = new FieldTypeLookup(null, Arrays.asList(mapper1, mapper2, mapper3), emptyList(), emptyList());
assertNotNull(lookup.get(field1 + ".some.key"));
assertNotNull(lookup.get(field2 + ".some.key"));
assertNotNull(lookup.get(field3 + ".some.key"));
Expand Down Expand Up @@ -125,7 +125,7 @@ public void testFieldLookupIterator() {
MockFieldMapper mapper = new MockFieldMapper("foo");
FlattenedFieldMapper flattenedMapper = createFlattenedMapper("object1.object2.field");

FieldTypeLookup lookup = new FieldTypeLookup("_doc", Arrays.asList(mapper, flattenedMapper), emptyList(), emptyList());
FieldTypeLookup lookup = new FieldTypeLookup(null, Arrays.asList(mapper, flattenedMapper), emptyList(), emptyList());

Set<String> fieldNames = new HashSet<>();
lookup.filter(ft -> true).forEach(ft -> fieldNames.add(ft.name()));
Expand Down