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

Fix `_parent.type` validation #11436

Merged

Conversation

@martijnvg
Copy link
Member

commented May 30, 2015

A _parent field can only point to a type that doesn't exist yet.
The validation that checks this relied on have all mappings in the
MapperService. The issue is that this check is performed on the
elected master node and it may not have the IndexService at all
to perform this check. In that case it creates a temporary IndexService and
MapperService to perform mapping validation, but only the mappings
that are part of the put index call are created, not the already existing mappings.
Because of that the _parent field validation can't be performed.

By changing the validation to rely on the cluster state's IndexMetaData instead
we can get around the issue with the IndexService/MapperService on the elected
master node. The IndexMetaData always holds the MappingMetaData instances
for all types.

this.hasParentField = true;
} else {
this.hasParentField = false;
this.parentType = ((Map<String, String>) withoutType.get("_parent")).get("type");

This comment has been minimized.

Copy link
@jpountz

jpountz May 31, 2015

Contributor

I don't think it's legal to cast to a Map<String, String> directly since the parent mapping can also take a map of fielddata settings? So we should first case the map to Map<String, String> and then case the value from Object to String?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 31, 2015

The change looks good. However, when I read:

but only the mappings that are part of the put index call are created, not the already existing mappings.

I'm wondering that this might be an issue for mapping validation in general, eg. if we want to enforce that different types on the same index have consistent field types. So maybe a better fix would be to load all mappings in this temporary index? cc @rjernst

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented May 31, 2015

@jpountz I thought about this too, but it never was an issue before because the there was no cross mapping/type validation. The _parent field is the first feature to do so. I didn't go for it because it adds an additional parsing cost that no other fields benefit from yet.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 31, 2015

I think we can merge your change as going back would be easy. Just wanted to make @rjernst aware of this. :)

A `_parent` field can only point to a type that doesn't exist yet.
The validation that checks this relied on have all mappings in the
MapperService. The issue is that this check is performed on the
elected master node and it may not have the IndexService at all
to perform this check. In that case it creates a temporary IndexService and
MapperService to perform mapping validation, but only the mappings
that are part of the put index call are created, not the already existing mappings.
Because of that the `_parent` field validation can't be performed.

By changing the validation to rely on the cluster state's IndexMetaData instead
we can get around the issue with the IndexService/MapperService on the elected
master node. The IndexMetaData always holds the MappingMetaData instances
for all types.
@martijnvg martijnvg force-pushed the martijnvg:parent-child/bug/_parent_field_validation branch from 860e788 to 2fe7cde May 31, 2015
@martijnvg martijnvg removed the review label May 31, 2015
martijnvg added a commit that referenced this pull request May 31, 2015
…ld_validation

parent/child: Fix `_parent.type` validation.
@martijnvg martijnvg merged commit 2c2f2fe into elastic:master May 31, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@clintongormley clintongormley changed the title parent/child: Fix `_parent.type` validation. Fix `_parent.type` validation May 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.