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

Require type group or nested for fields with subfields #629

Merged
merged 4 commits into from
Oct 3, 2023

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Oct 2, 2023

What does this PR do?

Require type group for fields with subfields.

Why is it important?

Fields with subfields should not have a definition on their own. This would lead to ambiguity if for
example an object would generate a dynamic mapping, but also definitions for their subelements.

The only cases where a field can contain subfields is when it is of type group, or nested.

Checklist

Related issues

@jsoriano jsoriano requested a review from andrewkroh October 2, 2023 16:04
@jsoriano jsoriano requested a review from a team as a code owner October 2, 2023 16:04
@jsoriano jsoriano self-assigned this Oct 2, 2023
@jsoriano
Copy link
Member Author

jsoriano commented Oct 2, 2023

test integrations

@jsoriano jsoriano mentioned this pull request Oct 2, 2023
@elasticmachine
Copy link

Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#8048

@elasticmachine
Copy link

💚 Build Succeeded

cc @jsoriano

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

LGTM

- name: object_with_subfields
description: This field should be of type group, because it contains subfields.
type: object
object_type: keyword
Copy link
Member

Choose a reason for hiding this comment

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

object_type is not necessary here, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it wouldn't be neccesary to produce this validation error, but I added it so it doesn't trigger also the error about missing object_type for object.

@mrodm
Copy link
Contributor

mrodm commented Oct 3, 2023

test integrations

@elasticmachine
Copy link

Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#8065

@jsoriano
Copy link
Member Author

jsoriano commented Oct 3, 2023

Thanks Mario for fixing the integrations jobg! From the packages failed in elastic/integrations#8065, noone fails by linting, so I will merge this.

@jsoriano jsoriano merged commit 5bee4ac into elastic:main Oct 3, 2023
@jsoriano jsoriano deleted the disallow-fields-with-subfields branch October 3, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants