Add a Detail field to Validation Error

pkg/api/errors/errors.go

@@ -30,7 +30,7 @@ type StatusError struct {
 	ErrStatus api.Status
 }
 
-var _ error = &statusError{}
+var _ error = &StatusError{}
 
 // Error implements the Error interface.
 func (e *StatusError) Error() string {

pkg/api/errors/errors_test.go

@@ -69,7 +69,7 @@ func TestNewInvalid(t *testing.T) {
 			},
 		},
 		{
-			NewFieldInvalid("field[0].name", "bar"),
+			NewFieldInvalid("field[0].name", "bar", "detail"),
 			&api.StatusDetails{
 				Kind: "kind",
 				ID:   "name",

pkg/api/errors/validation.go

@@ -78,42 +78,47 @@ type ValidationError struct {
 	Type     ValidationErrorType
 	Field    string
 	BadValue interface{}
+	Detail   string
 }
 
 var _ error = &ValidationError{}
 
 func (v *ValidationError) Error() string {
-	return fmt.Sprintf("%s: %v '%v'", v.Field, ValueOf(v.Type), v.BadValue)
+	s := fmt.Sprintf("%s: %s '%v'", v.Field, ValueOf(v.Type), v.BadValue)
+	if v.Detail != "" {
+		s += fmt.Sprintf(": %s", v.Detail)
+	}
+	return s
 }
 
 // NewFieldRequired returns a *ValidationError indicating "value required"
 func NewFieldRequired(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeRequired, field, value}
+	return &ValidationError{ValidationErrorTypeRequired, field, value, ""}
 }
 
 // NewFieldInvalid returns a *ValidationError indicating "invalid value"
-func NewFieldInvalid(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeInvalid, field, value}
+func NewFieldInvalid(field string, value interface{}, detail string) *ValidationError {
+	return &ValidationError{ValidationErrorTypeInvalid, field, value, detail}
 }
 
 // NewFieldNotSupported returns a *ValidationError indicating "unsupported value"
 func NewFieldNotSupported(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeNotSupported, field, value}
+	return &ValidationError{ValidationErrorTypeNotSupported, field, value, ""}
 }
 
 // NewFieldForbidden returns a *ValidationError indicating "forbidden"
 func NewFieldForbidden(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeForbidden, field, value}
+	return &ValidationError{ValidationErrorTypeForbidden, field, value, ""}
 }
 
 // NewFieldDuplicate returns a *ValidationError indicating "duplicate value"
 func NewFieldDuplicate(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeDuplicate, field, value}
+	return &ValidationError{ValidationErrorTypeDuplicate, field, value, ""}
 }
 
 // NewFieldNotFound returns a *ValidationError indicating "value not found"
 func NewFieldNotFound(field string, value interface{}) *ValidationError {
-	return &ValidationError{ValidationErrorTypeNotFound, field, value}
+	return &ValidationError{ValidationErrorTypeNotFound, field, value, ""}
 }
 
 // ValidationErrorList is a collection of ValidationErrors.  This does not

pkg/api/errors/validation_test.go

@@ -27,7 +27,7 @@ func TestMakeFuncs(t *testing.T) {
 		expected ValidationErrorType
 	}{
 		{
-			func() *ValidationError { return NewFieldInvalid("f", "v") },
+			func() *ValidationError { return NewFieldInvalid("f", "v", "d") },
 			ValidationErrorTypeInvalid,
 		},
 		{
@@ -57,8 +57,8 @@ func TestMakeFuncs(t *testing.T) {
 }
 
 func TestValidationError(t *testing.T) {
-	s := NewFieldInvalid("foo", "bar").Error()
-	if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) {
+	s := NewFieldInvalid("foo", "bar", "deet").Error()
+	if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, "deet") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) {
 		t.Errorf("error message did not contain expected values, got %s", s)
 	}
 }
