Refactor issue template parsing and fix API endpoint #29069

wxiaoguang · 2024-02-06T14:25:34Z

The old code GetTemplatesFromDefaultBranch(...) ([]*api.IssueTemplate, map[string]error) doesn't really follow Golang's habits, then the second returned value might be misused. For example, the API function GetIssueTemplates incorrectly checked the second returned value and always responds 500 error.

This PR refactors GetTemplatesFromDefaultBranch to ParseTemplatesFromDefaultBranch and clarify its behavior, and fix the API endpoint bug, and add some tests.

And by the way, add proper prefix X- for the header generated in checkDeprecatedAuthMethods, because non-standard HTTP headers should have X- prefix, and it is also consistent with the new code in GetIssueTemplates

silverwind · 2024-02-08T14:35:44Z

non-standard HTTP headers should have X- prefix

X- headers are deprecated sind 2012 as per MDN:

Custom proprietary headers have historically been used with an X- prefix, but this convention was deprecated in June 2012 because of the inconveniences it caused when nonstandard fields became standard in RFC 6648; others are listed in an IANA registry, whose original content was defined in RFC 4229. IANA also maintains a registry of proposed new HTTP headers.

Wouldn't matter for X-Gitea-Warning at least, because it's not aiming to become a web standard.

wxiaoguang · 2024-02-08T14:43:56Z

non-standard HTTP headers should have X- prefix

X- headers are deprecated sind 2012 as per MDN:
Wouldn't matter for X-Gitea-Warning at least, because it's not aiming to become a web standard.

Hmm, yes, since there are already many X- headers, I guess it's fine to follow it:

X-Gitea-OTP
X-Gitea-Object-Type
X-Total-Count
X-Content-Type-Options
X-Csrf-Token
X-Page
....

🤣

silverwind · 2024-02-08T16:33:29Z

Yeah it's no problem to keep using X- and some of these headers will never be changed. It's just forbidden for new "standard" headers to be introduced with X-, but I guess we can make an exception for exiting X-Gitea for the sake of consistency.

jolheiser · 2024-02-08T16:51:09Z

services/issue/template.go

