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 Add support for ref parameter to get raw file API #14602, now providing a duplicate parameter (and a deprecation path! 😄)
Support shortened commit SHAs in URLs #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 ?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:42:22Z

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.

wxiaoguang · 2023-04-24T13:29:55Z

Is this PR active?

wxiaoguang · 2023-05-01T03:57:04Z

It has been stale for a long time and I can't think of a way to handling it other than closing it. Feel free to reopen if there's any new progress and I could also help.

noerw added 4 commits September 29, 2021 23:05

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 September 29, 2021 21:22

noerw added backport/v1.15 modifies/api This PR adds API routes or modifies them type/bug type/docs This PR mainly updates/creates documentation labels Sep 29, 2021

delvh reviewed Sep 29, 2021

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. 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 All backports for this PR have been created label Oct 30, 2021

lunny added this to the 1.16.0 milestone Oct 30, 2021

Merge branch 'main' into fix-17179

e2903f4

techknowlogick modified the milestones: 1.16.0, 1.17.0 Nov 23, 2021

wxiaoguang removed backport/done All backports for this PR have been created backport/v1.15 labels Apr 3, 2022

lunny modified the milestones: 1.17.0, 1.18.0 Jun 3, 2022

lunny removed this from the 1.18.0 milestone Oct 17, 2022

lunny added this to the 1.19.0 milestone Oct 17, 2022

lunny removed this from the 1.19.0 milestone Feb 1, 2023

wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Apr 24, 2023

wxiaoguang closed this May 1, 2023

go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

delvh Sep 29, 2021

wxiaoguang Sep 30, 2021

noerw Sep 30, 2021 •

edited

Loading

wxiaoguang Sep 30, 2021

lunny commented Sep 30, 2021

lafriks commented Sep 30, 2021

noerw commented Sep 30, 2021 •

edited

Loading

noerw commented Sep 30, 2021

lunny commented Nov 19, 2021

wxiaoguang commented Apr 24, 2023

wxiaoguang commented May 1, 2023

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

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

Conversation

noerw commented Sep 29, 2021 • edited Loading

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 Loading

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 Loading

noerw commented Sep 30, 2021

lunny commented Nov 19, 2021

wxiaoguang commented Apr 24, 2023

wxiaoguang commented May 1, 2023

noerw commented Sep 29, 2021 •

edited

Loading

noerw Sep 30, 2021 •

edited

Loading

noerw commented Sep 30, 2021 •

edited

Loading