Return validation errors from storage create/update

pkg/api/errors/errors_test.go

@@ -18,6 +18,7 @@ package errors
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 )
 
@@ -56,8 +57,21 @@ func TestMakeFuncs(t *testing.T) {
 	}
 }
 
+func TestValidationError(t *testing.T) {
+	s := NewInvalid("foo", "bar").Error()
+	if !strings.Contains(s, "foo") || !strings.Contains(s, "bar") || !strings.Contains(s, ValueOf(ValidationErrorTypeInvalid)) {
+		t.Errorf("error message did not contain expected values, got %s", s)
+	}
+}
+
 func TestErrorList(t *testing.T) {
 	errList := ErrorList{}
+	if a := errList.ToError(); a != nil {
+		t.Errorf("unexpected non-nil error for empty list: %v", a)
+	}
+	if a := errorListInternal(errList).Error(); a != "" {
+		t.Errorf("expected empty string, got %v", a)
+	}
 	errList = append(errList, NewInvalid("field", "value"))
 	// The fact that this compiles is the test.
 }
@@ -87,3 +101,63 @@ func TestErrorListToError(t *testing.T) {
 		}
 	}
 }
+
+func TestErrListPrefix(t *testing.T) {
+	testCases := []struct {
+		Err      ValidationError
+		Expected string
+	}{
+		{
+			NewNotFound("[0].bar", "value"),
+			"foo[0].bar",
+		},
+		{
+			NewInvalid("field", "value"),
+			"foo.field",
+		},
+		{
+			NewDuplicate("", "value"),
+			"foo",
+		},
+	}
+	for _, testCase := range testCases {
+		errList := ErrorList{testCase.Err}
+		prefix := errList.Prefix("foo")
+		if prefix == nil || len(prefix) != len(errList) {
+			t.Errorf("Prefix should return self")
+		}
+		if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a {
+			t.Errorf("expected %s, got %s", e, a)
+		}
+	}
+}
+
+func TestErrListPrefixIndex(t *testing.T) {
+	testCases := []struct {
+		Err      ValidationError
+		Expected string
+	}{
+		{
+			NewNotFound("[0].bar", "value"),
+			"[1][0].bar",
+		},
+		{
+			NewInvalid("field", "value"),
+			"[1].field",
+		},
+		{
+			NewDuplicate("", "value"),
+			"[1]",
+		},
+	}
+	for _, testCase := range testCases {
+		errList := ErrorList{testCase.Err}
+		prefix := errList.PrefixIndex(1)
+		if prefix == nil || len(prefix) != len(errList) {
+			t.Errorf("PrefixIndex should return self")
+		}
+		if e, a := testCase.Expected, errList[0].(ValidationError).Field; e != a {
+			t.Errorf("expected %s, got %s", e, a)
+		}
+	}
+}

pkg/apiserver/errors.go

@@ -78,15 +78,25 @@ func NewConflictErr(kind, name string, err error) error {
 }
 
 // NewInvalidError returns an error indicating the item is invalid and cannot be processed.
