Streaming files as partial content #2342

Akazm · 2020-05-01T17:37:55Z

Better support for streaming files by returning 206 Partial Content when a range header is supplied with the correct content. Required when streaming to some browsers.

Akazm · 2020-05-02T12:59:08Z

I've noticed two bugs in there:

asByteBufferBounds() returnend incorrect values (fixed)
Incorrect parsing of range headers; fixed that, too.

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

grosch · 2020-05-02T16:04:07Z

Please add unit tests as well.

Akazm · 2020-05-02T16:49:06Z

renamed asHeaderString() to serialize()
added tests for serialization of ranges and content ranges
added tests for deserialization of ranges and content ranges
resolved all code style issues mentioned by @grosch (except for "Please move this to the line right after you assign rangeStr", since I seriously don't get it :()

Tests/VaporTests/HTTPHeaderTests.swift

MrLotU

Overall looking good! Left a few style comments/suggestions. Also, saw you used a few force unwraps, and although all in a safe way, probably still best to try and avoid those :)

Thanks for the PR!

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Akazm

Overall looking good! Left a few style comments/suggestions. Also, saw you used a few force unwraps, and although all in a safe way, probably still best to try and avoid those :)

Usually, I do. Just tried to not clutter the code here with stuff that is unnecessarily expressive. It's a tradeoff, so to speak.

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Akazm · 2020-05-03T12:23:02Z

Sources/Vapor/Utilities/FileIO.swift

+            response.headers.add(name: .accept, value: contentRange.unit.serialize())
+            let range = contentRange.ranges[0].asResponseContentRange(limit: fileSize)
+            response.headers.contentRange = ContentRangeHeader(unit: contentRange.unit, range: range)
+            (offset, byteCount) = contentRange.ranges[0].asByteBufferBounds(withMaxSize: fileSize)


I notice undefined behaviour here if the requested range exceeds the file size. Any proposals here?

I think a bad request would be appropriate in this case

mcritz

Love this. Thanks for putting in the work.

Sources/Vapor/Utilities/FileIO.swift

Joannis · 2020-05-06T07:48:29Z

Thanks so much for this PR. I'm afraid I don't have a quick solution for your question. But a .badRequest error would be an option in my book.

Akazm · 2020-05-06T07:49:45Z

One option would be to return 416, another would be to just return the whole file instead. Both options would be valid, I'd prefer a configurable solution with a reasonable default.

tanner0101

+1 to this change, thanks! Comments inline.

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

tanner0101 · 2020-05-08T19:34:39Z

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

