Serve LFS/attachment with http.ServeContent to Support Range-Request #18448

marxangels · 2022-01-29T10:45:06Z

The ServeData function now can't handle Range Request.
Due to the complexity of this part of HTTP protocol, it is difficult to fully implement it and requires a lot of testing.
The official module http.ServeContent has this implementation, and we can replace with it directly.
It can handle If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since, If-Range and more, perfectly!

Thanks for @wxiaoguang 's tips.

wxiaoguang · 2022-01-29T13:44:04Z

I haven't read through all related code carefully, but I have a feeling that if we call http.ServeContent directly, it would break the current behavior.

Currectly, Gitea's common.ServeData calls typesniffer.DetectContentType(buf) to do deep mime-type detection.

We had better keep the behavior. Since we could get the ReadSeekCloser, it's easy to read the a few bytes from beginning to do the DetectContentType. And it's better to work on common.ServeData directly to introduce http.ServeContent, indeed common.ServeData does do some specialized work for the response.

marxangels · 2022-01-29T23:35:17Z

According to my experience, the existing mime detection is problematic, some mp3/mp4 files can not be recognized accurately.
And the http.ServeContent found no errors so far.

wxiaoguang · 2022-01-30T03:35:28Z

Yes, there are related issues like:

Although I also think the typesniffer.DetectContentType might not be necessary and buggy for HTTP file serving, the feature was already there.

If this PR need to change the existing behavior, it need more people @go-gitea/maintainers to take a look at it.

My opinion: I prefer to use extension name to get mime types instead of typesniffer.DetectContentType (with some special case handling, eg: detecting text files without extension like README). The reason is: most files are using correct extension names, while the detection is buggy sometimes and no real benefit for files with extensions.

lunny · 2022-01-30T08:46:33Z

I think the security headers should be kept. And another problem is size is missed. But in fact we can get it.

lunny · 2022-01-30T18:01:28Z

size should be dropped to support range.

lunny · 2022-01-30T18:02:58Z

The function served attachment/git blob/git lfs file, so I think the name ServeLargeFile is not very suitable.

marxangels · 2022-01-30T21:59:14Z

size should be dropped to support range.

http.ServeContent uses a seek to the end of the content to determine its size.

marxangels · 2022-01-30T22:04:25Z

The function served attachment/git blob/git lfs file, so I think the name ServeLargeFile is not very suitable.

git blob is served by the existing function common.ServeData now.
git blob doesn't implement a Seeker interface, so we should separate it into two functions.
Or we must implement a Seek function for the git blob Struct.

zeripath · 2022-01-30T22:20:12Z

It appears that there's also changes to the mime type detection?

modules/typesniffer/typesniffer.go

routers/common/repo.go

Gusted · 2022-02-02T18:09:19Z

routers/common/repo.go

