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.

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

feat: tools-2777 associate validation errors with the config section that caused… #38

Merged
merged 10 commits into from
Apr 2, 2024
8 changes: 4 additions & 4 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ func newRootCmd() *cobra.Command {
multiErr = errors.Join(multiErr, err)
}

if cfgFile != "" {
logger.Infof("Using config file: %s", cfgFile)
}

lvl, err := cmd.Flags().GetString("log-level")
if err != nil {
multiErr = fmt.Errorf("%w, %w", multiErr, err)
Expand All @@ -59,6 +55,10 @@ func newRootCmd() *cobra.Command {

logger.SetLevel(lvlCode)

if cfgFile != "" {
logger.Infof("Using config file: %s", cfgFile)
}

return multiErr
},
}
Expand Down
2 changes: 0 additions & 2 deletions cmd/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ func newValidateCmd() *cobra.Command {
// verrs is an empty slice if err is not nil but no
// validation errors were found
if verrs != nil && len(verrs.Errors) > 0 {
// force validation errors to be written to stdout
// so they can more easily be grepd etc.
cmd.Print(verrs.Error())
return errors.Join(conf.ErrConfigValidation, ErrSilent)
}
Expand Down
114 changes: 113 additions & 1 deletion conf/config_validator.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package conf

import (
"encoding/json"
"errors"
"fmt"
"sort"
"strconv"
"strings"

"github.com/aerospike/aerospike-management-lib/asconfig"
Expand All @@ -29,7 +31,7 @@ func NewConfigValidator(confHandler ConfHandler, mgmtLogger logr.Logger, version
}

// Validate validates the parsed configuration against the schema for the given versions.
// ValidationErrors is not nil if any errors during validation occur.
// ValidationErrors is not nil if any errors occur during validation.
func (cv *ConfigValidator) Validate() (*ValidationErrors, error) {

valid, tempVerrs, err := cv.IsValid(cv.mgmtLogger, cv.version)
Expand All @@ -43,6 +45,31 @@ func (cv *ConfigValidator) Validate() (*ValidationErrors, error) {
}

if !valid || err != nil || len(verrs.Errors) > 0 {

config := cv.ToMap()

jsonConfigStr, err := json.Marshal(config)
if err != nil {
return nil, err
}

jsonConfig := map[string]any{}
err = json.Unmarshal(jsonConfigStr, &jsonConfig)

// check the context of each error and use that context to get the name
// of the field that is causing the error from the json config
for i, verr := range verrs.Errors {
context, _ := strings.CutPrefix(verr.Context, "(root).")
context, err := jsonToConfigContext(jsonConfig, context)
if err != nil {
// if we can't associate the error with its
// corresponding field, just use the current context
continue
}

verrs.Errors[i].Context = context
}

return &verrs, errors.Join(ErrConfigValidation, err)
}

Expand Down Expand Up @@ -107,3 +134,88 @@ func (o ValidationErrors) Error() string {

return errString
}

// jsonToConfigContext takes a json config and a context string and returns a copy
// of the context string with the field names instead of list indexes.
// For example, if the context is "namespaces.0", and the json config is:
//
// {
// "namespaces": [
// {
// "name": "test"
// }
// ]
// }
//
// The function will return "namespaces.test"
// An error is returned if the json or context are not in the expected management lib format.
func jsonToConfigContext(jsonConfig any, context string) (string, error) {
split := strings.SplitN(context, ".", 2)
key := split[0]

var res string

// check if key is an index
if index, err := strconv.Atoi(key); err == nil {
dwelch-spike marked this conversation as resolved.
Show resolved Hide resolved
// if key is an index, then context should be a slice
jsonSlice, ok := jsonConfig.([]any)
if !ok {
return "", fmt.Errorf("context is not a slice in json config at: %s", context)
}

// check if index is out of bounds
if len(jsonSlice) <= index {
return "", fmt.Errorf("index out of bounds json config at: %s", context)
}

// the indexed object should be a map
indexedMap, ok := jsonSlice[index].(map[string]any)
if !ok {
return "", fmt.Errorf("context is not a map in json config at: %s", context)
}

// get the name field from the indexed object
name, ok := indexedMap["name"]
if !ok {
return "", fmt.Errorf("name not found in json config at: %s", context)
}

// name should be a string
if nameStr, ok := name.(string); !ok {
return "", fmt.Errorf("name is not a string in json config at: %v", context)
} else {
// set res to the name instead of the index
res = nameStr
}

// set jsonConfig to the indexed object
jsonConfig = indexedMap
} else {
// if key is not an index, then context should be a map
jsonMap, ok := jsonConfig.(map[string]any)
if !ok {
return "", fmt.Errorf("context is not a map in json config at: %s", context)
}

// set jsonConfig to the object at key
jsonConfig, ok = jsonMap[key]
if !ok {
return "", fmt.Errorf("context not found in json config at: %s", key)
}

// set res to the context key
res = key
}

if len(split) > 1 {
// if we have more context to parse, recurse
val, err := jsonToConfigContext(jsonConfig, split[1])
if err != nil {
return "", err
}

res = fmt.Sprintf("%s.%s", res, val)
}

return res, nil
}
123 changes: 123 additions & 0 deletions conf/config_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,126 @@ func Test_asconf_Validate(t *testing.T) {
})
}
}

