Validate that the tag doesn't exist when creating a tag via the web

[kemzeb]

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.

[kemzeb]

To reproduce the bug, you can do the following:

• Create a tag using the Web UI
• Create the tag again using the Web UI
• The tag will now be considered a release

[wxiaoguang]

To reproduce the bug, you can do the following:

* Create a tag using the Web UI

* Create the tag again using the Web UI

* The tag will now be considered a release

Is it possible to write a test for this case?

[wxiaoguang]

The "type" magic could be left to the future ....... (to make it easier to backport)

[wxiaoguang]

2 approvals now, would you like to merge it as-is, or there are still changes to do?

[kemzeb]

Implementing test and changes 👍

[wxiaoguang]

Added your finding as comment in 7a5595c

[kemzeb]

Hmm just realized the Create Tag Only button does not appear when the validation error occurs.

But it also disappears with other form validation errors (while running my dev instance on the main branch):

It looks like it's a combination of:

• Passing the tag_name form field to the page that is sent back to the client
• This line in the template file

[wxiaoguang]

If we need to handle the "error form", I think we can use "form-fetch-action", then we do not need to reload the page or re-fill the form, then the logic could be simpler?

[wxiaoguang]

{{if not .tag_name}} was added by "Create tag on ui (#13467)", at that time, the {{if .PageIsEditRelease}} check doesn't control the "tag only" button, so it needs to hide the "tag only" button for "edit tag" (non-empty tag_name means "edit")

But now, the "tag only" button is in {{if .PageIsEditRelease}}{{else}} tag only {{end}}, so it doesn't need to be controlled by "tag_name" anymore.

[wxiaoguang]

By reading the code again, I found there are too many legacy bugs and I think it's worth to rewrite it. Major changes:

1. always prepare tmpl data correctly (even if error occurs)
2. correctly show/hide the "tag only" button (it is only shown for "new" page when there is no existing release)
3. correctly handle create/update errors and page render

---

Maybe it's not easy to backport, if we need the backport, we could take the simple fix before the large change ....... hmm, simple fix seems not quite right.

[wxiaoguang]

Done from my side, added some tests and comments, does the new logic look good to you?

[kemzeb]

Big fan of the test refactor. Everything looks good to me, the only problem that I see now is the "Create Tag Only" problem we discussed earlier:

To reproduce:

• Have an existing tag e.g. "v1.0"
• Try creating a tag with the same name

It looks like this happens because the "error form" page we return now has a tag_name that points to an existing Release, which would set the highlighted line to true.

Would it make sense to set this to something like:

Suggested change

	ctx.Data["TagNameReleaseExists"] = rel != nil
	ctx.Data["TagNameReleaseExists"] = rel != nil && !rel.IsTag

[wxiaoguang]

Hmm, I think the problem is: we shouldn't stop users from creating new "tag-only" if the error occurs, no matter whether the release exists. So I think this line should be completely removed.

[wxiaoguang]

Made some changes in 57aef6d

// 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.
ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil

Does it sound right?

[kemzeb]

Yes it sounds about right

Hmm, I think the problem is: we shouldn't stop users from creating new "tag-only" if the error occurs, no matter whether the release exists. So I think this line should be completely removed.

Hmm it looks like the only case where we do hide it is when a tag_name field is passed which refers to an existing tag (I guess we hide it to avoid any confusion by users). I think it is fine to follow your alternative approach and have the button always appear since the user would be shown an error in case they do press it, but it's up to you

[wxiaoguang]

Not easy to backport and the changes are quite a lot.

Maybe we only keep this fix in 1.24? (If there is a strong requirement to backport .... I could try to manually backport .....)