-// ServeBlob download a git.Blob
+func setCommonHeaders(ctx *context.Context, name string, data interface{}) error {
+	// Google Chrome dislike commas in filenames, so let's change it to a space
+	name = strings.ReplaceAll(name, ",", " ")


Ditto from the other PR:

I don't think this is needed actually, from what I remember about this weird bug, if you quote the filename(as being done in this function) chromium won't error about it.

That's strange. Is it dependent on different chromium versions?

It's a historical problem, I believe modern browsers (and modern frameworks) can handle it correctly, and we do not need hacky methods.

https://stackoverflow.com/questions/93551/how-to-encode-the-filename-parameter-of-content-disposition-header-in-http

6543 · 2022-02-03T10:48:07Z

integrations/download_test.go

@@ -73,19 +74,22 @@ func TestDownloadRawTextFileWithoutMimeTypeMapping(t *testing.T) {
 	req := NewRequest(t, "GET", "/user2/repo2/raw/branch/master/test.xml")
 	resp := session.MakeRequest(t, req, http.StatusOK)

-	assert.Equal(t, "text/plain; charset=utf-8", resp.HeaderMap.Get("Content-Type"))
+	assert.Equal(t, "text/xml; charset=utf-8", resp.HeaderMap.Get("Content-Type"))


-> RawTextFileWithoutMimeTypeMapping this test is intended to not not find a specific mime type

Yeah, but the type-detection is now taken care by mime.TypeByExtension. And the default mime of XML file is text/xml.

The type-detection of text files has known logic confusion now. we may fix it after this PR.
There are too many modifications in this PR.

To test a specific mime type not found, we can use any unknown extension, such as .foo .xyz

routers/common/repo.go

integrations/download_test.go

stale · 2022-04-16T09:06:54Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

6543 · 2022-04-25T20:31:25Z

routers/common/repo.go

+		var cs string
+		var err error
+		if reader, ok := data.(io.ReadSeeker); ok {
+			cs, err = charset.DetectEncodingFromReader(reader)


do we need a seeker reader explicit for http.ServeContent - else change the detection to read in a buffer and use multi reader would also be a good option

lunny · 2022-05-03T08:27:23Z

modules/typesniffer/typesniffer.go

@@ -78,15 +85,35 @@ func DetectContentType(data []byte) SniffedType {
 		data = data[:sniffLen]
 	}

-	if (strings.Contains(ct, "text/plain") || strings.Contains(ct, "text/html")) && svgTagRegex.Match(data) ||
-		strings.Contains(ct, "text/xml") && svgTagInXMLRegex.Match(data) {
+	if (strings.HasPrefix(ct, "text/plain") || strings.HasPrefix(ct, "text/html")) && svgTagRegex.Match(data) ||


Why not keep Contains?

LJFan · 2022-07-18T01:27:09Z

I need this feature badly. Can anyone merge it into the main branch?

integrations/download_test.go

6543 · 2022-07-25T03:07:35Z

modules/setting/mime_type_map.go

@@ -21,6 +26,10 @@ func newMimeTypeMap() {
 	m := make(map[string]string, len(keys))
 	for _, key := range keys {
 		m[strings.ToLower(key.Name())] = key.Value()
+		err := mime.AddExtensionType(key.Name(), key.Value())


please dont add unrelated changes in here

6543 · 2022-07-25T03:08:13Z

I need this feature badly. Can anyone merge it into the main branch?

as soon as it's ready ...

6543 · 2022-07-25T12:52:23Z

@marxangels whats going on?

silverwind · 2022-07-25T12:59:31Z

Maybe he lost interest, I think we should re-raise the PR in a cleaned up form. It has some valuable stuff in it.

6543 · 2022-07-25T13:32:23Z

-> #20480

I put it on my list of WIPs ...

6543 · 2022-07-25T13:32:53Z

... will work on it if i got a timeslot for it ... but feel free to also push to it ...

This comment has been minimized.

Sign in to view

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 29, 2022

lunny added this to the 1.17.0 milestone Jan 30, 2022

lunny added the type/enhancement An improvement of existing functionality label Jan 30, 2022

lunny reviewed Feb 1, 2022

View reviewed changes

modules/typesniffer/typesniffer.go Outdated Show resolved Hide resolved

silverwind reviewed Feb 2, 2022

View reviewed changes

routers/common/repo.go Outdated Show resolved Hide resolved

silverwind reviewed Feb 2, 2022

View reviewed changes

routers/common/repo.go Outdated Show resolved Hide resolved

marxangels mentioned this pull request Feb 2, 2022

Fix some mp3/mp4 media files show as Raw #18458

Closed

This comment was marked as off-topic.

Sign in to view

Gusted reviewed Feb 2, 2022

View reviewed changes

6543 reviewed Feb 3, 2022

View reviewed changes

wxiaoguang mentioned this pull request Feb 4, 2022

Set the LastModified header for raw files #18356

Merged

marxangels changed the title ~~handle LFS attachment/download with http.ServeContent~~ Serve LFS/attachment with http.ServeContent to Support Range-Request Feb 5, 2022

marxangels requested review from silverwind, lunny, 6543 and Gusted February 5, 2022 13:57

marxangels added 3 commits February 6, 2022 09:21

Serve LFS/attachment with http.ServeContent to Support Range-Request

55241a3

ctx.Resp.Header().Set("Content-Type", st.Mime())

34fa2ce

Update download_test.go

f2ee607

silverwind reviewed Feb 6, 2022

View reviewed changes

routers/common/repo.go Outdated Show resolved Hide resolved

silverwind reviewed Feb 10, 2022

View reviewed changes

integrations/download_test.go Show resolved Hide resolved

marxangels added 4 commits February 11, 2022 08:37

add testes

1953d84

Update typesniffer.go

37813a6

Update repo.go

c6630dd

Update download_test.go

6743d56

stale bot added the issue/stale label Apr 16, 2022

lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 16, 2022

stale bot removed the issue/stale label Apr 16, 2022

Merge branch 'main' into patch-1

8deb747

6543 reviewed Apr 25, 2022

View reviewed changes

wxiaoguang mentioned this pull request May 1, 2022

[LFS] video player cannot be controlled #16414

Closed

wxiaoguang linked an issue May 1, 2022 that may be closed by this pull request

[LFS] video player cannot be controlled #16414

Closed

lunny reviewed May 3, 2022

View reviewed changes

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

6543 added 2 commits July 25, 2022 04:57

Merge branch 'master' into patch-1

a456cde

ajust

ea62aab

6543 reviewed Jul 25, 2022

View reviewed changes

rm some unrelated diff

7c60fc8

marxangels closed this Jul 25, 2022

6543 mentioned this pull request Jul 25, 2022

WIP: Serve LFS/attachment with http.ServeContent to Support Range-Request #20480

Closed

lunny removed this from the 1.18.0 milestone Dec 20, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve LFS/attachment with http.ServeContent to Support Range-Request #18448

Serve LFS/attachment with http.ServeContent to Support Range-Request #18448

marxangels commented Jan 29, 2022

This comment has been minimized.

wxiaoguang commented Jan 29, 2022

marxangels commented Jan 29, 2022

wxiaoguang commented Jan 30, 2022 •

edited

Loading

lunny commented Jan 30, 2022

lunny commented Jan 30, 2022

lunny commented Jan 30, 2022

marxangels commented Jan 30, 2022

marxangels commented Jan 30, 2022 •

edited

Loading

zeripath commented Jan 30, 2022

This comment was marked as off-topic.

Gusted Feb 2, 2022

marxangels Feb 2, 2022

wxiaoguang Feb 3, 2022

6543 Feb 3, 2022

marxangels Feb 3, 2022

marxangels Feb 3, 2022

marxangels Feb 3, 2022

stale bot commented Apr 16, 2022

6543 Apr 25, 2022

lunny May 3, 2022

LJFan commented Jul 18, 2022

6543 Jul 25, 2022

6543 commented Jul 25, 2022

6543 commented Jul 25, 2022

silverwind commented Jul 25, 2022

6543 commented Jul 25, 2022

6543 commented Jul 25, 2022

Serve LFS/attachment with http.ServeContent to Support Range-Request #18448

Serve LFS/attachment with http.ServeContent to Support Range-Request #18448

Conversation

marxangels commented Jan 29, 2022

This comment has been minimized.

wxiaoguang commented Jan 29, 2022

marxangels commented Jan 29, 2022

wxiaoguang commented Jan 30, 2022 • edited Loading

lunny commented Jan 30, 2022

lunny commented Jan 30, 2022

lunny commented Jan 30, 2022

marxangels commented Jan 30, 2022

marxangels commented Jan 30, 2022 • edited Loading

zeripath commented Jan 30, 2022

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJFan commented Jul 18, 2022

Choose a reason for hiding this comment

6543 commented Jul 25, 2022

6543 commented Jul 25, 2022

silverwind commented Jul 25, 2022

6543 commented Jul 25, 2022

6543 commented Jul 25, 2022

wxiaoguang commented Jan 30, 2022 •

edited

Loading

marxangels commented Jan 30, 2022 •

edited

Loading