Skip to content

[system tests] Revisit multierrors when validating documents #2455

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 6, 2025
Merged

[system tests] Revisit multierrors when validating documents #2455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8aee23c
Revisit usage of multierrors
mrodm Mar 5, 2025
91188bb
Fix test to take into account multierrors
mrodm Mar 5, 2025
a05b221
Refactor
mrodm Mar 5, 2025
5825e42
Update expected errors in cisco_asa (test package)
mrodm Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 45 additions & 35 deletions internal/fields/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,9 +550,7 @@ func (v *Validator) ValidateDocumentBody(body json.RawMessage) multierror.Error
var c common.MapStr
err := json.Unmarshal(body, &c)
if err != nil {
var errs multierror.Error
errs = append(errs, fmt.Errorf("unmarshalling document body failed: %w", err))
return errs
return multierror.Error{fmt.Errorf("unmarshalling document body failed: %w", err)}
}

return v.ValidateDocumentMap(c)
Expand All @@ -565,7 +563,7 @@ func (v *Validator) ValidateDocumentMap(body common.MapStr) multierror.Error {
if len(errs) == 0 {
return nil
}
return errs
return errs.Unique()
}

var datasetFieldNames = []string{
Expand Down Expand Up @@ -672,14 +670,17 @@ func (v *Validator) validateMapElement(root string, elem common.MapStr, doc comm

err := v.validateScalarElement(key, val, doc)
if err != nil {
errs = append(errs, err)
errs = append(errs, err...)
Copy link
Member

@jsoriano jsoriano Mar 6, 2025

Choose a reason for hiding this comment

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

👍

}
}
}
return errs
if len(errs) > 0 {
return errs.Unique()
}
return nil
}

func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr) error {
func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr) multierror.Error {
if key == "" {
return nil // root key is always valid
}
Expand All @@ -692,26 +693,26 @@ func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr
case isFlattenedSubfield(key, v.Schema):
return nil // flattened subfield, it will be stored as member of the flattened ancestor.
case isArrayOfObjects(val):
return fmt.Errorf(`field %q is used as array of objects, expected explicit definition with type group or nested`, key)
return multierror.Error{fmt.Errorf(`field %q is used as array of objects, expected explicit definition with type group or nested`, key)}
case couldBeMultifield(key, v.Schema):
return fmt.Errorf(`field %q is undefined, could be a multifield`, key)
return multierror.Error{fmt.Errorf(`field %q is undefined, could be a multifield`, key)}
case !isParentEnabled(key, v.Schema):
return nil // parent mapping is disabled
default:
return fmt.Errorf(`field %q is undefined`, key)
return multierror.Error{fmt.Errorf(`field %q is undefined`, key)}
}
}

if !v.disabledNormalization {
err := v.validateExpectedNormalization(*definition, val)
if err != nil {
return fmt.Errorf("field %q is not normalized as expected: %w", key, err)
return multierror.Error{fmt.Errorf("field %q is not normalized as expected: %w", key, err)}
}
}

