Skip to content

Commit 830455d

Browse files
JustinKulidhaiducek
authored andcommitted
Fail generation and add help on field typos
When the input configuration does not match the schema in the go type, the generator will now respond with an error. The intention is to help users when there is a typo in the configuration, which otherwise would just be ignored (and in the case of a map, any configuration "inside" would be dropped). This also adds help text to these error cases, to try and identify what field the user might have meant. That implementation uses a library which does not compute the Levenshtein distance, but should work well enough, and has the advantage of already being in the import graph. Refs: - stolostron/backlog#19646 Signed-off-by: Justin Kulikauskas <[email protected]>
1 parent 494d753 commit 830455d

File tree

4 files changed

+297
-4
lines changed

4 files changed

+297
-4
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ module open-cluster-management.io/ocm-kustomize-generator-plugins
33
go 1.18
44

55
require (
6+
github.com/pmezard/go-difflib v1.0.0
67
github.com/spf13/pflag v1.0.5
78
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
89
k8s.io/apimachinery v0.23.3
@@ -32,7 +33,6 @@ require (
3233
github.com/modern-go/reflect2 v1.0.2 // indirect
3334
github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
3435
github.com/pkg/errors v0.9.1 // indirect
35-
github.com/pmezard/go-difflib v1.0.0 // indirect
3636
github.com/stretchr/testify v1.7.0 // indirect
3737
github.com/xlab/treeprint v1.1.0 // indirect
3838
go.starlark.net v0.0.0-20220213143740-c55a923347b1 // indirect

internal/plugin.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ const (
3838
// Plugin is used to store the PolicyGenerator configuration and the methods to generate the
3939
// desired policies.
4040
type Plugin struct {
41-
Metadata struct {
41+
APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty"`
42+
Kind string `json:"kind,omitempty" yaml:"kind,omitempty"`
43+
Metadata struct {
4244
Name string `json:"name,omitempty" yaml:"name,omitempty"`
4345
} `json:"metadata,omitempty" yaml:"metadata,omitempty"`
4446
PlacementBindingDefaults struct {
@@ -79,11 +81,14 @@ var defaults = types.PolicyDefaults{
7981
// Config validates the input PolicyGenerator configuration, applies any missing defaults, and
8082
// configures the Policy object.
8183
func (p *Plugin) Config(config []byte, baseDirectory string) error {
82-
err := yaml.Unmarshal(config, p)
84+
dec := yaml.NewDecoder(bytes.NewReader(config))
85+
dec.KnownFields(true) // emit an error on unknown fields in the input
86+
87+
err := dec.Decode(p)
8388
const errTemplate = "the PolicyGenerator configuration file is invalid: %w"
8489

8590
if err != nil {
86-
return fmt.Errorf(errTemplate, err)
91+
return fmt.Errorf(errTemplate, addFieldNotFoundHelp(err))
8792
}
8893

8994
var unmarshaledConfig map[string]interface{}

internal/typohelper.go

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
package internal
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"reflect"
7+
"regexp"
8+
"strings"
9+
10+
"github.com/pmezard/go-difflib/difflib"
11+
)
12+
13+
// addFieldNotFoundHelp adds help text to errors that occur when the input config
14+
// has a field that is not present in the Config struct (eg a typo), to assist
15+
// the user in debugging the problem. If the input error was not from a missing
16+
// field, then it is returned unchanged.
17+
func addFieldNotFoundHelp(err error) error {
18+
re := regexp.MustCompile(`field (\S*) not found in type (\S*)`)
19+
20+
repl := func(line string) string {
21+
match := re.FindStringSubmatch(line)
22+
23+
fieldType := reflect.TypeOf(Plugin{})
24+
fieldTag := "PolicyGenerator"
25+
26+
if match[2] != fieldType.String() {
27+
// Search the right type, if it's not the top-level object.
28+
fieldType, fieldTag = findNestedType(fieldType, match[2], "")
29+
}
30+
31+
msg := fmt.Sprintf("field %v found but not defined", match[1])
32+
33+
if fieldTag != "" {
34+
msg += fmt.Sprintf(" in type %v", fieldTag)
35+
}
36+
37+
suggestion := autocorrectField(match[1], fieldType)
38+
if suggestion != "" {
39+
msg += fmt.Sprintf(" - did you mean '%v'?", suggestion)
40+
}
41+
42+
return msg
43+
}
44+
45+
helpMsg := re.ReplaceAllStringFunc(err.Error(), repl)
46+
47+
if helpMsg == err.Error() {
48+
// Error was unchanged, return the original to preserve the type
49+
return err
50+
}
51+
52+
return errors.New(helpMsg)
53+
}
54+
55+
// autocorrectField returns the field in the containingType which most closely
56+
// matches the input badField. It will return an empty string if no match can
57+
// be found with sufficient confidence.
58+
func autocorrectField(badField string, containingType reflect.Type) string {
59+
if containingType == nil || containingType.Kind() != reflect.Struct {
60+
return ""
61+
}
62+
63+
matcher := difflib.NewMatcher(strings.Split(badField, ""), []string{""})
64+
bestRatio := 0.85 // require 85% or better match
65+
bestMatch := ""
66+
67+
// iterate over all fields in the struct that have a yaml tag.
68+
for _, field := range reflect.VisibleFields(containingType) {
69+
yamlTag := strings.SplitN(field.Tag.Get("yaml"), ",", 2)[0]
70+
if yamlTag == "" {
71+
continue
72+
}
73+
74+
matcher.SetSeq2(strings.Split(yamlTag, ""))
75+
76+
ratio := matcher.Ratio()
77+
if ratio > bestRatio {
78+
bestRatio = ratio
79+
bestMatch = yamlTag
80+
}
81+
}
82+
83+
return bestMatch
84+
}
85+
86+
// findNestedType searches through the given type and nested types (recursively)
87+
// for a type matching the wanted string. It will return the matching type if
88+
// found, or nil if not found. It will also return the yaml tag of the field
89+
// where the type was found.
90+
func findNestedType(baseType reflect.Type, want string, tag string) (reflect.Type, string) {
91+
if baseType.String() == want {
92+
return baseType, tag
93+
}
94+
95+
if baseType.Kind() == reflect.Array || baseType.Kind() == reflect.Slice || baseType.Kind() == reflect.Map {
96+
return findNestedType(baseType.Elem(), want, tag)
97+
}
98+
99+
if baseType.Kind() != reflect.Struct {
100+
return nil, ""
101+
}
102+
103+
// iterate over all fields in the struct that have a yaml tag.
104+
for _, field := range reflect.VisibleFields(baseType) {
105+
yamlTag := strings.SplitN(field.Tag.Get("yaml"), ",", 2)[0]
106+
107+
if field.Type.String() == want {
108+
return field.Type, yamlTag
109+
}
110+
111+
deeperType, deeperTag := findNestedType(field.Type, want, yamlTag)
112+
if deeperType != nil {
113+
return deeperType, deeperTag
114+
}
115+
}
116+
117+
return nil, ""
118+
}

internal/typohelper_test.go

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
package internal
2+
3+
import (
4+
"bytes"
5+
"fmt"
6+
"path"
7+
"regexp"
8+
"testing"
9+
)
10+
11+
func TestConfigTypos(t *testing.T) {
12+
t.Parallel()
13+
tmpDir := t.TempDir()
14+
createConfigMap(t, tmpDir, "configmap.yaml")
15+
16+
suggestFmt := "line %v: field %v found but not defined in type %v - did you mean '%v'?"
17+
18+
tests := map[string]struct {
19+
desiredErrs []string
20+
generator []byte
21+
}{
22+
"no typos": {
23+
desiredErrs: []string{},
24+
generator: []byte(`
25+
apiVersion: policy.open-cluster-management.io/v1
26+
kind: PolicyGenerator
27+
metadata:
28+
name: policy-generator-minimal
29+
policyDefaults:
30+
namespace: minimal
31+
policies:
32+
- name: my-minimal
33+
manifests:
34+
- path: @@@
35+
`),
36+
},
37+
"one typo with suggestion": {
38+
desiredErrs: []string{
39+
fmt.Sprintf(suggestFmt, "6", "policyDefault", "PolicyGenerator", "policyDefaults"),
40+
},
41+
generator: []byte(`
42+
apiVersion: policy.open-cluster-management.io/v1
43+
kind: PolicyGenerator
44+
metadata:
45+
name: policy-generator-minimal
46+
policyDefault:
47+
namespace: minimal
48+
policies:
49+
- name: my-minimal
50+
manifests:
51+
- path: @@@
52+
`),
53+
},
54+
"two typos with suggestions": {
55+
desiredErrs: []string{
56+
fmt.Sprintf(suggestFmt, "6", "policyDefault", "PolicyGenerator", "policyDefaults"),
57+
fmt.Sprintf(suggestFmt, "8", "policie", "PolicyGenerator", "policies"),
58+
},
59+
generator: []byte(`
60+
apiVersion: policy.open-cluster-management.io/v1
61+
kind: PolicyGenerator
62+
metadata:
63+
name: policy-generator-minimal
64+
policyDefault:
65+
namespace: minimal
66+
policie:
67+
- name: my-minimal
68+
manifests:
69+
- path: @@@
70+
`),
71+
},
72+
"one deeper typo": {
73+
desiredErrs: []string{
74+
fmt.Sprintf(suggestFmt, "7", "configPolicyAnnotations", "policyDefaults",
75+
"configurationPolicyAnnotations"),
76+
},
77+
generator: []byte(`
78+
apiVersion: policy.open-cluster-management.io/v1
79+
kind: PolicyGenerator
80+
metadata:
81+
name: policy-generator-minimal
82+
policyDefaults:
83+
configPolicyAnnotations: {}
84+
namespace: minimal
85+
policies:
86+
- name: my-minimal
87+
manifests:
88+
- path: @@@
89+
`),
90+
},
91+
"typo inside a list": {
92+
desiredErrs: []string{
93+
fmt.Sprintf(suggestFmt, "11", "paths", "manifests", "path"),
94+
},
95+
generator: []byte(`
96+
apiVersion: policy.open-cluster-management.io/v1
97+
kind: PolicyGenerator
98+
metadata:
99+
name: policy-generator-minimal
100+
policyDefaults:
101+
namespace: minimal
102+
policies:
103+
- name: my-minimal
104+
manifests:
105+
- paths: @@@
106+
`),
107+
},
108+
"typo no suggestion": {
109+
desiredErrs: []string{
110+
"line 12: field namespace found but not defined in type manifests$",
111+
},
112+
generator: []byte(`
113+
apiVersion: policy.open-cluster-management.io/v1
114+
kind: PolicyGenerator
115+
metadata:
116+
name: policy-generator-minimal
117+
policyDefaults:
118+
namespace: minimal
119+
policies:
120+
- name: my-minimal
121+
manifests:
122+
- path: @@@
123+
namespace: foo
124+
`),
125+
},
126+
"non-typo error": {
127+
desiredErrs: []string{
128+
"line 6: cannot unmarshal !!seq into string",
129+
},
130+
generator: []byte(`
131+
apiVersion: policy.open-cluster-management.io/v1
132+
kind: PolicyGenerator
133+
metadata:
134+
name:
135+
- policy-generator-minimal
136+
policyDefaults:
137+
namespace: minimal
138+
policies:
139+
- name: my-minimal
140+
manifests:
141+
- path: @@@
142+
`),
143+
},
144+
}
145+
146+
for name, test := range tests {
147+
test := test
148+
149+
t.Run(name, func(t *testing.T) {
150+
t.Parallel()
151+
152+
p := Plugin{}
153+
generatorYaml := bytes.ReplaceAll(
154+
test.generator,
155+
[]byte(`@@@`),
156+
[]byte(path.Join(tmpDir, "configmap.yaml")))
157+
158+
err := p.Config(generatorYaml, tmpDir)
159+
if err == nil && len(test.desiredErrs) > 0 {
160+
t.Fatal("Expected an error to be emitted, got nil")
161+
}
162+
163+
for _, want := range test.desiredErrs {
164+
if match, _ := regexp.MatchString(want, err.Error()); !match {
165+
t.Errorf("Expected error to include '%v' - got '%v'", want, err.Error())
166+
}
167+
}
168+
})
169+
}
170+
}

0 commit comments

Comments
 (0)