Fix delete nonexist oauth application 500 and prevent deadlock (#15384) (#15397)

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

* Fix more missed sess.Close

* Remove unnecessary blank line

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: 6543 <6543@obermui.de>

diff --git a/integrations/api_oauth2_apps_test.go b/integrations/api_oauth2_apps_test.go
index 998043a6fb0693b94b8ba822d6cc6019dd8fdebe..0ba56b6c9fca85dcfa67e3238d9633f5ef044192 100644 (file)
--- a/integrations/api_oauth2_apps_test.go
+++ b/integrations/api_oauth2_apps_test.go
@@ -92,6 +92,10 @@ func testAPIDeleteOAuth2Application(t *testing.T) {
        session.MakeRequest(t, req, http.StatusNoContent)
 
        models.AssertNotExistsBean(t, &models.OAuth2Application{UID: oldApp.UID, Name: oldApp.Name})
+
+       // Delete again will return not found
+       req = NewRequest(t, "DELETE", urlStr)
+       session.MakeRequest(t, req, http.StatusNotFound)
 }
 
 func testAPIGetOAuth2Application(t *testing.T) {

diff --git a/models/migrate.go b/models/migrate.go
index 2715c5bd9b01b78e57f9e2e848dea5725916e9d0..0b9d7da1ffe27b58b6fe5796a31cb08ad7ae3fd5 100644 (file)
--- a/models/migrate.go
+++ b/models/migrate.go
@@ -39,6 +39,7 @@ func InsertMilestones(ms ...*Milestone) (err error) {
 // InsertIssues insert issues to database
 func InsertIssues(issues ...*Issue) error {
        sess := x.NewSession()
+       defer sess.Close()
        if err := sess.Begin(); err != nil {
                return err
        }
@@ -194,6 +195,7 @@ func InsertPullRequests(prs ...*PullRequest) error {
 // InsertReleases migrates release
 func InsertReleases(rels ...*Release) error {
        sess := x.NewSession()
+       defer sess.Close()
        if err := sess.Begin(); err != nil {
                return err
        }

diff --git a/models/oauth2_application.go b/models/oauth2_application.go
index 1c7937685c86d669613339cf814bcb4e842ec137..e2f8753517573128e3f312db18d0c064d5255cca 100644 (file)
--- a/models/oauth2_application.go
+++ b/models/oauth2_application.go
@@ -233,7 +233,7 @@ func deleteOAuth2Application(sess *xorm.Session, id, userid int64) error {
        if deleted, err := sess.Delete(&OAuth2Application{ID: id, UID: userid}); err != nil {
                return err
        } else if deleted == 0 {
-               return fmt.Errorf("cannot find oauth2 application")
+               return ErrOAuthApplicationNotFound{ID: id}
        }
        codes := make([]*OAuth2AuthorizationCode, 0)
        // delete correlating auth codes
@@ -259,6 +259,7 @@ func deleteOAuth2Application(sess *xorm.Session, id, userid int64) error {
 // DeleteOAuth2Application deletes the application with the given id and the grants and auth codes related to it. It checks if the userid was the creator of the app.
 func DeleteOAuth2Application(id, userid int64) error {
        sess := x.NewSession()
+       defer sess.Close()
        if err := sess.Begin(); err != nil {
                return err
        }

diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go
index d02b8cea21e2e33b87fdca1dc941a0dc5f52c168..e5cea556a350709045b1453ca6cbd15498cd23f2 100644 (file)
--- a/routers/api/v1/user/app.go
+++ b/routers/api/v1/user/app.go
@@ -268,7 +268,11 @@ func DeleteOauth2Application(ctx *context.APIContext) {
        //     "$ref": "#/responses/empty"
        appID := ctx.ParamsInt64(":id")
        if err := models.DeleteOAuth2Application(appID, ctx.User.ID); err != nil {
-               ctx.Error(http.StatusInternalServerError, "DeleteOauth2ApplicationByID", err)
+               if models.IsErrOAuthApplicationNotFound(err) {
+                       ctx.NotFound()
+               } else {
+                       ctx.Error(http.StatusInternalServerError, "DeleteOauth2ApplicationByID", err)
+               }
                return
        }