Allow multiple files in generic packages (#20661)

* Allow multiple files in generic packages. * Add deletion of a single file. * Update docs. * Change version check. Co-authored-by: silverwind <me@silverwind.io> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
2022-08-09 06:39:24 +02:00 · 2022-08-09 06:39:24 +02:00 · cc6927b2d8
commit cc6927b2d8
parent 75d96f4a02
4 changed files with 254 additions and 76 deletions
--- a/docs/content/doc/packages/generic.en-us.md
+++ b/docs/content/doc/packages/generic.en-us.md
@ -27,7 +27,7 @@ To authenticate to the Package Registry, you need to provide [custom HTTP header
 ## Publish a package

 To publish a generic package perform a HTTP PUT operation with the package content in the request body.
-You cannot publish a package if a package of the same name and version already exists. You must delete the existing package first.
+You cannot publish a file with the same name twice to a package. You must delete the existing package version first.

 ```
 PUT https://gitea.example.com/api/packages/{owner}/generic/{package_name}/{package_version}/{file_name}
@ -36,9 +36,9 @@ PUT https://gitea.example.com/api/packages/{owner}/generic/{package_name}/{packa
 | Parameter         | Description |
 | ----------------- | ----------- |
 | `owner`           | The owner of the package. |
-| `package_name`    | The package name. It can contain only lowercase letters (`a-z`), uppercase letter (`A-Z`), numbers (`0-9`), dots (`.`), hyphens (`-`), or underscores (`_`). |
-| `package_version` | The package version, a non-empty string. |
-| `file_name`       | The filename. It can contain only lowercase letters (`a-z`), uppercase letter (`A-Z`), numbers (`0-9`), dots (`.`), hyphens (`-`), or underscores (`_`). |
+| `package_name`    | The package name. It can contain only lowercase letters (`a-z`), uppercase letter (`A-Z`), numbers (`0-9`), dots (`.`), hyphens (`-`), pluses (`+`), or underscores (`_`). |
+| `package_version` | The package version, a non-empty string without trailing or leading whitespaces. |
+| `file_name`       | The filename. It can contain only lowercase letters (`a-z`), uppercase letter (`A-Z`), numbers (`0-9`), dots (`.`), hyphens (`-`), pluses (`+`), or underscores (`_`). |

 Example request using HTTP Basic authentication:

@ -55,7 +55,8 @@ The server reponds with the following HTTP Status codes.
 | HTTP Status Code  | Meaning |
 | ----------------- | ------- |
 | `201 Created`     | The package has been published. |
-| `400 Bad Request` | The package name and/or version are invalid or a package with the same name and version already exist. |
+| `400 Bad Request` | The package name and/or version and/or file name are invalid. |
+| `409 Conflict`    | A file with the same name exist already in the package. |

 ## Download a package

@ -80,3 +81,67 @@ Example request using HTTP Basic authentication:
 curl --user your_username:your_token_or_password \
     https://gitea.example.com/api/packages/testuser/generic/test_package/1.0.0/file.bin
 ```
+
+The server reponds with the following HTTP Status codes.
+
+| HTTP Status Code  | Meaning |
+| ----------------- | ------- |
+| `200 OK`          | Success |
+| `404 Not Found`   | The package or file was not found. |
+
+## Delete a package
+
+To delete a generic package perform a HTTP DELETE operation. This will delete all files of this version.
+
+```
+DELETE https://gitea.example.com/api/packages/{owner}/generic/{package_name}/{package_version}
+```
+
+| Parameter         | Description |
+| ----------------- | ----------- |
+| `owner`           | The owner of the package. |
+| `package_name`    | The package name. |
+| `package_version` | The package version. |
+
+Example request using HTTP Basic authentication:
+
+```shell
+curl --user your_username:your_token_or_password -X DELETE \
+     https://gitea.example.com/api/packages/testuser/generic/test_package/1.0.0
+```
+
+The server reponds with the following HTTP Status codes.
+
+| HTTP Status Code  | Meaning |
+| ----------------- | ------- |
+| `204 No Content`  | Success |
+| `404 Not Found`   | The package was not found. |
+
+## Delete a package file
+
+To delete a file of a generic package perform a HTTP DELETE operation. This will delete the package version too if there is no file left.
+
+```
+DELETE https://gitea.example.com/api/packages/{owner}/generic/{package_name}/{package_version}/{filename}
+```
+
+| Parameter         | Description |
+| ----------------- | ----------- |
+| `owner`           | The owner of the package. |
+| `package_name`    | The package name. |
+| `package_version` | The package version. |
+| `filename`        | The filename. |
+
+Example request using HTTP Basic authentication:
+
+```shell
+curl --user your_username:your_token_or_password -X DELETE \
+     https://gitea.example.com/api/packages/testuser/generic/test_package/1.0.0/file.bin
+```
+
+The server reponds with the following HTTP Status codes.
+
+| HTTP Status Code  | Meaning |
+| ----------------- | ------- |
+| `204 No Content`  | Success |
+| `404 Not Found`   | The package or file was not found. |
--- a/integrations/api_packages_generic_test.go
+++ b/integrations/api_packages_generic_test.go
@ -23,16 +23,16 @@ func TestPackageGeneric(t *testing.T) {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User)

 	packageName := "te-st_pac.kage"
-	packageVersion := "1.0.3"
+	packageVersion := "1.0.3-te st"
 	filename := "fi-le_na.me"
 	content := []byte{1, 2, 3}

-	url := fmt.Sprintf("/api/packages/%s/generic/%s/%s/%s", user.Name, packageName, packageVersion, filename)
+	url := fmt.Sprintf("/api/packages/%s/generic/%s/%s", user.Name, packageName, packageVersion)

 	t.Run("Upload", func(t *testing.T) {
 		defer PrintCurrentTest(t)()

-		req := NewRequestWithBody(t, "PUT", url, bytes.NewReader(content))
+		req := NewRequestWithBody(t, "PUT", url+"/"+filename, bytes.NewReader(content))
 		AddBasicAuthHeader(req, user.Name)
 		MakeRequest(t, req, http.StatusCreated)

@ -55,54 +55,139 @@ func TestPackageGeneric(t *testing.T) {
 		pb, err := packages.GetBlobByID(db.DefaultContext, pfs[0].BlobID)
 		assert.NoError(t, err)
 		assert.Equal(t, int64(len(content)), pb.Size)
-	})

-	t.Run("UploadExists", func(t *testing.T) {
-		defer PrintCurrentTest(t)()
+		t.Run("Exists", func(t *testing.T) {
+			defer PrintCurrentTest(t)()

-		req := NewRequestWithBody(t, "PUT", url, bytes.NewReader(content))
-		AddBasicAuthHeader(req, user.Name)
-		MakeRequest(t, req, http.StatusBadRequest)
+			req := NewRequestWithBody(t, "PUT", url+"/"+filename, bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusConflict)
+		})
+
+		t.Run("Additional", func(t *testing.T) {
+			defer PrintCurrentTest(t)()
+
+			req := NewRequestWithBody(t, "PUT", url+"/dummy.bin", bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusCreated)
+
+			// Check deduplication
+			pfs, err := packages.GetFilesByVersionID(db.DefaultContext, pvs[0].ID)
+			assert.NoError(t, err)
+			assert.Len(t, pfs, 2)
+			assert.Equal(t, pfs[0].BlobID, pfs[1].BlobID)
+		})
+
+		t.Run("InvalidParameter", func(t *testing.T) {
+			defer PrintCurrentTest(t)()
+
+			req := NewRequestWithBody(t, "PUT", fmt.Sprintf("/api/packages/%s/generic/%s/%s/%s", user.Name, "invalid+package name", packageVersion, filename), bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusBadRequest)
+
+			req = NewRequestWithBody(t, "PUT", fmt.Sprintf("/api/packages/%s/generic/%s/%s/%s", user.Name, packageName, "%20test ", filename), bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusBadRequest)
+
+			req = NewRequestWithBody(t, "PUT", fmt.Sprintf("/api/packages/%s/generic/%s/%s/%s", user.Name, packageName, packageVersion, "inval+id.na me"), bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusBadRequest)
+		})
 	})

 	t.Run("Download", func(t *testing.T) {
 		defer PrintCurrentTest(t)()

-		req := NewRequest(t, "GET", url)
+		checkDownloadCount := func(count int64) {
+			pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
+			assert.NoError(t, err)
+			assert.Len(t, pvs, 1)
+			assert.Equal(t, count, pvs[0].DownloadCount)
+		}
+
+		checkDownloadCount(0)
+
+		req := NewRequest(t, "GET", url+"/"+filename)
 		resp := MakeRequest(t, req, http.StatusOK)

 		assert.Equal(t, content, resp.Body.Bytes())

-		pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
-		assert.NoError(t, err)
-		assert.Len(t, pvs, 1)
-		assert.Equal(t, int64(1), pvs[0].DownloadCount)
+		checkDownloadCount(1)
+
+		req = NewRequest(t, "GET", url+"/dummy.bin")
+		MakeRequest(t, req, http.StatusOK)
+
+		checkDownloadCount(2)
+
+		t.Run("NotExists", func(t *testing.T) {
+			defer PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", url+"/not.found")
+			MakeRequest(t, req, http.StatusNotFound)
+		})
 	})

 	t.Run("Delete", func(t *testing.T) {
 		defer PrintCurrentTest(t)()

-		req := NewRequest(t, "DELETE", url)
-		AddBasicAuthHeader(req, user.Name)
-		MakeRequest(t, req, http.StatusOK)
+		t.Run("File", func(t *testing.T) {
+			defer PrintCurrentTest(t)()

-		pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
-		assert.NoError(t, err)
-		assert.Empty(t, pvs)
-	})
+			req := NewRequest(t, "DELETE", url+"/"+filename)
+			MakeRequest(t, req, http.StatusUnauthorized)

-	t.Run("DownloadNotExists", func(t *testing.T) {
-		defer PrintCurrentTest(t)()
+			req = NewRequest(t, "DELETE", url+"/"+filename)
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusNoContent)

-		req := NewRequest(t, "GET", url)
-		MakeRequest(t, req, http.StatusNotFound)
-	})
+			req = NewRequest(t, "GET", url+"/"+filename)
+			MakeRequest(t, req, http.StatusNotFound)

-	t.Run("DeleteNotExists", func(t *testing.T) {
-		defer PrintCurrentTest(t)()
+			req = NewRequest(t, "DELETE", url+"/"+filename)
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusNotFound)

-		req := NewRequest(t, "DELETE", url)
-		AddBasicAuthHeader(req, user.Name)
-		MakeRequest(t, req, http.StatusNotFound)
+			pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
+			assert.NoError(t, err)
+			assert.Len(t, pvs, 1)
+
+			t.Run("RemovesVersion", func(t *testing.T) {
+				defer PrintCurrentTest(t)()
+
+				req = NewRequest(t, "DELETE", url+"/dummy.bin")
+				AddBasicAuthHeader(req, user.Name)
+				MakeRequest(t, req, http.StatusNoContent)
+
+				pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
+				assert.NoError(t, err)
+				assert.Empty(t, pvs)
+			})
+		})
+
+		t.Run("Version", func(t *testing.T) {
+			defer PrintCurrentTest(t)()
+
+			req := NewRequestWithBody(t, "PUT", url+"/"+filename, bytes.NewReader(content))
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusCreated)
+
+			req = NewRequest(t, "DELETE", url)
+			MakeRequest(t, req, http.StatusUnauthorized)
+
+			req = NewRequest(t, "DELETE", url)
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusNoContent)
+
+			pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeGeneric)
+			assert.NoError(t, err)
+			assert.Empty(t, pvs)
+
+			req = NewRequest(t, "GET", url+"/"+filename)
+			MakeRequest(t, req, http.StatusNotFound)
+
+			req = NewRequest(t, "DELETE", url)
+			AddBasicAuthHeader(req, user.Name)
+			MakeRequest(t, req, http.StatusNotFound)
+		})
 	})
 }
