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 validation of subfields of flattened objects #2088

Merged

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Sep 6, 2024

When a document doesn't has subobjects, like when subobjects: false or synthetic mode is used, if an undocumented field is found, we need to check its ancestors to see if it is a member of a flattened object.

This is producing failures for auditd_manager package when LogsDB is enabled: https://buildkite.com/elastic/integrations/builds/15559

test case failed: one or more errors found in documents stored in logs-auditd_manager.auditd-20702 data stream: [0] field "auditd.data.a0" is undefined, could be a multifield
[1] field "auditd.data.a1" is undefined, could be a multifield
[2] field "auditd.data.a2" is undefined, could be a multifield
[3] field "auditd.data.a3" is undefined, could be a multifield

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @jsoriano

Comment on lines +868 to +870
if ancestor.Type == "flattened" {
return true
}
Copy link
Contributor

@mrodm mrodm Sep 6, 2024

Choose a reason for hiding this comment

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

If it finds an ancestor with a different type of "flattened", should this return false directly here?

I was thinking in the case of the ancestor being something like: ancestor.Type = object and ancestor.ObjectTYpe == "keyword"

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, there are packages defining mappings for subfields of flattened objects (for example here). This doesn't make a lot of sense as a mapping, but this is allowed now, and can be used for example to provide documentation.

We probably should not allow to define mappings under flattened fields, but this would be a different task, and checked in a different place. And we would need to provide something for cases where developers want to provide docs for flattened subfields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, there are packages defining mappings for subfields of flattened objects (for example here). This doesn't make a lot of sense as a mapping, but this is allowed now, and can be used for example to provide documentation.

Ok, I didn't know about this kind of usage.

We probably should not allow to define mappings under flattened fields, but this would be a different task, and checked in a different place. And we would need to provide something for cases where developers want to provide docs for flattened subfields.

Agreed!

Comment on lines +862 to +864
i := strings.LastIndex(key, ".")
key = key[:i]
ancestor := FindElementDefinition(key, schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be replaced by findParentElementDefinition ?

Suggested change
i := strings.LastIndex(key, ".")
key = key[:i]
ancestor := FindElementDefinition(key, schema)
ancestor := findParentElementDefinition(key, schema)

Copy link
Contributor

Choose a reason for hiding this comment

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

mmm I guess it would be needed to somehow return also the key of the field, or being able to retrieve it from the ancestor struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this is the problem I found, we still need to loop over the ancestors.

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! Checked locally too with logsdb enabled for audtid_manager package.

@jsoriano jsoriano merged commit fe16a06 into elastic:main Sep 6, 2024
3 checks passed
@jsoriano jsoriano deleted the fix-matching-subfields-flattened-synthetics branch September 6, 2024 17: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