Validate that the tag doesn't exist when creating a tag via the web (#33241)

Found while investigating #33210.

This line no longer makes sense because the form field "TagName" is
required, so this would mean that this code path would never be covered.
Because it isn't covered, we end up going down the "update release"
logic where we eventually set `Release.IsTag` to false (meaning it will
now be treated as a release instead of a tag).

This snapshot rewrites the condition to ensure that we aren't trying to
create a tag that already exists.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

2 files changed, 230 insertions, 153 deletions

diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go
index 53655703fc..8909dedbb1 100644
--- a/routers/web/repo/release.go
+++ b/routers/web/repo/release.go
@@ -17,7 +17,6 @@ import (
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/markup/markdown"
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
@@ -330,12 +329,42 @@ func LatestRelease(ctx *context.Context) {
 	ctx.Redirect(release.Link())
 }
 
-// NewRelease render creating or edit release page
-func NewRelease(ctx *context.Context) {
+func newReleaseCommon(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("repo.release.new_release")
 	ctx.Data["PageIsReleaseList"] = true
+
+	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
+	if err != nil {
+		ctx.ServerError("GetTagNamesByRepoID", err)
+		return
+	}
+	ctx.Data["Tags"] = tags
+
+	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
+	assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository)
+	if err != nil {
+		ctx.ServerError("GetRepoAssignees", err)
+		return
+	}
+	ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers)
+
+	upload.AddUploadContext(ctx, "release")
+
+	PrepareBranchList(ctx) // for New Release page
+}
+
+// NewRelease render creating or edit release page
+func NewRelease(ctx *context.Context) {
+	newReleaseCommon(ctx)
+	if ctx.Written() {
+		return
+	}
+
+	ctx.Data["ShowCreateTagOnlyButton"] = true
+
+	// pre-fill the form with the tag name, target branch and the existing release (if exists)
 	ctx.Data["tag_target"] = ctx.Repo.Repository.DefaultBranch
-	if tagName := ctx.FormString("tag"); len(tagName) > 0 {
+	if tagName := ctx.FormString("tag"); tagName != "" {
 		rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, tagName)
 		if err != nil && !repo_model.IsErrReleaseNotExist(err) {
 			ctx.ServerError("GetRelease", err)
@@ -344,59 +373,51 @@ func NewRelease(ctx *context.Context) {
 
 		if rel != nil {
 			rel.Repo = ctx.Repo.Repository
-			if err := rel.LoadAttributes(ctx); err != nil {
+			if err = rel.LoadAttributes(ctx); err != nil {
 				ctx.ServerError("LoadAttributes", err)
 				return
 			}
 
+			ctx.Data["ShowCreateTagOnlyButton"] = false
 			ctx.Data["tag_name"] = rel.TagName
-			if rel.Target != "" {
-				ctx.Data["tag_target"] = rel.Target
-			}
+			ctx.Data["tag_target"] = rel.Target
 			ctx.Data["title"] = rel.Title
 			ctx.Data["content"] = rel.Note
 			ctx.Data["attachments"] = rel.Attachments
 		}
 	}
-	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
-	assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository)
-	if err != nil {
-		ctx.ServerError("GetRepoAssignees", err)
-		return
-	}
-	ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers)
-
-	upload.AddUploadContext(ctx, "release")
-
-	// For New Release page
-	PrepareBranchList(ctx)
-	if ctx.Written() {
-		return
-	}
-
-	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
-	if err != nil {
-		ctx.ServerError("GetTagNamesByRepoID", err)
-		return
-	}
-	ctx.Data["Tags"] = tags
 
 	ctx.HTML(http.StatusOK, tplReleaseNew)
 }
 
 // NewReleasePost response for creating a release
 func NewReleasePost(ctx *context.Context) {
+	newReleaseCommon(ctx)
+	if ctx.Written() {
+		return
+	}
+
 	form := web.GetForm(ctx).(*forms.NewReleaseForm)
-	ctx.Data["Title"] = ctx.Tr("repo.release.new_release")
-	ctx.Data["PageIsReleaseList"] = true
 
-	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
-	if err != nil {
-		ctx.ServerError("GetTagNamesByRepoID", err)
+	// first, check whether the release exists, and prepare "ShowCreateTagOnlyButton"
+	// the logic should be done before the form error check to make the tmpl has correct variables
+	rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName)
+	if err != nil && !repo_model.IsErrReleaseNotExist(err) {
+		ctx.ServerError("GetRelease", err)
 		return
 	}
-	ctx.Data["Tags"] = tags
 
+	// We should still show the "tag only" button if the user clicks it, no matter the release exists or not.
+	// Because if error occurs, end users need to have the chance to edit the name and submit the form with "tag-only" again.
+	// It is still not completely right, because there could still be cases like this:
+	// * user visit "new release" page, see the "tag only" button
+	// * input something, click other buttons but not "tag only"
+	// * error occurs, the "new release" page is rendered again, but the "tag only" button is gone
+	// Such cases are not able to be handled by current code, it needs frontend code to toggle the "tag-only" button if the input changes.
+	// Or another choice is "always show the tag-only button" if error occurs.
+	ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil
+
+	// do some form checks
 	if ctx.HasError() {
 		ctx.HTML(http.StatusOK, tplReleaseNew)
 		return
@@ -407,59 +428,49 @@ func NewReleasePost(ctx *context.Context) {
 		return
 	}
 
-	// Title of release cannot be empty
-	if len(form.TagOnly) == 0 && len(form.Title) == 0 {
+	if !form.TagOnly && form.Title == "" {
+		// if not "tag only", then the title of the release cannot be empty
 		ctx.RenderWithErr(ctx.Tr("repo.release.title_empty"), tplReleaseNew, &form)
 		return
 	}
 
-	var attachmentUUIDs []string
-	if setting.Attachment.Enabled {
-		attachmentUUIDs = form.Files
-	}
-
-	rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName)
-	if err != nil {
-		if !repo_model.IsErrReleaseNotExist(err) {
-			ctx.ServerError("GetRelease", err)
-			return
-		}
-
-		msg := ""
-		if len(form.Title) > 0 && form.AddTagMsg {
-			msg = form.Title + "\n\n" + form.Content
+	handleTagReleaseError := func(err error) {
+		ctx.Data["Err_TagName"] = true
+		switch {
+		case release_service.IsErrTagAlreadyExists(err):
+			ctx.RenderWithErr(ctx.Tr("repo.branch.tag_collision", form.TagName), tplReleaseNew, &form)
+		case repo_model.IsErrReleaseAlreadyExist(err):
+			ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
+		case release_service.IsErrInvalidTagName(err):
+			ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_invalid"), tplReleaseNew, &form)
+		case release_service.IsErrProtectedTagName(err):
+			ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_protected"), tplReleaseNew, &form)
+		default:
+			ctx.ServerError("handleTagReleaseError", err)
 		}
+	}
 
-		if len(form.TagOnly) > 0 {
-			if err = release_service.CreateNewTag(ctx, ctx.Doer, ctx.Repo.Repository, form.Target, form.TagName, msg); err != nil {
-				if release_service.IsErrTagAlreadyExists(err) {
-					e := err.(release_service.ErrTagAlreadyExists)
-					ctx.Flash.Error(ctx.Tr("repo.branch.tag_collision", e.TagName))
-					ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL())
-					return
-				}
-
-				if release_service.IsErrInvalidTagName(err) {
-					ctx.Flash.Error(ctx.Tr("repo.release.tag_name_invalid"))
-					ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL())
-					return
-				}
-
-				if release_service.IsErrProtectedTagName(err) {
-					ctx.Flash.Error(ctx.Tr("repo.release.tag_name_protected"))
-					ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL())
-					return
-				}
-
-				ctx.ServerError("release_service.CreateNewTag", err)
-				return
-			}
+	// prepare the git message for creating a new tag
+	newTagMsg := ""
+	if form.Title != "" && form.AddTagMsg {
+		newTagMsg = form.Title + "\n\n" + form.Content
+	}
 
-			ctx.Flash.Success(ctx.Tr("repo.tag.create_success", form.TagName))
-			ctx.Redirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName))
+	// no release, and tag only
+	if rel == nil && form.TagOnly {
+		if err = release_service.CreateNewTag(ctx, ctx.Doer, ctx.Repo.Repository, form.Target, form.TagName, newTagMsg); err != nil {
+			handleTagReleaseError(err)
 			return
 		}
+		ctx.Flash.Success(ctx.Tr("repo.tag.create_success", form.TagName))
+		ctx.Redirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName))
+		return
+	}
+
+	attachmentUUIDs := util.Iif(setting.Attachment.Enabled, form.Files, nil)
 
