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

Use a new vars template implementation #19156

Closed
wants to merge 6 commits into from
Closed
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
11 changes: 5 additions & 6 deletions modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
asymkey_service "code.gitea.io/gitea/services/asymkey"

"github.com/editorconfig/editorconfig-core-go/v2"
"github.com/unknwon/com"
)

// IssueTemplateDirCandidates issue templates directory
Expand Down Expand Up @@ -308,11 +307,11 @@ func EarlyResponseForGoGetMeta(ctx *Context) {
ctx.PlainText(http.StatusBadRequest, "invalid repository path")
return
}
ctx.PlainText(http.StatusOK, com.Expand(`<meta name="go-import" content="{GoGetImport} git {CloneLink}">`,
map[string]string{
"GoGetImport": ComposeGoGetImport(username, reponame),
"CloneLink": repo_model.ComposeHTTPSCloneURL(username, reponame),
}))

ctx.PlainText(http.StatusOK, fmt.Sprintf(`<meta name="go-import" content="%s git %s">`,
ComposeGoGetImport(username, reponame),
repo_model.ComposeHTTPSCloneURL(username, reponame),
))
}

// RedirectToRepo redirect to a differently-named repository
Expand Down
11 changes: 9 additions & 2 deletions modules/markup/html.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"code.gitea.io/gitea/modules/markup/common"
"code.gitea.io/gitea/modules/references"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/util"

