Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor issue template parsing and fix API endpoint #29069

Merged
merged 3 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion routers/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ func individualPermsChecker(ctx *context.APIContext) {
// check for and warn against deprecated authentication options
func checkDeprecatedAuthMethods(ctx *context.APIContext) {
if ctx.FormString("token") != "" || ctx.FormString("access_token") != "" {
ctx.Resp.Header().Set("Warning", "token and access_token API authentication is deprecated and will be removed in gitea 1.23. Please use AuthorizationHeaderToken instead. Existing queries will continue to work but without authorization.")
ctx.Resp.Header().Set("X-Gitea-Warning", "token and access_token API authentication is deprecated and will be removed in gitea 1.23. Please use AuthorizationHeaderToken instead. Existing queries will continue to work but without authorization.")
}
}

Expand Down
10 changes: 5 additions & 5 deletions routers/api/v1/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"net/http"
"slices"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -1161,12 +1162,11 @@ func GetIssueTemplates(ctx *context.APIContext) {
// "$ref": "#/responses/IssueTemplates"
// "404":
// "$ref": "#/responses/notFound"
ret, err := issue.GetTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetTemplatesFromDefaultBranch", err)
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
ctx.JSON(http.StatusOK, ret)
ctx.JSON(http.StatusOK, ret.IssueTemplates)
}

// GetIssueConfig returns the issue config for a repo
Expand Down
16 changes: 8 additions & 8 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -993,17 +993,17 @@ func NewIssue(ctx *context.Context) {
}
ctx.Data["Tags"] = tags

_, templateErrs := issue_service.GetTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
ret := issue_service.ParseTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
templateLoaded, errs := setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates)
for k, v := range errs {
templateErrs[k] = v
ret.TemplateErrors[k] = v
}
if ctx.Written() {
return
}

