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

Add flattened field validation logic #177

Merged
merged 3 commits into from
Nov 20, 2020

Conversation

andrewkroh
Copy link
Member

For flattened field types don't validate the leaf fields.

Fixes #176

For flattened field types don't validate the leaf fields.

Fixes elastic#176
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 19, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #177 updated]

  • Start Time: 2020-11-20T15:28:01.160+0000

  • Duration: 13 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 9
Skipped 0
Total 9

@@ -103,6 +103,9 @@ func (v *Validator) validateMapElement(root string, elem common.MapStr) multierr
}
}
case map[string]interface{}:
if definition := findElementDefinition("", key, v.schema); definition != nil && definition.Type == "flattened" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you reused the same invocation as below (findElementDefinition("", key, v.schema)). I think it's a good moment to refactor it a bit:

findElementDefinition(key, v.schema) which calls findElementDefinitionForRoot("", key, v.schema)

and apply then same in validateScalarElement()

I would also wrap the entire line with IsFieldFlattened or similar.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I asked for a small refactoring.

I really appreciate adding the test case. Could you please link the Github issue in the code comment? In general you can add a comment justifying why do we need to treat specially "flattened" fields.

EDIT:

I see that the PR build was aborted. We need a green status to merge the PR once all relevant changes are introduced.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I left one nit-pick. If the PR build goes green, feel free to merge it. You have to bump up the dependency in the Integrations repository (can be done together with your PR).

internal/fields/validate.go Outdated Show resolved Hide resolved
@andrewkroh
Copy link
Member Author

@mtojek I think you'll need to trigger testing in order to get a green build.

githubPrCheckApproved: The PR is not allowed to run in the CI yet. (Only users with write permission

@mtojek
Copy link
Contributor

mtojek commented Nov 20, 2020

jenkins run the tests please

@andrewkroh andrewkroh merged commit 58dc88f into elastic:master Nov 20, 2020
andrewkroh added a commit to andrewkroh/integrations that referenced this pull request Dec 1, 2020
The tests revealed a few issues. There was an error in the pipeline for update-user-json.log because
serviceEventDetails was not present. This was the error

            "error": {
                "message": "Cannot invoke \\\"Object.getClass()\\\" because \\\"receiver\\\" is null"
            }

The aws.cloudtrail.read_only field was mapped as keyword but was actual a JSON boolean.
I changed the type to boolean, but do not plan to backport this change to Filebeat.

And lastly some ECS user_agent fields were missing.

This depends on elastic/elastic-package#177 to make the flattened fields pass
test validation.
andrewkroh added a commit to elastic/integrations that referenced this pull request Dec 2, 2020
The tests revealed a few issues. There was an error in the pipeline for update-user-json.log because
serviceEventDetails was not present. This was the error

            "error": {
                "message": "Cannot invoke \\\"Object.getClass()\\\" because \\\"receiver\\\" is null"
            }

The aws.cloudtrail.read_only field was mapped as keyword but was actual a JSON boolean.
I changed the type to boolean, but do not plan to backport this change to Filebeat.

And lastly some ECS user_agent fields were missing.

This depends on elastic/elastic-package#177 to make the flattened fields pass
test validation.
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
The tests revealed a few issues. There was an error in the pipeline for update-user-json.log because
serviceEventDetails was not present. This was the error

            "error": {
                "message": "Cannot invoke \\\"Object.getClass()\\\" because \\\"receiver\\\" is null"
            }

The aws.cloudtrail.read_only field was mapped as keyword but was actual a JSON boolean.
I changed the type to boolean, but do not plan to backport this change to Filebeat.

And lastly some ECS user_agent fields were missing.

This depends on elastic/elastic-package#177 to make the flattened fields pass
test validation.
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.

Handle flattened data type in fields validator
3 participants