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 matching of leaf fields for objects #2054

Merged
merged 10 commits into from
Sep 4, 2024

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Aug 28, 2024

Validation of objects was happening only depending of the format. If there were no subobjects, the validation was happening, and if there were objects with subfields, the validation was not happening for the subfields. This leads to different results with different mapping settings for the same data. For example depending if synthetics source or subobjects: false is used.

Additionally, for subfields that should match the definition of the parent object, there were cases where this match was not happening, so the test could unexpectedly fail.

Even if usually it is recommended to add explicit wildcards in dynamic mappings, definitions like the following one are expected to match subfields, but currently they aren't.

    - name: user
      type: group
      fields:
        - name: names
          type: object
          object_type: keyword
          description: Array of user identification documents.

The definition above would not match for user.names.db.

Seen for several packages in https://buildkite.com/elastic/integrations/builds/15135 (elastic/integrations#10919).

@jsoriano jsoriano requested a review from a team August 28, 2024 19:20
@jsoriano jsoriano self-assigned this Aug 28, 2024
@jsoriano
Copy link
Member Author

Pending to confirm if this is actually needed or a regression in latest main, because this was not failing before in dailies.

@jsoriano
Copy link
Member Author

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10922

@jsoriano jsoriano force-pushed the fix-matching-objects-without-wildcard branch from 0d0c33d to 40bdac2 Compare August 29, 2024 22:06
@jsoriano
Copy link
Member Author

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10922

@jsoriano jsoriano changed the title Fix matching of leaf fields for objects that don't have a wildcard Fix matching of leaf fields for objects Aug 30, 2024
@jsoriano jsoriano marked this pull request as ready for review August 30, 2024 11:34
@jsoriano
Copy link
Member Author

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10922

@jsoriano
Copy link
Member Author

jsoriano commented Sep 2, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10922

@jsoriano jsoriano force-pushed the fix-matching-objects-without-wildcard branch from 18ffb90 to fe15ae8 Compare September 2, 2024 13:41
@jsoriano
Copy link
Member Author

jsoriano commented Sep 2, 2024

Changes related to unexpected types moved to #2065 and elastic/package-spec#791.

@jsoriano
Copy link
Member Author

jsoriano commented Sep 2, 2024

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10922

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!

definition.ObjectType = ""
return v.parseSingleElementValue(key, definition, val, doc)
case definition.Type == "object" && definition.ObjectType == "":
// Legacy mapping, abiguous definition not allowed by recent versions of the spec, ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Legacy mapping, abiguous definition not allowed by recent versions of the spec, ignore it.
// Legacy mapping, ambiguous definition not allowed by recent versions of the spec, ignore it.

@jsoriano jsoriano enabled auto-merge (squash) September 4, 2024 10:56
@jsoriano jsoriano merged commit dfb8b3a into elastic:main Sep 4, 2024
3 checks passed
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @jsoriano

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.

3 participants