--- a/routers/api/packages/api.go
+++ b/routers/api/packages/api.go
@ -156,12 +156,15 @@ func Routes() *web.Route {
 			})
 		})
 		r.Group("/generic", func() {
-			r.Group("/{packagename}/{packageversion}/{filename}", func() {
-				r.Get("", generic.DownloadPackageFile)
-				r.Group("", func() {
-					r.Put("", generic.UploadPackage)
-					r.Delete("", generic.DeletePackage)
-				}, reqPackageAccess(perm.AccessModeWrite))
+			r.Group("/{packagename}/{packageversion}", func() {
+				r.Delete("", reqPackageAccess(perm.AccessModeWrite), generic.DeletePackage)
+				r.Group("/{filename}", func() {
+					r.Get("", generic.DownloadPackageFile)
+					r.Group("", func() {
+						r.Put("", generic.UploadPackage)
+						r.Delete("", generic.DeletePackageFile)
+					}, reqPackageAccess(perm.AccessModeWrite))
+				})
 			})
 		})
 		r.Group("/helm", func() {
--- a/routers/api/packages/generic/generic.go
+++ b/routers/api/packages/generic/generic.go
@ -31,22 +31,16 @@ func apiError(ctx *context.Context, status int, obj interface{}) {

 // DownloadPackageFile serves the specific generic package.
 func DownloadPackageFile(ctx *context.Context) {
-	packageName, packageVersion, filename, err := sanitizeParameters(ctx)
-	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
-		return
-	}
-
 	s, pf, err := packages_service.GetFileStreamByPackageNameAndVersion(
 		ctx,
 		&packages_service.PackageInfo{
 			Owner:       ctx.Package.Owner,
 			PackageType: packages_model.TypeGeneric,
-			Name:        packageName,
-			Version:     packageVersion,
+			Name:        ctx.Params("packagename"),
+			Version:     ctx.Params("packageversion"),
 		},
 		&packages_service.PackageFileInfo{
-			Filename: filename,
+			Filename: ctx.Params("filename"),
 		},
 	)
 	if err != nil {
@ -65,9 +59,17 @@ func DownloadPackageFile(ctx *context.Context) {
 // UploadPackage uploads the specific generic package.
 // Duplicated packages get rejected.
 func UploadPackage(ctx *context.Context) {
-	packageName, packageVersion, filename, err := sanitizeParameters(ctx)
-	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
+	packageName := ctx.Params("packagename")
+	filename := ctx.Params("filename")
+
+	if !packageNameRegex.MatchString(packageName) || !filenameRegex.MatchString(filename) {
+		apiError(ctx, http.StatusBadRequest, errors.New("Invalid package name or filename"))
+		return
+	}
+
+	packageVersion := ctx.Params("packageversion")
+	if packageVersion != strings.TrimSpace(packageVersion) {
+		apiError(ctx, http.StatusBadRequest, errors.New("Invalid package version"))
 		return
 	}

@ -88,7 +90,7 @@ func UploadPackage(ctx *context.Context) {
 	}
 	defer buf.Close()

-	_, _, err = packages_service.CreatePackageAndAddFile(
+	_, _, err = packages_service.CreatePackageOrAddFileToExisting(
 		&packages_service.PackageCreationInfo{
 			PackageInfo: packages_service.PackageInfo{
 				Owner:       ctx.Package.Owner,
@ -107,8 +109,8 @@ func UploadPackage(ctx *context.Context) {
 		},
 	)
 	if err != nil {
-		if err == packages_model.ErrDuplicatePackageVersion {
-			apiError(ctx, http.StatusBadRequest, err)
+		if err == packages_model.ErrDuplicatePackageFile {
+			apiError(ctx, http.StatusConflict, err)
 			return
 		}
 		apiError(ctx, http.StatusInternalServerError, err)
@ -120,19 +122,13 @@ func UploadPackage(ctx *context.Context) {

 // DeletePackage deletes the specific generic package.
 func DeletePackage(ctx *context.Context) {
-	packageName, packageVersion, _, err := sanitizeParameters(ctx)
-	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
-		return
-	}
-
-	err = packages_service.RemovePackageVersionByNameAndVersion(
+	err := packages_service.RemovePackageVersionByNameAndVersion(
 		ctx.Doer,
 		&packages_service.PackageInfo{
 			Owner:       ctx.Package.Owner,
 			PackageType: packages_model.TypeGeneric,
-			Name:        packageName,
-			Version:     packageVersion,
+			Name:        ctx.Params("packagename"),
+			Version:     ctx.Params("packageversion"),
 		},
 	)
 	if err != nil {
@ -144,21 +140,50 @@ func DeletePackage(ctx *context.Context) {
 		return
 	}

-	ctx.Status(http.StatusOK)
+	ctx.Status(http.StatusNoContent)
 }

-func sanitizeParameters(ctx *context.Context) (string, string, string, error) {
-	packageName := ctx.Params("packagename")
-	filename := ctx.Params("filename")
+// DeletePackageFile deletes the specific file of a generic package.
+func DeletePackageFile(ctx *context.Context) {
+	pv, pf, err := func() (*packages_model.PackageVersion, *packages_model.PackageFile, error) {
+		pv, err := packages_model.GetVersionByNameAndVersion(ctx, ctx.Package.Owner.ID, packages_model.TypeGeneric, ctx.Params("packagename"), ctx.Params("packageversion"))
+		if err != nil {
+			return nil, nil, err
+		}

-	if !packageNameRegex.MatchString(packageName) || !filenameRegex.MatchString(filename) {
-		return "", "", "", errors.New("Invalid package name or filename")
+		pf, err := packages_model.GetFileForVersionByName(ctx, pv.ID, ctx.Params("filename"), packages_model.EmptyFileKey)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		return pv, pf, nil
+	}()
+	if err != nil {
+		if err == packages_model.ErrPackageNotExist || err == packages_model.ErrPackageFileNotExist {
+			apiError(ctx, http.StatusNotFound, err)
+			return
+		}
+		apiError(ctx, http.StatusInternalServerError, err)
+		return
 	}

-	packageVersion := strings.TrimSpace(ctx.Params("packageversion"))
-	if packageVersion == "" {
-		return "", "", "", errors.New("Invalid package version")
+	pfs, err := packages_model.GetFilesByVersionID(ctx, pv.ID)
+	if err != nil {
+		apiError(ctx, http.StatusInternalServerError, err)
+		return
 	}

-	return packageName, packageVersion, filename, nil
+	if len(pfs) == 1 {
+		if err := packages_service.RemovePackageVersion(ctx.Doer, pv); err != nil {
+			apiError(ctx, http.StatusInternalServerError, err)
+			return
+		}
+	} else {
+		if err := packages_service.DeletePackageFile(ctx, pf); err != nil {
+			apiError(ctx, http.StatusInternalServerError, err)
+			return
+		}
+	}
+
+	ctx.Status(http.StatusNoContent)
 }