Skip to content

fix(sirv): a request with range headers appends Content-Length, Conte…#75

Merged
lukeed merged 7 commits intolukeed:masterfrom
imtiazmangerah:master
Jul 31, 2020
Merged

fix(sirv): a request with range headers appends Content-Length, Conte…#75
lukeed merged 7 commits intolukeed:masterfrom
imtiazmangerah:master

Conversation

@imtiazmangerah
Copy link
Copy Markdown
Contributor

@imtiazmangerah imtiazmangerah commented Jul 31, 2020

A request with Range headers present will result in the headers object being modified for all subsequent requests.

To reproduce:

curl -X GET  http://localhost:5000/pnggrad16rgb.png  -H 'Cache-Control: no-cache'

{
  'Content-Length': 3974,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT'
}

curl -X GET http://localhost:5000/pnggrad16rgb.png -H 'Cache-Control: no-cache' -H 'Range: bytes=0-499'

{
  'Content-Length': 500,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT',
  'Content-Range': 'bytes 0-499/3974',
  'Accept-Ranges': 'bytes'
}

curl -X GET  http://localhost:5000/pnggrad16rgb.png  -H 'Cache-Control: no-cache'

{
  'Content-Length': 500,
  'Content-Type': 'image/png',
  'Last-Modified': 'Fri, 31 Jul 2020 04:30:09 GMT',
  'Content-Range': 'bytes 0-499/3974',
  'Accept-Ranges': 'bytes'
}

Image used for above test:

pnggrad16rgb

@imtiazmangerah
fix(sirv): a request with range headers appends Content-Length, Conte…
296843b 
…nt-Range and Accept-Ranges for all subsequent requests. Fixes lukeed#55
@imtiazmangerah imtiazmangerah mentioned this pull request Jul 31, 2020
Closed
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 31, 2020 •
edited
Loading

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files           2        2           
  Lines          67       67           
=======================================
  Hits           66       66           
  Misses          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8ea6a8...f500c6c. Read the comment docs.

@lukeed
Copy link
Copy Markdown
Owner

lukeed commented Jul 31, 2020 •
edited
Loading

Thanks! Do you think you could add test(s) for this? Could be able to do the same thing you've illustrated with curl but with any of the current assets/fixtures.

@imtiazmangerah
Copy link
Copy Markdown
Contributor Author

imtiazmangerah commented Jul 31, 2020

@lukeed I think the test suite will have to include a few more cases which will fail even with the fix, example:

bytes=0-499, -500

[x,y] would be parsed incorrectly, yet that is a valid header for bytes.

Is it the intention of sirv to have full support for range requests?

@lukeed
Copy link
Copy Markdown
Owner

lukeed commented Jul 31, 2020

Let's worry about the other Range values separately. That's easy to address, but this PR should focus on the mutable headers reference you identified

@imtiazmangerah
Copy link
Copy Markdown
Contributor Author

imtiazmangerah commented Jul 31, 2020

@lukeed gave the tests a go, first commit of tests passed, cleaned up the tests to avoid repetition and now checks failed. I cant spot what would cause that, any hints?

lukeed
lukeed approved these changes Jul 31, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Thank you :)

The CLI tests periodically fail. My process-spawner is a little rough but it gets the job done reasonably okay, haha

@lukeed lukeed merged commit b33bb15 into lukeed:master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukeed lukeed lukeed approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@imtiazmangerah @codecov-commenter @lukeed