-func NewInvalidError(kind, name string, errs errors.ErrorList) error {
+func NewInvalidErr(kind, name string, errs errors.ErrorList) error {
+	causes := make([]api.StatusCause, 0, len(errs))
+	for i := range errs {
+		if err, ok := errs[i].(errors.ValidationError); ok {
+			causes = append(causes, api.StatusCause{
+				Reason:  api.CauseReasonType(err.Type),
+				Message: err.Error(),
+				Field:   err.Field,
+			})
+		}
+	}
 	return &apiServerError{api.Status{
 		Status: api.StatusFailure,
 		Code:   422, // RFC 4918
 		Reason: api.ReasonTypeInvalid,
 		Details: &api.StatusDetails{
-			Kind: kind,
+			Kind:   kind,
-			ID:   name,
+			ID:     name,
-			// TODO: causes
+			Causes: causes,
 		},
 		Message: fmt.Sprintf("%s %q is invalid: %s", kind, name, errs.ToError()),
 	}}

pkg/apiserver/errors_test.go

@@ -19,9 +19,11 @@ package apiserver
 import (
 	"errors"
 	"fmt"
+	"reflect"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 )
 
 func TestErrorNew(t *testing.T) {
@@ -45,11 +47,86 @@ func TestErrorNew(t *testing.T) {
 	if !IsNotFound(NewNotFoundErr("test", "3")) {
 		t.Errorf("expected to be not found")
 	}
-	if !IsInvalid(NewInvalidError("test", "2", nil)) {
+	if !IsInvalid(NewInvalidErr("test", "2", nil)) {
 		t.Errorf("expected to be invalid")
 	}
 }
 
+func TestNewInvalidErr(t *testing.T) {
+	testCases := []struct {
+		Err     apierrors.ValidationError
+		Details *api.StatusDetails
+	}{
+		{
+			apierrors.NewDuplicate("field[0].name", "bar"),
+			&api.StatusDetails{
+				Kind: "kind",
+				ID:   "name",
+				Causes: []api.StatusCause{{
+					Reason: api.CauseReasonTypeFieldValueDuplicate,
+					Field:  "field[0].name",
+				}},
+			},
+		},
+		{
+			apierrors.NewInvalid("field[0].name", "bar"),
+			&api.StatusDetails{
+				Kind: "kind",
+				ID:   "name",
+				Causes: []api.StatusCause{{
+					Reason: api.CauseReasonTypeFieldValueInvalid,
+					Field:  "field[0].name",
+				}},
+			},
+		},
+		{
+			apierrors.NewNotFound("field[0].name", "bar"),
+			&api.StatusDetails{
+				Kind: "kind",
+				ID:   "name",
+				Causes: []api.StatusCause{{
+					Reason: api.CauseReasonTypeFieldValueNotFound,
+					Field:  "field[0].name",
+				}},
+			},
+		},
+		{
+			apierrors.NewNotSupported("field[0].name", "bar"),
+			&api.StatusDetails{
+				Kind: "kind",
+				ID:   "name",
+				Causes: []api.StatusCause{{
+					Reason: api.CauseReasonTypeFieldValueNotSupported,
+					Field:  "field[0].name",
+				}},
+			},
+		},
+		{
+			apierrors.NewRequired("field[0].name", "bar"),
+			&api.StatusDetails{
+				Kind: "kind",
+				ID:   "name",
+				Causes: []api.StatusCause{{
+					Reason: api.CauseReasonTypeFieldValueRequired,
+					Field:  "field[0].name",
+				}},
+			},
+		},
+	}
+	for i := range testCases {
+		vErr, expected := testCases[i].Err, testCases[i].Details
+		expected.Causes[0].Message = vErr.Error()
+		err := NewInvalidErr("kind", "name", apierrors.ErrorList{vErr})
+		status := errToAPIStatus(err)
+		if status.Code != 422 || status.Reason != api.ReasonTypeInvalid {
+			t.Errorf("unexpected status: %#v", status)
+		}
+		if !reflect.DeepEqual(expected, status.Details) {
+			t.Errorf("expected %#v, got %#v", expected, status.Details)
+		}
+	}
+}
+
 func Test_errToAPIStatus(t *testing.T) {
 	err := &apiServerError{}
 	status := errToAPIStatus(err)

pkg/registry/controller/storage.go

@@ -60,7 +60,7 @@ func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) {
 	// Pod Manifest ID should be assigned by the pod API
 	controller.DesiredState.PodTemplate.DesiredState.Manifest.ID = ""
 	if errs := api.ValidateReplicationController(controller); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("replicationController", controller.ID, errs)
 	}
 
 	controller.CreationTimestamp = util.Now()
@@ -117,7 +117,7 @@ func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) {
 		return nil, fmt.Errorf("not a replication controller: %#v", obj)
 	}
 	if errs := api.ValidateReplicationController(controller); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("replicationController", controller.ID, errs)
 	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		err := rs.registry.UpdateController(*controller)

pkg/registry/controller/storage_test.go

@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
 )
@@ -272,8 +273,8 @@ func TestControllerStorageValidatesCreate(t *testing.T) {
 		if c != nil {
 			t.Errorf("Expected nil channel")
 		}
-		if err == nil {
+		if !apiserver.IsInvalid(err) {
-			t.Errorf("Expected to get an error")
+			t.Errorf("Expected to get an invalid resource error, got %v", err)
 		}
 	}
 }
@@ -302,8 +303,8 @@ func TestControllerStorageValidatesUpdate(t *testing.T) {
 		if c != nil {
 			t.Errorf("Expected nil channel")
 		}
-		if err == nil {
+		if !apiserver.IsInvalid(err) {
-			t.Errorf("Expected to get an error")
+			t.Errorf("Expected to get an invalid resource error, got %v", err)
 		}
 	}
 }

pkg/registry/pod/storage.go

@@ -69,7 +69,7 @@ func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) {
 	}
 	pod.DesiredState.Manifest.ID = pod.ID
 	if errs := api.ValidatePod(pod); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("pod", pod.ID, errs)
 	}
 
 	pod.CreationTimestamp = util.Now()
