schema: better errors for AnyOf/AllOf + cleanups

AnyOf and AllOf have received better error messages and doc-strings.
They have also been changed so that they will accept any value if there
are no members.

Clean up mergeFieldErrors to be easer to read; Go allows appending to
nil slice and expansion of slices in append, so there is no need for
the if branch and nested for-loop that was there before.

Simplify error handling in Object.Validate slightly. Moves the ErrorMap
type from object.go to errors.go and add some helper methods.

Author: smyrman

schema/alloff.go renamed to schema/allof.go

@@ -1,6 +1,8 @@
 package schema
 
-// AllOf validates that all the sub field validators validates.
+// AllOf validates that all the sub field validators validates. Be aware that
+// the order of the validators matter, as the result of one successful
+// validation is passed as input to the next.
 type AllOf []FieldValidator
 
 // Compile implements the ReferenceCompiler interface.
@@ -15,12 +17,10 @@ func (v AllOf) Compile(rc ReferenceChecker) (err error) {
 	return
 }
 
-// ValidateQuery implements schema.FieldQueryValidator interface
+// ValidateQuery implements schema.FieldQueryValidator interface. Note the
+// result of one successful validation is passed as input to the next. The
+// result of the first error or last successful validation is returned.
 func (v AllOf) ValidateQuery(value interface{}) (interface{}, error) {
-	// This works like this:
-	// 1.	only the first validator gets passed the original value.
-	// 2. after that, each validator gets passed the value from the previous validator.
-	// 3. finally, the value returned from the last validator is returned for further use.
 	for _, validator := range v {
 		var err error
 		if validatorQuery, ok := validator.(FieldQueryValidator); ok {
@@ -35,12 +35,10 @@ func (v AllOf) ValidateQuery(value interface{}) (interface{}, error) {
 	return value, nil
 }
 
-// Validate ensures that all sub-validators validates.
+// Validate ensures that all sub-validators validates. Note the result of one
+// successful validation is passed as input to the next. The result of the first
+// error or last successful validation is returned.
 func (v AllOf) Validate(value interface{}) (interface{}, error) {
-	// This works like this:
-	// 1.	only the first validator gets passed the original value.
-	// 2. after that, each validator gets passed the value from the previous validator.
-	// 3. finally, the value returned from the last validator is returned for further use.
 	for _, validator := range v {
 		var err error
 		if value, err = validator.Validate(value); err != nil {

schema/alloff_test.go renamed to schema/allof_test.go

@@ -1,7 +1,5 @@
 package schema
 
-import "errors"
-
 // AnyOf validates if any of the sub field validators validates. If any of the
 // sub field validators implements the FieldSerializer interface, the *first*
 // implementation which does not error will be used.
@@ -20,8 +18,10 @@ func (v AnyOf) Compile(rc ReferenceChecker) error {
 	return nil
 }
 
-// ValidateQuery implements schema.FieldQueryValidator interface
+// ValidateQuery implements schema.FieldQueryValidator interface.
 func (v AnyOf) ValidateQuery(value interface{}) (interface{}, error) {
+	var errs ErrorSlice
+
 	for _, validator := range v {
 		var err error
 		var val interface{}
@@ -33,20 +33,31 @@ func (v AnyOf) ValidateQuery(value interface{}) (interface{}, error) {
 		if err == nil {
 			return val, nil
 		}
+		errs = errs.Append(err)
+	}
+
+	if len(errs) > 0 {
+		return nil, errs
 	}
-	// TODO: combine errors.
-	return nil, errors.New("invalid")
+	return nil, nil
 }
 
 // Validate ensures that at least one sub-validator validates.
 func (v AnyOf) Validate(value interface{}) (interface{}, error) {
+	var errs ErrorSlice
+
 	for _, validator := range v {
-		if value, err := validator.Validate(value); err == nil {
+		value, err := validator.Validate(value)
+		if err == nil {
 			return value, nil
 		}
+		errs = errs.Append(err)
+	}
+
+	if len(errs) > 0 {
+		return nil, errs
 	}
-	// TODO: combine errors.
-	return nil, errors.New("invalid")
+	return nil, nil
 }
 
 // Serialize attempts to serialize the value using the first available

schema/anyof.go

@@ -11,8 +11,8 @@ import (
 // hexByteArray implements the FieldSerializer interface.
 type hexByteArray struct{}
 
-// Validate is a dummy implemetation of the FieldValidator interface implemented
-// to allow inclusion in AnyOf.
+// Validate is a dummy implementation of the FieldValidator interface
+// implemented to allow inclusion in AnyOf.
 func (h hexByteArray) Validate(value interface{}) (interface{}, error) {
 	return nil, nil
 }
@@ -75,7 +75,13 @@ func TestAnyOfValidate(t *testing.T) {
 			Name:      `{Bool,Bool}.Validate("")`,
 			Validator: schema.AnyOf{&schema.Bool{}, &schema.Bool{}},
 			Input:     "",
-			Error:     "invalid",
+			Error:     "not a Boolean, not a Boolean",
+		},
+		{
+			Name:      "{Bool,String}.Validate(42)",
+			Validator: schema.AnyOf{&schema.Bool{}, &schema.String{}},
+			Input:     42,
+			Error:     "not a Boolean, not a string",
 		},
 		{
 			Name:      "{Bool,String}.Validate(true)",
@@ -128,7 +134,7 @@ func TestAnyOfQueryValidate(t *testing.T) {
 			Name:      `{Bool,Bool}.Validate("")`,
 			Validator: schema.AnyOf{&schema.Bool{}, &schema.Bool{}},
 			Input:     "",
-			Error:     "invalid",
+			Error:     "not a Boolean, not a Boolean",
 		},
 		{
 			Name:      "{Bool,String}.Validate(true)",

schema/anyof_test.go

@@ -0,0 +1,56 @@
+package schema
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+)
+
+// ErrorMap contains a map of errors by field name.
+type ErrorMap map[string][]interface{}
+
+// Error implements the built-in error interface.
+func (err ErrorMap) Error() string {
+	errs := make([]string, 0, len(err))
+	for key := range err {
+		errs = append(errs, key)
+	}
+	sort.Strings(errs)
+	for i, key := range errs {
+		errs[i] = fmt.Sprintf("%s is %s", key, err[key])
+	}
+	return strings.Join(errs, ", ")
+}
+
+// Merge copies all errors from other into err.
+func (err ErrorMap) Merge(other ErrorMap) {
+	for k, v := range other {
+		err[k] = append(err[k], v...)
+	}
+}
+
+// ErrorSlice contains a concatenation of several errors.
+type ErrorSlice []error
+
+// Append adds an error to err and returns a new slice if others is not nil. If
+// other is another ErrorSlice it is extended so that all elements are appended.
+func (err ErrorSlice) Append(other error) ErrorSlice {
+	switch et := other.(type) {
+	case nil:
+		// don't append nil errors.
+	case ErrorSlice:
+		// Merge error slices.
+		err = append(err, et...)
+	default:
+		err = append(err, et)
+	}
+	return err
+}
+
+func (err ErrorSlice) Error() string {
+	sl := make([]string, 0, len(err))
+	for _, err := range err {
+		sl = append(sl, err.Error())
+	}
+	return strings.Join(sl, ", ")
+}

schema/error.go

@@ -2,9 +2,6 @@ package schema
 
 import (
 	"errors"
-	"fmt"
-	"sort"
-	"strings"
 )
 
 // Object validates objects which are defined by Schemas.
@@ -31,9 +28,9 @@ func (v Object) Validate(value interface{}) (interface{}, error) {
 	}
 	dest, errs := v.Schema.Validate(nil, obj)
 	if len(errs) > 0 {
-		var errMap ErrorMap
-		errMap = errs
-		return nil, errMap
+		// Currently, tests expect FieldValidators to always return a nil value
+		// on validation errors.
+		return nil, ErrorMap(errs)
 	}
 	return dest, nil
 }
@@ -42,18 +39,3 @@ func (v Object) Validate(value interface{}) (interface{}, error) {
 func (v Object) GetField(name string) *Field {
 	return v.Schema.GetField(name)
 }
-
-// ErrorMap contains a map of errors by field name.
-type ErrorMap map[string][]interface{}
-
-func (e ErrorMap) Error() string {
-	errs := make([]string, 0, len(e))
-	for key := range e {
-		errs = append(errs, key)
-	}
-	sort.Strings(errs)
-	for i, key := range errs {
-		errs[i] = fmt.Sprintf("%s is %s", key, e[key])
-	}
-	return strings.Join(errs, ", ")
-}

schema/object.go

@@ -344,12 +344,6 @@ func addFieldError(errs map[string][]interface{}, field string, err interface{})
 func mergeFieldErrors(errs map[string][]interface{}, mergeErrs map[string][]interface{}) {
 	// TODO recursive merge
 	for field, values := range mergeErrs {
-		if dest, found := errs[field]; found {
-			for _, value := range values {
-				dest = append(dest, value)
-			}
-		} else {
-			errs[field] = values
-		}
+		errs[field] = append(errs[field], values...)
 	}
 }