Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nuspec manifest download for nuget packages #28921

Merged
merged 7 commits into from Apr 17, 2024

Conversation

viceice
Copy link
Contributor

@viceice viceice commented Jan 25, 2024

Support downloading nuget nuspec manifest1. This is useful for renovate because it uses this api to find the corresponding repository

  • Store nuspec along with nupkg on upload
  • allow downloading nuspec
  • add doctor command to add missing nuspec files

Footnotes

  1. https://learn.microsoft.com/en-us/nuget/api/package-base-address-resource#download-package-manifest-nuspec

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 25, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 25, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Jan 25, 2024
@viceice viceice mentioned this pull request Jan 25, 2024
@lunny lunny added the type/enhancement An improvement of existing functionality label Jan 25, 2024
@lunny lunny added this to the 1.22.0 milestone Jan 25, 2024
@delvh delvh changed the title feat(nuget): support nuspec manifest download Support nuspec manifest download for nuget packages Jan 25, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 25, 2024

I think it would be better if we would store the nuspec file as additional file in the package version instead of generating it. The original file could/will have additional data we don't need for the package registry and don't store in the metadata. So we can't re-generate the full nuspec file afterwards.

@viceice
Copy link
Contributor Author

viceice commented Jan 26, 2024

sure, but that's out of my go skills. sorry.

this implementation satisfies our needs for renovate bot.

@viceice
Copy link
Contributor Author

viceice commented Jan 31, 2024

new idea:

read nuspec from package and add to cache. so later request can leverage the cache.

cache can also be filled when package is uploaded.

WDYT?

@viceice
Copy link
Contributor Author

viceice commented Jan 31, 2024

we can also store it aside the package and serve directly if available. fallback to extract from package and put to cache in both cases.
this way we don't need a migration.

@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 4, 2024
@viceice
Copy link
Contributor Author

viceice commented Apr 4, 2024

@KN4CK3R Can you please check this version?

@viceice viceice marked this pull request as draft April 4, 2024 13:25
@viceice
Copy link
Contributor Author

viceice commented Apr 4, 2024

Converting to draft, doctor is not working as expected.

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 4, 2024

sure, but that's out of my go skills. sorry.

this implementation satisfies our needs for renovate bot.

It's easier than what you have currently actually:

diff --git a/modules/packages/nuget/metadata.go b/modules/packages/nuget/metadata.go
index 6769c514cc..1e98ddffde 100644
--- a/modules/packages/nuget/metadata.go
+++ b/modules/packages/nuget/metadata.go
@@ -48,10 +48,11 @@ const maxNuspecFileSize = 3 * 1024 * 1024
 
 // Package represents a Nuget package
 type Package struct {
-	PackageType PackageType
-	ID          string
-	Version     string
-	Metadata    *Metadata
+	PackageType   PackageType
+	ID            string
+	Version       string
+	Metadata      *Metadata
+	NuspecContent *bytes.Buffer
 }
 
 // Metadata represents the metadata of a Nuget package
