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.

Sign up for GitHub

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

Raw file API: deprecate {ref} path parameter + allow shortened commit refs #17185

Closed
wants to merge 6 commits into from
Closed

Raw file API: deprecate {ref} path parameter + allow shortened commit refs #17185

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2e128ba
put auth middleware before any parsing
noerw Sep 29, 2021
d4fc683
raw file api: clarify documentation
noerw Sep 29, 2021
df834a7
raw file API: add support for shortened commit SHA
noerw Sep 29, 2021
e1e33a8
add test for short shas
noerw Sep 29, 2021
436262f
Merge branch 'main' into fix-17179
zeripath Oct 20, 2021
e2903f4
Merge branch 'main' into fix-17179
lunny Nov 9, 2021
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
1 change: 1 addition & 0 deletions integrations/api_repo_raw_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ func TestAPIReposRaw(t *testing.T) {
"master", // Branch
"v1.1", // Tag
"65f1bf27bc3bf70f64657658635e66094edbcb4d", // Commit
"65f1bf2", // Shortened Commit
} {
req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/raw/%s/README.md?token="+token, user.Name, ref)
session.MakeRequest(t, req, http.StatusOK)
Expand Down
2 changes: 1 addition & 1 deletion modules/context/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ func RepoRefForAPI(next http.Handler) http.Handler {
return
}
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
} else if len(refName) == 40 {
} else if len(refName) >= 7 && len(refName) <= 40 {
Copy link
Member

Choose a reason for hiding this comment

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

I doubt that that will improve the situation.
"Great. Now I cannot even get the README.md".
Maybe it would be better to check whether that file is tracked when the if three lines below is entered before returning the error.

Or do I misunderstand what this is supposed to do?
Regarding the else block below, I am fairly certain that I am misunderstanding something here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think hard-coding the length is a good idea. Maybe git will use SHA256 in future.

https://stackoverflow.com/questions/28159071/why-doesnt-git-use-more-modern-sha

Copy link
Member Author

@noerw noerw Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@delvh no this won't happen, as we check if such a commit exists. If not, getRefName() falls back to treating the entire thing as a TreePath and returns Repo.DefaultBranch as ref, in which case we don't enter this if-clause at all.
@wxiaoguang this is all over the codebase, definitily a different issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can introduce a function like IsPossibleGitCommit and use it from now on. It's easier to understand and maintain.

ctx.Repo.CommitID = refName
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
if err != nil {
Expand Down
10 changes: 4 additions & 6 deletions modules/context/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,11 +693,8 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
if refName := getRefName(ctx, RepoRefTag); len(refName) > 0 {
return refName
}
// For legacy and API support only full commit sha
parts := strings.Split(path, "/")
if len(parts) > 0 && len(parts[0]) == 40 {
ctx.Repo.TreePath = strings.Join(parts[1:], "/")
return parts[0]
if refName := getRefName(ctx, RepoRefCommit); len(refName) > 0 {
return refName
}
if refName := getRefName(ctx, RepoRefBlob); len(refName) > 0 {
return refName
Expand All @@ -710,7 +707,8 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
return getRefNameFromPath(ctx, path, ctx.Repo.GitRepo.IsTagExist)
case RepoRefCommit:
parts := strings.Split(path, "/")
if len(parts) > 0 && len(parts[0]) >= 7 && len(parts[0]) <= 40 {
hasShaLength := len(parts[0]) >= 7 && len(parts[0]) <= 40
if len(parts) > 1 && hasShaLength && ctx.Repo.GitRepo.IsCommitExist(parts[0]) {
ctx.Repo.TreePath = strings.Join(parts[1:], "/")
return parts[0]
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ func Routes(sessioner func(http.Handler) http.Handler) *web.Route {
Put(reqAdmin(), repo.AddTeam).
Delete(reqAdmin(), repo.DeleteTeam)
}, reqToken())
m.Get("/raw/*", context.RepoRefForAPI, reqRepoReader(models.UnitTypeCode), repo.GetRawFile)
m.Get("/raw/*", reqRepoReader(models.UnitTypeCode), context.RepoRefForAPI, repo.GetRawFile)
m.Get("/archive/*", reqRepoReader(models.UnitTypeCode), repo.GetArchive)
m.Combo("/forks").Get(repo.ListForks).
Post(reqToken(), reqRepoReader(models.UnitTypeCode), bind(api.CreateForkOption{}), repo.CreateFork)
Expand Down
35 changes: 34 additions & 1 deletion routers/api/v1/repo/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,39 @@ import (

// GetRawFile get a file by path on a repository
func GetRawFile(ctx *context.APIContext) {
// swagger:operation GET /repos/{owner}/{repo}/raw/{ref}/{filepath} repository repoGetRawFileOld
// ---
// summary: Get a file from a repository
// deprecated: true
// produces:
// - application/json
// parameters:
// - name: owner
// in: path
// description: owner of the repo
// type: string
// required: true
// - name: repo
// in: path
// description: name of the repo
// type: string
// required: true
// - name: filepath
// in: path
// description: filepath of the file to get
// type: string
// required: true
// - name: ref
// in: path
// description: "The name of the commit/branch/tag. Default to the repository’s default branch (usually master)"
// type: string
// deprecated: true
// responses:
// 200:
// description: success
// "404":
// "$ref": "#/responses/notFound"

// swagger:operation GET /repos/{owner}/{repo}/raw/{filepath} repository repoGetRawFile
// ---
// summary: Get a file from a repository
Expand All @@ -46,7 +79,7 @@ func GetRawFile(ctx *context.APIContext) {
// required: true
// - name: ref
// in: query
// description: "The name of the commit/branch/tag. Default the repository’s default branch (usually master)"
// description: "The name of the commit/branch/tag. Default to the repository’s default branch (usually master)"
// type: string
// required: false
// responses:
Expand Down
52 changes: 51 additions & 1 deletion templates/swagger/v1_json.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8214,7 +8214,7 @@
},
{
"type": "string",
"description": "The name of the commit/branch/tag. Default the repository’s default branch (usually master)",
"description": "The name of the commit/branch/tag. Default to the repository’s default branch (usually master)",
"name": "ref",
"in": "query"
}
Expand All @@ -8229,6 +8229,56 @@
}
}
},
"/repos/{owner}/{repo}/raw/{ref}/{filepath}": {
"get": {
"produces": [
"application/json"
],
"tags": [
"repository"
],
"summary": "Get a file from a repository",
"operationId": "repoGetRawFileOld",
"deprecated": true,
"parameters": [
{
"type": "string",
"description": "owner of the repo",
"name": "owner",
"in": "path",
"required": true
},
{
"type": "string",
"description": "name of the repo",
"name": "repo",
"in": "path",
"required": true
},
{
"type": "string",
"description": "filepath of the file to get",
"name": "filepath",
"in": "path",
"required": true
},
{
"type": "string",
"description": "The name of the commit/branch/tag. Default to the repository’s default branch (usually master)",
"name": "ref",
"in": "path"
}
],
"responses": {
"200": {
"description": "success"
},
"404": {
"$ref": "#/responses/notFound"
}
}
}
},
"/repos/{owner}/{repo}/releases": {
"get": {
"produces": [
Expand Down