From 7a9591fd45e782cca2b0300de4c1165edbf89d1a Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Thu, 21 Sep 2017 00:52:33 +0300 Subject: [PATCH 1/4] Split queries to make them work on Postgres and MySQL Only SQLite supports multiple deletes in one transaction/statement, but other are not. --- api/datastore/sql/sql.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/api/datastore/sql/sql.go b/api/datastore/sql/sql.go index 62f1e0758..6cf36f45f 100644 --- a/api/datastore/sql/sql.go +++ b/api/datastore/sql/sql.go @@ -222,10 +222,7 @@ func (ds *sqlStore) UpdateApp(ctx context.Context, newapp *models.App) (*models. func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { res, err := ds.db.ExecContext(ctx, ds.db.Rebind( - `DELETE FROM apps WHERE name = ?; - DELETE FROM logs WHERE app_name=?; - DELETE FROM calls WHERE app_name=?; - DELETE FROM routes WHERE app_name=?;`), appName, appName, appName, appName) + `DELETE FROM apps WHERE name = ?`), appName) if err != nil { return err } @@ -233,7 +230,21 @@ func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { if err == sql.ErrNoRows { return models.ErrAppsNotFound } - return err + + deletes := []string{ + `DELETE FROM logs WHERE app_name=?`, + `DELETE FROM calls WHERE app_name=?`, + `DELETE FROM routes WHERE app_name=?`, + } + + for _, stmt := range deletes { + _, err = ds.db.ExecContext(ctx, ds.db.Rebind(stmt), appName) + if err != nil { + return err + } + } + + return nil } func (ds *sqlStore) GetApp(ctx context.Context, name string) (*models.App, error) { From a09159308c437216496053171eab533d0992ee29 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Thu, 21 Sep 2017 01:21:41 +0300 Subject: [PATCH 2/4] Run queries inside one transaction --- api/datastore/sql/sql.go | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/api/datastore/sql/sql.go b/api/datastore/sql/sql.go index 6cf36f45f..560f4345a 100644 --- a/api/datastore/sql/sql.go +++ b/api/datastore/sql/sql.go @@ -221,30 +221,32 @@ func (ds *sqlStore) UpdateApp(ctx context.Context, newapp *models.App) (*models. } func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { - res, err := ds.db.ExecContext(ctx, ds.db.Rebind( - `DELETE FROM apps WHERE name = ?`), appName) - if err != nil { - return err - } - _, err = res.RowsAffected() - if err == sql.ErrNoRows { - return models.ErrAppsNotFound - } - - deletes := []string{ - `DELETE FROM logs WHERE app_name=?`, - `DELETE FROM calls WHERE app_name=?`, - `DELETE FROM routes WHERE app_name=?`, - } - - for _, stmt := range deletes { - _, err = ds.db.ExecContext(ctx, ds.db.Rebind(stmt), appName) + return ds.Tx(func(tx *sqlx.Tx) error { + res, err := ds.db.ExecContext(ctx, ds.db.Rebind( + `DELETE FROM apps WHERE name = ?`), appName) if err != nil { return err } - } + _, err = res.RowsAffected() + if err == sql.ErrNoRows { + return models.ErrAppsNotFound + } - return nil + deletes := []string{ + `DELETE FROM logs WHERE app_name=?`, + `DELETE FROM calls WHERE app_name=?`, + `DELETE FROM routes WHERE app_name=?`, + } + + for _, stmt := range deletes { + _, err = ds.db.ExecContext(ctx, ds.db.Rebind(stmt), appName) + if err != nil { + return err + } + } + + return nil + }) } func (ds *sqlStore) GetApp(ctx context.Context, name string) (*models.App, error) { From 830c86efe779c41315cd67b36ab6e53145e85ed8 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Thu, 21 Sep 2017 02:04:27 +0300 Subject: [PATCH 3/4] Make app delete more stable We still need to delete app and check how may rows affected in apps table. But we don't really care about other tables and rows affected there: routes, calls, logs We need to delete app out of loop to check for invalid numbers of rows affected: - zero rows means nothing happend Despite apps table, zero rows affected if valid case for routes, calls and logs --- api/datastore/sql/sql.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/api/datastore/sql/sql.go b/api/datastore/sql/sql.go index 560f4345a..90416ea4e 100644 --- a/api/datastore/sql/sql.go +++ b/api/datastore/sql/sql.go @@ -222,13 +222,13 @@ func (ds *sqlStore) UpdateApp(ctx context.Context, newapp *models.App) (*models. func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { return ds.Tx(func(tx *sqlx.Tx) error { - res, err := ds.db.ExecContext(ctx, ds.db.Rebind( - `DELETE FROM apps WHERE name = ?`), appName) + res, err := tx.ExecContext(ctx, tx.Rebind(`DELETE FROM apps WHERE name=?`), appName) + n, err := res.RowsAffected() if err != nil { return err } - _, err = res.RowsAffected() - if err == sql.ErrNoRows { + + if n == 0 { return models.ErrAppsNotFound } @@ -239,12 +239,11 @@ func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { } for _, stmt := range deletes { - _, err = ds.db.ExecContext(ctx, ds.db.Rebind(stmt), appName) + _, err := tx.ExecContext(ctx, tx.Rebind(stmt), appName) if err != nil { return err } } - return nil }) } From f4699ea0babe66581770582c3d2642bc4ba448dc Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Thu, 21 Sep 2017 02:25:36 +0300 Subject: [PATCH 4/4] Adding API tests that verifies recently changed code bevaviour --- api/datastore/sql/sql.go | 4 +++- test/fn-api-tests/apps_api.go | 3 +++ test/fn-api-tests/apps_test.go | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/api/datastore/sql/sql.go b/api/datastore/sql/sql.go index 90416ea4e..f79b64fc7 100644 --- a/api/datastore/sql/sql.go +++ b/api/datastore/sql/sql.go @@ -223,11 +223,13 @@ func (ds *sqlStore) UpdateApp(ctx context.Context, newapp *models.App) (*models. func (ds *sqlStore) RemoveApp(ctx context.Context, appName string) error { return ds.Tx(func(tx *sqlx.Tx) error { res, err := tx.ExecContext(ctx, tx.Rebind(`DELETE FROM apps WHERE name=?`), appName) + if err != nil { + return err + } n, err := res.RowsAffected() if err != nil { return err } - if n == 0 { return models.ErrAppsNotFound } diff --git a/test/fn-api-tests/apps_api.go b/test/fn-api-tests/apps_api.go index 6145ea34e..cbfeffe15 100644 --- a/test/fn-api-tests/apps_api.go +++ b/test/fn-api-tests/apps_api.go @@ -14,6 +14,9 @@ import ( func CheckAppResponseError(t *testing.T, e error) { if e != nil { switch err := e.(type) { + case *apps.DeleteAppsAppNotFound: + t.Errorf("Unexpected error occurred: %v Original Location: %s", err.Payload.Error.Message, MyCaller()) + t.FailNow() case *apps.DeleteAppsAppDefault: t.Errorf("Unexpected error occurred: %v. Status code: %v Orig Location: %s", err.Payload.Error.Message, err.Code(), MyCaller()) t.FailNow() diff --git a/test/fn-api-tests/apps_test.go b/test/fn-api-tests/apps_test.go index c898d0862..fd0192fa1 100644 --- a/test/fn-api-tests/apps_test.go +++ b/test/fn-api-tests/apps_test.go @@ -11,6 +11,20 @@ import ( func TestApps(t *testing.T) { + t.Run("delete-app-not-found-test", func(t *testing.T) { + t.Parallel() + s := SetupDefaultSuite() + cfg := &apps.DeleteAppsAppParams{ + App: "missing-app", + Context: s.Context, + } + cfg.WithTimeout(time.Second * 60) + _, err := s.Client.Apps.DeleteAppsApp(cfg) + if err == nil { + t.Errorf("Error during app delete: we should get HTTP 404, but got: %s", err.Error()) + } + }) + t.Run("app-not-found-test", func(t *testing.T) { t.Parallel() s := SetupDefaultSuite()