Ensure Content-Length is set for Responses that need it #74
Conversation
ponylang-main
added
the
discuss during sync
SeanTAllen
added
the
changelog - fixed
|
Hi @mfelsche, The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be: Thanks. |
SeanTAllen
added
the
changelog - changed
|
@mfelsche I think this should get 2 changelog entries. And two release notes. As this is pretty much two changes.
can you manually add the slightly different and appropriate CHANGELOG entries. 1 to changed and 1 to fixed? and manually add to next-release.md. different entries with names that match those in the changelog. the fixed should be simple, for the changed, detail what has changed about the API and what needs to be updated to adjust? also you need to rebase it appears. |
except cor chunked encoding
especially change set_transfer_coding to set_transfer_encoding and allow ByteSeq instead of only Array[U8] val and add some more tests for the ResponseBuilder
mfelsche
force-pushed
the
content-length-response
branch
from
afcbc01 to
f166243
Compare
CHANGELOG.md
Outdated
| @@ -6,12 +6,24 @@ All notable changes to this project will be documented in this file. This projec | |||
|
|
|||
| ### Fixed | |||
|
|
|||
| - Responses without `Transfer-Encoding: chunked` had no `Content-Length` header set when not done explicitly. This made some clients (e.g. curl) hang. | |||
There was a problem hiding this comment.
This isn't a changelog entry. It's very long. This should be a short title and link to the PR
Same for all the others.
There was a problem hiding this comment.
our standard is along the lines of this one I pulled as an example from the file:
- Update to ponylang/net_ssl 1.3.2 ([PR #69](https://github.com/ponylang/http_server/pull/69))
.release-notes/next-release.md
Outdated
| ## Response Creation API changes and Additions | ||
|
|
||
| - `Response.transfer_coding()` changed to `Response.transfer_encoding()`. | ||
|
|
||
| - `ResponseBuilderHeaders.set_content_length(content_length: USize)` has been added to set a content-length from a numeric value. | ||
| - `ResponseBuilderBody.add_chunk()` now takes a `ByteSeq` instead of `Array[U8] val`. This allows for passing `String val` as well. | ||
|
|
||
| - `BuildableResponse.create()` now only takes a `Status` and a `Version`. Content-Length and Transfer-Encoding can be set later with `set_content_length()` and `set_transfer_encoding()` | ||
| - `BuildableResponse.delete_header(header_name: String)` was added to enable deletion of headers that have been set previously. | ||
| - `BuildableResponse.set_transfer_coding()` changed to `BuildableResponse.set_transfer_encoding()` |
There was a problem hiding this comment.
Can you make these individual entries as appropriate with "before and after" code examples?
The name for each ## entry should match up with an entry in the CHANGELOG.
SeanTAllen
removed
changelog - changed
ponylang-main
removed
the
discuss during sync
For all ways to construct responses, ensure that they have a
Content-Lengthheader, adding one with0as value if none was provided.Do not add one for responses with
Transfer-Encoding: chunked.This adds some overhead to building responses, but ensures (in a reasonable way) for legit API uses, that valid responses fall out in the end. Performance impact hasn't been measured yet, but can be done if desired.
This also included various internal fixes and some streamlining of response APIs. Overall I would consider this a non-backwards-compatible release.
Obsoletes #71