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.

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

Return both concrete fields and aliases in DocumentFieldMappers#getMapper. #31671

Merged
merged 8 commits into from
Jun 30, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jun 28, 2018

This PR updates DocumentFieldMappers#getMapper to return both concrete fields and field aliases. Its method signature now has a top-level Mapper as its return type as opposed to a FieldMapper.

To support incremental changes, there is still a getFieldMapper method that returns a FieldMapper as before. Almost all callers were migrated to use the method that returns a Mapper, except the ~250 instances in server_test that require a FieldMapper. The getFieldMapper method has been marked as deprecated.

Updating DocumentFieldMappers#getMappers in this way has a few advantages:

  • There is now only one recommended way to retrieve a field's type information, and this pathway will correctly resolve field aliases. By switching callers over to use the new method signature, I found a few bugs that would've been difficult to anticipate otherwise (see the fixes below around percolate queries and geo suggestion contexts). This also helps avoid future bugs from being introduced by a developer accessing MappedFieldType in the wrong way.
  • By returning a Mapper, we force consumers to consider how field aliases should be handled. This also caught a few bugs (see the fixes below around dynamic object mappers and copy_to).
  • Because we return both concrete fields and aliases, the 'get field mappings' API now supports aliases without any additional work.

Open questions:

  • Are we okay with ~250 deprecated method calls in tests? I briefly looked into removing them, and it is a very large amount of manual work.
  • A natural option would be to introduce a parent class for FieldMapper and FieldAliasMapper, instead of just using the top-level Mapper. I quickly tried this out and didn't think it made things much cleaner, and also adds complexity to an (already complex) mapper hierarchy. I would be happy to take a more detailed look at it though.

@jtibshirani jtibshirani added >enhancement WIP :Search Foundations/Mapping Index mappings, including merging and defining field types labels Jun 28, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jpountz
Copy link
Contributor

jpountz commented Jun 29, 2018

Are we okay with ~250 deprecated method calls in tests? I briefly looked into removing them, and it is a very large amount of manual work.

I'd say no. We can look into sharing the load.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Change looks good, but we should have a plan to migrate all tests off the deprecated getFieldMapper method.

@jtibshirani
Copy link
Contributor Author

Thanks @jpountz for the review.

About the deprecated calls in tests: it will still take a bit of time for this feature branch to be ready to merge, and I think that making far-reaching changes right now will increase the chances of merge conflicts. Would you be fine if we updated the tests as a last task before merging? I can add a 'technical debt' item to the meta-issue to make sure we don't drop it.

@jpountz
Copy link
Contributor

jpountz commented Jun 29, 2018

@jtibshirani Yes that would work for me. Thanks for tackling this!

@jtibshirani
Copy link
Contributor Author

Added an item to the meta-issue. Is this okay to merge, or do you have other thoughts?

@jpountz
Copy link
Contributor

jpountz commented Jun 30, 2018

+1 to merge

@jtibshirani jtibshirani merged commit 2d95123 into elastic:field-aliases Jun 30, 2018
@jtibshirani jtibshirani deleted the document-field-mappers branch June 30, 2018 18:20
jtibshirani added a commit that referenced this pull request Jul 4, 2018
…pper. (#31671)

* Rename DocumentFieldMappers#getMapper to getFieldMapper.
* Introduce a new DocumentFieldMappers#getMapper that returns a Mapper.
* Fix an issue around field aliases in geo suggestion contexts.
* Make sure a field alias can refer to a percolate query.
* Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper.
* Include alias mappers in DocumentFieldMappers#getMappers.
* Make sure we detect conflicts between dynamic object mappers and field aliases.
* Throw an exception if aliases are specified as the target of copy_to.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 16, 2018
…pper. (elastic#31671)

* Rename DocumentFieldMappers#getMapper to getFieldMapper.
* Introduce a new DocumentFieldMappers#getMapper that returns a Mapper.
* Fix an issue around field aliases in geo suggestion contexts.
* Make sure a field alias can refer to a percolate query.
* Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper.
* Include alias mappers in DocumentFieldMappers#getMappers.
* Make sure we detect conflicts between dynamic object mappers and field aliases.
* Throw an exception if aliases are specified as the target of copy_to.
jtibshirani added a commit that referenced this pull request Jul 17, 2018
…pper. (#31671)

* Rename DocumentFieldMappers#getMapper to getFieldMapper.
* Introduce a new DocumentFieldMappers#getMapper that returns a Mapper.
* Fix an issue around field aliases in geo suggestion contexts.
* Make sure a field alias can refer to a percolate query.
* Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper.
* Include alias mappers in DocumentFieldMappers#getMappers.
* Make sure we detect conflicts between dynamic object mappers and field aliases.
* Throw an exception if aliases are specified as the target of copy_to.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
…pper. (#31671)

* Rename DocumentFieldMappers#getMapper to getFieldMapper.
* Introduce a new DocumentFieldMappers#getMapper that returns a Mapper.
* Fix an issue around field aliases in geo suggestion contexts.
* Make sure a field alias can refer to a percolate query.
* Remove easy-to-fix uses of DocumentFieldMappers#getFieldMapper.
* Include alias mappers in DocumentFieldMappers#getMappers.
* Make sure we detect conflicts between dynamic object mappers and field aliases.
* Throw an exception if aliases are specified as the target of copy_to.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
* 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)
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 24, 2018
jtibshirani added a commit that referenced this pull request Jul 24, 2018
* 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.
@jtibshirani jtibshirani removed the WIP label Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants