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

Allow metadata fields in the _source #61590

Merged
merged 15 commits into from
Sep 18, 2020
Merged

Allow metadata fields in the _source #61590

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8a65d1c
Configurable metadata field mappers in the _source
csoulios Aug 26, 2020
d83b677
Changes to support metadata fields in _source
csoulios Sep 2, 2020
76f3dc3
Merge branch 'master' into metadata-source
csoulios Sep 8, 2020
4934129
Merged DocumentParserTests from master
csoulios Sep 9, 2020
a168a1b
Merge branch 'master' into metadata-source
csoulios Sep 9, 2020
2effa2f
Handle non string values
csoulios Sep 10, 2020
1f5aa63
Allow metadata fields to parse values/objects/arrays/null
csoulios Sep 16, 2020
78faca3
Removed MetadataFieldMapper.isAllowedInSource() method
csoulios Sep 16, 2020
1346865
Merge branch 'master' into metadata-source
csoulios Sep 16, 2020
39d19fe
Fixed bug that caused tests to break
csoulios Sep 16, 2020
f0bb957
Cleanup parsing for existing metadata fields
csoulios Sep 17, 2020
a734ea8
Cleanup parsing for existing metadata fields
csoulios Sep 17, 2020
b8b3a9d
Remove doParse() method
csoulios Sep 17, 2020
1560e73
Fix broken test
csoulios Sep 17, 2020
0c962c4
Lookup metadata mapper by name
csoulios Sep 17, 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
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper,
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
paths = splitAndValidatePath(currentFieldName);
if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) {
if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName)) == false) {
throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside"
+ " a document. Use the index API request parameters.");
} else if (containsDisabledObjectMapper(mapper, paths)) {
Expand Down
12 changes: 12 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,18 @@ public boolean isMetadataField(String field) {
return mapperRegistry.isMetadataField(indexVersionCreated, field);
}

/**
* @return Whether a field should be included in the document _source.
*/
public boolean isFieldAllowedInSource(String field) {
if(isMetadataField(field)) {
// Metadata fields may not be allowed in the document _source.
return mapperRegistry.isAllowedInSource(indexVersionCreated, field);
} else {
return true;
}
}

/** An analyzer wrapper that can lookup fields within the index mappings */
final class MapperAnalyzerWrapper extends DelegatingAnalyzerWrapper {

Expand Down
8 changes: 8 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ MetadataFieldMapper.Builder parse(String name, Map<String, Object> node,
* @param parserContext context that may be useful to build the field like analyzers
*/
MetadataFieldMapper getDefault(ParserContext parserContext);

/**
* @return Whether a metadata field can be included in the document _source.
*/
default boolean isAllowedInSource() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we really need this new flag isAllowedInSource. Maybe we could avoid adding a new flag and instead do the following:

  • For metadata fields that are not allowed in _source, make sure that MetadataFieldMapper#parse throws an descriptive error.
  • Many metadata fields currently have logic in parse that's unrelated to _source parsing. We could make sure to move it into the special methods preParse or postParse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtibshirani I agree with you that delegating this to MetadataFieldMapper#parse() would be a simpler solution. However I see two caveats:

  • The approach with isAllowedInSource() method detects that a metadata field is being parsed when parsing the field name. This is early in the parsing process. An exception is thrown and the field value parsing is skipped.

  • As you mention, I have seen classes (such as IgnoredFieldMapper and VersionFieldMapper) that override parse() method to do nothing on one hand, while calling super.parse() from preParse()/postParse() methods. This approach is too complicated imho, but I am not sure that refactoring this is very simple. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is early in the parsing process. An exception is thrown and the field value parsing is skipped.

I don't think we need to optimize performance in this case because it's an error condition (and should be a rare one too).

This approach is too complicated imho, but I am not sure that refactoring this is very simple.

I agree that the logic is complex/ hard to read! I haven't deeply looked into the refactoring myself -- maybe you could try it out and we can rediscuss the approach if you see a roadblock ?

// By default return false for metadata fields.
return false;
}
}

/**
Expand Down
12 changes: 12 additions & 0 deletions server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ public boolean isMetadataField(Version indexCreatedVersion, String field) {
return getMetadataMapperParsers(indexCreatedVersion).containsKey(field);
}

/**
* Returns true if the provided field can be included in the document _source.
*/
public boolean isAllowedInSource(Version indexCreatedVersion, String field) {
MetadataFieldMapper.TypeParser parser = getMetadataMapperParsers(indexCreatedVersion).get(field);
if (parser != null) {
return parser.isAllowedInSource();
}
// Non metadata fields should alway be allowed in document _source.
return true;
}

/**
* Returns a function that given an index name, returns a predicate that fields must match in order to be returned by get mappings,
* get index, get field mappings and field capabilities API. Useful to filter the fields that such API return.
Expand Down