@@ -73,7 +73,7 @@ func TestErrListPrefix(t *testing.T) {
 			"foo[0].bar",
 		},
 		{
-			NewFieldInvalid("field", "value"),
+			NewFieldInvalid("field", "value", ""),
 			"foo.field",
 		},
 		{
@@ -103,7 +103,7 @@ func TestErrListPrefixIndex(t *testing.T) {
 			"[1][0].bar",
 		},
 		{
-			NewFieldInvalid("field", "value"),
+			NewFieldInvalid("field", "value", ""),
 			"[1].field",
 		},
 		{

pkg/api/validation/events.go

@@ -26,10 +26,10 @@ import (
 func ValidateEvent(event *api.Event) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	if event.Namespace != event.InvolvedObject.Namespace {
-		allErrs = append(allErrs, errs.NewFieldInvalid("involvedObject.namespace", event.InvolvedObject.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("involvedObject.namespace", event.InvolvedObject.Namespace, "namespace does not match involvedObject"))
 	}
 	if !util.IsDNSSubdomain(event.Namespace) {
-		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", event.Namespace))
+		allErrs = append(allErrs, errs.NewFieldInvalid("namespace", event.Namespace, ""))
 	}
 	return allErrs
 }

pkg/config/config.go

@@ -17,10 +17,13 @@ limitations under the License.
 package config
 
 import (
+	"fmt"
+
 	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 // ClientFunc returns the RESTClient defined for given resource
@@ -30,33 +33,33 @@ type ClientFunc func(mapping *meta.RESTMapping) (*client.RESTClient, error)
 // be valid API type. It requires ObjectTyper to parse the Version and Kind and
 // RESTMapper to get the resource URI and REST client that knows how to create
 // given type
-func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor ClientFunc, objects []runtime.Object) errs.ValidationErrorList {
-	allErrors := errs.ValidationErrorList{}
+func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor ClientFunc, objects []runtime.Object) util.ErrorList {
+	allErrors := util.ErrorList{}
 	for i, obj := range objects {
 		version, kind, err := typer.ObjectVersionAndKind(obj)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldInvalid("kind", obj))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] kind: %v", i, err))
 			continue
 		}
 
 		mapping, err := mapper.RESTMapping(version, kind)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldNotSupported("mapping", err))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] mapping: %v", i, err))
 			continue
 		}
 
 		client, err := clientFor(mapping)
 		if err != nil {
-			reportError(&allErrors, i, errs.NewFieldNotSupported("client", obj))
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] client: %v", i, err))
 			continue
 		}
 
 		if err := CreateObject(client, mapping, obj); err != nil {
-			reportError(&allErrors, i, err)
+			allErrors = append(allErrors, fmt.Errorf("Config.item[%d]: %v", i, err))
 		}
 	}
 
-	return allErrors.Prefix("Config")
+	return allErrors
 }
 
 // CreateObject creates the obj using the provided clients and the resource URI
@@ -76,15 +79,8 @@ func CreateObject(client *client.RESTClient, mapping *meta.RESTMapping, obj runt
 	// TODO: This should be using RESTHelper
 	err = client.Post().Path(mapping.Resource).Namespace(namespace).Body(obj).Do().Error()
 	if err != nil {
-		return errs.NewFieldInvalid(name, err)
+		return errs.NewFieldInvalid(name, obj, err.Error())
 	}
 
 	return nil
 }
-
-// reportError reports the single item validation error and properly set the
-// prefix and index to match the Config item JSON index
-func reportError(allErrs *errs.ValidationErrorList, index int, err *errs.ValidationError) {
-	i := errs.ValidationErrorList{}
-	*allErrs = append(*allErrs, append(i, err).PrefixIndex(index).Prefix("item")...)
-}

pkg/config/config_test.go

@@ -21,10 +21,10 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
@@ -92,13 +92,9 @@ func TestCreateNoNameItem(t *testing.T) {
 		t.Errorf("Expected required value error for missing name")
 	}
 
-	e := errs[0].(*errors.ValidationError)
-	if errors.ValueOf(e.Type) != "required value" {
-		t.Errorf("Expected ValidationErrorTypeRequired error, got %#v", e)
-	}
-
-	if e.Field != "Config.item[0].name" {
-		t.Errorf("Expected 'Config.item[0].name' as error field, got '%#v'", e.Field)
+	errStr := errs[0].Error()
+	if !strings.Contains(errStr, "Config.item[0]: name") {
+		t.Errorf("Expected 'Config.item[0]: name' in error string, got '%s'", errStr)
 	}
 }
 
@@ -121,13 +117,9 @@ func TestCreateInvalidItem(t *testing.T) {
 		t.Errorf("Expected invalid value error for kind")
 	}
 
-	e := errs[0].(*errors.ValidationError)
-	if errors.ValueOf(e.Type) != "invalid value" {
-		t.Errorf("Expected ValidationErrorTypeInvalid error, got %#v", e)
-	}
-
-	if e.Field != "Config.item[0].kind" {
-		t.Errorf("Expected 'Config.item[0].kind' as error field, got '%#v'", e.Field)
+	errStr := errs[0].Error()
+	if !strings.Contains(errStr, "Config.item[0] kind") {
+		t.Errorf("Expected 'Config.item[0] kind' in error string, got '%s'", errStr)
 	}
 }
 
@@ -153,12 +145,8 @@ func TestCreateNoClientItems(t *testing.T) {
 		t.Errorf("Expected invalid value error for client")
 	}
 
