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

PassThroughObject nests aliases within objects for fields with dotted names #105298

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

kkrik-es
Copy link
Contributor

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

As part of #103648 that introduced PassThroughObject, ObjectMapper was modified to use the full name for FieldAliasMappers, so that aliases at the root level can contain dots. This led to a regression reported in Kibana tests where aliases were wrongly reported as duplicates. The culprit is that aliases with names containing dots were directly added under the root object, instead of being nested within objects. This change restores the right behavior.

Related to #103567

@elasticsearchmachine
Copy link
Collaborator

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

@kkrik-es kkrik-es changed the title Ignore duplicate FieldAliasMappers PassThroughObjectNest aliases i Feb 8, 2024
@kkrik-es kkrik-es changed the title PassThroughObjectNest aliases i PassThroughObject: nest aliases within objects for fields with dotted names Feb 8, 2024
@kkrik-es kkrik-es changed the title PassThroughObject: nest aliases within objects for fields with dotted names PassThroughObject nests aliases within objects for fields with dotted names Feb 8, 2024
@kkrik-es kkrik-es marked this pull request as ready for review February 8, 2024 20:36
@kkrik-es kkrik-es requested a review from martijnvg February 8, 2024 20:37
@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.

Maybe add a yaml test that simulated what the Kibana tests were doing? Otherwise LGTM.

pr: 105298
summary: Ignore duplicate `FieldAliasMappers`
area: TSDB
type: bug
Copy link
Member

Choose a reason for hiding this comment

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

This is a bug in unreleased code, let's mark it as non-issue and then a changelog isn't needed.

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.

@kkrik-es kkrik-es added >non-issue and removed >bug labels Feb 9, 2024
@kkrik-es
Copy link
Contributor Author

kkrik-es commented Feb 9, 2024

Maybe add a yaml test that simulated what the Kibana tests were doing? Otherwise LGTM.

Let's do this in a follow-up change, to avoid delaying this one. There's coverage for this in the 150_tsdb.yml test, we do need more tests outside the passthrough object.

@kkrik-es kkrik-es added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 9, 2024
@kkrik-es kkrik-es merged commit f65312e into elastic:main Feb 9, 2024
12 of 15 checks passed
@kkrik-es kkrik-es deleted the fix/103567 branch February 9, 2024 11:32
yctercero added a commit to elastic/kibana that referenced this pull request Feb 14, 2024
## Summary

Unskips tests that were skipped due to an upstream change and fixed in
elastic/elasticsearch#105298

Addresses #176105,
#176117,
#176270,
#176359,
#176360
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :StorageEngine/TSDB You know, for Metrics Team:StorageEngine v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants