Skip to content

Commit 1cb8aef

Browse files
authored
[system tests] Revisit multierrors when validating documents (#2455)
Reviewed how errors (multierrors.Error) are reported to avoid reporting duplicated errors in some scenarios when running system tests.
1 parent 9d94244 commit 1cb8aef

File tree

4 files changed

+53
-43
lines changed

4 files changed

+53
-43
lines changed

internal/fields/validate.go

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -550,9 +550,7 @@ func (v *Validator) ValidateDocumentBody(body json.RawMessage) multierror.Error
550550
var c common.MapStr
551551
err := json.Unmarshal(body, &c)
552552
if err != nil {
553-
var errs multierror.Error
554-
errs = append(errs, fmt.Errorf("unmarshalling document body failed: %w", err))
555-
return errs
553+
return multierror.Error{fmt.Errorf("unmarshalling document body failed: %w", err)}
556554
}
557555

558556
return v.ValidateDocumentMap(c)
@@ -565,7 +563,7 @@ func (v *Validator) ValidateDocumentMap(body common.MapStr) multierror.Error {
565563
if len(errs) == 0 {
566564
return nil
567565
}
568-
return errs
566+
return errs.Unique()
569567
}
570568

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

673671
err := v.validateScalarElement(key, val, doc)
674672
if err != nil {
675-
errs = append(errs, err)
673+
errs = append(errs, err...)
676674
}
677675
}
678676
}
679-
return errs
677+
if len(errs) > 0 {
678+
return errs.Unique()
679+
}
680+
return nil
680681
}
681682

682-
func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr) error {
683+
func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr) multierror.Error {
683684
if key == "" {
684685
return nil // root key is always valid
685686
}
@@ -692,26 +693,26 @@ func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr
692693
case isFlattenedSubfield(key, v.Schema):
693694
return nil // flattened subfield, it will be stored as member of the flattened ancestor.
694695
case isArrayOfObjects(val):
695-
return fmt.Errorf(`field %q is used as array of objects, expected explicit definition with type group or nested`, key)
696+
return multierror.Error{fmt.Errorf(`field %q is used as array of objects, expected explicit definition with type group or nested`, key)}
696697
case couldBeMultifield(key, v.Schema):
697-
return fmt.Errorf(`field %q is undefined, could be a multifield`, key)
698+
return multierror.Error{fmt.Errorf(`field %q is undefined, could be a multifield`, key)}
698699
case !isParentEnabled(key, v.Schema):
699700
return nil // parent mapping is disabled
700701
default:
701-
return fmt.Errorf(`field %q is undefined`, key)
702+
return multierror.Error{fmt.Errorf(`field %q is undefined`, key)}
702703
}
703704
}
704705

705706
if !v.disabledNormalization {
706707
err := v.validateExpectedNormalization(*definition, val)
707708
if err != nil {
708-
return fmt.Errorf("field %q is not normalized as expected: %w", key, err)
709+
return multierror.Error{fmt.Errorf("field %q is not normalized as expected: %w", key, err)}
709710
}
710711
}
711712

712-
err := v.parseElementValue(key, *definition, val, doc)
713-
if err != nil {
714-
return fmt.Errorf("parsing field value failed: %w", err)
713+
errs := v.parseElementValue(key, *definition, val, doc)
714+
if len(errs) > 0 {
715+
return errs.Unique()
715716
}
716717
return nil
717718
}
@@ -1080,17 +1081,22 @@ func validSubField(def FieldDefinition, extraPart string) bool {
10801081

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

10901092
// Perform validations that need to be done on several fields at the same time.
1091-
err = v.parseAllElementValues(key, definition, val, doc)
1092-
if err != nil {
1093-
return err
1093+
allElementsErr := v.parseAllElementValues(key, definition, val, doc)
1094+
if allElementsErr != nil {
1095+
errs = append(errs, allElementsErr)
1096+
}
1097+
1098+
if len(errs) > 0 {
1099+
return errs.Unique()
10941100
}
10951101

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

11131119
// parseSingeElementValue performs validations on individual values of each element.
1114-
func (v *Validator) parseSingleElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) error {
1115-
invalidTypeError := func() error {
1116-
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)
1120+
func (v *Validator) parseSingleElementValue(key string, definition FieldDefinition, val any, doc common.MapStr) multierror.Error {
1121+
invalidTypeError := func() multierror.Error {
1122+
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)}
11171123
}
11181124

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

11411147
if err := ensureConstantKeywordValueMatches(key, valStr, definition.Value); err != nil {
1142-
return err
1148+
return multierror.Error{err}
11431149
}
11441150
if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
1145-
return err
1151+
return multierror.Error{err}
11461152
}
11471153
if err := ensureAllowedValues(key, valStr, definition); err != nil {
1148-
return err
1154+
return multierror.Error{err}
11491155
}
11501156
// Normal text fields should be of type string.
11511157
// If a pattern is provided, it checks if the value matches.
@@ -1156,10 +1162,10 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
11561162
}
11571163

11581164
if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
1159-
return err
1165+
return multierror.Error{err}
11601166
}
11611167
if err := ensureAllowedValues(key, valStr, definition); err != nil {
1162-
return err
1168+
return multierror.Error{err}
11631169
}
11641170
// Dates are expected to be formatted as strings or as seconds or milliseconds
11651171
// since epoch.
@@ -1168,12 +1174,12 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
11681174
switch val := val.(type) {
11691175
case string:
11701176
if err := ensurePatternMatches(key, val, definition.Pattern); err != nil {
1171-
return err
1177+
return multierror.Error{err}
11721178
}
11731179
case float64:
11741180
// date as seconds or milliseconds since epoch
11751181
if definition.Pattern != "" {
1176-
return fmt.Errorf("numeric date in field %q, but pattern defined", key)
1182+
return multierror.Error{fmt.Errorf("numeric date in field %q, but pattern defined", key)}
11771183
}
11781184
default:
11791185
return invalidTypeError()
@@ -1188,11 +1194,11 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
11881194
}
11891195

11901196
if err := ensurePatternMatches(key, valStr, definition.Pattern); err != nil {
1191-
return err
1197+
return multierror.Error{err}
11921198
}
11931199

11941200
if v.enabledAllowedIPCheck && !v.isAllowedIPValue(valStr) {
1195-
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)
1201+
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)}
11961202
}
11971203
// Groups should only contain nested fields, not single values.
11981204
case "group", "nested", "object":
@@ -1207,7 +1213,7 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
12071213
if len(errs) == 0 {
12081214
return nil
12091215
}
1210-
return errs
1216+
return errs.Unique()
12111217
case []any:
12121218
// This can be an array of array of objects. Elasticsearh will probably
12131219
// flatten this. So even if this is quite unexpected, let's try to handle it.
@@ -1231,7 +1237,7 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
12311237
return nil
12321238
}
12331239

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

13101316
// forEachElementValue visits a function for each element in the given value if
13111317
// it is an array. If it is not an array, it calls the function with it.
1312-
func forEachElementValue(key string, definition FieldDefinition, val any, doc common.MapStr, fn func(string, FieldDefinition, any, common.MapStr) error) error {
1318+
func forEachElementValue(key string, definition FieldDefinition, val any, doc common.MapStr, fn func(string, FieldDefinition, any, common.MapStr) multierror.Error) multierror.Error {
13131319
arr, isArray := val.([]any)
13141320
if !isArray {
13151321
return fn(key, definition, val, doc)
13161322
}
1323+
var errs multierror.Error
13171324
for _, element := range arr {
13181325
err := fn(key, definition, element, doc)
13191326
if err != nil {
1320-
return err
1327+
errs = append(errs, err...)
13211328
}
13221329
}
1330+
if len(errs) > 0 {
1331+
return errs.Unique()
1332+
}
13231333
return nil
13241334
}
13251335

internal/fields/validate_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -827,14 +827,14 @@ func Test_parseElementValue(t *testing.T) {
827827
specVersion: test.specVersion,
828828
}
829829

830-
err := v.parseElementValue(test.key, test.definition, test.value, common.MapStr{})
830+
errs := v.parseElementValue(test.key, test.definition, test.value, common.MapStr{})
831831
if test.fail {
832-
require.Error(t, err)
832+
assert.Greater(t, len(errs), 0)
833833
if test.assertError != nil {
834-
test.assertError(t, err)
834+
test.assertError(t, errs)
835835
}
836836
} else {
837-
require.NoError(t, err)
837+
assert.Empty(t, errs)
838838
}
839839
})
840840
}

internal/testrunner/runners/static/tester.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ func (r tester) verifySampleEvent(pkgManifest *packages.PackageManifest) []testr
182182
if len(multiErr) > 0 {
183183
results, _ := resultComposer.WithError(testrunner.ErrTestCaseFailed{
184184
Reason: "one or more errors found in document",
185-
Details: multiErr.Error(),
185+
Details: multiErr.Unique().Error(),
186186
})
187187
return results
188188
}

test/packages/false_positives/cisco_asa.expected_errors

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
<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>
2-
<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>
3-
<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>
1+
<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>
2+
<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>
3+
<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>
44
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>
55
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>
66
<failure>detected ingest pipeline warnings: 2: character class has &#39;-&#39; without escape&#xA;regular expression has redundant nested repeat operator.*</failure>

0 commit comments

Comments
 (0)