+func ParseTemplatesFromDefaultBranch(repo *repo.Repository, gitRepo *git.Repository) (ret struct {
+	IssueTemplates []*api.IssueTemplate
+	TemplateErrors map[string]error
+},


I think I'd rather define a concrete struct type above, even if not exported.
I don't love named returns, much less an anonymous named return.

I think I'd rather define a concrete struct type above, even if not exported.

That's impossible to pass the lint, it violates "do not export private names"

I don't love named returns, much less an anonymous named return.

But I do love this usage. Anonymous struct is perfect use case for such function.

This is nearly the same thing, I'd rather nolint this case than use an anonymous struct, personally.

Sorry but I didn't get the point why "anonymous struct" is bad, we use it a lot.

The code is readable enough and there should be no maintainability problem, and no one needs to "grep" the name.

If you have better ideas, feel free to edit this PR directly.

delvh · 2024-02-08T12:48:01Z

routers/api/v1/repo/repo.go

-		return
+	ret := issue.ParseTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
+	if cnt := len(ret.TemplateErrors); cnt != 0 {
+		ctx.Resp.Header().Add("X-Gitea-Warning", "error occurs when parsing issue template: count="+strconv.Itoa(cnt))


Suggested change

ctx.Resp.Header().Add("X-Gitea-Warning", "error occurs when parsing issue template: count="+strconv.Itoa(cnt))

ctx.Resp.Header().Add("X-Gitea-Warning", fmt.Sprintf("errors occurred when parsing issue templates: %v", ret.TemplateErrors))

I think there is no end user really needs this error message, so I'd like to keep the message simple, and to avoid putting anything uncontrolled into the header.

So I'd like to keep the old code, if there is any user really needs the detailed error message, I will propose a complete solution in the future.

delvh · 2024-02-08T12:48:56Z

services/issue/template.go

-
+// ParseTemplatesFromDefaultBranch parses the issue templates in the repo's default branch,
+// returns valid templates and the errors of invalid template files (the errors map is guaranteed to be non-nil).
+func ParseTemplatesFromDefaultBranch(repo *repo.Repository, gitRepo *git.Repository) (ret struct {


Could we perhaps not define an inline return struct?

See: #29069 (comment)

Anonymous struct is perfect use case for such function, I prefer to use this syntax here.

But if you have better ideas, feel free to edit this PR directly.

delvh · 2024-02-08T12:50:05Z

tests/integration/api_issue_templates_test.go

+		DecodeJSON(t, resp, &issueTemplates)
+		assert.Len(t, issueTemplates, 1)
+		assert.Equal(t, "foo", issueTemplates[0].Name)
+		assert.Equal(t, "error occurs when parsing issue template: count=2", resp.Header().Get("X-Gitea-Warning"))


That line should be fixed as per my suggestion above

See #29069 (comment)

I'd like to keep things simple at the moment, until someone really needs it, then it needs a complete solution for "warning" messages.

tests/integration/api_issue_templates_test.go

GiteaBot · 2024-02-12T05:04:14Z

I was unable to create a backport for 1.21. @wxiaoguang, please send one manually. 🍵

go run ./contrib/backport 29069
...  // fix git conflicts if any
go run ./contrib/backport --continue

The old code `GetTemplatesFromDefaultBranch(...) ([]*api.IssueTemplate, map[string]error)` doesn't really follow Golang's habits, then the second returned value might be misused. For example, the API function `GetIssueTemplates` incorrectly checked the second returned value and always responds 500 error. This PR refactors GetTemplatesFromDefaultBranch to ParseTemplatesFromDefaultBranch and clarifies its behavior, and fixes the API endpoint bug, and adds some tests. And by the way, add proper prefix `X-` for the header generated in `checkDeprecatedAuthMethods`, because non-standard HTTP headers should have `X-` prefix, and it is also consistent with the new code in `GetIssueTemplates` # Conflicts: # routers/web/repo/issue.go

Backport #29069 The old code `GetTemplatesFromDefaultBranch(...) ([]*api.IssueTemplate, map[string]error)` doesn't really follow Golang's habits, then the second returned value might be misused. For example, the API function `GetIssueTemplates` incorrectly checked the second returned value and always responds 500 error. This PR refactors GetTemplatesFromDefaultBranch to ParseTemplatesFromDefaultBranch and clarifies its behavior, and fixes the API endpoint bug, and adds some tests. And by the way, add proper prefix `X-` for the header generated in `checkDeprecatedAuthMethods`, because non-standard HTTP headers should have `X-` prefix, and it is also consistent with the new code in `GetIssueTemplates`

* giteaofficial/main: (38 commits) Document how the TOC election process works (go-gitea#29135) Runner tokens are multi use (go-gitea#29153) Fix Gitpod logic of setting ROOT_URL (go-gitea#29162) Remove jQuery from the user search form in admin page (go-gitea#29151) Dont load Review if Comment is CommentTypeReviewRequest (go-gitea#28551) Show `View at this point in history` for every commit (go-gitea#29122) [skip ci] Updated translations via Crowdin Add merge style `fast-forward-only` (go-gitea#28954) Use Markdown alert syntax for notes in README (go-gitea#29150) Refactor issue template parsing and fix API endpoint (go-gitea#29069) [skip ci] Updated translations via Crowdin Update some translations and fix markdown formatting (go-gitea#29099) Show more settings for empty repositories (go-gitea#29130) Update JS and PY dependencies (go-gitea#29127) Add alert blocks in markdown (go-gitea#29121) Remove obsolete border-radius on comment content (go-gitea#29128) Make blockquote border size less aggressive (go-gitea#29124) Drop "@" from email sender to avoid spam filters (go-gitea#29109) [skip ci] Updated translations via Crowdin Disallow duplicate storage paths (go-gitea#26484) ...

The old code `GetTemplatesFromDefaultBranch(...) ([]*api.IssueTemplate, map[string]error)` doesn't really follow Golang's habits, then the second returned value might be misused. For example, the API function `GetIssueTemplates` incorrectly checked the second returned value and always responds 500 error. This PR refactors GetTemplatesFromDefaultBranch to ParseTemplatesFromDefaultBranch and clarifies its behavior, and fixes the API endpoint bug, and adds some tests. And by the way, add proper prefix `X-` for the header generated in `checkDeprecatedAuthMethods`, because non-standard HTTP headers should have `X-` prefix, and it is also consistent with the new code in `GetIssueTemplates`

github-actions · 2024-03-01T00:06:14Z

Automatically locked because of our CONTRIBUTING guidelines

refactor

a6b839e

wxiaoguang added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. backport/v1.21 This PR should be backported to Gitea 1.21 labels Feb 6, 2024

wxiaoguang added this to the 1.22.0 milestone Feb 6, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2024

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 6, 2024

github-actions bot added the modifies/api This PR adds API routes or modifies them label Feb 6, 2024

lunny approved these changes Feb 8, 2024

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 8, 2024

jolheiser reviewed Feb 8, 2024

View reviewed changes

delvh reviewed Feb 8, 2024

View reviewed changes

delvh approved these changes Feb 9, 2024

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 9, 2024

Merge branch 'main' into refactor-issue-template

4deed36

wxiaoguang commented Feb 12, 2024

View reviewed changes

tests/integration/api_issue_templates_test.go Outdated Show resolved Hide resolved

Update tests/integration/api_issue_templates_test.go

5a03583

wxiaoguang enabled auto-merge (squash) February 12, 2024 04:52

wxiaoguang merged commit ee242a0 into go-gitea:main Feb 12, 2024
26 checks passed

GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Feb 12, 2024

wxiaoguang deleted the refactor-issue-template branch February 12, 2024 05:09

wxiaoguang mentioned this pull request Feb 12, 2024

Refactor issue template parsing and fix API endpoint (#29069) #29140

Merged

wxiaoguang added the backport/done All backports for this PR have been created label Feb 12, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor issue template parsing and fix API endpoint #29069

Refactor issue template parsing and fix API endpoint #29069

wxiaoguang commented Feb 6, 2024 •

edited

Loading

silverwind commented Feb 8, 2024 •

edited

Loading

wxiaoguang commented Feb 8, 2024

silverwind commented Feb 8, 2024 •

edited

Loading

jolheiser Feb 8, 2024

wxiaoguang Feb 9, 2024

jolheiser Feb 9, 2024

wxiaoguang Feb 9, 2024 •

edited

Loading

delvh Feb 8, 2024

wxiaoguang Feb 9, 2024

delvh Feb 8, 2024

wxiaoguang Feb 9, 2024

delvh Feb 8, 2024

wxiaoguang Feb 9, 2024

GiteaBot commented Feb 12, 2024

github-actions bot commented Mar 1, 2024

	ctx.Resp.Header().Add("X-Gitea-Warning", "error occurs when parsing issue template: count="+strconv.Itoa(cnt))
	ctx.Resp.Header().Add("X-Gitea-Warning", fmt.Sprintf("errors occurred when parsing issue templates: %v", ret.TemplateErrors))

Refactor issue template parsing and fix API endpoint #29069

Refactor issue template parsing and fix API endpoint #29069

Conversation

wxiaoguang commented Feb 6, 2024 • edited Loading

silverwind commented Feb 8, 2024 • edited Loading

wxiaoguang commented Feb 8, 2024

silverwind commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxiaoguang Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiteaBot commented Feb 12, 2024

github-actions bot commented Mar 1, 2024

wxiaoguang commented Feb 6, 2024 •

edited

Loading

silverwind commented Feb 8, 2024 •

edited

Loading

silverwind commented Feb 8, 2024 •

edited

Loading

wxiaoguang Feb 9, 2024 •

edited

Loading