Simplify FieldError merging. (#99)

* checkpoint.

* Tried a new way.

* Some more slimplifying based on feedback.

* use bang vs == false.

* not sure why I did not use go style on this forloop, fixed.

apis/field_error.go

@@ -30,7 +30,7 @@ const CurrentField = ""
 // specific fields in a manner suitable for use in a recursive walk, so
 // that errors contain the appropriate field context.
 // FieldError methods are non-mutating.
-// +k8s:deepcopy-gen=false
+// +k8s:deepcopy-gen=true
 type FieldError struct {
 	Message string
 	Paths   []string
@@ -54,18 +54,21 @@ func (fe *FieldError) ViaField(prefix ...string) *FieldError {
 	if fe == nil {
 		return nil
 	}
-	newErr := &FieldError{}
+	// Copy over message and details, paths will be updated and errors come
-	for _, e := range fe.getNormalizedErrors() {
+	// along using .Also().
-		// Prepend the Prefix to existing errors.
+	newErr := &FieldError{
-		newPaths := make([]string, 0, len(e.Paths))
+		Message: fe.Message,
-		for _, oldPath := range e.Paths {
+		Details: fe.Details,
-			newPaths = append(newPaths, flatten(append(prefix, oldPath)))
+	}
-		}
-		sort.Slice(newPaths, func(i, j int) bool { return newPaths[i] < newPaths[j] })
-		e.Paths = newPaths
 
-		// Append the mutated error to the errors list.
+	// Prepend the Prefix to existing errors.
-		newErr = newErr.Also(&e)
+	newPaths := make([]string, 0, len(fe.Paths))
+	for _, oldPath := range fe.Paths {
+		newPaths = append(newPaths, flatten(append(prefix, oldPath)))
+	}
+	newErr.Paths = newPaths
+	for _, e := range fe.errors {
+		newErr = newErr.Also(e.ViaField(prefix...))
 	}
 	return newErr
 }
@@ -104,21 +107,32 @@ func (fe *FieldError) ViaFieldKey(field string, key string) *FieldError {
 
 // Also collects errors, returns a new collection of existing errors and new errors.
 func (fe *FieldError) Also(errs ...*FieldError) *FieldError {
-	newErr := &FieldError{}
+	var newErr *FieldError
 	// collect the current objects errors, if it has any
-	if fe != nil {
+	if !fe.isEmpty() {
-		newErr.errors = fe.getNormalizedErrors()
+		newErr = fe.DeepCopy()
+	} else {
+		newErr = &FieldError{}
 	}
 	// and then collect the passed in errors
 	for _, e := range errs {
-		newErr.errors = append(newErr.errors, e.getNormalizedErrors()...)
+		if !e.isEmpty() {
+			newErr.errors = append(newErr.errors, *e)
+		}
 	}
-	if len(newErr.errors) == 0 {
+	if newErr.isEmpty() {
 		return nil
 	}
 	return newErr
 }
 
+func (fe *FieldError) isEmpty() bool {
+	if fe == nil {
+		return true
+	}
+	return fe.Message == "" && fe.Details == "" && len(fe.errors) == 0 && len(fe.Paths) == 0
+}
+
 func (fe *FieldError) getNormalizedErrors() []FieldError {
 	// in case we call getNormalizedErrors on a nil object, return just an empty
 	// list. This can happen when .Error() is called on a nil object.
@@ -139,14 +153,34 @@ func (fe *FieldError) getNormalizedErrors() []FieldError {
 	for _, e := range fe.errors {
 		errors = append(errors, e.getNormalizedErrors()...)
 	}
-	sort.Slice(errors, func(i, j int) bool { return errors[i].Message < errors[j].Message })
 	return errors
 }
 
+// Error implements error
+func (fe *FieldError) Error() string {
+	var errs []string
+	// Get the list of errors as a flat merged list.
+	normedErrors := merge(fe.getNormalizedErrors())
+	for _, e := range normedErrors {
+		if e.Details == "" {
+			errs = append(errs, fmt.Sprintf("%v: %v", e.Message, strings.Join(e.Paths, ", ")))
+		} else {
+			errs = append(errs, fmt.Sprintf("%v: %v\n%v", e.Message, strings.Join(e.Paths, ", "), e.Details))
+		}
+	}
+	return strings.Join(errs, "\n")
+}
+
+// Helpers ---
+
 func asIndex(index int) string {
 	return fmt.Sprintf("[%d]", index)
 }
 
+func isIndex(part string) bool {
+	return strings.HasPrefix(part, "[") && strings.HasSuffix(part, "]")
+}
+
 func asKey(key string) string {
 	return fmt.Sprintf("[%s]", key)
 }
@@ -173,23 +207,80 @@ func flatten(path []string) string {
 	return strings.Join(newPath, ".")
 }
 
-func isIndex(part string) bool {
+// mergePaths takes in two string slices and returns the combination of them
-	return strings.HasPrefix(part, "[") && strings.HasSuffix(part, "]")
+// without any duplicate entries.
-}
+func mergePaths(a, b []string) []string {
-
+	newPaths := make([]string, 0, len(a)+len(b))
-// Error implements error
+	newPaths = append(newPaths, a...)
-func (fe *FieldError) Error() string {
+	for _, bi := range b {
-	var errs []string
+		if !containsString(newPaths, bi) {
-	for _, e := range fe.getNormalizedErrors() {
+			newPaths = append(newPaths, bi)
-		if e.Details == "" {
-			errs = append(errs, fmt.Sprintf("%v: %v", e.Message, strings.Join(e.Paths, ", ")))
-		} else {
-			errs = append(errs, fmt.Sprintf("%v: %v\n%v", e.Message, strings.Join(e.Paths, ", "), e.Details))
 		}
 	}
-	return strings.Join(errs, "\n")
+	return newPaths
 }
 
+// containsString takes in a string slice and looks for the provided string
+// within the slice.
+func containsString(slice []string, s string) bool {
+	for _, item := range slice {
+		if item == s {
+			return true
+		}
+	}
+	return false
+}
+
+// merge takes in a flat list of FieldErrors and returns back a merged list of
+// FiledErrors. FieldErrors have their Paths combined (and de-duped) if their
+// Message and Details are the same. Merge will not inspect FieldError.errors.
+// Merge will also sort the .Path slice, and the errors slice before returning.
+func merge(errs []FieldError) []FieldError {
+	// make a map big enough for all the errors.
+	m := make(map[string]FieldError, len(errs))
+
+	// Convert errs to a map where the key is <message>-<details> and the value
+	// is the error. If an error already exists in the map with the same key,
+	// then the paths will be merged.
+	for _, e := range errs {
+		k := key(&e)
+		if v, ok := m[k]; ok {
+			// Found a match, merge the keys.
+			v.Paths = mergePaths(v.Paths, e.Paths)
+			m[k] = v
+		} else {
+			// Does not exist in the map, save the error.
+			m[k] = e
+		}
+	}
+
+	// Take the map made previously and flatten it back out again.
+	newErrs := make([]FieldError, 0, len(m))
+	for _, v := range m {
+		// While we have access to the merged paths, sort them too.
+		sort.Slice(v.Paths, func(i, j int) bool { return v.Paths[i] < v.Paths[j] })
+		newErrs = append(newErrs, v)
+	}
+
+	// Sort the flattened map.
+	sort.Slice(newErrs, func(i, j int) bool {
+		if newErrs[i].Message == newErrs[j].Message {
+			return newErrs[i].Details < newErrs[j].Details
+		}
+		return newErrs[i].Message < newErrs[j].Message
+	})
+
+	// return back the merged list of sorted errors.
+	return newErrs
+}
+
+// key returns the key using the fields .Message and .Details.
+func key(err *FieldError) string {
+	return fmt.Sprintf("%s-%s", err.Message, err.Details)
+}
+
+// Public helpers ---
+
 // ErrMissingField is a variadic helper method for constructing a FieldError for
 // a set of missing fields.
 func ErrMissingField(fieldPaths ...string) *FieldError {

apis/field_error_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package apis
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
 	"testing"
@@ -127,6 +128,89 @@ can not use @, do not try`,
 		prefixes: [][]string{{"baz"}},
 		want: `invalid key name "b@r": baz.foo[0].name
 can not use @, do not try`,
+	}, {
+		name: "very complex to simple",
+		err: func() *FieldError {
+			fe := &FieldError{
+				Message: "First",
+				Paths:   []string{"A", "B", "C"},
+			}
+
+			fe = fe.Also(fe).Also(fe).Also(fe).Also(fe)
+
+			fe = fe.Also(&FieldError{
+				Message: "Second",
+				Paths:   []string{"Z", "X", "Y"},
+			})
+
+			fe = fe.Also(fe).Also(fe).Also(fe).Also(fe)
+
+			return fe
+		}(),
+		want: `First: A, B, C
+Second: X, Y, Z`,
+	}, {
+		name: "exponentially grows",
+		err: func() *FieldError {
+			fe := &FieldError{
+				Message: "Top",
+				Paths:   []string{"A", "B", "C"},
+			}
+
+			for _, p := range []string{"3", "2", "1"} {
+				for i := 0; i < 3; i++ {
+					fe = fe.Also(fe)
+				}
+				fe = fe.ViaField(p)
+			}
+
+			return fe
+		}(),
+		want: `Top: 1.2.3.A, 1.2.3.B, 1.2.3.C`,
+	}, {
+		name: "path grows but details are different",
+		err: func() *FieldError {
+			fe := &FieldError{
+				Message: "Top",
+				Paths:   []string{"A", "B", "C"},
+			}
+
+			for _, p := range []string{"3", "2", "1"} {
+				e := fe.ViaField(p)
+				e.Details = fmt.Sprintf("here at %s", p)
+				for i := 0; i < 3; i++ {
+					fe = fe.Also(e)
+				}
+			}
+
+			return fe
+		}(),
+		want: `Top: A, B, C
+Top: 1.A, 1.B, 1.C
+here at 1
+Top: 1.2.A, 1.2.B, 1.2.C, 2.A, 2.B, 2.C
+here at 2
+Top: 1.2.3.A, 1.2.3.B, 1.2.3.C, 1.3.A, 1.3.B, 1.3.C, 2.3.A, 2.3.B, 2.3.C, 3.A, 3.B, 3.C
+here at 3`,
+	}, {
+		name: "very complex to complex",
+		err: func() *FieldError {
+			fe := &FieldError{
+				Message: "First",
+				Paths:   []string{"A", "B", "C"},
+			}
+
+			fe = fe.ViaField("one").Also(fe).ViaField("two").Also(fe).ViaField("three").Also(fe)
+
+			fe = fe.Also(&FieldError{
+				Message: "Second",
+				Paths:   []string{"Z", "X", "Y"},
+			})
+
+			return fe
+		}(),
+		want: `First: A, B, C, three.A, three.B, three.C, three.two.A, three.two.B, three.two.C, three.two.one.A, three.two.one.B, three.two.one.C
+Second: X, Y, Z`,
 	}}
 
 	for _, test := range tests {
@@ -434,6 +518,111 @@ not without this: bar.C`,
 	}
 }
 
+func TestMergeFieldErrors(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      *FieldError
+		also     []FieldError
+		prefixes [][]string
+		want     string
+	}{{
+		name: "simple",
+		err: &FieldError{
+			Message: "A simple error message",
+			Paths:   []string{"bar"},
+		},
+		also: []FieldError{{
+			Message: "A simple error message",
+			Paths:   []string{"foo"},
+		}},
+		want: `A simple error message: bar, foo`,
+	}, {
+		name: "conflict",
+		err: &FieldError{
+			Message: "A simple error message",
+			Paths:   []string{"bar", "foo"},
+		},
+		also: []FieldError{{
+			Message: "A simple error message",
+			Paths:   []string{"foo"},
+		}},
+		want: `A simple error message: bar, foo`,
+	}, {
+		name: "lots of also",
+		err: (&FieldError{
+			Message: "this error",
+			Paths:   []string{"bar", "foo"},
+		}).Also(&FieldError{
+			Message: "another",
+			Paths:   []string{"right", "left"},
+		}).ViaField("head"),
+		also: []FieldError{{
+			Message: "An alpha error message",
+			Paths:   []string{"A"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"B"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"C"},
+		}, {
+			Message: "An alpha error message",
+			Paths:   []string{"D"},
+		}, {
+			Message: "this error",
+			Paths:   []string{"foo"},
+			Details: "devil is in the details",
+		}, {
+			Message: "this error",
+			Paths:   []string{"foo"},
+			Details: "more details",
+		}},
+		prefixes: [][]string{{"this"}},
+		want: `An alpha error message: this.A, this.B, this.C, this.D
+another: this.head.left, this.head.right
+this error: this.head.bar, this.head.foo
+this error: this.foo
+devil is in the details
+this error: this.foo
+more details`,
+	}}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fe := test.err
+			for _, err := range test.also {
+				fe = fe.Also(&err)
+			}
+			// Simulate propagation up a call stack.
+			for _, prefix := range test.prefixes {
+				fe = fe.ViaField(prefix...)
+			}
+			if test.want != "" {
+				got := fe.Error()
+				if got != test.want {
+					t.Errorf("%s: Error() = %v, wanted %v", test.name, got, test.want)
+				}
+			} else if fe != nil {
+				t.Errorf("%s: ViaField() = %v, wanted nil", test.name, fe)
+			}
+		})
+	}
+}
+
 func TestAlsoStaysNil(t *testing.T) {
 	var err *FieldError
 	if err != nil {

apis/zz_generated.deepcopy.go

@@ -20,6 +20,34 @@ limitations under the License.
 
 package apis
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *FieldError) DeepCopyInto(out *FieldError) {
+	*out = *in
+	if in.Paths != nil {
+		in, out := &in.Paths, &out.Paths
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
+	if in.errors != nil {
+		in, out := &in.errors, &out.errors
+		*out = make([]FieldError, len(*in))
+		for i := range *in {
+			(*in)[i].DeepCopyInto(&(*out)[i])
+		}
+	}
+	return
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FieldError.
+func (in *FieldError) DeepCopy() *FieldError {
+	if in == nil {
+		return nil
+	}
+	out := new(FieldError)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *VolatileTime) DeepCopyInto(out *VolatileTime) {
 	*out = *in