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>

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,34 +329,17 @@ 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
-	ctx.Data["tag_target"] = ctx.Repo.Repository.DefaultBranch
-	if tagName := ctx.FormString("tag"); len(tagName) > 0 {
-		rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, tagName)
-		if err != nil && !repo_model.IsErrReleaseNotExist(err) {
-			ctx.ServerError("GetRelease", err)
-			return
-		}
 
-		if rel != nil {
-			rel.Repo = ctx.Repo.Repository
-			if err := rel.LoadAttributes(ctx); err != nil {
-				ctx.ServerError("LoadAttributes", err)
-				return
-			}
-
-			ctx.Data["tag_name"] = rel.TagName
-			if rel.Target != "" {
-				ctx.Data["tag_target"] = rel.Target
-			}
-			ctx.Data["title"] = rel.Title
-			ctx.Data["content"] = rel.Note
-			ctx.Data["attachments"] = rel.Attachments
-		}
+	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 {
@@ -368,35 +350,74 @@ func NewRelease(ctx *context.Context) {
 
 	upload.AddUploadContext(ctx, "release")
 
-	// For New Release page
-	PrepareBranchList(ctx)
+	PrepareBranchList(ctx) // for New Release page
+}
+
+// NewRelease render creating or edit release page
+func NewRelease(ctx *context.Context) {
+	newReleaseCommon(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["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"); tagName != "" {
+		rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, tagName)
+		if err != nil && !repo_model.IsErrReleaseNotExist(err) {
+			ctx.ServerError("GetRelease", err)
+			return
+		}
+
+		if rel != nil {
+			rel.Repo = ctx.Repo.Repository
+			if err = rel.LoadAttributes(ctx); err != nil {
+				ctx.ServerError("LoadAttributes", err)
+				return
+			}
+
+			ctx.Data["ShowCreateTagOnlyButton"] = false
+			ctx.Data["tag_name"] = rel.TagName
+			ctx.Data["tag_target"] = rel.Target
+			ctx.Data["title"] = rel.Title
+			ctx.Data["content"] = rel.Note
+			ctx.Data["attachments"] = rel.Attachments
+		}
 	}
-	ctx.Data["Tags"] = tags
 
 	ctx.HTML(http.StatusOK, tplReleaseNew)
 }
 
 // NewReleasePost response for creating a release
 func NewReleasePost(ctx *context.Context) {
-	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)
+	newReleaseCommon(ctx)
+	if ctx.Written() {
 		return
 	}
-	ctx.Data["Tags"] = tags
 
+	form := web.GetForm(ctx).(*forms.NewReleaseForm)
+
+	// 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
+	}
+
+	// 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
+	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)
+		}
 	}
 
-	rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName)
-	if err != nil {
-		if !repo_model.IsErrReleaseNotExist(err) {
-			ctx.ServerError("GetRelease", err)
+	// prepare the git message for creating a new tag
+	newTagMsg := ""
+	if form.Title != "" && form.AddTagMsg {
+		newTagMsg = form.Title + "\n\n" + form.Content
+	}
+
+	// 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
+	}
 
-		msg := ""
-		if len(form.Title) > 0 && form.AddTagMsg {
-			msg = form.Title + "\n\n" + form.Content
-		}
-
-		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
-			}
-
-			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)
-			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)
+		if err = release_service.CreateRelease(ctx.Repo.GitRepo, rel, attachmentUUIDs, newTagMsg); err != nil {
+			handleTagReleaseError(err)
 			return
 		}
+		ctx.Redirect(ctx.Repo.RepoLink + "/releases")
+		return
 	}
-	log.Trace("Release created: %s/%s:%s", ctx.Doer.LowerName, ctx.Repo.Repository.Name, form.TagName)
 
+	// 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
+	}
+
+	// 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")
 }
 

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) {

services/forms/repo_form.go

@@ -651,8 +651,8 @@ type NewReleaseForm struct {
 	Target     string `form:"tag_target" binding:"Required;MaxSize(255)"`
 	Title      string `binding:"MaxSize(255)"`
 	Content    string
-	Draft      string
-	TagOnly    string
+	Draft      bool
+	TagOnly    bool
 	Prerelease bool
 	AddTagMsg  bool
 	Files      []string

templates/repo/release/new.tmpl

@@ -109,23 +109,17 @@
 								{{ctx.Locale.Tr "repo.release.delete_release"}}
 							</a>
 							{{if .IsDraft}}
-								<button class="ui small button" type="submit" name="draft" value="{{ctx.Locale.Tr "repo.release.save_draft"}}">{{ctx.Locale.Tr "repo.release.save_draft"}}</button>
-								<button class="ui small primary button">
-									{{ctx.Locale.Tr "repo.release.publish"}}
-								</button>
+								<button class="ui small button" type="submit" name="draft" value="1">{{ctx.Locale.Tr "repo.release.save_draft"}}</button>
+								<button class="ui small primary button">{{ctx.Locale.Tr "repo.release.publish"}}</button>
 							{{else}}
-								<button class="ui small primary button">
-									{{ctx.Locale.Tr "repo.release.edit_release"}}
-								</button>
+								<button class="ui small primary button">{{ctx.Locale.Tr "repo.release.edit_release"}}</button>
 							{{end}}
 						{{else}}
-							{{if not .tag_name}}
+							{{if .ShowCreateTagOnlyButton}}
 								<button class="ui small button" name="tag_only" value="1">{{ctx.Locale.Tr "repo.release.add_tag"}}</button>
 							{{end}}
 							<button class="ui small button" name="draft" value="1">{{ctx.Locale.Tr "repo.release.save_draft"}}</button>
-							<button class="ui small primary button">
-								{{ctx.Locale.Tr "repo.release.publish"}}
-							</button>
+							<button class="ui small primary button">{{ctx.Locale.Tr "repo.release.publish"}}</button>
 						{{end}}
 					</div>
 				</div>

tests/integration/release_test.go

@@ -39,7 +39,7 @@ func createNewRelease(t *testing.T, session *TestSession, repoURL, tag, title st
 		postData["prerelease"] = "on"
 	}
 	if draft {
-		postData["draft"] = "Save Draft"
+		postData["draft"] = "1"
 	}
 	req = NewRequestWithValues(t, "POST", link, postData)