Fix validation of multi-fields of external fields

internal/docs/exported_fields.go

@@ -23,15 +23,21 @@ type fieldsTableRecord struct {
 var escaper = strings.NewReplacer("*", "\\*", "{", "\\{", "}", "\\}", "<", "\\<", ">", "\\>")
 
 func renderExportedFields(fieldsParentDir string) (string, error) {
-	validator, err := fields.CreateValidatorForDirectory(fieldsParentDir)
+	injectOptions := fields.InjectFieldsOptions{
+		// Keep External parameter when rendering fields, so we can render
+		// documentation for empty groups imported from ECS, for backwards compatibility.
+		KeepExternal: true,
+
+		// SkipEmptyFields parameter when rendering fields. In other cases we want to
+		// keep them to accept them for validation.
+		SkipEmptyFields: true,
+	}
+	validator, err := fields.CreateValidatorForDirectory(fieldsParentDir, fields.WithInjectFieldsOptions(injectOptions))
 	if err != nil {
 		return "", fmt.Errorf("can't create fields validator instance (path: %s): %w", fieldsParentDir, err)
 	}
 
-	collected, err := collectFieldsFromDefinitions(validator)
-	if err != nil {
-		return "", fmt.Errorf("collecting fields files failed: %w", err)
-	}
+	collected := collectFieldsFromDefinitions(validator)
 
 	var builder strings.Builder
 	builder.WriteString("**Exported fields**\n\n")
@@ -100,42 +106,24 @@ func areMetricTypesPresent(collected []fieldsTableRecord) bool {
 	return false
 }
 
-func collectFieldsFromDefinitions(validator *fields.Validator) ([]fieldsTableRecord, error) {
+func collectFieldsFromDefinitions(validator *fields.Validator) []fieldsTableRecord {
 	var records []fieldsTableRecord
 
 	root := validator.Schema
-	var err error
 	for _, f := range root {
-		records, err = visitFields("", f, records, validator.FieldDependencyManager)
-		if err != nil {
-			return nil, fmt.Errorf("visiting fields failed: %w", err)
-		}
+		records = visitFields("", f, records)
 	}
-	return uniqueTableRecords(records), nil
+	return uniqueTableRecords(records)
 }
 
-func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTableRecord, fdm *fields.DependencyManager) ([]fieldsTableRecord, error) {
+func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTableRecord) []fieldsTableRecord {
 	var name = namePrefix
 	if namePrefix != "" {
 		name += "."
 	}
 	name += f.Name
 
-	if len(f.Fields) == 0 && f.Type != "group" {
-		if f.External != "" {
-			imported, err := fdm.ImportField(f.External, name)
-			if err != nil {
-				return nil, fmt.Errorf("can't import field: %w", err)
-			}
-
-			// Override imported fields with the definition, except for the type and external.
-			var updated fields.FieldDefinition
-			updated.Update(imported)
-			updated.Update(f)
-			updated.Type = imported.Type
-			updated.External = ""
-			f = updated
-		}
+	if (len(f.Fields) == 0 && f.Type != "group") || f.External != "" {
 		records = append(records, fieldsTableRecord{
 			name:        name,
 			description: f.Description,
@@ -151,17 +139,14 @@ func visitFields(namePrefix string, f fields.FieldDefinition, records []fieldsTa
 				aType:       multiField.Type,
 			})
 		}
-		return records, nil
+
+		return records
 	}
 
-	var err error
 	for _, fieldEntry := range f.Fields {
-		records, err = visitFields(name, fieldEntry, records, fdm)
-		if err != nil {
-			return nil, fmt.Errorf("recursive visiting fields failed (namePrefix: %s): %w", namePrefix, err)
-		}
+		records = visitFields(name, fieldEntry, records)
 	}
-	return records, nil
+	return records
 }
 
 func uniqueTableRecords(records []fieldsTableRecord) []fieldsTableRecord {

internal/fields/dependency_manager.go

@@ -138,20 +138,39 @@ func asGitReference(reference string) (string, error) {
 	return reference[len(gitReferencePrefix):], nil
 }
 
+// InjectFieldsOptions allow to configure fields injection.
+type InjectFieldsOptions struct {
+	// KeepExternal can be set to true to avoid deleting the `external` parameter
+	// of a field when resolving it. This helps keeping behaviours that depended
+	// in previous versions on lazy resolution of external fields.
+	KeepExternal bool
+
+	// SkipEmptyFields can be set to true to skip empty groups when injecting fields.
+	SkipEmptyFields bool
+
+	root string
+}
+
 // InjectFields function replaces external field references with target definitions.
 func (dm *DependencyManager) InjectFields(defs []common.MapStr) ([]common.MapStr, bool, error) {
-	return dm.injectFieldsWithRoot("", defs)
+	return dm.injectFieldsWithOptions(defs, InjectFieldsOptions{})
 }
 
-func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.MapStr) ([]common.MapStr, bool, error) {
+// InjectFieldsWithOptions function replaces external field references with target definitions.
+// It can be configured with options.
+func (dm *DependencyManager) InjectFieldsWithOptions(defs []common.MapStr, options InjectFieldsOptions) ([]common.MapStr, bool, error) {
+	return dm.injectFieldsWithOptions(defs, options)
+}
+
+func (dm *DependencyManager) injectFieldsWithOptions(defs []common.MapStr, options InjectFieldsOptions) ([]common.MapStr, bool, error) {
 	var updated []common.MapStr
 	var changed bool
 	for _, def := range defs {
-		fieldPath := buildFieldPath(root, def)
+		fieldPath := buildFieldPath(options.root, def)
 
 		external, _ := def.GetValue("external")
 		if external != nil {
-			imported, err := dm.ImportField(external.(string), fieldPath)
+			imported, err := dm.importField(external.(string), fieldPath)
 			if err != nil {
 				return nil, false, fmt.Errorf("can't import field: %w", err)
 			}
@@ -160,7 +179,10 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
 
 			// Allow overrides of everything, except the imported type, for consistency.
 			transformed.DeepUpdate(def)
-			transformed.Delete("external")
+
+			if !options.KeepExternal {
+				transformed.Delete("external")
+			}
 
 			// Allow to override the type only from keyword to constant_keyword,
 			// 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
 				if err != nil {
 					return nil, false, fmt.Errorf("can't convert fields: %w", err)
 				}
-				updatedFields, fieldsChanged, err := dm.injectFieldsWithRoot(fieldPath, fieldsMs)
+				childrenOptions := options
+				childrenOptions.root = fieldPath
+				updatedFields, fieldsChanged, err := dm.injectFieldsWithOptions(fieldsMs, childrenOptions)
 				if err != nil {
 					return nil, false, err
 				}
@@ -190,7 +214,8 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
 			}
 		}
 
-		if skipField(def) {
+		if options.SkipEmptyFields && skipField(def) {
+			changed = true
 			continue
 		}
 		updated = append(updated, def)
@@ -202,6 +227,12 @@ func (dm *DependencyManager) injectFieldsWithRoot(root string, defs []common.Map
 func skipField(def common.MapStr) bool {
 	t, _ := def.GetValue("type")
 	if t == "group" {
+		// Keep empty external groups for backwards compatibility in docs generation.
+		external, _ := def.GetValue("external")
+		if external != nil {
+			return false
+		}
+
 		fields, _ := def.GetValue("fields")
 		switch fields := fields.(type) {
 		case nil:
@@ -216,8 +247,8 @@ func skipField(def common.MapStr) bool {
 	return false
 }
 
-// ImportField method resolves dependency on a single external field using available schemas.
-func (dm *DependencyManager) ImportField(schemaName, fieldPath string) (FieldDefinition, error) {
+// importField method resolves dependency on a single external field using available schemas.
+func (dm *DependencyManager) importField(schemaName, fieldPath string) (FieldDefinition, error) {
 	if dm == nil {
 		return FieldDefinition{}, fmt.Errorf(`importing external field "%s": external fields not allowed because dependencies file "_dev/build/build.yml" is missing`, fieldPath)
 	}

internal/fields/dependency_manager_test.go

@@ -17,6 +17,7 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
 		title   string
 		defs    []common.MapStr
 		result  []common.MapStr
+		options InjectFieldsOptions
 		changed bool
 		valid   bool
 	}{
@@ -354,16 +355,113 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
 					"external": "test",
 				},
 			},
+			options: InjectFieldsOptions{
+				// Options used for fields injection for docs.
+				SkipEmptyFields: true,
+				KeepExternal:    true,
+			},
 			result: []common.MapStr{
+				{
+					"name":        "host",
+					"description": "A general computing instance",
+					"external":    "test",
+					"type":        "group",
+				},
 				{
 					"name":        "host.hostname",
 					"description": "Hostname of the host",
+					"external":    "test",
 					"type":        "keyword",
 				},
 			},
 			valid:   true,
 			changed: true,
 		},
+		{
+			title: "skip empty group for docs",
+			defs: []common.MapStr{
+				{
+					"name": "host",
+					"type": "group",
+				},
+			},
+			options: InjectFieldsOptions{
+				// Options used for fields injection for docs.
+				SkipEmptyFields: true,
+				KeepExternal:    true,
+			},
+			result:  nil,
+			valid:   true,
+			changed: true,
+		},
+		{
+			title: "keep empty group for package validation",
+			defs: []common.MapStr{
+				{
+					"name": "host",
+					"type": "group",
+				},
+			},
+			result: []common.MapStr{
+				{
+					"name": "host",
+					"type": "group",
+				},
+			},
+			valid:   true,
+			changed: false,
+		},
+		{
+			title: "sequence of nested definitions to ensure recursion does not have side effects",
+			defs: []common.MapStr{
+				{
+					"name": "container",
+					"type": "group",
+					"fields": []interface{}{
+						common.MapStr{
+							"name":     "id",
+							"external": "test",
+						},
+					},
+				},
+				{
+					"name": "host",
+					"type": "group",
+					"fields": []interface{}{
+						common.MapStr{
+							"name":     "id",
+							"external": "test",
+						},
+					},
+				},
+			},
+			result: []common.MapStr{
+				{
+					"name": "container",
+					"type": "group",
+					"fields": []common.MapStr{
+						{
+							"name":        "id",
+							"description": "Container identifier.",
+							"type":        "keyword",
+						},
+					},
+				},
+				{
+					"name": "host",
+					"type": "group",
+					"fields": []common.MapStr{
+						{
+							"name":        "id",
+							"description": "Unique host id",
+							"type":        "keyword",
+						},
+					},
+				},
+			},
+			valid:   true,
+			changed: true,
+		},
 	}
 
 	indexFalse := false
@@ -442,7 +540,7 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.title, func(t *testing.T) {
-			result, changed, err := dm.InjectFields(c.defs)
+			result, changed, err := dm.InjectFieldsWithOptions(c.defs, c.options)
 			if !c.valid {
 				assert.Error(t, err)
 				return

internal/fields/testdata/fields/fields.yml

@@ -73,3 +73,8 @@
     - name: protocol
     - name: start
     - name: user
+- name: process.name
+  type: wildcard
+  multi_fields:
+    - name: text
+      type: text

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

@@ -0,0 +1,13 @@
+{
+    "@timestamp": "2023-06-27T15:08:06.769Z",
+    "data_stream": {
+        "dataset": "mongodb.status",
+        "namespace": "ep",
+        "type": "metrics"
+    },
+    "process": {
+       "name": {
+         "text": "mongodb"
+       }
+    }
+}