"github.com/unknwon/com"
"golang.org/x/net/html"
"golang.org/x/net/html/atom"
"mvdan.cc/xurls/v2"
Expand Down Expand Up @@ -838,7 +838,14 @@ func issueIndexPatternProcessor(ctx *RenderContext, node *html.Node) {
reftext := node.Data[ref.RefLocation.Start:ref.RefLocation.End]
if exttrack && !ref.IsPull {
ctx.Metas["index"] = ref.Issue
link = createLink(com.Expand(ctx.Metas["format"], ctx.Metas), reftext, "ref-issue ref-external-issue")

res, err := vars.Expand(ctx.Metas["format"], ctx.Metas)
if err != nil {
log.Error(err.Error())
return
}

link = createLink(res, reftext, "ref-issue ref-external-issue")
} else {
// Path determines the type of link that will be rendered. It's unknown at this point whether
// the linked item is actually a PR or an issue. Luckily it's of no real consequence because
Expand Down
9 changes: 6 additions & 3 deletions modules/repository/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/options"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/util"
asymkey_service "code.gitea.io/gitea/services/asymkey"

"github.com/unknwon/com"
)

var (
Expand Down Expand Up @@ -250,8 +249,12 @@ func prepareRepoCommit(ctx context.Context, repo *repo_model.Repository, tmpDir,
"CloneURL.HTTPS": cloneLink.HTTPS,
"OwnerName": repo.OwnerName,
}
res, err := vars.Expand(string(data), match)
if err != nil {
return fmt.Errorf("expand README.md: %v", err)
}
if err = os.WriteFile(filepath.Join(tmpDir, "README.md"),
[]byte(com.Expand(string(data), match)), 0o644); err != nil {
[]byte(res), 0o644); err != nil {
return fmt.Errorf("write README.md: %v", err)
}

Expand Down
105 changes: 105 additions & 0 deletions modules/templates/vars/vars.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2022 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package vars

import (
"fmt"
"strings"
)

// ErrWrongSyntax represents a syntax error within a template
type ErrWrongSyntax struct {
Template string
lunny marked this conversation as resolved.
Show resolved Hide resolved
Reason string
}

func (err ErrWrongSyntax) Error() string {
return fmt.Sprintf("Wrong syntax found in %s: %s", err.Template, err.Reason)
}

// IsErrWrongSyntax returns true if the error is ErrWrongSyntax
func IsErrWrongSyntax(err error) bool {
_, ok := err.(ErrWrongSyntax)
return ok
}

// ErrNoMatchedVar represents an error that no matched vars
type ErrNoMatchedVar struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is that really an error?
How do you pass anything inside curly braces then that is not supposed to be a variable then?

Template string
Var string
}

func (err ErrNoMatchedVar) Error() string {
return fmt.Sprintf("No matched variable %s found for %s", err.Var, err.Template)
}

// IsErrNoMatchedVar returns true if the error is ErrNoMatchedVar
func IsErrNoMatchedVar(err error) bool {
_, ok := err.(ErrNoMatchedVar)
return ok
}

// Expand replaces all variables like {var} to match
func Expand(template string, match map[string]string, subs ...string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

You could save all the extra trouble with the positional arguments by using something like

Suggested change
func Expand(template string, match map[string]string, subs ...string) (string, error) {
// All positional arguments will be converted into corresponding map entries: match["x"] = subs[x])
func Expand(template string, match map[string]string, subs ...string) (string, error) {
for index, substitution := range subs {
match[strconv.Itoa(index)] = substitution
}

var (
buf strings.Builder
keyStartPos = -1
)
for i, c := range template {
switch {
case c == '{':
if keyStartPos > -1 {
return "", ErrWrongSyntax{
Template: template,
lunny marked this conversation as resolved.
Show resolved Hide resolved
Reason: "\"{\" is not allowed to occur again before closing the variable",
}
}
keyStartPos = i
case c == '}':
if keyStartPos == -1 {
return "", ErrWrongSyntax{
Template: template,
lunny marked this conversation as resolved.
Show resolved Hide resolved
Reason: "\"}\" can only occur after an opening \"{\"",
}
}
if i-keyStartPos <= 1 {
return "", ErrWrongSyntax{
Template: template,
lunny marked this conversation as resolved.
Show resolved Hide resolved
Reason: "the empty variable (\"{}\") is not allowed",
}
}

if len(match) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why is that an error case? I strongly disagree that not finding the corresponding variable should return an error.
  2. Even if I count that as an error case, it is an unnecessary one as that will already be tested down below with the v, ok := match[template[keyStartPos+1:i]] implicitly.
  3. Even more so, it is implemented incorrectly as it disregards the positional arguments.

return "", ErrNoMatchedVar{
Template: template,
Var: template[keyStartPos+1 : i],
}
}

v, ok := match[template[keyStartPos+1:i]]
if !ok {
if len(subs) == 0 {
return "", ErrNoMatchedVar{
Template: template,
Var: template[keyStartPos+1 : i],
}
}
v = subs[0]
}

if _, err := buf.WriteString(v); err != nil {
return "", err
}

keyStartPos = -1
case keyStartPos > -1:
default:
if _, err := buf.WriteRune(c); err != nil {
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
return "", err
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Currently, there is an edge case missing, as an unclosed parentheses at the end (i.e. test {template end as template) would not throw a syntax error.

return buf.String(), nil
}
70 changes: 70 additions & 0 deletions modules/templates/vars/vars_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2022 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package vars

import (
"testing"

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

func TestExpandVars(t *testing.T) {
kases := []struct {
template string
maps map[string]string
expected string
fail bool
}{
{
template: "{a}",
maps: map[string]string{
"a": "1",
},
expected: "1",
},
{
template: "expand {a}, {b} and {c}",
maps: map[string]string{
"a": "1",
"b": "2",
"c": "3",
},
expected: "expand 1, 2 and 3",
},
{
template: "中文内容 {一}, {二} 和 {三} 中文结尾",
maps: map[string]string{
"一": "11",
"二": "22",
"三": "33",
},
expected: "中文内容 11, 22 和 33 中文结尾",
},
{
template: "expand {{a}, {b} and {c}",
fail: true,
},
{
template: "expand {}, {b} and {c}",
fail: true,
},
{
template: "expand }, {b} and {c}",
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, the other case is currently missing.

fail: true,
},
}

for _, kase := range kases {
t.Run(kase.template, func(t *testing.T) {
res, err := Expand(kase.template, kase.maps)
if kase.fail {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
assert.EqualValues(t, kase.expected, res)
})
}
}
41 changes: 28 additions & 13 deletions routers/web/goget.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@ package web
import (
"net/http"
"net/url"
"os"
"path"
"strings"

repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"

"github.com/unknwon/com"
)

func goGet(ctx *context.Context) {
Expand Down Expand Up @@ -67,21 +66,37 @@ func goGet(ctx *context.Context) {
}
ctx.RespHeader().Set("Content-Type", "text/html")
ctx.Status(http.StatusOK)
_, _ = ctx.Write([]byte(com.Expand(`<!doctype html>
res := os.Expand(`<!doctype html>
<html>
<head>
<meta name="go-import" content="{GoGetImport} git {CloneLink}">
<meta name="go-source" content="{GoGetImport} _ {GoDocDirectory} {GoDocFile}">
<meta name="go-import" content="${GoGetImport} git ${CloneLink}">
<meta name="go-source" content="${GoGetImport} _ ${GoDocDirectory} ${GoDocFile}">
</head>
<body>
go get {Insecure}{GoGetImport}
go get ${Insecure}${GoGetImport}
</body>
</html>
`, map[string]string{
"GoGetImport": context.ComposeGoGetImport(ownerName, trimmedRepoName),
"CloneLink": repo_model.ComposeHTTPSCloneURL(ownerName, repoName),
"GoDocDirectory": prefix + "{/dir}",
"GoDocFile": prefix + "{/dir}/{file}#L{line}",
"Insecure": insecure,
})))
`, func(key string) string {
switch key {
case "GoGetImport":
return context.ComposeGoGetImport(ownerName, trimmedRepoName)
case "CloneLink":
return repo_model.ComposeHTTPSCloneURL(ownerName, repoName)
case "GoDocDirectory":
return prefix + "{/dir}"
case "GoDocFile":
return prefix + "{/dir}/{file}#L{line}"
case "Insecure":
return insecure
default:
return `<!doctype html>
<html>
<body>
invalid import path
</body>
</html>
`
}
})
_, _ = ctx.Write([]byte(res))
}
10 changes: 7 additions & 3 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"code.gitea.io/gitea/modules/markup/markdown"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/templates/vars"
"code.gitea.io/gitea/modules/upload"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
Expand All @@ -43,8 +44,6 @@ import (
"code.gitea.io/gitea/services/forms"
issue_service "code.gitea.io/gitea/services/issue"
pull_service "code.gitea.io/gitea/services/pull"

"github.com/unknwon/com"
)

const (
Expand Down Expand Up @@ -1113,7 +1112,12 @@ func ViewIssue(ctx *context.Context) {
if extIssueUnit.ExternalTrackerConfig().ExternalTrackerStyle == markup.IssueNameStyleNumeric || extIssueUnit.ExternalTrackerConfig().ExternalTrackerStyle == "" {
metas := ctx.Repo.Repository.ComposeMetas()
metas["index"] = ctx.Params(":index")
ctx.Redirect(com.Expand(extIssueUnit.ExternalTrackerConfig().ExternalTrackerFormat, metas))
res, err := vars.Expand(extIssueUnit.ExternalTrackerConfig().ExternalTrackerFormat, metas)
if err != nil {
ctx.ServerError("Expand", fmt.Errorf("unable to expand template vars for issue url. issue: %s, err: %s", metas["index"], err.Error()))
return
}
ctx.Redirect(res)
return
}
} else if err != nil && !repo_model.IsErrUnitTypeNotExist(err) {
Expand Down