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.

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

handle invalid content range #2777

Closed
wants to merge 3 commits into from
Closed

handle invalid content range #2777

wants to merge 3 commits into from

Conversation

@eklavya
Copy link
Contributor

@eklavya eklavya commented Aug 8, 2019

FileService should send RangeNotSatisfiable when range is invalid.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks! There's a failing test, and an enhancement suggestion below.

Loading

.value
}
} else {
F.pure(Some(Response[F](status = Status.RangeNotSatisfiable)))
Copy link
Member

@rossabaker rossabaker Aug 8, 2019

Choose a reason for hiding this comment

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

From the spec:

When this status code is generated in response to a byte-range request, the sender SHOULD generate a Content-Range header field specifying the current length of the selected representation (Section 4.2).

We should try to add that header.

Loading

@eklavya
Copy link
Contributor Author

@eklavya eklavya commented Aug 8, 2019

Sorry @rossabaker I was just sure this wasn't gonna break anything 👅

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 8, 2019

I think it's okay to change that test... the 200 it was testing for is made better by this.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

One build failure was a test flake, and one was a Travis flake. I restarted both.

This looks close. I think we could backport it to 0.20 when it's done, even.

Loading

}
} else {
F.suspend {
val size = file.length()
Copy link
Member

@rossabaker rossabaker Aug 9, 2019

Choose a reason for hiding this comment

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

We get the requisite laziness from suspend, but I think think this would read more naturally:

F.delay(file.length()).map { size =>
  Some(???)
}

Loading

@@ -80,8 +80,7 @@ class FileServiceSpec extends Http4sSpec with StaticContentShared {
)
val reqs = ranges.map(r => Request[IO](uri = uri("testresource.txt")).withHeaders(r))
forall(reqs) { req =>
routes.orNotFound(req) must returnStatus(Status.Ok)
routes.orNotFound(req) must returnBody(testResource)
routes.orNotFound(req) must returnStatus(Status.RangeNotSatisfiable)
Copy link
Member

@rossabaker rossabaker Aug 9, 2019

Choose a reason for hiding this comment

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

I'd throw in a test for the header here as well.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

But let's cherry-pick to 0.20 instead of merging.

Loading

rossabaker added a commit to rossabaker/http4s that referenced this issue Aug 14, 2019
rossabaker added a commit to rossabaker/http4s that referenced this issue Aug 14, 2019
@rossabaker rossabaker mentioned this pull request Aug 14, 2019
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 14, 2019

Incorporated into #2790.

Loading

@rossabaker rossabaker closed this Aug 14, 2019
@rossabaker rossabaker mentioned this pull request Aug 14, 2019
rossabaker added a commit to rossabaker/http4s that referenced this issue Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants