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

Jump to bottom

Add API to serve blob or LFS file content #19689

Merged
merged 18 commits into from
Jun 4, 2022
Merged

Add API to serve blob or LFS file content #19689

merged 18 commits into from
Jun 4, 2022

Conversation

qwerty287
Copy link
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. modifies/api This PR adds API routes or modifies them labels May 12, 2022
@6543 6543 added this to the 1.17.0 milestone May 12, 2022
Gusted
Gusted suggested changes May 14, 2022
View reviewed changes
Copy link
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.

routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 14, 2022
Gusted
Gusted reviewed May 15, 2022
View reviewed changes
Copy link
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 ;)

routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
@qwerty287
Update routers/api/v1/repo/file.go
a938479 
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Gusted
Gusted reviewed May 15, 2022
View reviewed changes
routers/api/v1/repo/file.go Outdated Show resolved Hide resolved
@qwerty287 qwerty287 requested a review from Gusted May 20, 2022 05:55
@Gusted
Copy link
Contributor

Gusted commented May 21, 2022

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

@qwerty287
Copy link
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
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)

Gusted
Gusted reviewed May 22, 2022
View reviewed changes
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
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
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.

qwerty287 and others added 3 commits June 2, 2022 08:35
@qwerty287
Merge branch 'main' into lfs-api
1532e23
@qwerty287
Improve handling
0a0b2c1
@zeripath
Slight restructures
f90b218 
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>
zeripath
zeripath approved these changes Jun 3, 2022
View reviewed changes
Copy link
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
@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
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
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
ed4bb80 
* 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
@qwerty287 @zeripath
@lunny @AbdulrhmnGhanem
Add API to serve blob or LFS file content (go-gitea#19689)
bb49c17 
* 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.
Reviewers

@zeripath zeripath zeripath approved these changes

@Gusted Gusted Gusted requested changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

[API] Get LFS objects
6 participants
@qwerty287 @Gusted @lunny @zeripath @GiteaBot @6543