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 <1012112796@qq.com>
This commit is contained in:
JakobDev 2023-04-12 11:05:23 +02:00 committed by GitHub
parent e03e827dcb
commit 42919ccb7c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 60 additions and 15 deletions

View File

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

View File

@ -7,6 +7,7 @@ package repo
import ( import (
"context" "context"
"fmt" "fmt"
"net/url"
"sort" "sort"
"strconv" "strconv"
"strings" "strings"
@ -372,6 +373,34 @@ func GetReleaseAttachments(ctx context.Context, rels ...*Release) (err error) {
sortedRels.Rel[currentIndex].Attachments = append(sortedRels.Rel[currentIndex].Attachments, attachment) 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 return err
} }

View File

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

View File

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

View File

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