+
+public struct ContentRangeHeader : Equatable {
+    public let unit: RangeUnit
+    public let range: ResponseContentRange


Instead of an enum, what about something like:

public var range: Range<Int>? public var size: Int?

Where nil on either is serialized as *.

This actually has been my first approach on doing this, but I wanted to eliminate optionals here. Has the advantage of avoiding a lot of code cluttering.

If you insist on this change (same goes for Request Range), please confirm this!

tanner0101 · 2020-05-08T19:37:13Z

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

+
+public struct RangeHeader : Equatable {
+    public let unit: RangeUnit
+    public let ranges: [RequestedRange]


Could we use Swift.Range here instead?

This actually has been my first approach on doing this, but I wanted to eliminate optionals here. Has the advantage of avoiding a lot of code cluttering.

If you insist on this change (same goes for Response Content Range), please confirm this!

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Sources/Vapor/Utilities/FileIO.swift

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Tests/VaporTests/HTTPHeaderTests.swift

tanner0101

The latest changes look good, thanks! Most of my comments here are style nits. Please take a look at the other source files in this project for comparison and follow their style.

More importantly though, header value parsing should be handled by the HTTPHeaders.Directive helpers. From what I can tell this should handle the Range and ContentRange header syntax. Simple string parsing is error prone and will not handle edge cases.

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Sources/Vapor/Utilities/FileIO.swift

Akazm · 2020-05-28T06:26:31Z

Will take a look at it on Saturday.

Akazm · 2020-05-31T22:51:29Z

Done, @tanner0101!

tanner0101 · 2020-06-24T23:46:56Z

@Akazm sorry for the late response. The latest updates look great, thank you! Would you mind adding some comments to the code? Public method docblocks and inline comments around terse code would be especially helpful. The header parsing tests are great, but we should have at least one test ensuring the actual partial file reading works. Ideally this would go through a real route. This file has some good examples: https://github.com/vapor/vapor/blob/master/Tests/VaporTests/FileTests.swift. Let me know if you need help writing that.

Akazm · 2020-06-25T05:52:28Z

Comments won't be a problem, writing test code for streaming files is a thing I've never done before and since I'm kind of occupied for the next couple of weeks, any help on this would be appreciated.

2nd squash

Akazm · 2020-07-15T06:34:33Z

@tanner0101 Will take care of the tests on the weekend!

Akazm · 2020-07-19T18:39:00Z

I'm sorry, but I simply couldn't do it this weekend, @tanner0101.

I didn't forget about it and I'll take care of it as soon as I can, probably on Tuesday afternoon, CEST.

nerdsupremacist · 2020-08-26T12:23:22Z

@Akazm What's the status on this? I kinda need it right now.

I'll do a fork of this for now, please reach out if you need any help

Akazm · 2020-08-26T12:27:30Z

It's a motivation for me that other people want this as well.

I promise to solve this during the wekeend. Time is a serious issue for me right now and I'm sorry for not having been able to continue here within the past few months.

Any help is appreciated.

0xTim

Thanks for doing this (and poking me!)

I think this is mostly good to go, just a few small things to clarify. Also, if you could update the PR comments as they'll become the release notes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

0xTim · 2020-12-22T10:12:38Z

Sources/Vapor/Utilities/FileIO.swift

+            response.headers.add(name: .accept, value: contentRange.unit.serialize())
+            let range = contentRange.ranges[0].asResponseContentRange(limit: fileSize)
+            response.headers.contentRange = ContentRangeHeader(unit: contentRange.unit, range: range)
+            (offset, byteCount) = contentRange.ranges[0].asByteBufferBounds(withMaxSize: fileSize)


I think a bad request would be appropriate in this case

Sources/Vapor/Utilities/FileIO.swift

Stale

siemensikkema

I left some suggestions to remove some force unwraps and to clarify the code here and there a bit.

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

Sources/Vapor/Utilities/FileIO.swift

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

0xTim · 2021-01-06T16:09:26Z

I think this is finally ready - @siemensikkema @gwynne if you want to do a final review.

@Akazm is there anything else that needs addressing before we merge?

kylebrowning · 2021-01-06T18:44:43Z

Sources/Vapor/Utilities/FileIO.swift

+            byteCount:
+            fileSize.intValue,


nit: extra newline?

I think this is finally ready - @siemensikkema @gwynne if you want to do a final review.

@Akazm is there anything else that needs addressing before we merge?

Hello 0xTim,
please don't ask me for this. I've been in trouble since last year, I'm unfortunately occupied since my last activity here (since Corona gave our company many oppurtinities).

Nevertheless: I'm happy to see that there's some progress here made by other people since this is my first contribution to Open Source ever!

0xTim · 2021-01-07T12:41:54Z

Confirmed I can play an MP4 in Safari so will merge, thank you everyone!

tanner0101 · 2021-01-07T12:42:03Z

These changes are now available in 4.37.0

vzsg requested a review from tanner0101 May 2, 2020 13:49

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

grosch reviewed May 2, 2020

View reviewed changes

Tests/VaporTests/HTTPHeaderTests.swift Outdated Show resolved Hide resolved

MrLotU reviewed May 3, 2020

View reviewed changes

Akazm commented May 3, 2020

View reviewed changes

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift Outdated Show resolved Hide resolved

Akazm commented May 3, 2020

View reviewed changes

mcritz reviewed May 6, 2020

View reviewed changes

Joannis reviewed May 6, 2020

View reviewed changes

Sources/Vapor/Utilities/FileIO.swift Outdated Show resolved Hide resolved

tanner0101 added the enhancement New feature or request label May 8, 2020

tanner0101 added this to Awaiting Review in Vapor 4 via automation May 8, 2020

tanner0101 requested changes May 8, 2020

View reviewed changes

tanner0101 previously requested changes May 26, 2020

View reviewed changes

tanner0101 moved this from Awaiting Review to Awaiting Updates in Vapor 4 May 26, 2020

Allow custom media types for FileIO.streamFile

77a55c5

2nd squash

0xTim added the semver-minor Contains new API label Dec 22, 2020

0xTim requested changes Dec 22, 2020

View reviewed changes

Vapor 4 automation moved this from Awaiting Updates to Awaiting Review Dec 22, 2020

0xTim mentioned this pull request Dec 22, 2020

FileIO support range header #1762

Closed

0xTim linked an issue Dec 22, 2020 that may be closed by this pull request

FileIO support range header #1762

Closed

siemensikkema reviewed Dec 22, 2020

View reviewed changes

0xTim and others added 10 commits January 5, 2021 17:07

Update Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

1706df2

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Update Sources/Vapor/Utilities/FileIO.swift

bd793c3

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Update Sources/Vapor/Utilities/FileIO.swift

bcd4a94

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Merge branch 'master' into partial-content-content-range

2b5a22c

Update Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

a84e3c8

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Update Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

1e35235

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Update Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift

b1e4115

Co-authored-by: Siemen Sikkema <siemensikkema@users.noreply.github.com>

Make the compiler happy

c71a5c7

Add better bounds checking to range count

ac03166

More bounds checking

1f25aac

0xTim approved these changes Jan 6, 2021

View reviewed changes

kylebrowning approved these changes Jan 6, 2021

View reviewed changes

siemensikkema approved these changes Jan 7, 2021

View reviewed changes

0xTim merged commit 1ccc4a2 into vapor:master Jan 7, 2021

Vapor 4 automation moved this from Awaiting Review to Done Jan 7, 2021

0xTim mentioned this pull request Jan 7, 2021

Support for Content-Range #2341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streaming files as partial content #2342

Streaming files as partial content #2342

Akazm commented May 1, 2020 •

edited by 0xTim

Akazm commented May 2, 2020

grosch commented May 2, 2020

Akazm commented May 2, 2020

MrLotU left a comment

Akazm left a comment

Akazm May 3, 2020

0xTim Dec 22, 2020

mcritz left a comment

Joannis commented May 6, 2020

Akazm commented May 6, 2020

tanner0101 left a comment

tanner0101 May 8, 2020

Akazm May 8, 2020

tanner0101 May 8, 2020

Akazm May 8, 2020

tanner0101 left a comment •

edited

Akazm commented May 28, 2020

Akazm commented May 31, 2020

tanner0101 commented Jun 24, 2020 •

edited

Akazm commented Jun 25, 2020

Akazm commented Jul 15, 2020

Akazm commented Jul 19, 2020

nerdsupremacist commented Aug 26, 2020

Akazm commented Aug 26, 2020

0xTim left a comment

0xTim Dec 22, 2020

siemensikkema left a comment

0xTim commented Jan 6, 2021

kylebrowning Jan 6, 2021

Akazm Jan 7, 2021

0xTim commented Jan 7, 2021

tanner0101 commented Jan 7, 2021

Streaming files as partial content #2342

Streaming files as partial content #2342

Conversation

Akazm commented May 1, 2020 • edited by 0xTim

Akazm commented May 2, 2020

grosch commented May 2, 2020

Akazm commented May 2, 2020

MrLotU left a comment

Choose a reason for hiding this comment

Akazm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcritz left a comment

Choose a reason for hiding this comment

Joannis commented May 6, 2020

Akazm commented May 6, 2020

tanner0101 left a 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

tanner0101 left a comment • edited

Choose a reason for hiding this comment

Akazm commented May 28, 2020

Akazm commented May 31, 2020

tanner0101 commented Jun 24, 2020 • edited

Akazm commented Jun 25, 2020

Akazm commented Jul 15, 2020

Akazm commented Jul 19, 2020

nerdsupremacist commented Aug 26, 2020

Akazm commented Aug 26, 2020

0xTim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siemensikkema left a comment

Choose a reason for hiding this comment

0xTim commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xTim commented Jan 7, 2021

tanner0101 commented Jan 7, 2021

Akazm commented May 1, 2020 •

edited by 0xTim

tanner0101 left a comment •

edited

tanner0101 commented Jun 24, 2020 •

edited