@@ -135,7 +135,7 @@ func (rs RegistryStorage) New() interface{} {
 func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) {
 	pod := obj.(*api.Pod)
 	if errs := api.ValidatePod(pod); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("pod", pod.ID, errs)
 	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		if err := rs.registry.UpdatePod(*pod); err != nil {

pkg/registry/pod/storage_test.go

@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
@@ -330,8 +331,8 @@ func TestPodStorageValidatesCreate(t *testing.T) {
 	if c != nil {
 		t.Errorf("Expected nil channel")
 	}
-	if err == nil {
+	if !apiserver.IsInvalid(err) {
-		t.Errorf("Expected to get an error")
+		t.Errorf("Expected to get an invalid resource error, got %v", err)
 	}
 }
 
@@ -346,8 +347,8 @@ func TestPodStorageValidatesUpdate(t *testing.T) {
 	if c != nil {
 		t.Errorf("Expected nil channel")
 	}
-	if err == nil {
+	if !apiserver.IsInvalid(err) {
-		t.Errorf("Expected to get an error")
+		t.Errorf("Expected to get an invalid resource error, got %v", err)
 	}
 }
 

pkg/registry/service/storage.go

@@ -48,7 +48,7 @@ func NewRegistryStorage(registry Registry, cloud cloudprovider.Interface, machin
 func (rs *RegistryStorage) Create(obj interface{}) (<-chan interface{}, error) {
 	srv := obj.(*api.Service)
 	if errs := api.ValidateService(srv); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("service", srv.ID, errs)
 	}
 
 	srv.CreationTimestamp = util.Now()
@@ -147,11 +147,8 @@ func GetServiceEnvironmentVariables(registry Registry, machine string) ([]api.En
 
 func (rs *RegistryStorage) Update(obj interface{}) (<-chan interface{}, error) {
 	srv := obj.(*api.Service)
-	if srv.ID == "" {
-		return nil, fmt.Errorf("ID should not be empty: %#v", srv)
-	}
 	if errs := api.ValidateService(srv); len(errs) > 0 {
-		return nil, fmt.Errorf("Validation errors: %v", errs)
+		return nil, apiserver.NewInvalidErr("service", srv.ID, errs)
 	}
 	return apiserver.MakeAsync(func() (interface{}, error) {
 		// TODO: check to see if external load balancer status changed

pkg/registry/service/storage_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
 	cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion"
@@ -78,8 +79,8 @@ func TestServiceStorageValidatesCreate(t *testing.T) {
 		if c != nil {
 			t.Errorf("Expected nil channel")
 		}
-		if err == nil {
+		if !apiserver.IsInvalid(err) {
-			t.Errorf("Expected to get an error")
+			t.Errorf("Expected to get an invalid resource error, got %v", err)
 		}
 
 	}
@@ -139,8 +140,8 @@ func TestServiceStorageValidatesUpdate(t *testing.T) {
 		if c != nil {
 			t.Errorf("Expected nil channel")
 		}
-		if err == nil {
+		if !apiserver.IsInvalid(err) {
-			t.Errorf("Expected to get an error")
+			t.Errorf("Expected to get an invalid resource error, got %v", err)
 		}
 	}
 }