Skip to content

Commit

Permalink
Clarify path param naming (#32969)
Browse files Browse the repository at this point in the history
In history (from some legacy frameworks), both `:name` and `name` are
supported as path path name, `:name` is an alias to `name`.

To make code consistent, now we should only use `name` but not `:name`.

Also added panic check in related functions to make sure the name won't
be abused in case some downstreams still use them.
  • Loading branch information
wxiaoguang authored Dec 24, 2024
1 parent b8b690f commit 2a828e2
Show file tree
Hide file tree
Showing 102 changed files with 461 additions and 429 deletions.
6 changes: 6 additions & 0 deletions modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,9 @@ func checkOverlappedPath(name, path string) {
}
configuredPaths[path] = name
}

func PanicInDevOrTesting(msg string, a ...any) {
if !IsProd || IsInTesting {
panic(fmt.Sprintf(msg, a...))
}
}
4 changes: 1 addition & 3 deletions modules/templates/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,5 @@ func QueryBuild(a ...any) template.URL {
}

func panicIfDevOrTesting() {
if !setting.IsProd || setting.IsInTesting {
panic("legacy template functions are for backward compatibility only, do not use them in new code")
}
setting.PanicInDevOrTesting("legacy template functions are for backward compatibility only, do not use them in new code")
}
8 changes: 4 additions & 4 deletions routers/api/v1/admin/adopt.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ func AdoptRepository(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"
// "403":
// "$ref": "#/responses/forbidden"
ownerName := ctx.PathParam(":username")
repoName := ctx.PathParam(":reponame")
ownerName := ctx.PathParam("username")
repoName := ctx.PathParam("reponame")

ctxUser, err := user_model.GetUserByName(ctx, ownerName)
if err != nil {
Expand Down Expand Up @@ -142,8 +142,8 @@ func DeleteUnadoptedRepository(ctx *context.APIContext) {
// "$ref": "#/responses/empty"
// "403":
// "$ref": "#/responses/forbidden"
ownerName := ctx.PathParam(":username")
repoName := ctx.PathParam(":reponame")
ownerName := ctx.PathParam("username")
repoName := ctx.PathParam("reponame")

ctxUser, err := user_model.GetUserByName(ctx, ownerName)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/admin/cron.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func PostCronTask(ctx *context.APIContext) {
// "$ref": "#/responses/empty"
// "404":
// "$ref": "#/responses/notFound"
task := cron.GetTask(ctx.PathParam(":task"))
task := cron.GetTask(ctx.PathParam("task"))
if task == nil {
ctx.NotFound()
return
Expand Down
4 changes: 2 additions & 2 deletions routers/api/v1/admin/email.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func GetAllEmails(ctx *context.APIContext) {
listOptions := utils.GetListOptions(ctx)

emails, maxResults, err := user_model.SearchEmails(ctx, &user_model.SearchEmailOptions{
Keyword: ctx.PathParam(":email"),
Keyword: ctx.PathParam("email"),
ListOptions: listOptions,
})
if err != nil {
Expand Down Expand Up @@ -82,6 +82,6 @@ func SearchEmail(ctx *context.APIContext) {
// "403":
// "$ref": "#/responses/forbidden"

ctx.SetPathParam(":email", ctx.FormTrim("q"))
ctx.SetPathParam("email", ctx.FormTrim("q"))
GetAllEmails(ctx)
}
6 changes: 3 additions & 3 deletions routers/api/v1/admin/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func GetHook(ctx *context.APIContext) {
// "200":
// "$ref": "#/responses/Hook"

hookID := ctx.PathParamInt64(":id")
hookID := ctx.PathParamInt64("id")
hook, err := webhook.GetSystemOrDefaultWebhook(ctx, hookID)
if err != nil {
if errors.Is(err, util.ErrNotExist) {
Expand Down Expand Up @@ -142,7 +142,7 @@ func EditHook(ctx *context.APIContext) {
form := web.GetForm(ctx).(*api.EditHookOption)

// TODO in body params
hookID := ctx.PathParamInt64(":id")
hookID := ctx.PathParamInt64("id")
utils.EditSystemHook(ctx, form, hookID)
}

Expand All @@ -164,7 +164,7 @@ func DeleteHook(ctx *context.APIContext) {
// "204":
// "$ref": "#/responses/empty"

hookID := ctx.PathParamInt64(":id")
hookID := ctx.PathParamInt64("id")
if err := webhook.DeleteDefaultSystemWebhook(ctx, hookID); err != nil {
if errors.Is(err, util.ErrNotExist) {
ctx.NotFound()
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/admin/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ func DeleteUserPublicKey(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

if err := asymkey_service.DeletePublicKey(ctx, ctx.ContextUser, ctx.PathParamInt64(":id")); err != nil {
if err := asymkey_service.DeletePublicKey(ctx, ctx.ContextUser, ctx.PathParamInt64("id")); err != nil {
if asymkey_model.IsErrKeyNotExist(err) {
ctx.NotFound()
} else if asymkey_model.IsErrKeyAccessDenied(err) {
Expand Down
8 changes: 4 additions & 4 deletions routers/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,12 @@ func orgAssignment(args ...bool) func(ctx *context.APIContext) {

var err error
if assignOrg {
ctx.Org.Organization, err = organization.GetOrgByName(ctx, ctx.PathParam(":org"))
ctx.Org.Organization, err = organization.GetOrgByName(ctx, ctx.PathParam("org"))
if err != nil {
if organization.IsErrOrgNotExist(err) {
redirectUserID, err := user_model.LookupUserRedirect(ctx, ctx.PathParam(":org"))
redirectUserID, err := user_model.LookupUserRedirect(ctx, ctx.PathParam("org"))
if err == nil {
context.RedirectToUser(ctx.Base, ctx.PathParam(":org"), redirectUserID)
context.RedirectToUser(ctx.Base, ctx.PathParam("org"), redirectUserID)
} else if user_model.IsErrUserRedirectNotExist(err) {
ctx.NotFound("GetOrgByName", err)
} else {
Expand All @@ -616,7 +616,7 @@ func orgAssignment(args ...bool) func(ctx *context.APIContext) {
}

if assignTeam {
ctx.Org.Team, err = organization.GetTeamByID(ctx, ctx.PathParamInt64(":teamid"))
ctx.Org.Team, err = organization.GetTeamByID(ctx, ctx.PathParamInt64("teamid"))
if err != nil {
if organization.IsErrTeamNotExist(err) {
ctx.NotFound()
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/notify/threads.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func ReadThread(ctx *context.APIContext) {
}

func getThread(ctx *context.APIContext) *activities_model.Notification {
n, err := activities_model.GetNotificationByID(ctx, ctx.PathParamInt64(":id"))
n, err := activities_model.GetNotificationByID(ctx, ctx.PathParamInt64("id"))
if err != nil {
if db.IsErrNotExist(err) {
ctx.Error(http.StatusNotFound, "GetNotificationByID", err)
Expand Down
6 changes: 3 additions & 3 deletions routers/api/v1/org/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func GetLabel(ctx *context.APIContext) {
label *issues_model.Label
err error
)
strID := ctx.PathParam(":id")
strID := ctx.PathParam("id")
if intID, err2 := strconv.ParseInt(strID, 10, 64); err2 != nil {
label, err = issues_model.GetLabelInOrgByName(ctx, ctx.Org.Organization.ID, strID)
} else {
Expand Down Expand Up @@ -190,7 +190,7 @@ func EditLabel(ctx *context.APIContext) {
// "422":
// "$ref": "#/responses/validationError"
form := web.GetForm(ctx).(*api.EditLabelOption)
l, err := issues_model.GetLabelInOrgByID(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64(":id"))
l, err := issues_model.GetLabelInOrgByID(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64("id"))
if err != nil {
if issues_model.IsErrOrgLabelNotExist(err) {
ctx.NotFound()
Expand Down Expand Up @@ -249,7 +249,7 @@ func DeleteLabel(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

if err := issues_model.DeleteLabel(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64(":id")); err != nil {
if err := issues_model.DeleteLabel(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64("id")); err != nil {
ctx.Error(http.StatusInternalServerError, "DeleteLabel", err)
return
}
Expand Down
10 changes: 5 additions & 5 deletions routers/api/v1/org/member.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func IsMember(ctx *context.APIContext) {
// "404":
// description: user is not a member

userToCheck := user.GetUserByParams(ctx)
userToCheck := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -194,7 +194,7 @@ func IsPublicMember(ctx *context.APIContext) {
// "404":
// description: user is not a public member

userToCheck := user.GetUserByParams(ctx)
userToCheck := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -236,7 +236,7 @@ func PublicizeMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

userToPublicize := user.GetUserByParams(ctx)
userToPublicize := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -278,7 +278,7 @@ func ConcealMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

userToConceal := user.GetUserByParams(ctx)
userToConceal := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -318,7 +318,7 @@ func DeleteMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

member := user.GetUserByParams(ctx)
member := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/org/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func GetUserOrgsPermissions(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"

var o *user_model.User
if o = user.GetUserByParamsName(ctx, ":org"); o == nil {
if o = user.GetUserByPathParam(ctx, "org"); o == nil {
return
}

Expand Down
8 changes: 4 additions & 4 deletions routers/api/v1/org/team.go
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ func GetTeamMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

u := user.GetUserByParams(ctx)
u := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -492,7 +492,7 @@ func AddTeamMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

u := user.GetUserByParams(ctx)
u := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -532,7 +532,7 @@ func RemoveTeamMember(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

u := user.GetUserByParams(ctx)
u := user.GetContextUserByPathParam(ctx)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -645,7 +645,7 @@ func GetTeamRepo(ctx *context.APIContext) {

// getRepositoryByParams get repository by a team's organization ID and repo name
func getRepositoryByParams(ctx *context.APIContext) *repo_model.Repository {
repo, err := repo_model.GetRepositoryByName(ctx, ctx.Org.Team.OrgID, ctx.PathParam(":reponame"))
repo, err := repo_model.GetRepositoryByName(ctx, ctx.Org.Team.OrgID, ctx.PathParam("reponame"))
if err != nil {
if repo_model.IsErrRepoNotExist(err) {
ctx.NotFound()
Expand Down
6 changes: 3 additions & 3 deletions routers/api/v1/repo/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ func GetBranchProtection(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"

repo := ctx.Repo.Repository
bpName := ctx.PathParam(":name")
bpName := ctx.PathParam("name")
bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err)
Expand Down Expand Up @@ -805,7 +805,7 @@ func EditBranchProtection(ctx *context.APIContext) {
// "$ref": "#/responses/repoArchivedError"
form := web.GetForm(ctx).(*api.EditBranchProtectionOption)
repo := ctx.Repo.Repository
bpName := ctx.PathParam(":name")
bpName := ctx.PathParam("name")
protectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err)
Expand Down Expand Up @@ -1124,7 +1124,7 @@ func DeleteBranchProtection(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"

repo := ctx.Repo.Repository
bpName := ctx.PathParam(":name")
bpName := ctx.PathParam("name")
bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchByID", err)
Expand Down
10 changes: 5 additions & 5 deletions routers/api/v1/repo/collaborators.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func IsCollaborator(ctx *context.APIContext) {
// "422":
// "$ref": "#/responses/validationError"

user, err := user_model.GetUserByName(ctx, ctx.PathParam(":collaborator"))
user, err := user_model.GetUserByName(ctx, ctx.PathParam("collaborator"))
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
Expand Down Expand Up @@ -163,7 +163,7 @@ func AddOrUpdateCollaborator(ctx *context.APIContext) {

form := web.GetForm(ctx).(*api.AddCollaboratorOption)

collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam(":collaborator"))
collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam("collaborator"))
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
Expand Down Expand Up @@ -226,7 +226,7 @@ func DeleteCollaborator(ctx *context.APIContext) {
// "422":
// "$ref": "#/responses/validationError"

collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam(":collaborator"))
collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam("collaborator"))
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.Error(http.StatusUnprocessableEntity, "", err)
Expand Down Expand Up @@ -274,12 +274,12 @@ func GetRepoPermissions(ctx *context.APIContext) {
// "403":
// "$ref": "#/responses/forbidden"

if !ctx.Doer.IsAdmin && ctx.Doer.LoginName != ctx.PathParam(":collaborator") && !ctx.IsUserRepoAdmin() {
if !ctx.Doer.IsAdmin && ctx.Doer.LoginName != ctx.PathParam("collaborator") && !ctx.IsUserRepoAdmin() {
ctx.Error(http.StatusForbidden, "User", "Only admins can query all permissions, repo admins can query all repo permissions, collaborators can query only their own")
return
}

collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam(":collaborator"))
collaborator, err := user_model.GetUserByName(ctx, ctx.PathParam("collaborator"))
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.Error(http.StatusNotFound, "GetUserByName", err)
Expand Down
6 changes: 3 additions & 3 deletions routers/api/v1/repo/commits.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func GetSingleCommit(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

sha := ctx.PathParam(":sha")
sha := ctx.PathParam("sha")
if !git.IsValidRefPattern(sha) {
ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
return
Expand Down Expand Up @@ -312,8 +312,8 @@ func DownloadCommitDiffOrPatch(ctx *context.APIContext) {
// "$ref": "#/responses/string"
// "404":
// "$ref": "#/responses/notFound"
sha := ctx.PathParam(":sha")
diffType := git.RawDiffType(ctx.PathParam(":diffType"))
sha := ctx.PathParam("sha")
diffType := git.RawDiffType(ctx.PathParam("diffType"))

if err := git.GetRawDiff(ctx.Repo.GitRepo, sha, diffType, ctx.Resp); err != nil {
if git.IsErrNotExist(err) {
Expand Down
6 changes: 3 additions & 3 deletions routers/api/v1/repo/git_hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func GetGitHook(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

hookID := ctx.PathParam(":id")
hookID := ctx.PathParam("id")
hook, err := ctx.Repo.GitRepo.GetHook(hookID)
if err != nil {
if err == git.ErrNotValidHook {
Expand Down Expand Up @@ -126,7 +126,7 @@ func EditGitHook(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"

form := web.GetForm(ctx).(*api.EditGitHookOption)
hookID := ctx.PathParam(":id")
hookID := ctx.PathParam("id")
hook, err := ctx.Repo.GitRepo.GetHook(hookID)
if err != nil {
if err == git.ErrNotValidHook {
Expand Down Expand Up @@ -175,7 +175,7 @@ func DeleteGitHook(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

hookID := ctx.PathParam(":id")
hookID := ctx.PathParam("id")
hook, err := ctx.Repo.GitRepo.GetHook(hookID)
if err != nil {
if err == git.ErrNotValidHook {
Expand Down
Loading

0 comments on commit 2a828e2

Please sign in to comment.