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

validation: Allow loose validation for sets #247

Merged
merged 1 commit into from
Sep 26, 2023
Merged

validation: Allow loose validation for sets #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions typed/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,13 @@ func (p ParseableType) IsValid() bool {

// FromYAML parses a yaml string into an object with the current schema
// and the type "typename" or an error if validation fails.
func (p ParseableType) FromYAML(object YAMLObject) (*TypedValue, error) {
func (p ParseableType) FromYAML(object YAMLObject, opts ...ValidationOptions) (*TypedValue, error) {
var v interface{}
err := yaml.Unmarshal([]byte(object), &v)
if err != nil {
return nil, err
}
return AsTyped(value.NewValueInterface(v), p.Schema, p.TypeRef)
return AsTyped(value.NewValueInterface(v), p.Schema, p.TypeRef, opts...)
}

// FromUnstructured converts a go "interface{}" type, typically an
Expand All @@ -108,21 +108,21 @@ func (p ParseableType) FromYAML(object YAMLObject) (*TypedValue, error) {
// The provided interface{} must be one of: map[string]interface{},
// map[interface{}]interface{}, []interface{}, int types, float types,
// string or boolean. Nested interface{} must also be one of these types.
func (p ParseableType) FromUnstructured(in interface{}) (*TypedValue, error) {
return AsTyped(value.NewValueInterface(in), p.Schema, p.TypeRef)
func (p ParseableType) FromUnstructured(in interface{}, opts ...ValidationOptions) (*TypedValue, error) {
return AsTyped(value.NewValueInterface(in), p.Schema, p.TypeRef, opts...)
}

// FromStructured converts a go "interface{}" type, typically an structured object in
// Kubernetes, to a TypedValue. It will return an error if the resulting object fails
// schema validation. The provided "interface{}" value must be a pointer so that the
// value can be modified via reflection. The provided "interface{}" may contain structs
// and types that are converted to Values by the jsonMarshaler interface.
func (p ParseableType) FromStructured(in interface{}) (*TypedValue, error) {
func (p ParseableType) FromStructured(in interface{}, opts ...ValidationOptions) (*TypedValue, error) {
v, err := value.NewValueReflect(in)
if err != nil {
return nil, fmt.Errorf("error creating struct value reflector: %v", err)
}
return AsTyped(v, p.Schema, p.TypeRef)
return AsTyped(v, p.Schema, p.TypeRef, opts...)
}

// DeducedParseableType is a ParseableType that deduces the type from
Expand Down
20 changes: 17 additions & 3 deletions typed/typed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,24 @@ import (
"sigs.k8s.io/structured-merge-diff/v4/value"
)

// ValidationOptions is the list of all the options available when running the validation.
type ValidationOptions int
apelisse marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@alexzielenski alexzielenski Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Think it would be better if this was not an int and instead something like a func (v *validatingObjectWalker) or something similar so that we aren't bound to very simple options in the future.


const (
// AllowDuplicates means that sets and associative lists can have duplicate similar items.
AllowDuplicates ValidationOptions = iota
)

// AsTyped accepts a value and a type and returns a TypedValue. 'v' must have
// type 'typeName' in the schema. An error is returned if the v doesn't conform
// to the schema.
func AsTyped(v value.Value, s *schema.Schema, typeRef schema.TypeRef) (*TypedValue, error) {
func AsTyped(v value.Value, s *schema.Schema, typeRef schema.TypeRef, opts ...ValidationOptions) (*TypedValue, 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.

I've decided to use that convention since we want to backport this code and I didn't want to break the compatibility with existing code (forcing me to bump the major version). Alternative could be to make a AsTypedWithDuplicates

tv := &TypedValue{
value: v,
typeRef: typeRef,
schema: s,
}
if err := tv.Validate(); err != nil {
if err := tv.Validate(opts...); err != nil {
return nil, err
}
return tv, nil
Expand Down Expand Up @@ -79,8 +87,14 @@ func (tv TypedValue) Schema() *schema.Schema {
}

// Validate returns an error with a list of every spec violation.
func (tv TypedValue) Validate() error {
func (tv TypedValue) Validate(opts ...ValidationOptions) error {
w := tv.walker()
for _, opt := range opts {
switch opt {
case AllowDuplicates:
w.allowDuplicates = true
}
}
defer w.finished()
if errs := w.validate(nil); len(errs) != 0 {
return errs
Expand Down
6 changes: 5 additions & 1 deletion typed/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func (tv TypedValue) walker() *validatingObjectWalker {
v.value = tv.value
v.schema = tv.schema
v.typeRef = tv.typeRef
v.allowDuplicates = false
if v.allocator == nil {
v.allocator = value.NewFreelistAllocator()
}
Expand All @@ -49,6 +50,9 @@ type validatingObjectWalker struct {
value value.Value
schema *schema.Schema
typeRef schema.TypeRef
// If set to true, duplicates will be allowed in
// associativeLists/sets.
allowDuplicates bool

// Allocate only as many walkers as needed for the depth by storing them here.
spareWalkers *[]*validatingObjectWalker
Expand Down Expand Up @@ -137,7 +141,7 @@ func (v *validatingObjectWalker) visitListItems(t *schema.List, list value.List)
// this element.
return
}
if observedKeys.Has(pe) {
if observedKeys.Has(pe) && !v.allowDuplicates {
errs = append(errs, errorf("duplicate entries for key %v", pe.String())...)
}
observedKeys.Insert(pe)
Expand Down
34 changes: 26 additions & 8 deletions typed/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ type validationTestCase struct {
schema typed.YAMLObject
validObjects []typed.YAMLObject
invalidObjects []typed.YAMLObject
// duplicatesObjects are valid with AllowDuplicates validation, invalid otherwise.
duplicatesObjects []typed.YAMLObject
}

var validationCases = []validationTestCase{{
Expand Down Expand Up @@ -63,7 +65,6 @@ var validationCases = []validationTestCase{{
`{"key":"foo","value":null}`,
`{"key":"foo"}`,
`{"key":"foo","value":true}`,
`{"key":"foo","value":true}`,
`{"key":null}`,
},
invalidObjects: []typed.YAMLObject{
Expand Down Expand Up @@ -136,28 +137,27 @@ var validationCases = []validationTestCase{{
`{"bool":"aoeu"}`,
`{"bool":{"a":1}}`,
`{"bool":["foo"]}`,
`{"setStr":["a","a"]}`,
`{"setBool":[true,false,true]}`,
`{"setNumeric":[1,2,3,3.14159,1]}`,
`{"setStr":[1]}`,
`{"setStr":[true]}`,
`{"setStr":[1.5]}`,
`{"setStr":[null]}`,
`{"setStr":[{}]}`,
`{"setStr":[[]]}`,
`{"setBool":[true,false,true]}`,
`{"setBool":[1]}`,
`{"setBool":[1.5]}`,
`{"setBool":[null]}`,
`{"setBool":[{}]}`,
`{"setBool":[[]]}`,
`{"setBool":["a"]}`,
`{"setNumeric":[1,2,3,3.14159,1]}`,
`{"setNumeric":[null]}`,
`{"setNumeric":[true]}`,
`{"setNumeric":["a"]}`,
`{"setNumeric":[[]]}`,
`{"setNumeric":[{}]}`,
}, duplicatesObjects: []typed.YAMLObject{
`{"setStr":["a","a"]}`,
`{"setBool":[true,false,true]}`,
`{"setNumeric":[1,2,3,3.14159,1]}`,
},
}, {
name: "associative list",
Expand Down Expand Up @@ -232,9 +232,10 @@ var validationCases = []validationTestCase{{
`{"list":[{}]}`,
`{"list":[{"value":{"a":"a"},"bv":true,"nv":3.14}]}`,
`{"list":[{"key":"a","id":1,"value":{"a":1}}]}`,
`{"list":[{"key":"a","id":1},{"key":"a","id":1}]}`,
`{"list":[{"key":"a","id":1,"value":{"a":"a"},"bv":"true","nv":3.14}]}`,
`{"list":[{"key":"a","id":1,"value":{"a":"a"},"bv":true,"nv":false}]}`,
}, duplicatesObjects: []typed.YAMLObject{
`{"list":[{"key":"a","id":1},{"key":"a","id":1}]}`,
},
}}

Expand Down Expand Up @@ -262,13 +263,30 @@ func (tt validationTestCase) test(t *testing.T) {
t.Parallel()
_, err := pt.FromYAML(iv)
if err == nil {
t.Errorf("Object should fail: %v\n%v", err, iv)
t.Fatalf("Object should fail:\n%v", iv)
}
if strings.Contains(err.Error(), "invalid atom") {
t.Errorf("Error should be useful, but got: %v\n%v", err, iv)
}
})
}
for i, iv := range tt.duplicatesObjects {
iv := iv
t.Run(fmt.Sprintf("%v-duplicates-%v", tt.name, i), func(t *testing.T) {
t.Parallel()
_, err := pt.FromYAML(iv)
if err == nil {
t.Fatalf("Object should fail:\n%v", iv)
}
if strings.Contains(err.Error(), "invalid atom") {
t.Errorf("Error should be useful, but got: %v\n%v", err, iv)
}
_, err = pt.FromYAML(iv, typed.AllowDuplicates)
if err != nil {
t.Errorf("failed to parse/validate yaml: %v\n%v", err, iv)
}
})
}
}

func TestSchemaValidation(t *testing.T) {
Expand Down