-	e := errs[0].(*errors.ValidationError)
-	if errors.ValueOf(e.Type) != "unsupported value" {
-		t.Errorf("Expected ValidationErrorTypeUnsupported error, got %#v", e)
-	}
-
-	if e.Field != "Config.item[0].client" {
-		t.Errorf("Expected 'Config.item[0].client' as error field, got '%#v'", e.Field)
+	errStr := errs[0].Error()
+	if !strings.Contains(errStr, "Config.item[0] client") {
+		t.Errorf("Expected 'Config.item[0] client' in error string, got '%s'", errStr)
 	}
 }

pkg/kubectl/cmd/createall.go

@@ -17,49 +17,44 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
 	"io"
 
-	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/config"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/golang/glog"
 	"github.com/spf13/cobra"
 	"gopkg.in/v1/yaml"
 )
 
 // DataToObjects converts the raw JSON data into API objects
-func DataToObjects(m meta.RESTMapper, t runtime.ObjectTyper, data []byte) (result []runtime.Object, errors errs.ValidationErrorList) {
+func DataToObjects(m meta.RESTMapper, t runtime.ObjectTyper, data []byte) (result []runtime.Object, errors util.ErrorList) {
 	configObj := []runtime.RawExtension{}
 
 	if err := yaml.Unmarshal(data, &configObj); err != nil {
-		errors = append(errors, errs.NewFieldInvalid("unmarshal", err))
-		return result, errors.Prefix("Config")
+		errors = append(errors, fmt.Errorf("config unmarshal: %v", err))
+		return result, errors
 	}
 
 	for i, in := range configObj {
 		version, kind, err := t.DataVersionAndKind(in.RawJSON)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
-			itemErrs = append(itemErrs, errs.NewFieldInvalid("kind", string(in.RawJSON)))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
+			errors = append(errors, fmt.Errorf("item[%d] kind: %v", i, err))
 			continue
 		}
 
 		mapping, err := m.RESTMapping(version, kind)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
-			itemErrs = append(itemErrs, errs.NewFieldRequired("mapping", err))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
+			errors = append(errors, fmt.Errorf("item[%d] mapping: %v", i, err))
 			continue
 		}
 
 		obj, err := mapping.Codec.Decode(in.RawJSON)
 		if err != nil {
-			itemErrs := errs.ValidationErrorList{}
-			itemErrs = append(itemErrs, errs.NewFieldInvalid("decode", err))
-			errors = append(errors, itemErrs.PrefixIndex(i).Prefix("item")...)
+			errors = append(errors, fmt.Errorf("item[%d] decode: %v", i, err))
 			continue
 		}
 		result = append(result, obj)

pkg/registry/service/rest.go

@@ -100,8 +100,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	} else {
 		// Try to respect the requested IP.
 		if err := rs.portalMgr.Allocate(net.ParseIP(service.Spec.PortalIP)); err != nil {
-			// TODO: Differentiate "IP already allocated" from real errors.
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP)}
+			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, err.Error())}
 			return nil, errors.NewInvalid("service", service.Name, el)
 		}
 	}
@@ -222,9 +221,8 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 		if err != nil {
 			return nil, err
 		}
-		if service.Spec.PortalIP != cur.Spec.PortalIP {
-			// TODO: Would be nice to pass "field is immutable" to users.
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP)}
+		if service.Spec.PortalIP != "" && service.Spec.PortalIP != cur.Spec.PortalIP {
+			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")}
 			return nil, errors.NewInvalid("service", service.Name, el)
 		}
 		// Copy over non-user fields.

pkg/tools/etcd_tools_watch.go

@@ -50,7 +50,8 @@ func ParseWatchResourceVersion(resourceVersion, kind string) (uint64, error) {
 	}
 	version, err := strconv.ParseUint(resourceVersion, 10, 64)
 	if err != nil {
-		return 0, errors.NewInvalid(kind, "", errors.ValidationErrorList{errors.NewFieldInvalid("resourceVersion", resourceVersion)})
+		// TODO: Does this need to be a ValidationErrorList?  I can't convince myself it does.
+		return 0, errors.NewInvalid(kind, "", errors.ValidationErrorList{errors.NewFieldInvalid("resourceVersion", resourceVersion, err.Error())})
 	}
 	return version + 1, nil
 }

test/integration/auth_test.go

@@ -273,7 +273,7 @@ func getTestRequests() []struct {
 		// Normal methods on services
 		{"GET", "/api/v1beta1/services", "", code200},
 		{"POST", "/api/v1beta1/services" + syncFlags, aService, code200},
-		{"PUT", "/api/v1beta1/services/a" + syncFlags, aService, code422}, // TODO: GET and put back server-provided fields to avoid a 422
+		{"PUT", "/api/v1beta1/services/a" + syncFlags, aService, code409}, // TODO: GET and put back server-provided fields to avoid a 422
 		{"GET", "/api/v1beta1/services", "", code200},
 		{"GET", "/api/v1beta1/services/a", "", code200},
 		{"DELETE", "/api/v1beta1/services/a" + syncFlags, "", code200},