Fix default setting (#740)

* push validate/defaults into datastore

we weren't setting a timestamp in route insert when we needed to create an app
there. that whole thing isn't atomic, but this fixes the timestamp issue.

closes #738

seems like we should do similar with the FireBeforeX stuff too.

* fix tests

* app name validation was buggy, an upper cased letter failed. now it doesn't.
uses unicode now.
* removes duplicate errors for datastore and models validation that were used
interchangably but weren't.

api/datastore/internal/datastoretest/test.go

@@ -280,8 +280,8 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 		}
 
 		_, err = ds.InsertApp(ctx, &models.App{})
-		if err != models.ErrDatastoreEmptyAppName {
+		if err != models.ErrAppsMissingName {
-			t.Fatalf("Test InsertApp(&{}): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyAppName, err)
+			t.Fatalf("Test InsertApp(&{}): expected error `%v`, but it was `%v`", models.ErrAppsMissingName, err)
 		}
 
 		inserted, err := ds.InsertApp(ctx, testApp)
@@ -333,8 +333,8 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 
 		// Testing get app
 		_, err = ds.GetApp(ctx, "")
-		if err != models.ErrDatastoreEmptyAppName {
+		if err != models.ErrAppsMissingName {
-			t.Fatalf("Test GetApp: expected error to be %v, but it was %s", models.ErrDatastoreEmptyAppName, err)
+			t.Fatalf("Test GetApp: expected error to be %v, but it was %s", models.ErrAppsMissingName, err)
 		}
 
 		app, err := ds.GetApp(ctx, testApp.Name)
@@ -419,8 +419,8 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 
 		// Testing app delete
 		err = ds.RemoveApp(ctx, "")
-		if err != models.ErrDatastoreEmptyAppName {
+		if err != models.ErrAppsMissingName {
-			t.Fatalf("Test RemoveApp: expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyAppName, err)
+			t.Fatalf("Test RemoveApp: expected error `%v`, but it was `%v`", models.ErrAppsMissingName, err)
 		}
 
 		err = ds.RemoveApp(ctx, testApp.Name)
@@ -462,7 +462,9 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 				t.Fatalf("Test InsertRoute(nil): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyRoute, err)
 			}
 
-			_, err = ds.InsertRoute(ctx, &models.Route{AppName: "notreal", Path: "/test"})
+			copyRoute := *testRoute
+			copyRoute.AppName = "notreal"
+			_, err = ds.InsertRoute(ctx, &copyRoute)
 			if err != models.ErrAppsNotFound {
 				t.Fatalf("Test InsertRoute: expected error `%v`, but it was `%v`", models.ErrAppsNotFound, err)
 			}
@@ -481,13 +483,13 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 		// Testing get
 		{
 			_, err = ds.GetRoute(ctx, "a", "")
-			if err != models.ErrDatastoreEmptyRoutePath {
+			if err != models.ErrRoutesMissingPath {
-				t.Fatalf("Test GetRoute(empty route path): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyRoutePath, err)
+				t.Fatalf("Test GetRoute(empty route path): expected error `%v`, but it was `%v`", models.ErrRoutesMissingPath, err)
 			}
 
 			_, err = ds.GetRoute(ctx, "", "a")
-			if err != models.ErrDatastoreEmptyAppName {
+			if err != models.ErrAppsMissingName {
-				t.Fatalf("Test GetRoute(empty app name): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyAppName, err)
+				t.Fatalf("Test GetRoute(empty app name): expected error `%v`, but it was `%v`", models.ErrAppsMissingName, err)
 			}
 
 			route, err := ds.GetRoute(ctx, testApp.Name, testRoute.Path)
@@ -682,13 +684,13 @@ func Test(t *testing.T, dsf func(t *testing.T) models.Datastore) {
 
 		// Testing route delete
 		err = ds.RemoveRoute(ctx, "", "")
-		if err != models.ErrDatastoreEmptyAppName {
+		if err != models.ErrAppsMissingName {
-			t.Fatalf("Test RemoveRoute(empty app name): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyAppName, err)
+			t.Fatalf("Test RemoveRoute(empty app name): expected error `%v`, but it was `%v`", models.ErrAppsMissingName, err)
 		}
 
 		err = ds.RemoveRoute(ctx, "a", "")
-		if err != models.ErrDatastoreEmptyRoutePath {
+		if err != models.ErrRoutesMissingPath {
-			t.Fatalf("Test RemoveRoute(empty route path): expected error `%v`, but it was `%v`", models.ErrDatastoreEmptyRoutePath, err)
+			t.Fatalf("Test RemoveRoute(empty route path): expected error `%v`, but it was `%v`", models.ErrRoutesMissingPath, err)
 		}
 
 		err = ds.RemoveRoute(ctx, testRoute.AppName, testRoute.Path)

api/datastore/internal/datastoreutil/validator.go

@@ -20,7 +20,7 @@ type validator struct {
 // name will never be empty.
 func (v *validator) GetApp(ctx context.Context, name string) (app *models.App, err error) {
 	if name == "" {
-		return nil, models.ErrDatastoreEmptyAppName
+		return nil, models.ErrAppsMissingName
 	}
 	return v.Datastore.GetApp(ctx, name)
 }
@@ -34,8 +34,10 @@ func (v *validator) InsertApp(ctx context.Context, app *models.App) (*models.App
 	if app == nil {
 		return nil, models.ErrDatastoreEmptyApp
 	}
-	if app.Name == "" {
+
-		return nil, models.ErrDatastoreEmptyAppName
+	app.SetDefaults()
+	if err := app.Validate(); err != nil {
+		return nil, err
 	}
 
 	return v.Datastore.InsertApp(ctx, app)
@@ -47,7 +49,7 @@ func (v *validator) UpdateApp(ctx context.Context, app *models.App) (*models.App
 		return nil, models.ErrDatastoreEmptyApp
 	}
 	if app.Name == "" {
-		return nil, models.ErrDatastoreEmptyAppName
+		return nil, models.ErrAppsMissingName
 	}
 	return v.Datastore.UpdateApp(ctx, app)
 }
@@ -55,7 +57,7 @@ func (v *validator) UpdateApp(ctx context.Context, app *models.App) (*models.App
 // name will never be empty.
 func (v *validator) RemoveApp(ctx context.Context, name string) error {
 	if name == "" {
-		return models.ErrDatastoreEmptyAppName
+		return models.ErrAppsMissingName
 	}
 
 	return v.Datastore.RemoveApp(ctx, name)
@@ -64,10 +66,10 @@ func (v *validator) RemoveApp(ctx context.Context, name string) error {
 // appName and routePath will never be empty.
 func (v *validator) GetRoute(ctx context.Context, appName, routePath string) (*models.Route, error) {
 	if appName == "" {
-		return nil, models.ErrDatastoreEmptyAppName
+		return nil, models.ErrAppsMissingName
 	}
 	if routePath == "" {
-		return nil, models.ErrDatastoreEmptyRoutePath
+		return nil, models.ErrRoutesMissingPath
 	}
 
 	return v.Datastore.GetRoute(ctx, appName, routePath)
@@ -76,7 +78,7 @@ func (v *validator) GetRoute(ctx context.Context, appName, routePath string) (*m
 // appName will never be empty
 func (v *validator) GetRoutesByApp(ctx context.Context, appName string, routeFilter *models.RouteFilter) (routes []*models.Route, err error) {
 	if appName == "" {
-		return nil, models.ErrDatastoreEmptyAppName
+		return nil, models.ErrAppsMissingName
 	}
 	return v.Datastore.GetRoutesByApp(ctx, appName, routeFilter)
 }
@@ -86,11 +88,10 @@ func (v *validator) InsertRoute(ctx context.Context, route *models.Route) (*mode
 	if route == nil {
 		return nil, models.ErrDatastoreEmptyRoute
 	}
-	if route.AppName == "" {
+
-		return nil, models.ErrDatastoreEmptyAppName
+	route.SetDefaults()
-	}
+	if err := route.Validate(); err != nil {
-	if route.Path == "" {
+		return nil, err
-		return nil, models.ErrDatastoreEmptyRoutePath
 	}
 
 	return v.Datastore.InsertRoute(ctx, route)
@@ -102,10 +103,10 @@ func (v *validator) UpdateRoute(ctx context.Context, newroute *models.Route) (*m
 		return nil, models.ErrDatastoreEmptyRoute
 	}
 	if newroute.AppName == "" {
-		return nil, models.ErrDatastoreEmptyAppName
+		return nil, models.ErrAppsMissingName
 	}
 	if newroute.Path == "" {
-		return nil, models.ErrDatastoreEmptyRoutePath
+		return nil, models.ErrRoutesMissingPath
 	}
 	return v.Datastore.UpdateRoute(ctx, newroute)
 }
@@ -113,10 +114,10 @@ func (v *validator) UpdateRoute(ctx context.Context, newroute *models.Route) (*m
 // appName and routePath will never be empty.
 func (v *validator) RemoveRoute(ctx context.Context, appName, routePath string) error {
 	if appName == "" {
-		return models.ErrDatastoreEmptyAppName
+		return models.ErrAppsMissingName
 	}
 	if routePath == "" {
-		return models.ErrDatastoreEmptyRoutePath
+		return models.ErrRoutesMissingPath
 	}
 
 	return v.Datastore.RemoveRoute(ctx, appName, routePath)

api/models/app.go

@@ -2,6 +2,7 @@ package models
 
 import (
 	"time"
+	"unicode"
 
 	"github.com/go-openapi/strfmt"
 )
@@ -34,7 +35,7 @@ func (a *App) Validate() error {
 		return ErrAppsTooLongName
 	}
 	for _, c := range a.Name {
-		if (c < '0' || '9' < c) && (c < 'A' || 'Z' > c) && (c < 'a' || 'z' < c) && c != '_' && c != '-' {
+		if !(unicode.IsLetter(c) || unicode.IsNumber(c) || c == '_' || c == '-') {
 			return ErrAppsInvalidName
 		}
 	}

api/models/error.go

@@ -56,14 +56,6 @@ var (
 		code:  http.StatusConflict,
 		error: errors.New("Cannot remove apps with routes"),
 	}
-	ErrDatastoreEmptyAppName = err{
-		code:  http.StatusBadRequest,
-		error: errors.New("Missing app name"),
-	}
-	ErrDatastoreEmptyRoutePath = err{
-		code:  http.StatusBadRequest,
-		error: errors.New("Missing route name"),
-	}
 	ErrDatastoreEmptyApp = err{
 		code:  http.StatusBadRequest,
 		error: errors.New("Missing app"),

api/server/apps_create.go

@@ -28,12 +28,6 @@ func (s *Server) handleAppCreate(c *gin.Context) {
 		return
 	}
 
-	app.SetDefaults()
-	if err = app.Validate(); err != nil {
-		handleErrorResponse(c, err)
-		return
-	}
-
 	err = s.FireBeforeAppCreate(ctx, app)
 	if err != nil {
 		handleErrorResponse(c, err)

api/server/routes_create_update.go

@@ -49,11 +49,6 @@ func (s *Server) handleRoutesPostPutPatch(c *gin.Context) {
 }
 
 func (s *Server) submitRoute(ctx context.Context, wroute *models.RouteWrapper) error {
-	wroute.Route.SetDefaults()
-	err := wroute.Route.Validate()
-	if err != nil {
-		return err
-	}
 	r, err := s.datastore.InsertRoute(ctx, wroute.Route)
 	if err != nil {
 		return err
@@ -114,9 +109,6 @@ func (s *Server) ensureApp(ctx context.Context, wroute *models.RouteWrapper, met
 	} else if app == nil {
 		// Create a new application
 		newapp := &models.App{Name: wroute.Route.AppName}
-		if err = newapp.Validate(); err != nil {
-			return err
-		}
 
 		err = s.FireBeforeAppCreate(ctx, newapp)
 		if err != nil {