Refactor container package (#34877)

Use standard db.WithTx and introduce db.WithTx2

models/db/context.go

@@ -178,6 +178,15 @@ func WithTx(parentCtx context.Context, f func(ctx context.Context) error) error
 	return txWithNoCheck(parentCtx, f)
 }
 
+// WithTx2 is similar to WithTx, but it has two return values: result and error.
+func WithTx2[T any](parentCtx context.Context, f func(ctx context.Context) (T, error)) (ret T, errRet error) {
+	errRet = WithTx(parentCtx, func(ctx context.Context) (errInner error) {
+		ret, errInner = f(ctx)
+		return errInner
+	})
+	return ret, errRet
+}
+
 func txWithNoCheck(parentCtx context.Context, f func(ctx context.Context) error) error {
 	sess := xormEngine.NewSession()
 	defer sess.Close()

routers/api/packages/container/auth.go

@@ -21,7 +21,7 @@ func (a *Auth) Name() string {
 }
 
 // Verify extracts the user from the Bearer token
-// If it's an anonymous session a ghost user is returned
+// If it's an anonymous session, a ghost user is returned
 func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) {
 	packageMeta, err := packages.ParseAuthorizationRequest(req)
 	if err != nil {

routers/api/packages/container/blob.go

@@ -95,15 +95,13 @@ func containerGlobalLockKey(piOwnerID int64, piName, usage string) string {
 }
 
 func getOrCreateUploadVersion(ctx context.Context, pi *packages_service.PackageInfo) (*packages_model.PackageVersion, error) {
-	var uploadVersion *packages_model.PackageVersion
-
 	releaser, err := globallock.Lock(ctx, containerGlobalLockKey(pi.Owner.ID, pi.Name, "package"))
 	if err != nil {
 		return nil, err
 	}
 	defer releaser()
 
-	err = db.WithTx(ctx, func(ctx context.Context) error {
+	return db.WithTx2(ctx, func(ctx context.Context) (*packages_model.PackageVersion, error) {
 		created := true
 		p := &packages_model.Package{
 			OwnerID:   pi.Owner.ID,
@@ -115,7 +113,7 @@ func getOrCreateUploadVersion(ctx context.Context, pi *packages_service.PackageI
 		if p, err = packages_model.TryInsertPackage(ctx, p); err != nil {
 			if !errors.Is(err, packages_model.ErrDuplicatePackage) {
 				log.Error("Error inserting package: %v", err)
-				return err
+				return nil, err
 			}
 			created = false
 		}
@@ -123,7 +121,7 @@ func getOrCreateUploadVersion(ctx context.Context, pi *packages_service.PackageI
 		if created {
 			if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(pi.Owner.LowerName+"/"+pi.Name)); err != nil {
 				log.Error("Error setting package property: %v", err)
-				return err
+				return nil, err
 			}
 		}
 
@@ -138,16 +136,11 @@ func getOrCreateUploadVersion(ctx context.Context, pi *packages_service.PackageI
 		if pv, err = packages_model.GetOrInsertVersion(ctx, pv); err != nil {
 			if !errors.Is(err, packages_model.ErrDuplicatePackageVersion) {
 				log.Error("Error inserting package: %v", err)
-				return err
+				return nil, err
 			}
 		}
-
-		uploadVersion = pv
-
-		return nil
+		return pv, nil
 	})
-
-	return uploadVersion, err
 }
 
 func createFileForBlob(ctx context.Context, pv *packages_model.PackageVersion, pb *packages_model.PackageBlob) error {

routers/api/packages/container/container.go

@@ -13,6 +13,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 
 	auth_model "code.gitea.io/gitea/models/auth"
 	packages_model "code.gitea.io/gitea/models/packages"
@@ -39,10 +40,14 @@ import (
 // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pushing-manifests
 const maxManifestSize = 10 * 1024 * 1024
 
-var (
-	imageNamePattern = regexp.MustCompile(`\A[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*\z`)
-	referencePattern = regexp.MustCompile(`\A[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}\z`)
-)
+var globalVars = sync.OnceValue(func() (ret struct {
+	imageNamePattern, referencePattern *regexp.Regexp
+},
+) {
+	ret.imageNamePattern = regexp.MustCompile(`\A[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*\z`)
+	ret.referencePattern = regexp.MustCompile(`\A[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}\z`)
+	return ret
+})
 
 type containerHeaders struct {
 	Status        int
@@ -84,9 +89,7 @@ func jsonResponse(ctx *context.Context, status int, obj any) {
 		Status:      status,
 		ContentType: "application/json",
 	})
-	if err := json.NewEncoder(ctx.Resp).Encode(obj); err != nil {
-		log.Error("JSON encode: %v", err)
-	}
+	_ = json.NewEncoder(ctx.Resp).Encode(obj) // ignore network errors
 }
 
 func apiError(ctx *context.Context, status int, err error) {
@@ -134,7 +137,7 @@ func ReqContainerAccess(ctx *context.Context) {
 
 // VerifyImageName is a middleware which checks if the image name is allowed
 func VerifyImageName(ctx *context.Context) {
-	if !imageNamePattern.MatchString(ctx.PathParam("image")) {
+	if !globalVars().imageNamePattern.MatchString(ctx.PathParam("image")) {
 		apiErrorDefined(ctx, errNameInvalid)
 	}
 }
@@ -216,7 +219,7 @@ func GetRepositoryList(ctx *context.Context) {
 	if len(repositories) == n {
 		v := url.Values{}
 		if n > 0 {
-			v.Add("n", strconv.Itoa(n))
+			v.Add("n", strconv.Itoa(n)) // FIXME: "n" can't be zero here, the logic is inconsistent with GetTagsList
 		}
 		v.Add("last", repositories[len(repositories)-1])
 
@@ -565,7 +568,7 @@ func PutManifest(ctx *context.Context) {
 		IsTagged:  digest.Digest(reference).Validate() != nil,
 	}
 
-	if mci.IsTagged && !referencePattern.MatchString(reference) {
+	if mci.IsTagged && !globalVars().referencePattern.MatchString(reference) {
 		apiErrorDefined(ctx, errManifestInvalid.WithMessage("Tag is invalid"))
 		return
 	}
@@ -618,7 +621,7 @@ func getBlobSearchOptionsFromContext(ctx *context.Context) (*container_model.Blo
 	reference := ctx.PathParam("reference")
 	if d := digest.Digest(reference); d.Validate() == nil {
 		opts.Digest = string(d)
-	} else if referencePattern.MatchString(reference) {
+	} else if globalVars().referencePattern.MatchString(reference) {
 		opts.Tag = reference
 		opts.OnlyLead = true
 	} else {
@@ -782,7 +785,8 @@ func GetTagsList(ctx *context.Context) {
 	})
 }
 
-// FIXME: Workaround to be removed in v1.20
+// FIXME: Workaround to be removed in v1.20.
+// Update maybe we should never really remote it, as long as there is legacy data?
 // https://github.com/go-gitea/gitea/issues/19586
 func workaroundGetContainerBlob(ctx *context.Context, opts *container_model.BlobSearchOptions) (*packages_model.PackageFileDescriptor, error) {
 	blob, err := container_model.GetContainerBlob(ctx, opts)

routers/api/packages/container/manifest.go

@@ -46,11 +46,9 @@ func processManifest(ctx context.Context, mci *manifestCreationInfo, buf *packag
 	if err := json.NewDecoder(buf).Decode(&index); err != nil {
 		return "", err
 	}
-
 	if index.SchemaVersion != 2 {
 		return "", errUnsupported.WithMessage("Schema version is not supported")
 	}
-
 	if _, err := buf.Seek(0, io.SeekStart); err != nil {
 		return "", err
 	}
@@ -77,24 +75,41 @@ func processManifest(ctx context.Context, mci *manifestCreationInfo, buf *packag
 	return "", errManifestInvalid
 }
 
-func processOciImageManifest(ctx context.Context, mci *manifestCreationInfo, buf *packages_module.HashedBuffer) (string, error) {
-	manifestDigest := ""
+type processManifestTxRet struct {
+	pv      *packages_model.PackageVersion
+	pb      *packages_model.PackageBlob
+	created bool
+	digest  string
+}
 
-	err := func() error {
-		manifest, configDescriptor, metadata, err := container_service.ParseManifestMetadata(ctx, buf, mci.Owner.ID, mci.Image)
-		if err != nil {
-			return err
-		}
-		if _, err = buf.Seek(0, io.SeekStart); err != nil {
-			return err
+func handleCreateManifestResult(ctx context.Context, err error, mci *manifestCreationInfo, contentStore *packages_module.ContentStore, txRet *processManifestTxRet) (string, error) {
+	if err != nil && txRet.created && txRet.pb != nil {
+		if err := contentStore.Delete(packages_module.BlobHash256Key(txRet.pb.HashSHA256)); err != nil {
+			log.Error("Error deleting package blob from content store: %v", err)
 		}
+		return "", err
+	}
+	pd, err := packages_model.GetPackageDescriptor(ctx, txRet.pv)
+	if err != nil {
+		log.Error("Error getting package descriptor: %v", err) // ignore this error
+	} else {
+		notify_service.PackageCreate(ctx, mci.Creator, pd)
+	}
+	return txRet.digest, nil
+}
 
-		ctx, committer, err := db.TxContext(ctx)
-		if err != nil {
-			return err
-		}
-		defer committer.Close()
+func processOciImageManifest(ctx context.Context, mci *manifestCreationInfo, buf *packages_module.HashedBuffer) (manifestDigest string, errRet error) {
+	manifest, configDescriptor, metadata, err := container_service.ParseManifestMetadata(ctx, buf, mci.Owner.ID, mci.Image)
+	if err != nil {
+		return "", err
+	}
+	if _, err = buf.Seek(0, io.SeekStart); err != nil {
+		return "", err
+	}
 
+	contentStore := packages_module.NewContentStore()
+	var txRet processManifestTxRet
+	err = db.WithTx(ctx, func(ctx context.Context) (err error) {
 		blobReferences := make([]*blobReference, 0, 1+len(manifest.Layers))
 		blobReferences = append(blobReferences, &blobReference{
 			Digest:       manifest.Config.Digest,
@@ -127,7 +142,7 @@ func processOciImageManifest(ctx context.Context, mci *manifestCreationInfo, buf
 		}
 
 		uploadVersion, err := packages_model.GetInternalVersionByNameAndVersion(ctx, mci.Owner.ID, packages_model.TypeContainer, mci.Image, container_module.UploadVersion)
-		if err != nil && err != packages_model.ErrPackageNotExist {
+		if err != nil && !errors.Is(err, packages_model.ErrPackageNotExist) {
 			return err
 		}
 
@@ -136,61 +151,26 @@ func processOciImageManifest(ctx context.Context, mci *manifestCreationInfo, buf
 				return err
 			}
 		}
+		txRet.pv = pv
+		txRet.pb, txRet.created, txRet.digest, err = createManifestBlob(ctx, contentStore, mci, pv, buf)
+		return err
+	})
 
-		pb, created, digest, err := createManifestBlob(ctx, mci, pv, buf)
-		removeBlob := false
-		defer func() {
-			if removeBlob {
-				contentStore := packages_module.NewContentStore()
-				if err := contentStore.Delete(packages_module.BlobHash256Key(pb.HashSHA256)); err != nil {
-					log.Error("Error deleting package blob from content store: %v", err)
-				}
-			}
-		}()
-		if err != nil {
-			removeBlob = created
-			return err
-		}
+	return handleCreateManifestResult(ctx, err, mci, contentStore, &txRet)
+}
 
-		if err := committer.Commit(); err != nil {
-			removeBlob = created
-			return err
-		}
-
-		if err := notifyPackageCreate(ctx, mci.Creator, pv); err != nil {
-			return err
-		}
-
-		manifestDigest = digest
-
-		return nil
-	}()
-	if err != nil {
+func processOciImageIndex(ctx context.Context, mci *manifestCreationInfo, buf *packages_module.HashedBuffer) (manifestDigest string, errRet error) {
+	var index oci.Index
+	if err := json.NewDecoder(buf).Decode(&index); err != nil {
+		return "", err
+	}
+	if _, err := buf.Seek(0, io.SeekStart); err != nil {
 		return "", err
 	}
 
-	return manifestDigest, nil
-}
-
-func processOciImageIndex(ctx context.Context, mci *manifestCreationInfo, buf *packages_module.HashedBuffer) (string, error) {
-	manifestDigest := ""
-
-	err := func() error {
-		var index oci.Index
-		if err := json.NewDecoder(buf).Decode(&index); err != nil {
-			return err
-		}
-
-		if _, err := buf.Seek(0, io.SeekStart); err != nil {
-			return err
-		}
-
-		ctx, committer, err := db.TxContext(ctx)
-		if err != nil {
-			return err
-		}
-		defer committer.Close()
-
+	contentStore := packages_module.NewContentStore()
+	var txRet processManifestTxRet
+	err := db.WithTx(ctx, func(ctx context.Context) (err error) {
 		metadata := &container_module.Metadata{
 			Type:      container_module.TypeOCI,
 			Manifests: make([]*container_module.Manifest, 0, len(index.Manifests)),
@@ -241,50 +221,12 @@ func processOciImageIndex(ctx context.Context, mci *manifestCreationInfo, buf *p
 			return err
 		}
 
-		pb, created, digest, err := createManifestBlob(ctx, mci, pv, buf)
-		removeBlob := false
-		defer func() {
-			if removeBlob {
-				contentStore := packages_module.NewContentStore()
-				if err := contentStore.Delete(packages_module.BlobHash256Key(pb.HashSHA256)); err != nil {
-					log.Error("Error deleting package blob from content store: %v", err)
-				}
-			}
-		}()
-		if err != nil {
-			removeBlob = created
-			return err
-		}
-
-		if err := committer.Commit(); err != nil {
-			removeBlob = created
-			return err
-		}
-
-		if err := notifyPackageCreate(ctx, mci.Creator, pv); err != nil {
-			return err
-		}
-
-		manifestDigest = digest
-
-		return nil
-	}()
-	if err != nil {
-		return "", err
-	}
-
-	return manifestDigest, nil
-}
-
-func notifyPackageCreate(ctx context.Context, doer *user_model.User, pv *packages_model.PackageVersion) error {
-	pd, err := packages_model.GetPackageDescriptor(ctx, pv)
-	if err != nil {
+		txRet.pv = pv
+		txRet.pb, txRet.created, txRet.digest, err = createManifestBlob(ctx, contentStore, mci, pv, buf)
 		return err
-	}
+	})
 
-	notify_service.PackageCreate(ctx, doer, pd)
-
-	return nil
+	return handleCreateManifestResult(ctx, err, mci, contentStore, &txRet)
 }
 
 func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, metadata *container_module.Metadata) (*packages_model.PackageVersion, error) {
@@ -437,7 +379,7 @@ func createFileFromBlobReference(ctx context.Context, pv, uploadVersion *package
 	return pf, nil
 }
 
-func createManifestBlob(ctx context.Context, mci *manifestCreationInfo, pv *packages_model.PackageVersion, buf *packages_module.HashedBuffer) (*packages_model.PackageBlob, bool, string, error) {
+func createManifestBlob(ctx context.Context, contentStore *packages_module.ContentStore, mci *manifestCreationInfo, pv *packages_model.PackageVersion, buf *packages_module.HashedBuffer) (_ *packages_model.PackageBlob, created bool, manifestDigest string, _ error) {
 	pb, exists, err := packages_model.GetOrInsertBlob(ctx, packages_service.NewPackageBlob(buf))
 	if err != nil {
 		log.Error("Error inserting package blob: %v", err)
@@ -446,21 +388,20 @@ func createManifestBlob(ctx context.Context, mci *manifestCreationInfo, pv *pack
 	// FIXME: Workaround to be removed in v1.20
 	// https://github.com/go-gitea/gitea/issues/19586
 	if exists {
-		err = packages_module.NewContentStore().Has(packages_module.BlobHash256Key(pb.HashSHA256))
+		err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256))
 		if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) {
 			log.Debug("Package registry inconsistent: blob %s does not exist on file system", pb.HashSHA256)
 			exists = false
 		}
 	}
 	if !exists {
-		contentStore := packages_module.NewContentStore()
 		if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), buf, buf.Size()); err != nil {
 			log.Error("Error saving package blob in content store: %v", err)
 			return nil, false, "", err
 		}
 	}
 
-	manifestDigest := digestFromHashSummer(buf)
+	manifestDigest = digestFromHashSummer(buf)
 	pf, err := createFileFromBlobReference(ctx, pv, nil, &blobReference{
 		Digest:       digest.Digest(manifestDigest),
 		MediaType:    mci.MediaType,

services/packages/container/common.go

@@ -48,7 +48,7 @@ func ParseManifestMetadata(ctx context.Context, rd io.Reader, ownerID int64, ima
 	configDescriptor, err := container_service.GetContainerBlob(ctx, &container_service.BlobSearchOptions{
 		OwnerID: ownerID,
 		Image:   imageName,
-		Digest:  string(manifest.Config.Digest),
+		Digest:  manifest.Config.Digest.String(),
 	})
 	if err != nil {
 		return nil, nil, nil, err