fix(api): use not found status code when appropriate (#6885)

- Use a 404 error when the issue not found instead of returning an internal server error.
- Resolves #4005
- Added integration test.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6885
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: ThomasBoom89 <thomasboom89@noreply.codeberg.org>
Co-committed-by: ThomasBoom89 <thomasboom89@noreply.codeberg.org>

routers/api/v1/repo/issue_comment.go

@@ -62,6 +62,10 @@ func ListIssueComments(ctx *context.APIContext) {
 	//     "$ref": "#/responses/CommentList"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
 
 	before, since, err := context.GetQueryBeforeSince(ctx.Base)
 	if err != nil {
@@ -70,6 +74,10 @@ func ListIssueComments(ctx *context.APIContext) {
 	}
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
+		if issues_model.IsErrIssueNotExist(err) {
+			ctx.NotFound("IsErrIssueNotExist", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "GetRawIssueByIndex", err)
 		return
 	}
@@ -166,6 +174,10 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) {
 	//     "$ref": "#/responses/TimelineList"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
 
 	before, since, err := context.GetQueryBeforeSince(ctx.Base)
 	if err != nil {
@@ -174,6 +186,10 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) {
 	}
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
+		if issues_model.IsErrIssueNotExist(err) {
+			ctx.NotFound("IsErrIssueNotExist", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "GetRawIssueByIndex", err)
 		return
 	}
@@ -271,6 +287,10 @@ func ListRepoIssueComments(ctx *context.APIContext) {
 	//     "$ref": "#/responses/CommentList"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
 
 	before, since, err := context.GetQueryBeforeSince(ctx.Base)
 	if err != nil {
@@ -378,9 +398,16 @@ func CreateIssueComment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
+
 	form := web.GetForm(ctx).(*api.CreateIssueCommentOption)
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
+		if issues_model.IsErrIssueNotExist(err) {
+			ctx.NotFound("IsErrIssueNotExist", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "GetIssueByIndex", err)
 		return
 	}
@@ -449,6 +476,8 @@ func GetIssueComment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
 
 	comment := ctx.Comment
 
@@ -511,6 +540,9 @@ func EditIssueComment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
+
 	form := web.GetForm(ctx).(*api.EditIssueCommentOption)
 	editIssueComment(ctx, *form)
 }
@@ -560,6 +592,8 @@ func EditIssueCommentDeprecated(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "500":
+	//     "$ref": "#/responses/internalServerError"
 
 	form := web.GetForm(ctx).(*api.EditIssueCommentOption)
 	editIssueComment(ctx, *form)
@@ -626,8 +660,8 @@ func DeleteIssueComment(ctx *context.APIContext) {
 	//     "$ref": "#/responses/empty"
 	//   "403":
 	//     "$ref": "#/responses/forbidden"
-	//   "404":
+	//   "500":
-	//     "$ref": "#/responses/notFound"
+	//     "$ref": "#/responses/internalServerError"
 
 	deleteIssueComment(ctx, issues_model.CommentTypeComment)
 }
@@ -665,8 +699,8 @@ func DeleteIssueCommentDeprecated(ctx *context.APIContext) {
 	//     "$ref": "#/responses/empty"
 	//   "403":
 	//     "$ref": "#/responses/forbidden"
-	//   "404":
+	//   "500":
-	//     "$ref": "#/responses/notFound"
+	//     "$ref": "#/responses/internalServerError"
 
 	deleteIssueComment(ctx, issues_model.CommentTypeComment)
 }

services/context/api.go

@@ -157,6 +157,17 @@ type swaggerAPIRepoArchivedError struct {
 	Body APIRepoArchivedError `json:"body"`
 }
 
+type APIInternalServerError struct {
+	APIError
+}
+
+// APIInternalServerError is an error that is raised when an internal server error occurs
+// swagger:response internalServerError
+type swaggerAPIInternalServerError struct {
+	// in:body
+	Body APIInternalServerError `json:"body"`
+}
+
 // ServerError responds with error message, status is 500
 func (ctx *APIContext) ServerError(title string, err error) {
 	ctx.Error(http.StatusInternalServerError, title, err)

templates/swagger/v1_json.tmpl

@@ -8667,6 +8667,12 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/validationError"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       }
@@ -8720,6 +8726,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       },
@@ -8760,8 +8769,8 @@
           "403": {
             "$ref": "#/responses/forbidden"
           },
-          "404": {
+          "500": {
-            "$ref": "#/responses/notFound"
+            "$ref": "#/responses/internalServerError"
           }
         }
       },
@@ -8823,6 +8832,9 @@
           },
           "423": {
             "$ref": "#/responses/repoArchivedError"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       }
@@ -9959,6 +9971,12 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/validationError"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       },
@@ -10017,6 +10035,9 @@
           },
           "423": {
             "$ref": "#/responses/repoArchivedError"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       }
@@ -10067,8 +10088,8 @@
           "403": {
             "$ref": "#/responses/forbidden"
           },
-          "404": {
+          "500": {
-            "$ref": "#/responses/notFound"
+            "$ref": "#/responses/internalServerError"
           }
         }
       },
@@ -10135,6 +10156,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       }
@@ -11386,6 +11410,12 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/validationError"
+          },
+          "500": {
+            "$ref": "#/responses/internalServerError"
           }
         }
       }