+	// no existing release, create a new release
+	if rel == nil {
 		rel = &repo_model.Release{
 			RepoID:       ctx.Repo.Repository.ID,
 			Repo:         ctx.Repo.Repository,
@@ -469,48 +480,39 @@ func NewReleasePost(ctx *context.Context) {
 			TagName:      form.TagName,
 			Target:       form.Target,
 			Note:         form.Content,
-			IsDraft:      len(form.Draft) > 0,
+			IsDraft:      form.Draft,
 			IsPrerelease: form.Prerelease,
 			IsTag:        false,
 		}
-
-		if err = release_service.CreateRelease(ctx.Repo.GitRepo, rel, attachmentUUIDs, msg); err != nil {
-			ctx.Data["Err_TagName"] = true
-			switch {
-			case repo_model.IsErrReleaseAlreadyExist(err):
-				ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
-			case release_service.IsErrInvalidTagName(err):
-				ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_invalid"), tplReleaseNew, &form)
-			case release_service.IsErrProtectedTagName(err):
-				ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_protected"), tplReleaseNew, &form)
-			default:
-				ctx.ServerError("CreateRelease", err)
-			}
-			return
-		}
-	} else {
-		if !rel.IsTag {
-			ctx.Data["Err_TagName"] = true
-			ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
+		if err = release_service.CreateRelease(ctx.Repo.GitRepo, rel, attachmentUUIDs, newTagMsg); err != nil {
+			handleTagReleaseError(err)
 			return
 		}
+		ctx.Redirect(ctx.Repo.RepoLink + "/releases")
+		return
+	}
 
-		rel.Title = form.Title
-		rel.Note = form.Content
-		rel.Target = form.Target
-		rel.IsDraft = len(form.Draft) > 0
-		rel.IsPrerelease = form.Prerelease
-		rel.PublisherID = ctx.Doer.ID
-		rel.IsTag = false
-
-		if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil); err != nil {
-			ctx.Data["Err_TagName"] = true
-			ctx.ServerError("UpdateRelease", err)
-			return
-		}
+	// tag exists, try to convert it to a real release
+	// old logic: if the release is not a tag (it is a real release), do not update it on the "new release" page
+	// add new logic: if tag-only, do not convert the tag to a release
+	if form.TagOnly || !rel.IsTag {
+		ctx.Data["Err_TagName"] = true
+		ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form)
+		return
 	}
-	log.Trace("Release created: %s/%s:%s", ctx.Doer.LowerName, ctx.Repo.Repository.Name, form.TagName)
 
+	// convert a tag to a real release (set is_tag=false)
+	rel.Title = form.Title
+	rel.Note = form.Content
+	rel.Target = form.Target
+	rel.IsDraft = form.Draft
+	rel.IsPrerelease = form.Prerelease
+	rel.PublisherID = ctx.Doer.ID
+	rel.IsTag = false
+	if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil); err != nil {
+		handleTagReleaseError(err)
+		return
+	}
 	ctx.Redirect(ctx.Repo.RepoLink + "/releases")
 }
 
diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go
index 7ebea4c3fb..9f49fc7500 100644
--- a/routers/web/repo/release_test.go
+++ b/routers/web/repo/release_test.go
@@ -11,60 +11,135 @@ import (
 	"code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/models/unittest"
 	"code.gitea.io/gitea/modules/web"
+	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/contexttest"
 	"code.gitea.io/gitea/services/forms"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewReleasePost(t *testing.T) {
-	for _, testCase := range []struct {
-		RepoID  int64
-		UserID  int64
-		TagName string
-		Form    forms.NewReleaseForm
-	}{
-		{
-			RepoID:  1,
-			UserID:  2,
-			TagName: "v1.1", // pre-existing tag
-			Form: forms.NewReleaseForm{
-				TagName: "newtag",
-				Target:  "master",
-				Title:   "title",
-				Content: "content",
-			},
-		},
-		{
-			RepoID:  1,
-			UserID:  2,
-			TagName: "newtag",
-			Form: forms.NewReleaseForm{
-				TagName: "newtag",
-				Target:  "master",
-				Title:   "title",
-				Content: "content",
-			},
-		},
-	} {
-		unittest.PrepareTestEnv(t)
+	unittest.PrepareTestEnv(t)
 
+	get := func(t *testing.T, tagName string) *context.Context {
+		ctx, _ := contexttest.MockContext(t, "user2/repo1/releases/new?tag="+tagName)
+		contexttest.LoadUser(t, ctx, 2)
+		contexttest.LoadRepo(t, ctx, 1)
+		contexttest.LoadGitRepo(t, ctx)
+		defer ctx.Repo.GitRepo.Close()
+		NewRelease(ctx)
+		return ctx
+	}
+
+	t.Run("NewReleasePage", func(t *testing.T) {
+		ctx := get(t, "v1.1")
+		assert.Empty(t, ctx.Data["ShowCreateTagOnlyButton"])
+		ctx = get(t, "new-tag-name")
+		assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"])
+	})
+
+	post := func(t *testing.T, form forms.NewReleaseForm) *context.Context {
 		ctx, _ := contexttest.MockContext(t, "user2/repo1/releases/new")
 		contexttest.LoadUser(t, ctx, 2)
 		contexttest.LoadRepo(t, ctx, 1)
 		contexttest.LoadGitRepo(t, ctx)
-		web.SetForm(ctx, &testCase.Form)
+		defer ctx.Repo.GitRepo.Close()
+		web.SetForm(ctx, &form)
 		NewReleasePost(ctx)
-		unittest.AssertExistsAndLoadBean(t, &repo_model.Release{
-			RepoID:      1,
-			PublisherID: 2,
-			TagName:     testCase.Form.TagName,
-			Target:      testCase.Form.Target,
-			Title:       testCase.Form.Title,
-			Note:        testCase.Form.Content,
-		}, unittest.Cond("is_draft=?", len(testCase.Form.Draft) > 0))
-		ctx.Repo.GitRepo.Close()
+		return ctx
+	}
+
+	loadRelease := func(t *testing.T, tagName string) *repo_model.Release {
+		return unittest.GetBean(t, &repo_model.Release{}, unittest.Cond("repo_id=1 AND tag_name=?", tagName))
 	}
+
+	t.Run("NewTagRelease", func(t *testing.T) {
+		post(t, forms.NewReleaseForm{
+			TagName: "newtag",
+			Target:  "master",
+			Title:   "title",
+			Content: "content",
+		})
+		rel := loadRelease(t, "newtag")
+		require.NotNil(t, rel)
+		assert.False(t, rel.IsTag)
+		assert.Equal(t, "master", rel.Target)
+		assert.Equal(t, "title", rel.Title)
+		assert.Equal(t, "content", rel.Note)
+	})
+
+	t.Run("ReleaseExistsDoUpdate(non-tag)", func(t *testing.T) {
+		ctx := post(t, forms.NewReleaseForm{
+			TagName: "v1.1",
+			Target:  "master",
+			Title:   "updated-title",
+			Content: "updated-content",
+		})
+		rel := loadRelease(t, "v1.1")
+		require.NotNil(t, rel)
+		assert.False(t, rel.IsTag)
+		assert.Equal(t, "testing-release", rel.Title)
+		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+	})
+
+	t.Run("ReleaseExistsDoUpdate(tag-only)", func(t *testing.T) {
+		ctx := post(t, forms.NewReleaseForm{
+			TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture
+			Target:  "master",
+			Title:   "updated-title",
+			Content: "updated-content",
+			TagOnly: true,
+		})
+		rel := loadRelease(t, "delete-tag")
+		require.NotNil(t, rel)
+		assert.True(t, rel.IsTag) // the record should not be updated because the request is "tag-only". TODO: need to improve the logic?
+		assert.Equal(t, "delete-tag", rel.Title)
+		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+		assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"]) // still show the "tag-only" button
+	})
+
+	t.Run("ReleaseExistsDoUpdate(tag-release)", func(t *testing.T) {
+		ctx := post(t, forms.NewReleaseForm{
+			TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture
+			Target:  "master",
+			Title:   "updated-title",
+			Content: "updated-content",
+		})
+		rel := loadRelease(t, "delete-tag")
+		require.NotNil(t, rel)
+		assert.False(t, rel.IsTag) // the tag has been "updated" to be a real "release"
+		assert.Equal(t, "updated-title", rel.Title)
+		assert.Empty(t, ctx.Flash.ErrorMsg)
+	})
+
+	t.Run("TagOnly", func(t *testing.T) {
+		ctx := post(t, forms.NewReleaseForm{
+			TagName: "new-tag-only",
+			Target:  "master",
+			Title:   "title",
+			Content: "content",
+			TagOnly: true,
+		})
+		rel := loadRelease(t, "new-tag-only")
+		require.NotNil(t, rel)
+		assert.True(t, rel.IsTag)
+		assert.Empty(t, ctx.Flash.ErrorMsg)
+	})
+
+	t.Run("TagOnlyConflict", func(t *testing.T) {
+		ctx := post(t, forms.NewReleaseForm{
+			TagName: "v1.1",
+			Target:  "master",
+			Title:   "title",
+			Content: "content",
+			TagOnly: true,
+		})
+		rel := loadRelease(t, "v1.1")
+		require.NotNil(t, rel)
+		assert.False(t, rel.IsTag)
+		assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+	})
 }
 
 func TestCalReleaseNumCommitsBehind(t *testing.T) {