Make Release Download URLs predictable (#23891)

As promised in #23817, I have this made a PR to make Release Download
URLs predictable. It currently follows the schema
`<repo>/releases/download/<tag>/<filename>`. this already works, but it
is nowhere shown in the UI or the API. The Problem is, that it is
currently possible to have multiple files with the same name (why do we
even allow this) for a release. I had written some Code to check, if a
Release has 2 or more files with the same Name. If yes, it uses the old
`attachments/<uuid>` URlL if no it uses the new fancy URL.

I had also changed `<repo>/releases/download/<tag>/<filename>` to
directly serve the File instead of redirecting, so people who who use
automatic update checker don't end up with the `attachments/<uuid>` URL.

Fixes #10919

---------

Co-authored-by: a1012112796 <[email protected]>

Author: JakobDev

models/repo/attachment.go

@@ -18,17 +18,18 @@ import (
 
 // Attachment represent a attachment of issue/comment/release.
 type Attachment struct {
-	ID            int64  `xorm:"pk autoincr"`
-	UUID          string `xorm:"uuid UNIQUE"`
-	RepoID        int64  `xorm:"INDEX"`           // this should not be zero
-	IssueID       int64  `xorm:"INDEX"`           // maybe zero when creating
-	ReleaseID     int64  `xorm:"INDEX"`           // maybe zero when creating
-	UploaderID    int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
-	CommentID     int64
-	Name          string
-	DownloadCount int64              `xorm:"DEFAULT 0"`
-	Size          int64              `xorm:"DEFAULT 0"`
-	CreatedUnix   timeutil.TimeStamp `xorm:"created"`
+	ID                int64  `xorm:"pk autoincr"`
+	UUID              string `xorm:"uuid UNIQUE"`
+	RepoID            int64  `xorm:"INDEX"`           // this should not be zero
+	IssueID           int64  `xorm:"INDEX"`           // maybe zero when creating
+	ReleaseID         int64  `xorm:"INDEX"`           // maybe zero when creating
+	UploaderID        int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
+	CommentID         int64
+	Name              string
+	DownloadCount     int64              `xorm:"DEFAULT 0"`
+	Size              int64              `xorm:"DEFAULT 0"`
+	CreatedUnix       timeutil.TimeStamp `xorm:"created"`
+	CustomDownloadURL string             `xorm:"-"`
 }
 
 func init() {
@@ -57,6 +58,10 @@ func (a *Attachment) RelativePath() string {
 
 // DownloadURL returns the download url of the attached file
 func (a *Attachment) DownloadURL() string {
+	if a.CustomDownloadURL != "" {
+		return a.CustomDownloadURL
+	}
+
 	return setting.AppURL + "attachments/" + url.PathEscape(a.UUID)
 }
 

models/repo/release.go

@@ -7,6 +7,7 @@ package repo
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"sort"
 	"strconv"
 	"strings"
@@ -372,6 +373,34 @@ func GetReleaseAttachments(ctx context.Context, rels ...*Release) (err error) {
 		sortedRels.Rel[currentIndex].Attachments = append(sortedRels.Rel[currentIndex].Attachments, attachment)
 	}
 
+	// Makes URL's predictable
+	for _, release := range rels {
+		// If we have no Repo, we don't need to execute this loop
+		if release.Repo == nil {
+			continue
+		}
+
+		// Check if there are two or more attachments with the same name
+		hasDuplicates := false
+		foundNames := make(map[string]bool)
+		for _, attachment := range release.Attachments {
+			_, found := foundNames[attachment.Name]
+			if found {
+				hasDuplicates = true
+				break
+			} else {
+				foundNames[attachment.Name] = true
+			}
+		}
+
+		// If the names unique, use the URL with the Name instead of the UUID
+		if !hasDuplicates {
+			for _, attachment := range release.Attachments {
+				attachment.CustomDownloadURL = release.Repo.HTMLURL() + "/releases/download/" + url.PathEscape(release.TagName) + "/" + url.PathEscape(attachment.Name)
+			}
+		}
+	}
+
 	return err
 }
 

routers/web/repo/attachment.go

@@ -86,9 +86,9 @@ func DeleteAttachment(ctx *context.Context) {
 	})
 }
 
-// GetAttachment serve attachments
-func GetAttachment(ctx *context.Context) {
-	attach, err := repo_model.GetAttachmentByUUID(ctx, ctx.Params(":uuid"))
+// GetAttachment serve attachments with the given UUID
+func ServeAttachment(ctx *context.Context, uuid string) {
+	attach, err := repo_model.GetAttachmentByUUID(ctx, uuid)
 	if err != nil {
 		if repo_model.IsErrAttachmentNotExist(err) {
 			ctx.Error(http.StatusNotFound)
@@ -153,3 +153,8 @@ func GetAttachment(ctx *context.Context) {
 		return
 	}
 }
+
+// GetAttachment serve attachments
+func GetAttachment(ctx *context.Context) {
+	ServeAttachment(ctx, ctx.Params(":uuid"))
+}

routers/web/repo/release.go

@@ -142,6 +142,10 @@ func releasesOrTags(ctx *context.Context, isTagList bool) {
 		return
 	}
 
+	for _, release := range releases {
+		release.Repo = ctx.Repo.Repository
+	}
+
 	if err = repo_model.GetReleaseAttachments(ctx, releases...); err != nil {
 		ctx.ServerError("GetReleaseAttachments", err)
 		return
@@ -248,6 +252,8 @@ func SingleRelease(ctx *context.Context) {
 		ctx.Data["Title"] = release.Title
 	}
 
+	release.Repo = ctx.Repo.Repository
+
 	err = repo_model.GetReleaseAttachments(ctx, release)
 	if err != nil {
 		ctx.ServerError("GetReleaseAttachments", err)

routers/web/repo/repo.go

@@ -373,7 +373,7 @@ func RedirectDownload(ctx *context.Context) {
 			return
 		}
 		if att != nil {
-			ctx.Redirect(att.DownloadURL())
+			ServeAttachment(ctx, att.UUID)
 			return
 		}
 	}