@@ -20453,6 +20483,20 @@
       },
       "x-go-package": "code.gitea.io/gitea/services/context"
     },
+    "APIInternalServerError": {
+      "type": "object",
+      "properties": {
+        "message": {
+          "type": "string",
+          "x-go-name": "Message"
+        },
+        "url": {
+          "type": "string",
+          "x-go-name": "URL"
+        }
+      },
+      "x-go-package": "code.gitea.io/gitea/services/context"
+    },
     "APIInvalidTopicsError": {
       "type": "object",
       "properties": {
@@ -29165,6 +29209,12 @@
         "$ref": "#/definitions/APIForbiddenError"
       }
     },
+    "internalServerError": {
+      "description": "APIInternalServerError is an error that is raised when an internal server error occurs",
+      "schema": {
+        "$ref": "#/definitions/APIInternalServerError"
+      }
+    },
     "invalidTopicsError": {
       "description": "APIInvalidTopicsError is error format response to invalid topics",
       "schema": {

tests/integration/api_comment_test.go

@@ -25,6 +25,8 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+const IssueIDNotExist = 10000
+
 func TestAPIListRepoComments(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
@@ -89,6 +91,10 @@ func TestAPIListIssueComments(t *testing.T) {
 	expectedCount := unittest.GetCount(t, &issues_model.Comment{IssueID: issue.ID},
 		unittest.Cond("type = ?", issues_model.CommentTypeComment))
 	assert.Len(t, comments, expectedCount)
+
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/%d/comments", repoOwner.Name, repo.Name, IssueIDNotExist).
+		AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusNotFound)
 }
 
 func TestAPICreateComment(t *testing.T) {
@@ -111,6 +117,13 @@ func TestAPICreateComment(t *testing.T) {
 	DecodeJSON(t, resp, &updatedComment)
 	assert.EqualValues(t, commentBody, updatedComment.Body)
 	unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: updatedComment.ID, IssueID: issue.ID, Content: commentBody})
+
+	urlStr = fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/comments",
+		repoOwner.Name, repo.Name, IssueIDNotExist)
+	req = NewRequestWithValues(t, "POST", urlStr, map[string]string{
+		"body": commentBody,
+	}).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusNotFound)
 }
 
 func TestAPICreateCommentAutoDate(t *testing.T) {
@@ -464,4 +477,7 @@ func TestAPIListIssueTimeline(t *testing.T) {
 	DecodeJSON(t, resp, &comments)
 	expectedCount := unittest.GetCount(t, &issues_model.Comment{IssueID: issue.ID})
 	assert.Len(t, comments, expectedCount)
+
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/%d/timeline", repoOwner.Name, repo.Name, IssueIDNotExist)
+	MakeRequest(t, req, http.StatusNotFound)
 }