err := v.parseElementValue(key, *definition, val, doc)
if err != nil {
return fmt.Errorf("parsing field value failed: %w", err)
errs := v.parseElementValue(key, *definition, val, doc)
if len(errs) > 0 {
return errs.Unique()
Comment on lines +713 to +715
Copy link
Contributor Author

@mrodm mrodm Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One of the main changes in this PR, here it is removed the parsing field value failed from the error (and just return all the errors that could be found).

To avoid errors in the output like:

[0] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined

Copy link
Contributor Author

@mrodm mrodm Mar 6, 2025

Choose a reason for hiding this comment

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

Could those errors be reported without adding parsing field value failed?

Another example found:

[0] parsing field value failed: [0] parsing field value failed: field "cloudflare_logpush.email_security_alerts.attachments.Decrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)
[1] parsing field value failed: field "cloudflare_logpush.email_security_alerts.attachments.Encrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)

With the changes introduced in this PR, those errors would be written as:

[0] field "cloudflare_logpush.email_security_alerts.attachments.Decrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)
[1] field "cloudflare_logpush.email_security_alerts.attachments.Encrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)

}
return nil
}
Expand Down Expand Up @@ -1080,17 +1081,22 @@ func validSubField(def FieldDefinition, extraPart string) bool {

// parseElementValue checks that the value stored in a field matches the field definition. For
// arrays it checks it for each Element.
func (v *Validator) parseElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) error {
func (v *Validator) parseElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) multierror.Error {
// Validate types first for each element, so other checks don't need to worry about types.
var errs multierror.Error
err := forEachElementValue(key, definition, val, doc, v.parseSingleElementValue)
if err != nil {
return err
errs = append(errs, err...)
}

// Perform validations that need to be done on several fields at the same time.
err = v.parseAllElementValues(key, definition, val, doc)
if err != nil {
return err
allElementsErr := v.parseAllElementValues(key, definition, val, doc)
if allElementsErr != nil {
errs = append(errs, allElementsErr)
}

if len(errs) > 0 {
return errs.Unique()
}

return nil
Expand All @@ -1111,9 +1117,9 @@ func (v *Validator) parseAllElementValues(key string, definition FieldDefinition
}

// parseSingeElementValue performs validations on individual values of each element.
func (v *Validator) parseSingleElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) error {
invalidTypeError := func() error {
return fmt.Errorf("field %q's Go type, %T, does not match the expected field type: %s (field value: %v)", key, val, definition.Type, val)
func (v *Validator) parseSingleElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) multierror.Error {
invalidTypeError := func() multierror.Error {
return multierror.Error{fmt.Errorf("field %q's Go type, %T, does not match the expected field type: %s (field value: %v)", key, val, definition.Type, val)}
}

stringValue := func() (string, bool) {
Expand All @@ -1139,13 +1145,13 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
}

if err := ensureConstantKeywordValueMatches(key, valStr, definition.Value); err != nil {
return err
return multierror.Error{err}
}
if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
return err
return multierror.Error{err}
}
if err := ensureAllowedValues(key, valStr, definition); err != nil {
return err
return multierror.Error{err}
}
// Normal text fields should be of type string.
// If a pattern is provided, it checks if the value matches.
Expand All @@ -1156,10 +1162,10 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
}

if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
return err
return multierror.Error{err}
}
if err := ensureAllowedValues(key, valStr, definition); err != nil {
return err
return multierror.Error{err}
}
// Dates are expected to be formatted as strings or as seconds or milliseconds
// since epoch.
Expand All @@ -1168,12 +1174,12 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
switch val := val.(type) {
case string:
if err := ensurePatternMatches(key, val, definition.Pattern); err != nil {
return err
return multierror.Error{err}
}
case float64:
// date as seconds or milliseconds since epoch
if definition.Pattern != "" {
return fmt.Errorf("numeric date in field %q, but pattern defined", key)
return multierror.Error{fmt.Errorf("numeric date in field %q, but pattern defined", key)}
}
default:
return invalidTypeError()
Expand All @@ -1188,11 +1194,11 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
}

if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
return err
return multierror.Error{err}
}

if v.enabledAllowedIPCheck && !v.isAllowedIPValue(valStr) {
return fmt.Errorf("the IP %q is not one of the allowed test IPs (see: https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt)", valStr)
return multierror.Error{fmt.Errorf("the IP %q is not one of the allowed test IPs (see: https://github.com/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt)", valStr)}
}
// Groups should only contain nested fields, not single values.
case "group", "nested", "object":
Expand All @@ -1207,7 +1213,7 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
if len(errs) == 0 {
return nil
}
return errs
return errs.Unique()
Comment on lines 1213 to +1216
Copy link
Contributor Author

@mrodm mrodm Mar 5, 2025

Choose a reason for hiding this comment

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

This error variable comes from validateMapElement that returns a multierror.Error.

That's why here I updated all the functions calling this to return also multierror.Error (parseSingleElementValue and others)

