Skip to content

Commit

Permalink
Return 500 server error on database failures (#191)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmattheis
饺子w authored and jmattheis committed May 25, 2019
1 parent 11165fa commit 67493c6
Show file tree
Hide file tree
Showing 30 changed files with 1,123 additions and 485 deletions.
72 changes: 52 additions & 20 deletions api/application.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import (
// The ApplicationDatabase interface for encapsulating database access.
type ApplicationDatabase interface {
CreateApplication(application *model.Application) error
GetApplicationByToken(token string) *model.Application
GetApplicationByID(id uint) *model.Application
GetApplicationsByUser(userID uint) []*model.Application
GetApplicationByToken(token string) (*model.Application, error)
GetApplicationByID(id uint) (*model.Application, error)
GetApplicationsByUser(userID uint) ([]*model.Application, error)
DeleteApplicationByID(id uint) error
UpdateApplication(application *model.Application) error
}
Expand Down Expand Up @@ -68,7 +68,9 @@ func (a *ApplicationAPI) CreateApplication(ctx *gin.Context) {
app.Token = auth.GenerateNotExistingToken(generateApplicationToken, a.applicationExists)
app.UserID = auth.GetUserID(ctx)
app.Internal = false
a.DB.CreateApplication(&app)
if success := successOrAbort(ctx, 500, a.DB.CreateApplication(&app)); !success {
return
}
ctx.JSON(200, withResolvedImage(&app))
}
}
Expand Down Expand Up @@ -99,7 +101,10 @@ func (a *ApplicationAPI) CreateApplication(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ApplicationAPI) GetApplications(ctx *gin.Context) {
userID := auth.GetUserID(ctx)
apps := a.DB.GetApplicationsByUser(userID)
apps, err := a.DB.GetApplicationsByUser(userID)
if success := successOrAbort(ctx, 500, err); !success {
return
}
for _, app := range apps {
withResolvedImage(app)
}
Expand Down Expand Up @@ -142,12 +147,18 @@ func (a *ApplicationAPI) GetApplications(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ApplicationAPI) DeleteApplication(ctx *gin.Context) {
withID(ctx, "id", func(id uint) {
if app := a.DB.GetApplicationByID(id); app != nil && app.UserID == auth.GetUserID(ctx) {
app, err := a.DB.GetApplicationByID(id)
if success := successOrAbort(ctx, 500, err); !success {
return
}
if app != nil && app.UserID == auth.GetUserID(ctx) {
if app.Internal {
ctx.AbortWithError(400, errors.New("cannot delete internal application"))
return
}
a.DB.DeleteApplicationByID(id)
if success := successOrAbort(ctx, 500, a.DB.DeleteApplicationByID(id)); !success {
return
}
if app.Image != "" {
os.Remove(a.ImageDir + app.Image)
}
Expand Down Expand Up @@ -201,15 +212,21 @@ func (a *ApplicationAPI) DeleteApplication(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ApplicationAPI) UpdateApplication(ctx *gin.Context) {
withID(ctx, "id", func(id uint) {
if app := a.DB.GetApplicationByID(id); app != nil && app.UserID == auth.GetUserID(ctx) {
app, err := a.DB.GetApplicationByID(id)
if success := successOrAbort(ctx, 500, err); !success {
return
}
if app != nil && app.UserID == auth.GetUserID(ctx) {
newValues := &model.Application{}
if err := ctx.Bind(newValues); err == nil {
app.Description = newValues.Description
app.Name = newValues.Name

a.DB.UpdateApplication(app)

if success := successOrAbort(ctx, 500, a.DB.UpdateApplication(app)); !success {
return
}
ctx.JSON(200, withResolvedImage(app))

}
} else {
ctx.AbortWithError(404, fmt.Errorf("app with id %d doesn't exists", id))
Expand Down Expand Up @@ -265,7 +282,11 @@ func (a *ApplicationAPI) UpdateApplication(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
withID(ctx, "id", func(id uint) {
if app := a.DB.GetApplicationByID(id); app != nil && app.UserID == auth.GetUserID(ctx) {
app, err := a.DB.GetApplicationByID(id)
if success := successOrAbort(ctx, 500, err); !success {
return
}
if app != nil && app.UserID == auth.GetUserID(ctx) {
file, err := ctx.FormFile("file")
if err == http.ErrMissingFile {
ctx.AbortWithError(400, errors.New("file with key 'file' must be present"))
Expand All @@ -284,12 +305,11 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {

ext := filepath.Ext(file.Filename)

name := generateImageName()
for exist(a.ImageDir + name + ext) {
name = generateImageName()
}
name := generateNonExistingImageName(a.ImageDir, func() string {
return generateImageName() + ext
})

err = ctx.SaveUploadedFile(file, a.ImageDir+name+ext)
err = ctx.SaveUploadedFile(file, a.ImageDir+name)
if err != nil {
ctx.AbortWithError(500, err)
return
Expand All @@ -299,11 +319,13 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
os.Remove(a.ImageDir + app.Image)
}

app.Image = name + ext
a.DB.UpdateApplication(app)
app.Image = name
if success := successOrAbort(ctx, 500, a.DB.UpdateApplication(app)); !success {
return
}
ctx.JSON(200, withResolvedImage(app))
} else {
ctx.AbortWithError(404, fmt.Errorf("client with id %d doesn't exists", id))
ctx.AbortWithError(404, fmt.Errorf("app with id %d doesn't exists", id))
}
})
}
Expand All @@ -318,7 +340,8 @@ func withResolvedImage(app *model.Application) *model.Application {
}

func (a *ApplicationAPI) applicationExists(token string) bool {
return a.DB.GetApplicationByToken(token) != nil
app, _ := a.DB.GetApplicationByToken(token)
return app != nil
}

func exist(path string) bool {
Expand All @@ -327,3 +350,12 @@ func exist(path string) bool {
}
return true
}

func generateNonExistingImageName(imgDir string, gen func() string) string {
for {
name := gen()
if !exist(imgDir + name) {
return name
}
}
}
48 changes: 33 additions & 15 deletions api/application_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ func (s *ApplicationSuite) Test_CreateApplication_mapAllParameters() {
Description: "description_text",
}
assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), expected, s.db.GetApplicationByID(1))
if app, err := s.db.GetApplicationByID(1); assert.NoError(s.T(), err) {
assert.Equal(s.T(), expected, app)
}
}
func (s *ApplicationSuite) Test_ensureApplicationHasCorrectJsonRepresentation() {
actual := &model.Application{
Expand All @@ -96,7 +98,9 @@ func (s *ApplicationSuite) Test_CreateApplication_expectBadRequestOnEmptyName()
s.a.CreateApplication(s.ctx)

assert.Equal(s.T(), 400, s.recorder.Code)
assert.Empty(s.T(), s.db.GetApplicationsByUser(5))
if app, err := s.db.GetApplicationsByUser(5); assert.NoError(s.T(), err) {
assert.Empty(s.T(), app)
}
}

func (s *ApplicationSuite) Test_DeleteApplication_expectNotFoundOnCurrentUserIsNotOwner() {
Expand All @@ -122,7 +126,9 @@ func (s *ApplicationSuite) Test_CreateApplication_onlyRequiredParameters() {

expected := &model.Application{ID: 1, Token: firstApplicationToken, Name: "custom_name", UserID: 5}
assert.Equal(s.T(), 200, s.recorder.Code)
assert.Contains(s.T(), s.db.GetApplicationsByUser(5), expected)
if app, err := s.db.GetApplicationsByUser(5); assert.NoError(s.T(), err) {
assert.Contains(s.T(), app, expected)
}
}

func (s *ApplicationSuite) Test_CreateApplication_returnsApplicationWithID() {
Expand Down Expand Up @@ -155,7 +161,9 @@ func (s *ApplicationSuite) Test_CreateApplication_withExistingToken() {

expected := &model.Application{ID: 2, Token: secondApplicationToken, Name: "custom_name", UserID: 5}
assert.Equal(s.T(), 200, s.recorder.Code)
assert.Contains(s.T(), s.db.GetApplicationsByUser(5), expected)
if app, err := s.db.GetApplicationsByUser(5); assert.NoError(s.T(), err) {
assert.Contains(s.T(), app, expected)
}
}

func (s *ApplicationSuite) Test_GetApplications() {
Expand Down Expand Up @@ -275,16 +283,18 @@ func (s *ApplicationSuite) Test_UploadAppImage_WithImageFile_expectSuccess() {

s.a.UploadApplicationImage(s.ctx)

imgName := s.db.GetApplicationByID(1).Image
if app, err := s.db.GetApplicationByID(1); assert.NoError(s.T(), err) {
imgName := app.Image

assert.Equal(s.T(), 200, s.recorder.Code)
_, err = os.Stat(imgName)
assert.Nil(s.T(), err)
assert.Equal(s.T(), 200, s.recorder.Code)
_, err = os.Stat(imgName)
assert.Nil(s.T(), err)

s.a.DeleteApplication(s.ctx)
s.a.DeleteApplication(s.ctx)

_, err = os.Stat(imgName)
assert.True(s.T(), os.IsNotExist(err))
_, err = os.Stat(imgName)
assert.True(s.T(), os.IsNotExist(err))
}
}

func (s *ApplicationSuite) Test_UploadAppImage_WithImageFile_DeleteExstingImageAndGenerateNewName() {
Expand Down Expand Up @@ -399,7 +409,9 @@ func (s *ApplicationSuite) Test_UpdateApplicationNameAndDescription_expectSucces
}

assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), expected, s.db.GetApplicationByID(2))
if app, err := s.db.GetApplicationByID(2); assert.NoError(s.T(), err) {
assert.Equal(s.T(), expected, app)
}
}

func (s *ApplicationSuite) Test_UpdateApplicationName_expectSuccess() {
Expand All @@ -419,7 +431,9 @@ func (s *ApplicationSuite) Test_UpdateApplicationName_expectSuccess() {
}

assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), expected, s.db.GetApplicationByID(2))
if app, err := s.db.GetApplicationByID(2); assert.NoError(s.T(), err) {
assert.Equal(s.T(), expected, app)
}
}

func (s *ApplicationSuite) Test_UpdateApplication_preservesImage() {
Expand All @@ -434,7 +448,9 @@ func (s *ApplicationSuite) Test_UpdateApplication_preservesImage() {
s.a.UpdateApplication(s.ctx)

assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), "existing.png", s.db.GetApplicationByID(2).Image)
if app, err := s.db.GetApplicationByID(2); assert.NoError(s.T(), err) {
assert.Equal(s.T(), "existing.png", app.Image)
}
}

func (s *ApplicationSuite) Test_UpdateApplication_setEmptyDescription() {
Expand All @@ -449,7 +465,9 @@ func (s *ApplicationSuite) Test_UpdateApplication_setEmptyDescription() {
s.a.UpdateApplication(s.ctx)

assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), "", s.db.GetApplicationByID(2).Description)
if app, err := s.db.GetApplicationByID(2); assert.NoError(s.T(), err) {
assert.Equal(s.T(), "", app.Description)
}
}

func (s *ApplicationSuite) Test_UpdateApplication_expectNotFound() {
Expand Down
37 changes: 26 additions & 11 deletions api/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import (
// The ClientDatabase interface for encapsulating database access.
type ClientDatabase interface {
CreateClient(client *model.Client) error
GetClientByToken(token string) *model.Client
GetClientByID(id uint) *model.Client
GetClientsByUser(userID uint) []*model.Client
GetClientByToken(token string) (*model.Client, error)
GetClientByID(id uint) (*model.Client, error)
GetClientsByUser(userID uint) ([]*model.Client, error)
DeleteClientByID(id uint) error
UpdateClient(client *model.Client) error
}
Expand Down Expand Up @@ -69,13 +69,18 @@ type ClientAPI struct {
// $ref: "#/definitions/Error"
func (a *ClientAPI) UpdateClient(ctx *gin.Context) {
withID(ctx, "id", func(id uint) {
if client := a.DB.GetClientByID(id); client != nil && client.UserID == auth.GetUserID(ctx) {
client, err := a.DB.GetClientByID(id)
if success := successOrAbort(ctx, 500, err); !success {
return
}
if client != nil && client.UserID == auth.GetUserID(ctx) {
newValues := &model.Client{}
if err := ctx.Bind(newValues); err == nil {
client.Name = newValues.Name

a.DB.UpdateClient(client)

if success := successOrAbort(ctx, 500, a.DB.UpdateClient(client)); !success {
return
}
ctx.JSON(200, client)
}
} else {
Expand Down Expand Up @@ -122,7 +127,9 @@ func (a *ClientAPI) CreateClient(ctx *gin.Context) {
if err := ctx.Bind(&client); err == nil {
client.Token = auth.GenerateNotExistingToken(generateClientToken, a.clientExists)
client.UserID = auth.GetUserID(ctx)
a.DB.CreateClient(&client)
if success := successOrAbort(ctx, 500, a.DB.CreateClient(&client)); !success {
return
}
ctx.JSON(200, client)
}
}
Expand Down Expand Up @@ -153,7 +160,10 @@ func (a *ClientAPI) CreateClient(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ClientAPI) GetClients(ctx *gin.Context) {
userID := auth.GetUserID(ctx)
clients := a.DB.GetClientsByUser(userID)
clients, err := a.DB.GetClientsByUser(userID)
if success := successOrAbort(ctx, 500, err); !success {
return
}
ctx.JSON(200, clients)
}

Expand Down Expand Up @@ -193,15 +203,20 @@ func (a *ClientAPI) GetClients(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *ClientAPI) DeleteClient(ctx *gin.Context) {
withID(ctx, "id", func(id uint) {
if client := a.DB.GetClientByID(id); client != nil && client.UserID == auth.GetUserID(ctx) {
client, err := a.DB.GetClientByID(id)
if success := successOrAbort(ctx, 500, err); !success {
return
}
if client != nil && client.UserID == auth.GetUserID(ctx) {
a.NotifyDeleted(client.UserID, client.Token)
a.DB.DeleteClientByID(id)
successOrAbort(ctx, 500, a.DB.DeleteClientByID(id))
} else {
ctx.AbortWithError(404, fmt.Errorf("client with id %d doesn't exists", id))
}
})
}

func (a *ClientAPI) clientExists(token string) bool {
return a.DB.GetClientByToken(token) != nil
client, _ := a.DB.GetClientByToken(token)
return client != nil
}
12 changes: 9 additions & 3 deletions api/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ func (s *ClientSuite) Test_CreateClient_mapAllParameters() {

expected := &model.Client{ID: 1, Token: firstClientToken, UserID: 5, Name: "custom_name"}
assert.Equal(s.T(), 200, s.recorder.Code)
assert.Contains(s.T(), s.db.GetClientsByUser(5), expected)
if clients, err := s.db.GetClientsByUser(5); assert.NoError(s.T(), err) {
assert.Contains(s.T(), clients, expected)
}
}

func (s *ClientSuite) Test_CreateClient_expectBadRequestOnEmptyName() {
Expand All @@ -83,7 +85,9 @@ func (s *ClientSuite) Test_CreateClient_expectBadRequestOnEmptyName() {
s.a.CreateClient(s.ctx)

assert.Equal(s.T(), 400, s.recorder.Code)
assert.Empty(s.T(), s.db.GetClientsByUser(5))
if clients, err := s.db.GetClientsByUser(5); assert.NoError(s.T(), err) {
assert.Empty(s.T(), clients)
}
}

func (s *ClientSuite) Test_DeleteClient_expectNotFoundOnCurrentUserIsNotOwner() {
Expand Down Expand Up @@ -184,7 +188,9 @@ func (s *ClientSuite) Test_UpdateClient_expectSuccess() {
}

assert.Equal(s.T(), 200, s.recorder.Code)
assert.Equal(s.T(), expected, s.db.GetClientByID(1))
if client, err := s.db.GetClientByID(1); assert.NoError(s.T(), err) {
assert.Equal(s.T(), expected, client)
}
}

func (s *ClientSuite) Test_UpdateClient_expectNotFound() {
Expand Down
10 changes: 10 additions & 0 deletions api/errorHandling.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package api

import "github.com/gin-gonic/gin"

func successOrAbort(ctx *gin.Context, code int, err error) (success bool) {
if err != nil {
ctx.AbortWithError(code, err)
}
return err == nil
}
Loading

0 comments on commit 67493c6

Please sign in to comment.