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

Nest pass-through objects within objects #105062

Merged
merged 9 commits into from Feb 5, 2024
Merged

Conversation

kkrik-es
Copy link
Contributor

@kkrik-es kkrik-es commented Feb 2, 2024

This is a follow-up on #103648, lifting the limitation that pass-through objects need to be defined at the root level.

Pass-through objects still need to be "leaf" fields, they can't have child objects.

Related to #103567

@elasticsearchmachine
Copy link
Collaborator

Hi @kkrik-es, I've created a changelog YAML for you.

@kkrik-es kkrik-es marked this pull request as ready for review February 2, 2024 13:44
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Just one idea for an additional case to test. Otherwise LGTM!

@@ -360,50 +360,150 @@ public void testPassThroughObjectWithAliases() throws IOException {
assertThat(mapperService.mappingLookup().getMapper("labels.dim"), instanceOf(KeywordFieldMapper.class));
}

public void testPassThroughObjectNested() throws IOException {
MapperService mapperService = createMapperService(mapping(b -> {
Copy link
Member

Choose a reason for hiding this comment

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

In the same test case, could you also add a top level attributes passthrough field? Just to check that this doesn’t cause a naming conflict because there’s also an attributes field in the resource object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, had a test for that in yaml too.

@kkrik-es kkrik-es merged commit e85bb5a into elastic:main Feb 5, 2024
15 checks passed
@kkrik-es kkrik-es deleted the fix/103567 branch February 8, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants