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

Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING #6793

Closed
wants to merge 2 commits into from

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Jul 9, 2014

At the moment one can iterate the MapperService to go through all document mappers. This includes the document mapper of DEFAULT_MAPPING, which may be surprising and lead to unintended results. This commit removes the Iterable implementation and add a docMappers method that asks the caller to make an explicit choice

…n DEFAULT_MAPPING

At the moment one can iterate the MapperService to go through all document mappers. This includes the document mapper of DEFAULT_MAPPING, which may be surprising and lead to unintended results. This commit removes the Iterable implementation and add a docMappers method that asks the caller to make an explicit choice
@Override
public UnmodifiableIterator<DocumentMapper> iterator() {
return Iterators.unmodifiableIterator(mappers.values().iterator());
public Iterable<DocumentMapper> docMappers(final boolean includingDefaultMapping) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe add some java doc? (emphasising that includingDefaultMapping should be false must of the time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. Will add.

@martijnvg
Copy link
Member

LGTM, left one minor comment

if (includingDefaultMapping) {
iter = mappers.values().iterator();
} else {
iter = Iterators.filter(mappers.values().iterator(), new Predicate<DocumentMapper>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a static final inner class for this predicate? it seems static?

@bleskes
Copy link
Contributor Author

bleskes commented Jul 9, 2014

pushed another commit based on the feedback

@jpountz
Copy link
Contributor

jpountz commented Jul 9, 2014

LGTM

@bleskes bleskes closed this in 684e698 Jul 9, 2014
bleskes added a commit that referenced this pull request Jul 9, 2014
…n DEFAULT_MAPPING

At the moment one can iterate the MapperService to go through all document mappers. This includes the document mapper of DEFAULT_MAPPING, which may be surprising and lead to unintended results. This commit removes the Iterable implementation and add a docMappers method that asks the caller to make an explicit choice

Closes #6793
@bleskes bleskes deleted the mapper_service_iterator branch July 9, 2014 09:38
@jpountz jpountz removed the review label Jul 16, 2014
@clintongormley clintongormley changed the title [Code] Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING Internal: Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING Jul 16, 2014
@clintongormley clintongormley added the :Search/Mapping Index mappings, including merging and defining field types label Jun 7, 2015
@clintongormley clintongormley changed the title Internal: Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Mapping Index mappings, including merging and defining field types v1.3.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants