Skip to content
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

Fix validation of properties and parameters based on mappings #2288

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Fix validation of properties and parameters based on mappings #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bb9734
Ensure properties subfields are validated accordingly
mrodm Dec 18, 2024
707395e
Reorder parameters for consistency
mrodm Dec 18, 2024
882c90e
Rename parameter
mrodm Dec 18, 2024
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
27 changes: 16 additions & 11 deletions internal/fields/mappings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (v *MappingValidator) ValidateIndexMappings(ctx context.Context) multierror
return errs.Unique()
}

mappingErrs := v.compareMappings("", rawPreview, rawActual)
mappingErrs := v.compareMappings("", false, rawPreview, rawActual)
errs = append(errs, mappingErrs...)

if len(errs) > 0 {
Expand Down Expand Up @@ -481,7 +481,7 @@ func validateConstantKeywordField(path string, preview, actual map[string]any) (
return isConstantKeyword, nil
}

func (v *MappingValidator) compareMappings(path string, preview, actual map[string]any) multierror.Error {
func (v *MappingValidator) compareMappings(path string, couldBeParametersDefinition bool, preview, actual map[string]any) multierror.Error {
var errs multierror.Error

isConstantKeywordType, err := validateConstantKeywordField(path, preview, actual)
Expand All @@ -502,7 +502,9 @@ func (v *MappingValidator) compareMappings(path string, preview, actual map[stri
return nil
}

if isObject(actual) {
// Ensure to validate properties from an object (subfields) in the right location of the mappings
// there could be "sub-fields" with name "properties" too
if couldBeParametersDefinition && isObject(actual) {
if isObjectFullyDynamic(preview) {
// TODO: Skip for now, it should be required to compare with dynamic templates
logger.Debugf("Pending to validate with the dynamic templates defined the path: %q", path)
Expand All @@ -519,7 +521,7 @@ func (v *MappingValidator) compareMappings(path string, preview, actual map[stri
if err != nil {
errs = append(errs, fmt.Errorf("found invalid properties type in actual mappings for path %q: %w", path, err))
}
compareErrors := v.compareMappings(path, previewProperties, actualProperties)
compareErrors := v.compareMappings(path, false, previewProperties, actualProperties)
errs = append(errs, compareErrors...)

if len(errs) == 0 {
Expand All @@ -542,27 +544,28 @@ func (v *MappingValidator) compareMappings(path string, preview, actual map[stri
if err != nil {
errs = append(errs, fmt.Errorf("found invalid multi_fields type in actual mappings for path %q: %w", path, err))
}
compareErrors := v.compareMappings(path, previewFields, actualFields)
compareErrors := v.compareMappings(path, false, previewFields, actualFields)
errs = append(errs, compareErrors...)
// not returning here to keep validating the other fields of this object if any
}

// Compare and validate the elements under "properties": objects or fields and its parameters
propertiesErrs := v.validateObjectProperties(path, containsMultifield, actual, preview)
propertiesErrs := v.validateObjectProperties(path, true, containsMultifield, preview, actual)
errs = append(errs, propertiesErrs...)
if len(errs) == 0 {
return nil
}
return errs.Unique()
}

func (v *MappingValidator) validateObjectProperties(path string, containsMultifield bool, actual, preview map[string]any) multierror.Error {
func (v *MappingValidator) validateObjectProperties(path string, couldBeParametersDefinition, containsMultifield bool, preview, actual map[string]any) multierror.Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reordered preview and actual parameters to keep consistency with the other methods.

var errs multierror.Error
for key, value := range actual {
if containsMultifield && key == "fields" {
// already checked
continue
}

currentPath := currentMappingPath(path, key)
if skipValidationForField(currentPath) {
continue
Expand All @@ -578,12 +581,14 @@ func (v *MappingValidator) validateObjectProperties(path string, containsMultifi
}
ecsErrors := v.validateMappingsNotInPreview(currentPath, childField)
errs = append(errs, ecsErrors...)
continue
}

// Parameter not defined
errs = append(errs, fmt.Errorf("field %q is undefined", currentPath))
continue
}

fieldErrs := v.validateObjectMappingAndParameters(preview[key], value, currentPath)
fieldErrs := v.validateObjectMappingAndParameters(preview[key], value, currentPath, true)
errs = append(errs, fieldErrs...)
}
if len(errs) == 0 {
Expand Down Expand Up @@ -634,7 +639,7 @@ func (v *MappingValidator) validateMappingsNotInPreview(currentPath string, chil

// validateObjectMappingAndParameters validates the current object or field parameter (currentPath) comparing the values
// in the actual mapping with the values in the preview mapping.
func (v *MappingValidator) validateObjectMappingAndParameters(previewValue, actualValue any, currentPath string) multierror.Error {
func (v *MappingValidator) validateObjectMappingAndParameters(previewValue, actualValue any, currentPath string, couldBeParametersDefinition bool) multierror.Error {
var errs multierror.Error
switch actualValue.(type) {
case map[string]any:
Expand All @@ -647,7 +652,7 @@ func (v *MappingValidator) validateObjectMappingAndParameters(previewValue, actu
if !ok {
errs = append(errs, fmt.Errorf("unexpected type in actual mappings for path: %q", currentPath))
}
errs = append(errs, v.compareMappings(currentPath, previewField, actualField)...)
errs = append(errs, v.compareMappings(currentPath, couldBeParametersDefinition, previewField, actualField)...)
case any:
// Validate each setting/parameter of the mapping
// If a mapping exist in both preview and actual, they should be the same. But forcing to compare each parameter just in case
Expand Down
82 changes: 79 additions & 3 deletions internal/fields/mappings_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,16 @@ func TestComparingMappings(t *testing.T) {
},
},
},
"bar": map[string]any{
"properties": map[string]any{
"type": map[string]any{
"type": "keyword",
},
"properties": map[string]any{
"type": "keyword",
},
},
},
},
actual: map[string]any{
"foo": map[string]any{
Expand All @@ -552,9 +562,24 @@ func TestComparingMappings(t *testing.T) {
},
},
},
"bar": map[string]any{
"properties": map[string]any{
"type": map[string]any{
"type": "keyword",
"ignore_above": 1024,
},
"properties": map[string]any{
"type": "keyword",
"ignore_above": 1024,
},
},
},
},
schema: []FieldDefinition{},
expectedErrors: []string{
`field "bar.type.ignore_above" is undefined`,
`field "bar.properties.ignore_above" is undefined`,
},
schema: []FieldDefinition{},
expectedErrors: []string{},
},
{
title: "different parameter values within an object",
Expand Down Expand Up @@ -584,6 +609,32 @@ func TestComparingMappings(t *testing.T) {
`unexpected value found in mapping for field "foo.type.ignore_above": preview mappings value (1024) different from the actual mappings value (2048)`,
},
},
{
title: "undefined parameter values within an object",
preview: map[string]any{
"foo": map[string]any{
"properties": map[string]any{
"type": map[string]any{
"type": "keyword",
},
},
},
},
actual: map[string]any{
"foo": map[string]any{
"properties": map[string]any{
"type": map[string]any{
"type": "keyword",
"time_series_matric": "counter",
},
},
},
},
schema: []FieldDefinition{},
expectedErrors: []string{
`field "foo.type.time_series_matric" is undefined`,
},
},
{
title: "different number types",
preview: map[string]any{
Expand Down Expand Up @@ -625,11 +676,36 @@ func TestComparingMappings(t *testing.T) {
},
},
},
// foo is added to the exception list because it is type nested
exceptionFields: []string{"foo"},
spec: "3.0.0",
schema: []FieldDefinition{},
expectedErrors: []string{},
},
{
title: "validate nested types starting spec 3.0.1",
preview: map[string]any{
"foo": map[string]any{
"type": "nested",
},
},
actual: map[string]any{
"foo": map[string]any{
"type": "nested",
"properties": map[string]any{
"bar": map[string]any{
"type": "long",
},
},
},
},
exceptionFields: []string{},
spec: "3.0.1",
schema: []FieldDefinition{},
expectedErrors: []string{
`not found properties in preview mappings for path: "foo"`,
},
},
}

for _, c := range cases {
Expand All @@ -647,7 +723,7 @@ func TestComparingMappings(t *testing.T) {
)
require.NoError(t, err)

errs := v.compareMappings("", c.preview, c.actual)
errs := v.compareMappings("", false, c.preview, c.actual)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new parameter here to know in which cases properties could be part of the parameters of a mapping or a subfield named properties.

if len(c.expectedErrors) > 0 {
assert.Len(t, errs, len(c.expectedErrors))
for _, err := range errs {
Expand Down