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

add Merging mappings subobjects:false check #105099

Conversation

ghostspiders
Copy link
Contributor

If the subobjects parameter is set to false, Elasticsearch will only validate that objects within the mappings configured during parsing do not contain object mappers, but it does not validate when merging mappings that include subobjects (assertions are used for validation in the test environment, but assertions are disabled in production), which may lead to invalid mappings. Add validation for merging.
#103497

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.13.0 needs:triage Requires assignment of a team area label labels Feb 3, 2024
@javanna javanna added :Search Foundations/Mapping Index mappings, including merging and defining field types >bug and removed needs:triage Requires assignment of a team area label labels Feb 5, 2024
@javanna javanna self-assigned this Feb 5, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Feb 5, 2024
@javanna
Copy link
Member

javanna commented Feb 5, 2024

thanks for opening this PR @ghostspiders ! It would be great to have a new test added that reproduces the issue that this change fixed. ObjectMapperMergeTests would be a place to start. Is that something you'd be willing to add?

Copy link

cla-checker-service bot commented Feb 10, 2024

❌ Author of the following commits did not sign a Contributor Agreement:
, d87549e, 4b6173d, f17896d, 6361d4d, 8d5d0fd, 8b89333, b41c3c8, 2294c92

Please, read and sign the above mentioned agreement if you want to contribute to this project

@ghostspiders
Copy link
Contributor Author

@javanna testMergedMappingSubobjectsFalse Function reproduces the issue,My current idea is to modify the doMerge method in the MapperService.java class by removing the assert keyword and enabling the assertSerialization method in the production environment to fix this issue.

@javanna
Copy link
Member

javanna commented Feb 13, 2024

Thanks @ghostspiders I think that you removed the fix. Can you add it back?

@javanna
Copy link
Member

javanna commented Feb 13, 2024

Would you mind signing the CLA if you'd like your change to be merged? Thanks! Perhaps the problem is that you have commits in your PR from two users, and only one of those has signed the CLA.

@felixbarny
Copy link
Member

#103542 should fix this problem as well. I've included a similar test case as the one in your PR.

@javanna javanna removed the v8.14.0 label Feb 15, 2024
@ghostspiders ghostspiders deleted the add-Merging-mappings-subobjectsfalse-check branch February 17, 2024 15:28
@ghostspiders ghostspiders restored the add-Merging-mappings-subobjectsfalse-check branch February 17, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug external-contributor Pull request authored by a developer outside the Elasticsearch team :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants