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

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

Signed-off-by: Andrew Thornton <art27@cantab.net>
* Update integrations/api_oauth2_apps_test.go

* Fix more missed sess.Close

* Remove unnecessary blank line

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 9e44f32e5461eadd27a527151f2422da77c4a571..28b6707473733e3d79bc05df9f592b13b64542ff 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 1b544e4e9e195e9edca457357eb3aec316b026c0..679fdb18f957deceed24dda482c44335b0ff4739 100644 (file)
--- a/models/oauth2_application.go
+++ b/models/oauth2_application.go
@@ -235,7 +235,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
@@ -261,6 +261,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 33b27d60e0c1f638728134631ef2f6f25862cb92..b88ed6fbd2726fcd0d05f308a55eb02643cacca4 100644 (file)
--- a/routers/api/v1/user/app.go
+++ b/routers/api/v1/user/app.go
@@ -274,7 +274,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
        }