if len(templateErrs) > 0 {
ctx.Flash.Warning(renderErrorOfTemplates(ctx, templateErrs), true)
if len(ret.TemplateErrors) > 0 {
ctx.Flash.Warning(renderErrorOfTemplates(ctx, ret.TemplateErrors), true)
}

ctx.Data["HasIssuesOrPullsWritePermission"] = ctx.Repo.CanWrite(unit.TypeIssues)
Expand Down Expand Up @@ -1046,11 +1046,11 @@ func NewIssueChooseTemplate(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("repo.issues.new")
ctx.Data["PageIsIssueList"] = true

issueTemplates, errs := issue_service.GetTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
ctx.Data["IssueTemplates"] = issueTemplates
ret := issue_service.ParseTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
ctx.Data["IssueTemplates"] = ret.IssueTemplates

if len(errs) > 0 {
ctx.Flash.Warning(renderErrorOfTemplates(ctx, errs), true)
if len(ret.TemplateErrors) > 0 {
ctx.Flash.Warning(renderErrorOfTemplates(ctx, ret.TemplateErrors), true)
}

if !issue_service.HasTemplatesOrContactLinks(ctx.Repo.Repository, ctx.Repo.GitRepo) {
Expand Down
4 changes: 2 additions & 2 deletions routers/web/repo/milestone.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ func MilestoneIssuesAndPulls(ctx *context.Context) {

issues(ctx, milestoneID, projectID, util.OptionalBoolNone)

ret, _ := issue.GetTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
ctx.Data["NewIssueChooseTemplate"] = len(ret) > 0
ret := issue.ParseTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
ctx.Data["NewIssueChooseTemplate"] = len(ret.IssueTemplates) > 0

ctx.Data["CanWriteIssues"] = ctx.Repo.CanWriteIssuesOrPulls(false)
ctx.Data["CanWritePulls"] = ctx.Repo.CanWriteIssuesOrPulls(true)
Expand Down
30 changes: 16 additions & 14 deletions services/issue/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,23 @@ func IsTemplateConfig(path string) bool {
return false
}

// GetTemplatesFromDefaultBranch checks for issue templates in the repo's default branch,
// returns valid templates and the errors of invalid template files.
func GetTemplatesFromDefaultBranch(repo *repo.Repository, gitRepo *git.Repository) ([]*api.IssueTemplate, map[string]error) {
var issueTemplates []*api.IssueTemplate

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps not define an inline return struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

IssueTemplates []*api.IssueTemplate
TemplateErrors map[string]error
},
Comment on lines +114 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wxiaoguang wxiaoguang Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
ret.TemplateErrors = map[string]error{}
if repo.IsEmpty {
return issueTemplates, nil
return ret
}

commit, err := gitRepo.GetBranchCommit(repo.DefaultBranch)
if err != nil {
return issueTemplates, nil
return ret
}

invalidFiles := map[string]error{}
for _, dirName := range templateDirCandidates {
tree, err := commit.SubTree(dirName)
if err != nil {
Expand All @@ -133,24 +135,24 @@ func GetTemplatesFromDefaultBranch(repo *repo.Repository, gitRepo *git.Repositor
entries, err := tree.ListEntries()
if err != nil {
log.Debug("list entries in %s: %v", dirName, err)
return issueTemplates, nil
return ret
}
for _, entry := range entries {
if !template.CouldBe(entry.Name()) {
continue
}
fullName := path.Join(dirName, entry.Name())
if it, err := template.UnmarshalFromEntry(entry, dirName); err != nil {
invalidFiles[fullName] = err
ret.TemplateErrors[fullName] = err
} else {
if !strings.HasPrefix(it.Ref, "refs/") { // Assume that the ref intended is always a branch - for tags users should use refs/tags/<ref>
it.Ref = git.BranchPrefix + it.Ref
}
issueTemplates = append(issueTemplates, it)
ret.IssueTemplates = append(ret.IssueTemplates, it)
}
}
}
return issueTemplates, invalidFiles
return ret
}

// GetTemplateConfigFromDefaultBranch returns the issue config for this repo.
Expand Down Expand Up @@ -179,8 +181,8 @@ func GetTemplateConfigFromDefaultBranch(repo *repo.Repository, gitRepo *git.Repo
}

func HasTemplatesOrContactLinks(repo *repo.Repository, gitRepo *git.Repository) bool {
ret, _ := GetTemplatesFromDefaultBranch(repo, gitRepo)
if len(ret) > 0 {
ret := ParseTemplatesFromDefaultBranch(repo, gitRepo)
if len(ret.IssueTemplates) > 0 {
return true
}

Expand Down
55 changes: 55 additions & 0 deletions tests/integration/api_issue_templates_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2024 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package integration

import (
"net/http"
"net/url"
"testing"

repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
api "code.gitea.io/gitea/modules/structs"

"github.com/stretchr/testify/assert"
)

func TestAPIIssueTemplateList(t *testing.T) {
onGiteaRun(t, func(*testing.T, *url.URL) {
var issueTemplates []*api.IssueTemplate

user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"})
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})

// no issue template
req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/issue_templates")
resp := MakeRequest(t, req, http.StatusOK)
issueTemplates = nil
DecodeJSON(t, resp, &issueTemplates)
assert.Empty(t, issueTemplates)

// one correct issue template and some incorrect issue templates
err := createOrReplaceFileInBranch(user, repo, ".gitea/ISSUE_TEMPLATE/tmpl-ok.md", repo.DefaultBranch, `----
name: foo
about: bar
----
`)
assert.NoError(t, err)

err = createOrReplaceFileInBranch(user, repo, ".gitea/ISSUE_TEMPLATE/tmpl-err1.yml", repo.DefaultBranch, `name: '`)
assert.NoError(t, err)

err = createOrReplaceFileInBranch(user, repo, ".gitea/ISSUE_TEMPLATE/tmpl-err2.yml", repo.DefaultBranch, `other: `)
assert.NoError(t, err)

req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/issue_templates")
resp = MakeRequest(t, req, http.StatusOK)
issueTemplates = nil
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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That line should be fixed as per my suggestion above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
}
Loading