Skip to content

Commit 744a033

Browse files
jsorianojsoriano
authored
Fix validation of multi-fields of external fields (#1335)
* Look for field definitions also in multi-fields * Add test that reproduces the issue * Inject external fields when creating the validator * Remove unneeded reference to field dependency manager * Make importField private * Keep options provided by caller * Update test files * Remove irrelevant data streams * Remove unneeded error handling * Keep printing empty groups from ECS, for backwards compatibility reasons * Add godocs to KeepExternal * Regenerate README * Keep empty fields for fields validation, but not for documentation * Review unit tests * Fix inject fields recursion * Add test case
1 parent 949cfcf commit 744a033

30 files changed

+4106
-112
lines changed

‎internal/docs/exported_fields.go‎

Lines changed: 20 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,21 @@ type fieldsTableRecord struct {
2323
var escaper = strings.NewReplacer("*", "\\*", "{", "\\{", "}", "\\}", "<", "\\<", ">", "\\>")
2424

2525
func renderExportedFields(fieldsParentDir string) (string, error) {
26-
validator, err := fields.CreateValidatorForDirectory(fieldsParentDir)
26+
injectOptions := fields.InjectFieldsOptions{
27+
// Keep External parameter when rendering fields, so we can render
28+
// documentation for empty groups imported from ECS, for backwards compatibility.
29+
KeepExternal: true,
30+
31+
// SkipEmptyFields parameter when rendering fields. In other cases we want to
32+
// keep them to accept them for validation.
33+
SkipEmptyFields: true,
34+
}
35+
validator, err := fields.CreateValidatorForDirectory(fieldsParentDir, fields.WithInjectFieldsOptions(injectOptions))
2736
if err != nil {
2837
return "", fmt.Errorf("can't create fields validator instance (path: %s): %w", fieldsParentDir, err)
2938
}
3039

31-
collected, err := collectFieldsFromDefinitions(validator)
32-
if err != nil {
33-
return "", fmt.Errorf("collecting fields files failed: %w", err)
34-
}
40+
collected := collectFieldsFromDefinitions(validator)
3541

3642
var builder strings.Builder
3743
builder.WriteString("**Exported fields**\n\n")
@@ -100,42 +106,24 @@ func areMetricTypesPresent(collected []fieldsTableRecord) bool {
100106
return false
101107
}
102108

103-
func collectFieldsFromDefinitions(validator *fields.Validator) ([]fieldsTableRecord, error) {
109+
func collectFieldsFromDefinitions(validator *fields.Validator) []fieldsTableRecord {
104110
var records []fieldsTableRecord
105111

106112
root := validator.Schema
107-
var err error
108113
for _, f := range root {
109-
records, err = visitFields("", f, records, validator.FieldDependencyManager)
110-
if err != nil {
111-
return nil, fmt.Errorf("visiting fields failed: %w", err)
112-
}
114+
records = visitFields("", f, records)
113115
}
114-
return uniqueTableRecords(records), nil
116+
return uniqueTableRecords(records)
115117
}
116118

117-
func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTableRecord, fdm *fields.DependencyManager) ([]fieldsTableRecord, error) {
119+
func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTableRecord) []fieldsTableRecord {
118120
var name = namePrefix
119121
if namePrefix != "" {
120122
name += "."
121123
}
122124
name += f.Name
123125

124-
if len(f.Fields) == 0 && f.Type != "group" {
125-
if f.External != "" {
126-
imported, err := fdm.ImportField(f.External, name)
127-
if err != nil {
128-
return nil, fmt.Errorf("can't import field: %w", err)
129-
}
130-
131-
// Override imported fields with the definition, except for the type and external.
132-
var updated fields.FieldDefinition
133-
updated.Update(imported)
134-
updated.Update(f)
135-
updated.Type = imported.Type
136-
updated.External = ""
137-
f = updated
138-
}
126+
if (len(f.Fields) == 0 && f.Type != "group") || f.External != "" {
139127
records = append(records, fieldsTableRecord{
140128
name: name,
141129
description: f.Description,
@@ -151,17 +139,14 @@ func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTa
151139
aType: multiField.Type,
152140
})
153141
}
154-
return records, nil
142+
143+
return records
155144
}
156145

157-
var err error
158146
for _, fieldEntry := range f.Fields {
159-
records, err = visitFields(name, fieldEntry, records, fdm)
160-
if err != nil {
161-
return nil, fmt.Errorf("recursive visiting fields failed (namePrefix: %s): %w", namePrefix, err)
162-
}
147+
records = visitFields(name, fieldEntry, records)
163148
}
164-
return records, nil
149+
return records
165150
}
166151

167152
func uniqueTableRecords(records []fieldsTableRecord) []fieldsTableRecord {

‎internal/fields/dependency_manager.go‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -138,20 +138,39 @@ func asGitReference(reference string) (string, error) {
138138
return reference[len(gitReferencePrefix):], nil
139139
}
140140

141+
// InjectFieldsOptions allow to configure fields injection.
142+
type InjectFieldsOptions struct {
143+
// KeepExternal can be set to true to avoid deleting the `external` parameter
144+
// of a field when resolving it. This helps keeping behaviours that depended
145+
// in previous versions on lazy resolution of external fields.
146+
KeepExternal bool
147+
148+
// SkipEmptyFields can be set to true to skip empty groups when injecting fields.
149+
SkipEmptyFields bool
150+
151+
root string
152+
}
153+
141154
// InjectFields function replaces external field references with target definitions.
142155
func (dm *DependencyManager) InjectFields(defs []common.MapStr) ([]common.MapStr, bool, error) {
143-
return dm.injectFieldsWithRoot("", defs)
156+
return dm.injectFieldsWithOptions(defs, InjectFieldsOptions{})
144157
}
145158

146-
func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.MapStr) ([]common.MapStr, bool, error) {
159+
// InjectFieldsWithOptions function replaces external field references with target definitions.
160+
// It can be configured with options.
161+
func (dm *DependencyManager) InjectFieldsWithOptions(defs []common.MapStr, options InjectFieldsOptions) ([]common.MapStr, bool, error) {
162+
return dm.injectFieldsWithOptions(defs, options)
163+
}
164+
165+
func (dm *DependencyManager) injectFieldsWithOptions(defs []common.MapStr, options InjectFieldsOptions) ([]common.MapStr, bool, error) {
147166
var updated []common.MapStr
148167
var changed bool
149168
for _, def := range defs {
150-
fieldPath := buildFieldPath(root, def)
169+
fieldPath := buildFieldPath(options.root, def)
151170

152171
external, _ := def.GetValue("external")
153172
if external != nil {
154-
imported, err := dm.ImportField(external.(string), fieldPath)
173+
imported, err := dm.importField(external.(string), fieldPath)
155174
if err != nil {
156175
return nil, false, fmt.Errorf("can't import field: %w", err)
157176
}
@@ -160,7 +179,10 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
160179

161180
// Allow overrides of everything, except the imported type, for consistency.
162181
transformed.DeepUpdate(def)
163-
transformed.Delete("external")
182+
183+
if !options.KeepExternal {
184+
transformed.Delete("external")
185+
}
164186

165187
// Allow to override the type only from keyword to constant_keyword,
166188
// to support the case of setting the value already in the mappings.
@@ -177,7 +199,9 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
177199
if err != nil {
178200
return nil, false, fmt.Errorf("can't convert fields: %w", err)
179201
}
180-
updatedFields, fieldsChanged, err := dm.injectFieldsWithRoot(fieldPath, fieldsMs)
202+
childrenOptions := options
203+
childrenOptions.root = fieldPath
204+
updatedFields, fieldsChanged, err := dm.injectFieldsWithOptions(fieldsMs, childrenOptions)
181205
if err != nil {
182206
return nil, false, err
183207
}
@@ -190,7 +214,8 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
190214
}
191215
}
192216

193-
if skipField(def) {
217+
if options.SkipEmptyFields && skipField(def) {
218+
changed = true
194219
continue
195220
}
196221
updated = append(updated, def)
@@ -202,6 +227,12 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
202227
func skipField(def common.MapStr) bool {
203228
t, _ := def.GetValue("type")
204229
if t == "group" {
230+
// Keep empty external groups for backwards compatibility in docs generation.
231+
external, _ := def.GetValue("external")
232+
if external != nil {
233+
return false
234+
}
235+
205236
fields, _ := def.GetValue("fields")
206237
switch fields := fields.(type) {
207238
case nil:
@@ -216,8 +247,8 @@ func skipField(def common.MapStr) bool {
216247
return false
217248
}
218249

219-
// ImportField method resolves dependency on a single external field using available schemas.
220-
func (dm *DependencyManager) ImportField(schemaName, fieldPath string) (FieldDefinition, error) {
250+
// importField method resolves dependency on a single external field using available schemas.
251+
func (dm *DependencyManager) importField(schemaName, fieldPath string) (FieldDefinition, error) {
221252
if dm == nil {
222253
return FieldDefinition{}, fmt.Errorf(`importing external field "%s": external fields not allowed because dependencies file "_dev/build/build.yml" is missing`, fieldPath)
223254
}

‎internal/fields/dependency_manager_test.go‎

Lines changed: 99 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
1717
title string
1818
defs []common.MapStr
1919
result []common.MapStr
20+
options InjectFieldsOptions
2021
changed bool
2122
valid bool
2223
}{
@@ -354,16 +355,113 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
354355
"external": "test",
355356
},
356357
},
358+
options: InjectFieldsOptions{
359+
// Options used for fields injection for docs.
360+
SkipEmptyFields: true,
361+
KeepExternal: true,
362+
},
357363
result: []common.MapStr{
364+
{
365+
"name": "host",
366+
"description": "A general computing instance",
367+
"external": "test",
368+
"type": "group",
369+
},
358370
{
359371
"name": "host.hostname",
360372
"description": "Hostname of the host",
373+
"external": "test",
361374
"type": "keyword",
362375
},
363376
},
364377
valid: true,
365378
changed: true,
366379
},
380+
{
381+
title: "skip empty group for docs",
382+
defs: []common.MapStr{
383+
{
384+
"name": "host",
385+
"type": "group",
386+
},
387+
},
388+
options: InjectFieldsOptions{
389+
// Options used for fields injection for docs.
390+
SkipEmptyFields: true,
391+
KeepExternal: true,
392+
},
393+
result: nil,
394+
valid: true,
395+
changed: true,
396+
},
397+
{
398+
title: "keep empty group for package validation",
399+
defs: []common.MapStr{
400+
{
401+
"name": "host",
402+
"type": "group",
403+
},
404+
},
405+
result: []common.MapStr{
406+
{
407+
"name": "host",
408+
"type": "group",
409+
},
410+
},
411+
valid: true,
412+
changed: false,
413+
},
414+
{
415+
title: "sequence of nested definitions to ensure recursion does not have side effects",
416+
defs: []common.MapStr{
417+
{
418+
"name": "container",
419+
"type": "group",
420+
"fields": []interface{}{
421+
common.MapStr{
422+
"name": "id",
423+
"external": "test",
424+
},
425+
},
426+
},
427+
{
428+
"name": "host",
429+
"type": "group",
430+
"fields": []interface{}{
431+
common.MapStr{
432+
"name": "id",
433+
"external": "test",
434+
},
435+
},
436+
},
437+
},
438+
result: []common.MapStr{
439+
{
440+
"name": "container",
441+
"type": "group",
442+
"fields": []common.MapStr{
443+
{
444+
"name": "id",
445+
"description": "Container identifier.",
446+
"type": "keyword",
447+
},
448+
},
449+
},
450+
{
451+
"name": "host",
452+
"type": "group",
453+
"fields": []common.MapStr{
454+
{
455+
"name": "id",
456+
"description": "Unique host id",
457+
"type": "keyword",
458+
},
459+
},
460+
},
461+
},
462+
valid: true,
463+
changed: true,
464+
},
367465
}
368466

369467
indexFalse := false
@@ -442,7 +540,7 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
442540

443541
for _, c := range cases {
444542
t.Run(c.title, func(t *testing.T) {
445-
result, changed, err := dm.InjectFields(c.defs)
543+
result, changed, err := dm.InjectFieldsWithOptions(c.defs, c.options)
446544
if !c.valid {
447545
assert.Error(t, err)
448546
return

‎internal/fields/testdata/fields/fields.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,8 @@
7373
- name: protocol
7474
- name: start
7575
- name: user
76+
- name: process.name
77+
type: wildcard
78+
multi_fields:
79+
- name: text
80+
type: text

‎internal/fields/testdata/mongodb-multi-fields.json‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
{
2+
"@timestamp": "2023-06-27T15:08:06.769Z",
3+
"data_stream": {
4+
"dataset": "mongodb.status",
5+
"namespace": "ep",
6+
"type": "metrics"
7+
},
8+
"process": {
9+
"name": {
10+
"text": "mongodb"
11+
}
12+
}
13+
}

0 commit comments

Comments
 (0)