-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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.
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
Add validation for field alias mappings. #31518
Add validation for field alias mappings. #31518
Conversation
b71de74
to
b62c73a
Compare
3d2dfe4
to
df9f323
Compare
Pinging @elastic/es-search-aggs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minor comment but it looks good to me. Thanks for tackling this!
} | ||
} | ||
|
||
private void validateAlias(String aliasName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe some docs would help on the two above methods, for instance to say that new aliases have not been added yet when validateField is called and that new fields have already been added when validateAlias is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
import java.util.Set; | ||
import java.util.stream.Stream; | ||
|
||
public class MapperMergeValidator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be pkg-private? please also add some minimal javadocs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
String pathScope = getNestedScope(path, fullPathObjectMappers); | ||
if (!Objects.equals(aliasScope, pathScope)) { | ||
throw new IllegalArgumentException("Invalid [path] value [" + path + "] for field alias [" + | ||
aliasName + "]: an alias must have the same nested scope as its target."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add what we computed as the scope of the alias and the path to the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@jtibshirani Could you update the description of this PR to describe what is being validated? This is important since this PR might be linked from release notes. |
5c399fd
to
f4fabf5
Compare
f4fabf5
to
f34f57d
Compare
* Perform basic validation on the target of an alias. * Validate that an alias and its target have the same nested scope. * Pull out some validation logic into a separate class. * Validate the uniqueness of field alias mappings.
* Perform basic validation on the target of an alias. * Validate that an alias and its target have the same nested scope. * Pull out some validation logic into a separate class. * Validate the uniqueness of field alias mappings.
* Perform basic validation on the target of an alias. * Validate that an alias and its target have the same nested scope. * Pull out some validation logic into a separate class. * Validate the uniqueness of field alias mappings.
* Perform basic validation on the target of an alias. * Validate that an alias and its target have the same nested scope. * Pull out some validation logic into a separate class. * Validate the uniqueness of field alias mappings.
* Perform basic validation on the target of an alias. * Validate that an alias and its target have the same nested scope. * Pull out some validation logic into a separate class. * Validate the uniqueness of field alias mappings.
* Add basic support for field aliases in index mappings. (#31287) * Allow for aliases when fetching stored fields. (#31411) * Add tests around accessing field aliases in scripts. (#31417) * Add documentation around field aliases. (#31538) * Add validation for field alias mappings. (#31518) * Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671) * Make sure that field-level security is enforced when using field aliases. (#31807) * Add more comprehensive tests for field aliases in queries + aggregations. (#31565) * Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148)
* Add basic support for field aliases in index mappings. (#31287) * Allow for aliases when fetching stored fields. (#31411) * Add tests around accessing field aliases in scripts. (#31417) * Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671) * Add documentation around field aliases. (#31538) * Add validation for field alias mappings. (#31518) * Make sure that field-level security is enforced when using field aliases. (#31807) * Add more comprehensive tests for field aliases in queries + aggregations. (#31565) * Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148) * Ensure that field aliases cannot be used in multi-fields. (#32219) * Make sure that field aliases count towards the total fields limit. (#32222) * Fix a test bug around nested aggregations and field aliases. (#32287) * Make sure the _uid field is correctly loaded in scripts. * Fix the failing test case FieldLevelSecurityTests#testParentChild_parentField. * Enforce that field aliases can only be specified on indexes with a single type.
The following restrictions are enforced on the target of an alias: