Skip to content

Fix 204 beavior #375

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

Merged
rgrinberg merged 3 commits into from
Jun 19, 2015
Merged

Fix 204 beavior #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45a7b56
leading space removal
rgrinberg Jun 6, 2015
7391f3a
Improve lwt test for 204's
rgrinberg Jun 6, 2015
4a97fbc
Fix #371
rgrinberg Jun 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions lib/response.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,26 @@ module Make(IO : S.IO) = struct
let flush = false in
return (`Ok { encoding; headers; version; status; flush })

let has_body {status; encoding} =
(* rfc7230#section-5.7.1 *)
match status with
| #Code.informational_status | `No_content | `Not_modified -> `No
| #Code.status_code -> Transfer.has_body encoding
let allowed_body response = (* rfc7230#section-5.7.1 *)
match status response with
| #Code.informational_status | `No_content | `Not_modified -> false
| #Code.status_code -> true

let has_body response =
if allowed_body response
then Transfer.has_body (encoding response)
else `No

let make_body_reader {encoding} ic = Transfer_IO.make_reader encoding ic
let read_body_chunk = Transfer_IO.read

let write_header res oc =
write oc (Printf.sprintf "%s %s\r\n" (Code.string_of_version res.version)
(Code.string_of_status res.status)) >>= fun () ->
let headers = Header.add_transfer_encoding res.headers res.encoding in
let headers =
if allowed_body res
then Header.add_transfer_encoding res.headers res.encoding
Copy link
Copy Markdown
Member

@avsm avsm Jun 8, 2015

Choose a reason for hiding this comment

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

shouldn't this check to see if the response already has a Transfer-encoding header and not override it if so?

Copy link
Copy Markdown
Member Author

@rgrinberg rgrinberg Jun 14, 2015

Choose a reason for hiding this comment

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

Probably. I'll take a look at this separately since this PR doesn't change this aspect of the behavior

else res.headers in
Header_IO.write headers oc

let make_body_writer ?flush {encoding} oc =
Expand Down
4 changes: 4 additions & 0 deletions lib_test/test_sanity.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
open Lwt
open OUnit
open Cohttp
open Cohttp_lwt_unix
open Cohttp_lwt_unix_test

Expand Down Expand Up @@ -57,6 +58,9 @@ let ts =
let not_modified_has_no_body () =
Client.get uri >>= fun (resp, body) ->
assert_equal (Response.status resp) `Not_modified;
let headers = Response.headers resp in
assert_equal ~printer:Transfer.string_of_encoding
Transfer.Unknown (Header.get_transfer_encoding headers);
body |> Body.is_empty >|= fun is_empty ->
assert_bool "No body returned when not modified" is_empty in
[ "sanity test", t
Expand Down
2 changes: 1 addition & 1 deletion lwt/cohttp_lwt.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ module Make_server(IO:IO) = struct
Filename.concat docroot rel_path

let respond ?headers ?(flush=true) ~status ~body () =
let encoding =
let encoding =
match headers with
| None -> Cohttp_lwt_body.transfer_encoding body
| Some headers ->
Expand Down