case []any:
// This can be an array of array of objects. Elasticsearh will probably
// flatten this. So even if this is quite unexpected, let's try to handle it.
Expand All @@ -1231,7 +1237,7 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
return nil
}

return fmt.Errorf("field %q is a group of fields of type %s, it cannot store values", key, definition.Type)
return multierror.Error{fmt.Errorf("field %q is a group of fields of type %s, it cannot store values", key, definition.Type)}
}
// Numbers should have been parsed as float64, otherwise they are not numbers.
case "float", "long", "double":
Expand Down Expand Up @@ -1309,17 +1315,21 @@ func (v *Validator) isAllowedIPValue(s string) bool {

// forEachElementValue visits a function for each element in the given value if
// it is an array. If it is not an array, it calls the function with it.
func forEachElementValue(key string, definition FieldDefinition, val any, doc common.MapStr, fn func(string, FieldDefinition, any, common.MapStr) error) error {
func forEachElementValue(key string, definition FieldDefinition, val any, doc common.MapStr, fn func(string, FieldDefinition, any, common.MapStr) multierror.Error) multierror.Error {
arr, isArray := val.([]any)
if !isArray {
return fn(key, definition, val, doc)
}
var errs multierror.Error
for _, element := range arr {
err := fn(key, definition, element, doc)
if err != nil {
return err
errs = append(errs, err...)
}
}
if len(errs) > 0 {
return errs.Unique()
}
return nil
}

Expand Down
8 changes: 4 additions & 4 deletions internal/fields/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,14 +827,14 @@ func Test_parseElementValue(t *testing.T) {
specVersion: test.specVersion,
}

err := v.parseElementValue(test.key, test.definition, test.value, common.MapStr{})
errs := v.parseElementValue(test.key, test.definition, test.value, common.MapStr{})
if test.fail {
require.Error(t, err)
assert.Greater(t, len(errs), 0)
if test.assertError != nil {
test.assertError(t, err)
test.assertError(t, errs)
}
} else {
require.NoError(t, err)
assert.Empty(t, errs)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion internal/testrunner/runners/static/tester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (r tester) verifySampleEvent(pkgManifest *packages.PackageManifest) []testr
if len(multiErr) > 0 {
results, _ := resultComposer.WithError(testrunner.ErrTestCaseFailed{
Reason: "one or more errors found in document",
Details: multiErr.Error(),
Details: multiErr.Unique().Error(),
})
return results
}
Expand Down
6 changes: 3 additions & 3 deletions test/packages/false_positives/cisco_asa.expected_errors
View file
Open in desktop
Copy link
Contributor Author

@mrodm mrodm Mar 5, 2025

Choose a reason for hiding this comment

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

Remove parsing field value failed: from the error messages

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<failure>test case failed: one or more problems with fields found in documents: \[0\] parsing field value failed: field &#34;event.type&#34; value &#34;change&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)&#xA;\[1\] parsing field value failed: field &#34;event.type&#34; value &#34;deletion&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)&#xA;\[2\] parsing field value failed: field &#34;event.type&#34; value &#34;error&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>test case failed: one or more problems with fields found in documents: \[0\] parsing field value failed: field &#34;event.type&#34; value &#34;error&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>test case failed: one or more problems with fields found in documents: \[0\] parsing field value failed: field &#34;event.type&#34; value &#34;deletion&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>test case failed: one or more problems with fields found in documents: \[0\] field &#34;event.type&#34; value &#34;change&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)&#xA;\[1\] field &#34;event.type&#34; value &#34;deletion&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)&#xA;\[2\] field &#34;event.type&#34; value &#34;error&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>test case failed: one or more problems with fields found in documents: \[0\] field &#34;event.type&#34; value &#34;error&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>test case failed: one or more problems with fields found in documents: \[0\] field &#34;event.type&#34; value &#34;deletion&#34; is not one of the expected values \(access, allowed, connection, denied, end, info, protocol, start\) for any of the values of &#34;event.category&#34; \(network\)</failure>
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>
Expand Down