Contents API should return 404 on not exist (#10323)

* Return 404 on not exist

* swagger update and use git.IsErrNotExist

* Handle delete too

* Handle delete too x2

* Fix pr 10323 (#3)

* fix TESTS

* leafe a note for fututre

* placate golangci-lint

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update integrations/api_repo_file_delete_test.go

Co-Authored-By: 6543 <6543@obermui.de>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

Author: 5 people

integrations/api_repo_file_delete_test.go

@@ -11,8 +11,6 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/context"
-	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 
 	"github.com/stretchr/testify/assert"
@@ -109,18 +107,10 @@ func TestAPIDeleteFile(t *testing.T) {
 		treePath = fmt.Sprintf("delete/file%d.txt", fileID)
 		createFile(user2, repo1, treePath)
 		deleteFileOptions = getDeleteFileOptions()
-		correctSHA := deleteFileOptions.SHA
 		deleteFileOptions.SHA = "badsha"
 		url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2)
 		req = NewRequestWithJSON(t, "DELETE", url, &deleteFileOptions)
-		resp = session.MakeRequest(t, req, http.StatusInternalServerError)
-		expectedAPIError := context.APIError{
-			Message: "sha does not match [given: " + deleteFileOptions.SHA + ", expected: " + correctSHA + "]",
-			URL:     setting.API.SwaggerURL,
-		}
-		var apiError context.APIError
-		DecodeJSON(t, resp, &apiError)
-		assert.Equal(t, expectedAPIError, apiError)
+		resp = session.MakeRequest(t, req, http.StatusBadRequest)
 
 		// Test creating a file in repo16 by user4 who does not have write access
 		fileID++

integrations/api_repo_get_contents_list_test.go

@@ -11,7 +11,6 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/git"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
@@ -139,14 +138,7 @@ func testAPIGetContentsList(t *testing.T, u *url.URL) {
 	// Test file contents a file with a bad ref
 	ref = "badref"
 	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?ref=%s", user2.Name, repo1.Name, treePath, ref)
-	resp = session.MakeRequest(t, req, http.StatusInternalServerError)
-	expectedAPIError := context.APIError{
-		Message: "object does not exist [id: " + ref + ", rel_path: ]",
-		URL:     setting.API.SwaggerURL,
-	}
-	var apiError context.APIError
-	DecodeJSON(t, resp, &apiError)
-	assert.Equal(t, expectedAPIError, apiError)
+	resp = session.MakeRequest(t, req, http.StatusNotFound)
 
 	// Test accessing private ref with user token that does not have access - should fail
 	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo16.Name, treePath, token4)

integrations/api_repo_get_contents_test.go

@@ -10,7 +10,6 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/git"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
@@ -141,14 +140,7 @@ func testAPIGetContents(t *testing.T, u *url.URL) {
 	// Test file contents a file with a bad ref
 	ref = "badref"
 	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?ref=%s", user2.Name, repo1.Name, treePath, ref)
-	resp = session.MakeRequest(t, req, http.StatusInternalServerError)
-	expectedAPIError := context.APIError{
-		Message: "object does not exist [id: " + ref + ", rel_path: ]",
-		URL:     setting.API.SwaggerURL,
-	}
-	var apiError context.APIError
-	DecodeJSON(t, resp, &apiError)
-	assert.Equal(t, expectedAPIError, apiError)
+	resp = session.MakeRequest(t, req, http.StatusNotFound)
 
 	// Test accessing private ref with user token that does not have access - should fail
 	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo16.Name, treePath, token4)

routers/api/v1/repo/file.go

@@ -362,9 +362,15 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) {
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/FileDeleteResponse"
+	//   "400":
+	//     "$ref": "#/responses/error"
+	//   "403":
+	//     "$ref": "#/responses/error"
+	//   "404":
+	//     "$ref": "#/responses/error"
 
 	if !CanWriteFiles(ctx.Repo) {
-		ctx.Error(http.StatusInternalServerError, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{
+		ctx.Error(http.StatusForbidden, "DeleteFile", models.ErrUserDoesNotHaveAccessToRepo{
 			UserID:   ctx.User.ID,
 			RepoName: ctx.Repo.Repository.LowerName,
 		})
@@ -402,9 +408,23 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) {
 	}
 
 	if fileResponse, err := repofiles.DeleteRepoFile(ctx.Repo.Repository, ctx.User, opts); err != nil {
+		if git.IsErrBranchNotExist(err) || models.IsErrRepoFileDoesNotExist(err) || git.IsErrNotExist(err) {
+			ctx.Error(http.StatusNotFound, "DeleteFile", err)
+			return
+		} else if models.IsErrBranchAlreadyExists(err) ||
+			models.IsErrFilenameInvalid(err) ||
+			models.IsErrSHADoesNotMatch(err) ||
+			models.IsErrCommitIDDoesNotMatch(err) ||
+			models.IsErrSHAOrCommitIDNotProvided(err) {
+			ctx.Error(http.StatusBadRequest, "DeleteFile", err)
+			return
+		} else if models.IsErrUserCannotCommit(err) {
+			ctx.Error(http.StatusForbidden, "DeleteFile", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "DeleteFile", err)
 	} else {
-		ctx.JSON(http.StatusOK, fileResponse)
+		ctx.JSON(http.StatusOK, fileResponse) // FIXME on APIv2: return http.StatusNoContent
 	}
 }
 
@@ -439,6 +459,8 @@ func GetContents(ctx *context.APIContext) {
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/ContentsResponse"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
 
 	if !CanReadFiles(ctx.Repo) {
 		ctx.Error(http.StatusInternalServerError, "GetContentsOrList", models.ErrUserDoesNotHaveAccessToRepo{
@@ -452,6 +474,10 @@ func GetContents(ctx *context.APIContext) {
 	ref := ctx.QueryTrim("ref")
 
 	if fileList, err := repofiles.GetContentsOrList(ctx.Repo.Repository, treePath, ref); err != nil {
+		if git.IsErrNotExist(err) {
+			ctx.NotFound("GetContentsOrList", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "GetContentsOrList", err)
 	} else {
 		ctx.JSON(http.StatusOK, fileList)
@@ -484,6 +510,8 @@ func GetContentsList(ctx *context.APIContext) {
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/ContentsListResponse"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
 
 	// same as GetContents(), this function is here because swagger fails if path is empty in GetContents() interface
 	GetContents(ctx)

templates/swagger/v1_json.tmpl

@@ -2638,6 +2638,9 @@
         "responses": {
           "200": {
             "$ref": "#/responses/ContentsListResponse"
+          },
+          "404": {
+            "$ref": "#/responses/notFound"
           }
         }
       }
@@ -2684,6 +2687,9 @@
         "responses": {
           "200": {
             "$ref": "#/responses/ContentsResponse"
+          },
+          "404": {
+            "$ref": "#/responses/notFound"
           }
         }
       },
@@ -2831,6 +2837,15 @@
         "responses": {
           "200": {
             "$ref": "#/responses/FileDeleteResponse"
+          },
+          "400": {
+            "$ref": "#/responses/error"
+          },
+          "403": {
+            "$ref": "#/responses/error"
+          },
+          "404": {
+            "$ref": "#/responses/error"
           }
         }
       }