From beabd87ad98e6d1a1c708b91c39446d741c581e1 Mon Sep 17 00:00:00 2001
From: Jaime Soriano Pastor <jaime.soriano@elastic.co>
Date: Thu, 5 Oct 2023 18:48:30 +0200
Subject: [PATCH] Validate arrays of objects (#1489)

Add validation for arrays of objects, considering them as valid when
they are inside nested or group.
When group is used, a filterable error is given, mentioning that nested
should be used instead on this case.
---
 internal/fields/validate.go      | 20 ++++++--
 internal/fields/validate_test.go | 85 ++++++++++++++++++++++++++++----
 2 files changed, 93 insertions(+), 12 deletions(-)

diff --git a/internal/fields/validate.go b/internal/fields/validate.go
index e2c05a545..b1da005ce 100644
--- a/internal/fields/validate.go
+++ b/internal/fields/validate.go
@@ -32,8 +32,11 @@ import (
 var (
 	semver2_0_0 = semver.MustParse("2.0.0")
 	semver2_3_0 = semver.MustParse("2.3.0")
+	semver3_0_0 = semver.MustParse("3.0.0")
 
 	defaultExternal = "ecs"
+
+	errArrayOfObjects = errors.New("array of objects not used as nested type can lead to unexpected results")
 )
 
 // Validator is responsible for fields validation.
@@ -783,11 +786,22 @@ func (v *Validator) parseSingleElementValue(key string, definition FieldDefiniti
 			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)
 		}
 	// Groups should only contain nested fields, not single values.
-	case "group":
-		switch val.(type) {
+	case "group", "nested":
+		switch val := val.(type) {
 		case map[string]interface{}:
-			// TODO: This is probably an element from an array of objects,
+			// This is probably an element from an array of objects,
 			// even if not recommended, it should be validated.
+			if v.specVersion.LessThan(semver3_0_0) {
+				break
+			}
+			errs := v.validateMapElement(key, common.MapStr(val), doc)
+			if definition.Type == "group" {
+				errs = append(errs, errArrayOfObjects)
+			}
+			if len(errs) == 0 {
+				return nil
+			}
+			return errs
 		default:
 			return fmt.Errorf("field %q is a group of fields, it cannot store values", key)
 		}
diff --git a/internal/fields/validate_test.go b/internal/fields/validate_test.go
index 65ff80e28..3ba7a04e9 100644
--- a/internal/fields/validate_test.go
+++ b/internal/fields/validate_test.go
@@ -11,10 +11,12 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/Masterminds/semver/v3"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/elastic/elastic-package/internal/common"
+	"github.com/elastic/elastic-package/internal/multierror"
 )
 
 type results struct {
@@ -309,10 +311,12 @@ func TestValidate_ExpectedDatasets(t *testing.T) {
 
 func Test_parseElementValue(t *testing.T) {
 	for _, test := range []struct {
-		key        string
-		value      interface{}
-		definition FieldDefinition
-		fail       bool
+		key         string
+		value       interface{}
+		definition  FieldDefinition
+		fail        bool
+		assertError func(t *testing.T, err error)
+		specVersion semver.Version
 	}{
 		// Arrays
 		{
@@ -619,17 +623,80 @@ func Test_parseElementValue(t *testing.T) {
 				},
 			},
 		},
+		// elements in arrays of objects should be validated
+		{
+			key: "details",
+			value: []interface{}{
+				map[string]interface{}{
+					"id":       "somehost-id",
+					"hostname": "somehost",
+				},
+			},
+			definition: FieldDefinition{
+				Name: "details",
+				Type: "group",
+				Fields: []FieldDefinition{
+					{
+						Name: "id",
+						Type: "keyword",
+					},
+				},
+			},
+			specVersion: *semver3_0_0,
+			fail:        true,
+			assertError: func(t *testing.T, err error) {
+				errs := err.(multierror.Error)
+				if assert.Len(t, errs, 2) {
+					assert.Contains(t, errs[0].Error(), `"details.hostname" is undefined`)
+					assert.ErrorIs(t, errs[1], errArrayOfObjects)
+				}
+			},
+		},
+		// elements in nested objects
+		{
+			key: "nested",
+			value: []interface{}{
+				map[string]interface{}{
+					"id":       "somehost-id",
+					"hostname": "somehost",
+				},
+			},
+			definition: FieldDefinition{
+				Name: "nested",
+				Type: "nested",
+				Fields: []FieldDefinition{
+					{
+						Name: "id",
+						Type: "keyword",
+					},
+				},
+			},
+			specVersion: *semver3_0_0,
+			fail:        true,
+			assertError: func(t *testing.T, err error) {
+				errs := err.(multierror.Error)
+				if assert.Len(t, errs, 1) {
+					assert.Contains(t, errs[0].Error(), `"nested.hostname" is undefined`)
+				}
+			},
+		},
 	} {
-		v := Validator{
-			disabledDependencyManagement: true,
-			enabledAllowedIPCheck:        true,
-			allowedCIDRs:                 initializeAllowedCIDRsList(),
-		}
 
 		t.Run(test.key, func(t *testing.T) {
+			v := Validator{
+				Schema:                       []FieldDefinition{test.definition},
+				disabledDependencyManagement: true,
+				enabledAllowedIPCheck:        true,
+				allowedCIDRs:                 initializeAllowedCIDRsList(),
+				specVersion:                  test.specVersion,
+			}
+
 			err := v.parseElementValue(test.key, test.definition, test.value, common.MapStr{})
 			if test.fail {
 				require.Error(t, err)
+				if test.assertError != nil {
+					test.assertError(t, err)
+				}
 			} else {
 				require.NoError(t, err)
 			}