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

Validate that event.type is aligned with event.category #961

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Sep 7, 2022

Fields like event.category in ECS define a list of allowed values for event.type, check that these values are aligned.

This check is only enabled for packages using at least format version 2.0.0.

Fixes #837.
Part of elastic/package-spec#399.

@jsoriano jsoriano self-assigned this Sep 7, 2022
@jsoriano jsoriano requested a review from a team September 7, 2022 18:09
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 7, 2022

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-08T10:12:04.566+0000

  • Duration: 25 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 808
Skipped 0
Total 808

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

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 admit that it wasn't easy to go through the logic :) Maybe it's the first symptom to refactor the fields code.

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

jsoriano commented Sep 8, 2022

I admit that it wasn't easy to go through the logic :) Maybe it's the first symptom to refactor the fields code.

Yeah, this is the first time we need to validate the values of two fields at the same time, this is why I ended up passing the whole document through all the calls chain so when validating one field we can check the value of the other.

Two possible alternatives would be:

  • Internally instantiate a document validator for each document, this validator could keep state for each document validation, or the document itself.
  • Given that by now we only need this for the event.type and event.category fields, validate them explicitly in ValidateDocumentMap, but this would give special consideration to these fields, what can be tricky in the future if ECS decides to use expected_event_types in other fields apart of event.category.

@mtojek mtojek self-requested a review September 8, 2022 09:11
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 8, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (33/33) 💚
Files 67.213% (82/122) 👍
Classes 62.941% (107/170) 👍
Methods 49.928% (349/699) 👍 0.36
Lines 33.355% (3151/9447) 👍 0.223
Conditionals 100.0% (0/0) 💚

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.

Frankly speaking, both propositions are rather hackish. I think that I'd rather keep your current proposal (PR).

@jsoriano
Copy link
Member Author

jsoriano commented Sep 8, 2022

Frankly speaking, both propositions are rather hackish. I think that I'd rather keep your current proposal (PR).

Yep, I also prefer the current approach even if not so nice to have to pass the document around.

@jsoriano
Copy link
Member Author

jsoriano commented Sep 8, 2022

Integration tests found an issue, event.type is defined as array in ECS. Fixed code and tests to cover this case. Apache package fixed, it was missing event.type in one data stream.

After this change I will start with #963.

@jsoriano jsoriano requested a review from mtojek September 8, 2022 10:22
@jsoriano jsoriano merged commit 046a146 into elastic:main Sep 8, 2022
@jsoriano jsoriano deleted the expected-event-types branch September 8, 2022 11:14
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.

[field validation] Check event.type values are aligned with event.category
3 participants