Skip to content

Add API to serve blob or LFS file content#19689

Merged
lunny merged 18 commits into
go-gitea:mainfrom
qwerty287:lfs-api
Jun 4, 2022
Merged

Add API to serve blob or LFS file content#19689
lunny merged 18 commits into
go-gitea:mainfrom
qwerty287:lfs-api

Conversation

@qwerty287
Copy link
Copy Markdown
Contributor

Add an endpoint to get file content or to serve LFS object directly. Code is mainly from the web UI endpoint.
Closes #19685

@6543 6543 added type/feature Completely new functionality. Can only be merged if feature freeze is not active. kind/api labels May 12, 2022
@6543 6543 added this to the 1.17.0 milestone May 12, 2022
Copy link
Copy Markdown
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

I think we would need to test cases for all the different codepaths(etag handling, normal raw file, lfs object, redirection for lfs), to ensure they return the correct content.

Comment thread routers/api/v1/repo/file.go Outdated
Comment thread routers/api/v1/repo/file.go Outdated
Comment thread routers/api/v1/repo/file.go Outdated
Comment thread routers/api/v1/repo/file.go Outdated
Comment thread routers/api/v1/repo/file.go Outdated
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 14, 2022
Copy link
Copy Markdown
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

A left-over ;)

Comment thread routers/api/v1/repo/file.go Outdated
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Comment thread routers/api/v1/repo/file.go Outdated
@qwerty287 qwerty287 requested a review from Gusted May 20, 2022 05:55
@Gusted
Copy link
Copy Markdown
Contributor

Gusted commented May 21, 2022

I still think we should have some test-cases for these API's.

@qwerty287
Copy link
Copy Markdown
Contributor Author

I tried this now for a while, but I don't get it working because I don't know how to store an LFS object in a test repo. Any idea how I can do this to get the file afterwards?

@Gusted
Copy link
Copy Markdown
Contributor

Gusted commented May 21, 2022

I tried this now for a while, but I don't get it working because I don't know how to store an LFS object in a test repo. Any idea how I can do this to get the file afterwards?

We already should have tests that works closely with LFS(look at the references of lfsCommitAndPushTest & mediaTest)

Comment thread integrations/api_repo_file_get_test.go Outdated

reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/media/"+lfs)
respLFS := MakeRequestNilResponseRecorder(t, reqLFS, http.StatusOK)
assert.Equal(t, littleSize, respLFS.Length)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but seems like we need to do some cleanup here in order to not conflict with other tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it t create a new repo and delete it afterwards.

Comment thread routers/api/v1/repo/file.go Outdated
qwerty287 and others added 3 commits June 2, 2022 08:35
1. Avoid reading the blob data multiple times
2. Ensure that caching is only checked when about to serve the blob/lfs
3. Avoid nesting by returning early
4. Make log message a bit more clear
5. Ensure that the dataRc is closed by defer when passed to ServeData

Signed-off-by: Andrew Thornton <art27@cantab.net>
Copy link
Copy Markdown
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I've pushed a slight restructure of the code up - this ensures that the caching is not checked until we know what we're actually going to serve. (The previous code had a potential to re-serve a pointer file if the LFSMetaObject entry was fixed in the intervening time.)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 3, 2022
Comment thread routers/api/v1/repo/file.go
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 4, 2022
@lunny
Copy link
Copy Markdown
Member

lunny commented Jun 4, 2022

make L-G-T-M work.

@lunny lunny merged commit df9612b into go-gitea:main Jun 4, 2022
@qwerty287 qwerty287 deleted the lfs-api branch June 4, 2022 13:27
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 4, 2022
* giteaofficial/main:
  Add API to serve blob or LFS file content (go-gitea#19689)
  Disable unnecessary mirroring elements (go-gitea#18527)
  [skip ci] Updated translations via Crowdin
  Remove customized (unmaintained) dropdown, improve aria a11y for dropdown (go-gitea#19861)
  Set Setpgid on child git processes (go-gitea#19865)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Add LFS API

* Update routers/api/v1/repo/file.go

Co-authored-by: Gusted <williamzijl7@hotmail.com>

* Apply suggestions

* Apply suggestions

* Update routers/api/v1/repo/file.go

Co-authored-by: Gusted <williamzijl7@hotmail.com>

* Report errors

* ADd test

* Use own repo for test

* Use different repo name

* Improve handling

* Slight restructures

1. Avoid reading the blob data multiple times
2. Ensure that caching is only checked when about to serve the blob/lfs
3. Avoid nesting by returning early
4. Make log message a bit more clear
5. Ensure that the dataRc is closed by defer when passed to ServeData

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API] Get LFS objects

6 participants