@@ -138,8 +139,9 @@ func ParsePackageMetaData(r io.ReaderAt, size int64) (*Package, error) {
 
 // ParseNuspecMetaData parses a Nuspec file to retrieve the metadata of a Nuget package
 func ParseNuspecMetaData(archive *zip.Reader, r io.Reader) (*Package, error) {
+	var nuspecBuf bytes.Buffer
 	var p nuspecPackage
-	if err := xml.NewDecoder(r).Decode(&p); err != nil {
+	if err := xml.NewDecoder(io.TeeReader(r, &nuspecBuf)).Decode(&p); err != nil {
 		return nil, err
 	}
 
@@ -212,10 +214,11 @@ func ParseNuspecMetaData(archive *zip.Reader, r io.Reader) (*Package, error) {
 		}
 	}
 	return &Package{
-		PackageType: packageType,
-		ID:          p.Metadata.ID,
-		Version:     toNormalizedVersion(v),
-		Metadata:    m,
+		PackageType:   packageType,
+		ID:            p.Metadata.ID,
+		Version:       toNormalizedVersion(v),
+		Metadata:      m,
+		NuspecContent: &nuspecBuf,
 	}, nil
 }

diff --git a/routers/api/packages/nuget/nuget.go b/routers/api/packages/nuget/nuget.go
index c28bc6c9d9..34dcda4e2d 100644
--- a/routers/api/packages/nuget/nuget.go
+++ b/routers/api/packages/nuget/nuget.go
@@ -431,7 +431,7 @@ func UploadPackage(ctx *context.Context) {
 		return
 	}
 
-	_, _, err := packages_service.CreatePackageAndAddFile(
+	pv, _, err := packages_service.CreatePackageAndAddFile(
 		ctx,
 		&packages_service.PackageCreationInfo{
 			PackageInfo: packages_service.PackageInfo{
@@ -465,6 +465,35 @@ func UploadPackage(ctx *context.Context) {
 		return
 	}
 
+	nuspecBuf, err := packages_module.CreateHashedBufferFromReaderWithSize(np.NuspecContent, np.NuspecContent.Len())
+	if err != nil {
+		apiError(ctx, http.StatusInternalServerError, err)
+		return
+	}
+	defer nuspecBuf.Close()
+
+	_, err = packages_service.AddFileToPackageVersionInternal(
+		ctx,
+		pv,
+		&packages_service.PackageFileCreationInfo{
+			PackageFileInfo: packages_service.PackageFileInfo{
+				Filename: strings.ToLower(fmt.Sprintf("%s.nuspec", np.ID)),
+			},
+			Creator: ctx.Doer,
+			Data:    nuspecBuf,
+			IsLead:  false,
+		},
+	)
+	if err != nil {
+		switch err {
+		case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize:
+			apiError(ctx, http.StatusForbidden, err)
+		default:
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
+		return
+	}
+
 	ctx.Status(http.StatusCreated)
 }

Nothing else is needed (+ tests). I have a branch currently with some NuGet changes for #30265. If you don't mind I could add this there too and add you as co-author. It doesn't add the nuspec file to existing packages with a migration but that may not be a big problem. I thought about a button in the package settings which would reinitialize (meta) data from the stored package (similar to what your doctor command would do) but that would be different PR.

@viceice
Copy link
Contributor Author

viceice commented Apr 4, 2024

I've done the complex part, because I don't want to store the full manifest in the database a second time.

most other nuget API implementations are storing the nuspec aside the nupkg.

feel free to do your own implementation instead of this.

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 4, 2024

It's not stored in the database but as file, same as your code.

I'm surprised the tests are passing because the hashed buffer is read twice (first nuspec parsing and when storing the file). The second read should already be at the end.

@viceice
Copy link
Contributor Author

viceice commented Apr 4, 2024

It's not stored in the database but as file, same as your code.

🙈 I should have read your diff more closely

I'm surprised the tests are passing because the hashed buffer is read twice (first nuspec parsing and when storing the file). The second read should already be at the end.

I've a seek to begin somewhere 🙃

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 4, 2024

I've a seek to begin somewhere 🙃

Searched and did not find it. But it's here in line 633
https://github.com/go-gitea/gitea/pull/28921/files#diff-efe687650466302ae2e35f382b2a66571e28065c6909e25893e515d5c9cc79f9R633

@viceice
Copy link
Contributor Author

viceice commented Apr 5, 2024

Updated PR with your suggestions and added a doctor integration test

@viceice viceice marked this pull request as ready for review April 5, 2024 07:14
@viceice viceice force-pushed the feat/nuget-api-nuspec branch 2 times, most recently from 3618990 to 5a54d47 Compare April 5, 2024 08:25
services/doctor/doctor.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 16, 2024
@viceice viceice requested a review from wxiaoguang April 16, 2024 11:08
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 16, 2024
@viceice
Copy link
Contributor Author

viceice commented Apr 16, 2024

dropped the doctor command

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 16, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 16, 2024 via email

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 17, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2024
@wxiaoguang wxiaoguang removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2024
@wxiaoguang
Copy link
Contributor

CI fails

@KN4CK3R KN4CK3R enabled auto-merge (squash) April 17, 2024 14:09
@KN4CK3R KN4CK3R merged commit bafb80f into go-gitea:main Apr 17, 2024
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 18, 2024
* giteaofficial/main:
  Add an api test for updating user (go-gitea#30539)
  [skip ci] Updated translations via Crowdin
  Expose fuzzy search for issues/pulls (go-gitea#29701)
  Allow everyone to read or write a wiki by a repo unit setting (go-gitea#30495)
  Support nuspec manifest download for nuget packages (go-gitea#28921)
  Fix branch_protection api shows users/teams who has no readAccess (go-gitea#30291)
  Correct locale string rendering (go-gitea#30522)
  Run `go generate` and `go vet` on all packages (go-gitea#30529)
  Fix and tweak pull request commit list (go-gitea#30528)
  Refactor web routes (go-gitea#30519)
  Fix install page checkboxes and dropdown width (go-gitea#30526)

# Conflicts:
#	routers/web/user/home.go
#	templates/user/dashboard/issues.tmpl
@wxiaoguang wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/packages type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants