Extend support of allowedFields to getMatchingFieldNames and getAllFields #106862
Conversation
…elds The SearchExecutionContext supports the notion of allowed fields, provided via a specific setter method. Fields are though only filtered for the getFieldType method. There needs to be consistency between getMatchingFieldNames and getFieldType. In fact there are places in the code where getMatchingFieldNames is called to resolve field name patterns, and later getFieldType is called on each of the resolved fields. If the former resolves to one field that we can't retrieve a field type for, that is unexpected and to be considered a bug. In addition, this commit adds consistency for getAllFields: this is only called by field caps, hence a different codepath that does not seem to set allowed fields for now, but it's important for the context to provide consistency around fields access, especially for methods that are as broad as getAllFields, despite their currently very specific usage. This surfaced as we are trying to move fetching of the `_ignored` field to use value fetchers, which use a search execution context and resolve the field type, whereas until now they are retrieved directly via StoredFieldsPhase and completely bypass such check. This commit also adds a test that was missing around verifying that SearchExecutionContext applies the allowedFields predicate when provided.
javanna
added
>bug
:Search Foundations/Mapping
elasticsearchmachine
added
Team:Search
|
Hi @javanna, I've created a changelog YAML for you. |
|
Pinging @elastic/es-security (Team:Security) |
|
Pinging @elastic/es-search (Team:Search) |
javanna
removed
:Security/Security
| } | ||
| } | ||
| } | ||
| return matches; | ||
| // If the field is not allowed, behave as if it is not mapped | ||
| return allowedFields == null ? matches : matches.stream().filter(allowedFields).collect(Collectors.toSet()); |
There was a problem hiding this comment.
I see here and below we always need to check for allowedFields to be null or not to decide if we need to filter one or more fields. I am wondering if we should require it to be not null and force the caller to use an identity predicate lambda (fieldName -> true) in case no field is filtered out.
There was a problem hiding this comment.
I am not too sure what is best here. Skip the filtering entirely when the predicate is null, or always filter although the predicate always returns true? I went for the former, which leaves things as close as they are outside of api keys queries (when setAllowedFields is not called).
There was a problem hiding this comment.
I agree that, maybe, performance-wise not applying the filter at all might be better if there is nothing to filter out...which is the reason why I said it looks good to me...it is just that all those null checks are annoying for me.
|
LGTM...I just left a couple of comments. |
| } | ||
| } | ||
| } | ||
| return matches; | ||
| // If the field is not allowed, behave as if it is not mapped | ||
| return allowedFields == null ? matches : matches.stream().filter(allowedFields).collect(Collectors.toSet()); |
There was a problem hiding this comment.
If I understand correctly this is just the same we were doing before + an additional filter on allowed fields based n field names...maybe we can just extract the existing method into something like a private getMatchingFieldNamesInternal to which we apply the filter?
There was a problem hiding this comment.
what is the gain compared to a single method?
The SearchExecutionContext supports the notion of allowed fields, provided via a specific setter method. Fields are though only filtered for the getFieldType method. There needs to be consistency between getMatchingFieldNames and getFieldType. In fact there are places in the code where getMatchingFieldNames is called to resolve field name patterns, and later getFieldType is called on each of the resolved fields. If the former resolves to one field that we can't retrieve a field type for, that is unexpected and to be considered a bug.
In addition, this commit adds consistency for getAllFields: this is only called by field caps, hence a different codepath that does not seem to set allowed fields for now, but it's important for the context to provide consistency around fields access, especially for methods that are as broad as getAllFields, despite their currently very specific usage.
This surfaced as we are trying to move fetching of the
_ignoredfield to use value fetchers, which use a search execution context and resolve the field type, whereas until now they are retrieved directly via StoredFieldsPhase and completely bypass such check.This commit also adds a test that was missing around verifying that SearchExecutionContext applies the allowedFields predicate when provided.