func Test_jsonToConfigContext(t *testing.T) {
type args struct {
jsonConfig any
context string
}
tests := []struct {
name string
args args
want string
wantErr bool
}{
{
name: "positive",
args: args{
jsonConfig: map[string]any{
"test0": map[string]any{
"name": "nested0",
"test1": "test2",
},
"test3": "test4",
"test5": []any{
"test6",
map[string]any{
"name": "nested1",
"test7": "test8",
"test9": "test10",
"test11": []any{
"test12",
"test13",
map[string]any{
"name": "nested2",
"test14": "test15",
},
},
},
},
},
context: "test5.1.test11.2",
},
want: "test5.nested1.test11.nested2",
},
{
name: "negative context is not a slice",
args: args{
jsonConfig: map[string]any{
"test0": map[string]any{
"name": "nested0",
"test1": "test2",
},
"test3": "test4",
},
context: "test0.1.test1",
},
wantErr: true,
},
{
name: "negative context is not a map",
args: args{
jsonConfig: map[string]any{
"test0": []any{
"test1",
},
},
context: "test0.0",
},
wantErr: true,
},
{
name: "negative name not found",
args: args{
jsonConfig: map[string]any{
"test0": []any{
map[string]any{
"test1": "test2",
},
},
},
context: "test0.0",
},
wantErr: true,
},
{
name: "negative name is not a string",
args: args{
jsonConfig: map[string]any{
"test0": []any{
map[string]any{
"name": 1,
"test1": "test2",
},
},
},
context: "test0.0",
},
wantErr: true,
},
{
name: "negative context is not a map, non numeric key",
args: args{
jsonConfig: map[string]any{
"test0": []any{
"test1",
},
},
context: "test0.test1",
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := jsonToConfigContext(tt.args.jsonConfig, tt.args.context)
if (err != nil) != tt.wantErr {
t.Errorf("jsonToConfigContext() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("jsonToConfigContext() = %v, want %v", got, tt.want)
}
})
}
}
25 changes: 12 additions & 13 deletions integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1001,38 +1001,38 @@ type validateTest struct {

var validateTests = []validateTest{
{
arguments: []string{"validate", "-a", "6.2.0", filepath.Join(sourcePath, "pmem_cluster_cr.yaml")},
arguments: []string{"validate", "-a", "6.2.0", "-l", "panic", filepath.Join(sourcePath, "pmem_cluster_cr.yaml")},
expectError: false,
expectedResult: "",
source: filepath.Join(sourcePath, "pmem_cluster_cr.yaml"),
},
{
arguments: []string{"validate", filepath.Join(extraTestPath, "metadata", "metadata.conf")},
arguments: []string{"validate", "-l", "panic", filepath.Join(extraTestPath, "metadata", "metadata.conf")},
expectError: false,
expectedResult: "",
source: filepath.Join(extraTestPath, "metadata", "metadata.conf"),
},
{
arguments: []string{"validate", "-a", "7.0.0", filepath.Join(extraTestPath, "server64", "server64.yaml")},
arguments: []string{"validate", "-a", "7.0.0", "-l", "panic", filepath.Join(extraTestPath, "server64", "server64.yaml")},
expectError: true,
source: filepath.Join(extraTestPath, "server64", "server64.yaml"),
expectedResult: `context: (root).namespaces.0
expectedResult: `context: namespaces.ns1
- description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed
context: (root).namespaces.0.storage-engine
- description: devices is required, error-type: required
context: (root).namespaces.1
- description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed
context: (root).namespaces.1.index-type
context: namespaces.ns1.index-type
- description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed
- description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed
- description: mounts-budget is required, error-type: required
context: (root).namespaces.1.sindex-type
context: namespaces.ns1.sindex-type
- description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed
- description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed
- description: mounts-budget is required, error-type: required
context: (root).namespaces.1.storage-engine
context: namespaces.ns1.storage-engine
- description: devices is required, error-type: required
context: (root).service
context: namespaces.ns2
- description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed
context: namespaces.ns2.storage-engine
- description: devices is required, error-type: required
context: service
- description: cluster-name is required, error-type: required
`,
},
Expand All @@ -1050,7 +1050,6 @@ func TestValidate(t *testing.T) {
if string(out) != tf.expectedResult {
t.Errorf("\nTESTCASE: %+v\nACTUAL: %s\nEXPECTED: %s", tf.arguments, string(out), tf.expectedResult)
}

}
}

Expand Down
Loading