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

noerw · 2021-09-29T21:21:20Z

Looking into #17179 I noticed a couple oddities with the raw file API:

it supports an undocumented path parameter to specify the ref ({repo_api_url}/raw/{ref}/{tree_path}).
Parsing this is a mess, see context/repo.go getRefName()
due to lacking docs, a new query param ?ref= was added in #14602, now providing a duplicate parameter (and a deprecation path! ðŸ˜„)
#13686 brought support for abbreviated commit SHAs, but omitted support for that in the API route, I'm not sure why (@lafriks?)

All this culminates in the weird failure mode of #17179.

This PR aims to fix #17179, but also to find a way out of this mess:

adds support for short commit SHAs on the old {ref} path parameter, to fix above bug
documents the old path parameter {ref}
deprecates the old path parameter

Backporting the actual bugfix is simpler, as mentioned in df834a7

The text was updated successfully, but these errors were encountered:

the ?ref= param was added recently in go-gitea#14602, and can fully replace the (previously undocumented) ref-prefix of the filepath

we just deprecated this API, but this is a short fix to go-gitea#17179 A more minimal fix would be to check if len(parts) > 1 in line context/repo.go#L698

delvh · 2021-09-29T21:48:33Z

modules/context/api.go

@@ -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 {


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.

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

@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.

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

lunny · 2021-09-30T02:14:50Z

Maybe we should have a break change.{repo_api_url}/raw/{ref}/{tree_path} is the only router and drop {repo_api_url}/raw/{tree_path}.

lafriks · 2021-09-30T04:13:05Z

Answering to your question shortened commit sha was added because of support needed for pkg.go.dev

As for API I did not see need for it as for API you will usually work with full commit sha

As for legacy urls we should drop support for them imho, it's been quite some time already

noerw · 2021-09-30T08:22:12Z

Maybe we should have a break change. {repo_api_url}/raw/{ref}/{tree_path} is the only router and drop {repo_api_url}/raw/{tree_path}.

@lunny Aesthetically I too prefer the {ref} path parameter, but

this was never documented, so removing (or better deprecating, I'd bet there are users of this) this one and keeping the ?ref= variant is not a breaking change
this is a pain to parse, specifying ref in a separate query param simplifies the code a lot
(using ?ref= is also closer to GH API, though they have changed their raw file handling a couple times over, so compatibility isn't given anyway)

noerw · 2021-09-30T10:53:14Z

regarding the CI fail in check:

310See errors below:
311- "paths./repos/{owner}/{repo}/raw/{ref}/{filepath}.get.parameters" must validate one and only one schema (oneOf). Found none valid
313- in operation "repoGetRawFileOld",path param "ref" must be declared as required

well.. the ref param isn't required. is there a workaround wrt the linter? (It dawns upon me that this is the reason this param was never documented..)

...when path contains no hash-path-separator ('/') This is a workaround to go-gitea#17179. Entering this case when `path` does not contain a '/' does not really make sense, as that means the tree path is empty, but this case is only entered for routes that expect a non-empty tree path. Treepaths like <40-char-dirname>/<filename> will still fail, but hopefully don't occur that often. A more complete fix that avoids this case too is outlined in go-gitea#17185, but too big of a change to backport

...when path contains no hash-path-separator ('/') This is a workaround to #17179. Entering this case when `path` does not contain a '/' does not really make sense, as that means the tree path is empty, but this case is only entered for routes that expect a non-empty tree path. Treepaths like <40-char-dirname>/<filename> will still fail, but hopefully don't occur that often. A more complete fix that avoids this case too is outlined in #17185, but too big of a change to backport

lunny · 2021-11-19T06:22:06Z

Please resolve the conflicts.

noerw added 4 commits Sep 29, 2021

put auth middleware before any parsing

2e128ba

raw file api: clarify documentation

d4fc683

the ?ref= param was added recently in go-gitea#14602, and can fully replace the (previously undocumented) ref-prefix of the filepath

raw file API: add support for shortened commit SHA

df834a7

we just deprecated this API, but this is a short fix to go-gitea#17179 A more minimal fix would be to check if len(parts) > 1 in line context/repo.go#L698

add test for short shas

e1e33a8

noerw requested a review from lafriks Sep 29, 2021

noerw added backport/v1.15 kind/api kind/bug kind/docs labels Sep 29, 2021

delvh reviewed Sep 29, 2021

View changes

GiteaBot added the lgtm/need 2 label Sep 29, 2021

noerw mentioned this pull request Oct 8, 2021

Raw file API: don't try to interpret 40char filenames as commit SHA (#17185) #17272

Merged

Merge branch 'main' into fix-17179

436262f

lunny added the backport/done label Oct 30, 2021

lunny added this to the 1.16.0 milestone Oct 30, 2021

Merge branch 'main' into fix-17179

e2903f4

techknowlogick removed this from the 1.16.0 milestone Nov 23, 2021

techknowlogick added this to the 1.17.0 milestone Nov 23, 2021

Dec	JAN	May
	08
2021	2022	2023

go-gitea / gitea Public

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

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

noerw commented Sep 29, 2021 •

edited

delvh Sep 29, 2021

wxiaoguang Sep 30, 2021

noerw Sep 30, 2021 •

edited

wxiaoguang Sep 30, 2021

lunny commented Sep 30, 2021

lafriks commented Sep 30, 2021

noerw commented Sep 30, 2021 •

edited

noerw commented Sep 30, 2021

lunny commented Nov 19, 2021

go-gitea / gitea Public

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

Are you sure you want to change the base?

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

Conversation

noerw commented Sep 29, 2021 • edited

delvh Sep 29, 2021

Choose a reason for hiding this comment

wxiaoguang Sep 30, 2021

Choose a reason for hiding this comment

noerw Sep 30, 2021 • edited

Choose a reason for hiding this comment

wxiaoguang Sep 30, 2021

Choose a reason for hiding this comment

lunny commented Sep 30, 2021

lafriks commented Sep 30, 2021

noerw commented Sep 30, 2021 • edited

noerw commented Sep 30, 2021

lunny commented Nov 19, 2021

noerw commented Sep 29, 2021 •

edited

noerw Sep 30, 2021 •

edited

noerw commented Sep 30, 2021 •

edited