Introduce new HttpBodyHandler to easily extract full body of HttpRequest and HttpClientResponse.

Introduce new HttpBodyHandler to easily extract full body of HttpRequest and HttpClientResponse.


BUG=https://code.google.com/p/dart/issues/detail?id=8834

Committed: https://code.google.com/p/dart/source/detail?r=21202

[Anders Johnsen, 2013-04-10 06:12:56]

Soeren: This is based almost 100% on the proposal you made.

Bill, Mads: Feel free to comment on the API/comments, if you have any
questions/concerns. As it's a new API, I want to make sure we take the time to
think it through :)

Right now, if the JSON parsing fails, it'll complete with a Format Error. An
alternative would be to default back to the binary data. Thoughts?

- Anders

[Søren Gjesse, 2013-04-10 07:09:11]

LGTM, thanks for doing this - I think people will be happy.

The only thing we should consider is how flexible/configurable we want this to
be and make sure that we allow for that going forward.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:18: * The content body is parsed, depending on the set
'Content-Type' header field.
Remove 'set'.

I think we should enumerate the mime types which are handled, and add that other
mime types will be handled as binary.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:20: class HttpBodyHandler
I like the transformer class with static methods for handling one object.

However I came to think of one thing. Do we want to support configuring this
class e.g. configure that Content-type text/json should be handled as JSON as
well or that application/json should be handled as BINARY. Without an instance
it is difficult to do this for processRequest and processResponse unless we add
a configuration object which can be passed.

var handler = new HttpBodyHandler();
handler.configureContentType(new ContentType("text/json"), HttpBody.JSON);
handler.configureContentType(new ContentType("application/json"),
HttpBody.BINARY);

...

handler.processRequest(...);

vs.

var cfg = new HttpBodyHandlerConfiguration();
cfg.configureContentType(new ContentType("text/json"), HttpBody.JSON);
cfg.configureContentType(new ContentType("application/json"), HttpBody.BINARY);

...

HttpBodyHandler.processRequest(..., configuration: cfg);

Maybe that flexibility is not needed.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:63: * The actual body. The type depends on [type]
([Map] for [JSON] and [FROM],
FROM -> FORM

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:86: String get text;
Should we add isBinary, isJson, etc. as well?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
File sdk/lib/io/http_body_impl.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:32: HttpHeaders headers) {
Indentation.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:33: return stream.fold(
We could do a stream transformation to String here instead of using asText
below. We do know from the Content-Type whether asString will be called.

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
File tests/standalone/io/http_body_test.dart (right):

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
tests/standalone/io/http_body_test.dart:59: test("text/plain; charset=utf-8",
"body".codeUnits, "body", HttpBody.TEXT);
Test this with charset iso-8859-1 and us-ascii as well.

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
tests/standalone/io/http_body_test.dart:84: HttpServer.bind().then((server) {
Shouldn't we have two modes for this test. One that uses a transformer as here
and one where HttpBodyHandler.processRequest is used?

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
tests/standalone/io/http_body_test.dart:127: test("text/plain; charset=utf-8",
"body".codeUnits, "body", HttpBody.TEXT);
Ditto.

[Mads Ager (google), 2013-04-10 08:20:26]

LGTM once a couple of methods in the implementation class have been made
private.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:8: * A helper-class for HTTP library, for parsing and
collecting HTTP message
Simplify to:

[HttpBodyHandler] is a helper class for parsing and collecting HTTP message data
in an easy-to-use [HttpBody] object.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:13: * [processRequest]).
How about adding the actual code for the two approaches to make it trivial for
users to plug this in?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:18: * The content body is parsed, depending on the set
'Content-Type' header field.
How about integrating this sentence in the first paragraph instead?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:45: static int BINARY = 0;
Make this a real HttpBodyType type instead of an int using the usual enum
pattern?

That way, the type of the 'type' getter can be restricted from int to
HttpBodyType.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:60: int get type;
HttpBodyType get type;

?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:69: * [Map] if [JSON], null otherwise.
[Map] if [type] equals [JSON], null otherwise.

?

Ditto for the ones below.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:89: abstract class HttpRequestBody extends HttpBody {
Add a top level comment for HttpRequestBody explaining that it is for a request
and gives access to the response object. Maybe add example code?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
File sdk/lib/io/http_body_impl.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:11: void handleData(HttpRequest request,
EventSink<HttpRequestBody> sink) {
You need to make this private. It is not part of the interface.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:31: static Future<HttpBody>
process(Stream<List<int>> stream,
You need to make this private, it is not part of the interface.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:69: // TODO(ajohnsen): Add support for FORM data.
For this commit, remove the FORM type so people are not confused by getting it
as BINARY?

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:90: Map get form => type == HttpBody.FORM ? body
: null;
Remove until supported?

[Anders Johnsen, 2013-04-10 12:52:48]

PTAL, changes based on offline chat.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:8: * A helper-class for HTTP library, for parsing and
collecting HTTP message
On 2013/04/10 08:20:26, Mads Ager wrote:
> Simplify to:
> 
> [HttpBodyHandler] is a helper class for parsing and collecting HTTP message
data
> in an easy-to-use [HttpBody] object.

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:13: * [processRequest]).
On 2013/04/10 08:20:26, Mads Ager wrote:
> How about adding the actual code for the two approaches to make it trivial for
> users to plug this in?

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:18: * The content body is parsed, depending on the set
'Content-Type' header field.
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> Remove 'set'.
> 
> I think we should enumerate the mime types which are handled, and add that
other
> mime types will be handled as binary.

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:20: class HttpBodyHandler
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> I like the transformer class with static methods for handling one object.
> 
> However I came to think of one thing. Do we want to support configuring this
> class e.g. configure that Content-type text/json should be handled as JSON as
> well or that application/json should be handled as BINARY. Without an instance
> it is difficult to do this for processRequest and processResponse unless we
add
> a configuration object which can be passed.
> 
> var handler = new HttpBodyHandler();
> handler.configureContentType(new ContentType("text/json"), HttpBody.JSON);
> handler.configureContentType(new ContentType("application/json"),
> HttpBody.BINARY);
> 
> ...
> 
> handler.processRequest(...);
> 
> vs.
> 
> var cfg = new HttpBodyHandlerConfiguration();
> cfg.configureContentType(new ContentType("text/json"), HttpBody.JSON);
> cfg.configureContentType(new ContentType("application/json"),
HttpBody.BINARY);
> 
> ...
> 
> HttpBodyHandler.processRequest(..., configuration: cfg);
> 
> Maybe that flexibility is not needed.

As discussed offline, this will serve as a great addition down the road. (post
M4).

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:45: static int BINARY = 0;
On 2013/04/10 08:20:26, Mads Ager wrote:
> Make this a real HttpBodyType type instead of an int using the usual enum
> pattern?
> 
> That way, the type of the 'type' getter can be restricted from int to
> HttpBodyType. 

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:60: int get type;
On 2013/04/10 08:20:26, Mads Ager wrote:
> HttpBodyType get type;
> 
> ?

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:63: * The actual body. The type depends on [type]
([Map] for [JSON] and [FROM],
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> FROM -> FORM

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:69: * [Map] if [JSON], null otherwise.
On 2013/04/10 08:20:26, Mads Ager wrote:
> [Map] if [type] equals [JSON], null otherwise.
> 
> ?
> 
> Ditto for the ones below.

These are removed.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:86: String get text;
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> Should we add isBinary, isJson, etc. as well?

No (due to offline chat).

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body.dart#...
sdk/lib/io/http_body.dart:89: abstract class HttpRequestBody extends HttpBody {
On 2013/04/10 08:20:26, Mads Ager wrote:
> Add a top level comment for HttpRequestBody explaining that it is for a
request
> and gives access to the response object. Maybe add example code?

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
File sdk/lib/io/http_body_impl.dart (right):

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:11: void handleData(HttpRequest request,
EventSink<HttpRequestBody> sink) {
On 2013/04/10 08:20:26, Mads Ager wrote:
> You need to make this private. It is not part of the interface.

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:31: static Future<HttpBody>
process(Stream<List<int>> stream,
On 2013/04/10 08:20:26, Mads Ager wrote:
> You need to make this private, it is not part of the interface.

It's static, so it should be safe.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:32: HttpHeaders headers) {
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> Indentation.

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:33: return stream.fold(
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> We could do a stream transformation to String here instead of using asText
> below. We do know from the Content-Type whether asString will be called.

It would complicate stuff, as we would have to fold over differend types
(_bufferlist, stringbuffer, etc). I'll keep as is for now, and we can look into
changing it with the planned changes to extend with new types.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:69: // TODO(ajohnsen): Add support for FORM data.
On 2013/04/10 08:20:26, Mads Ager wrote:
> For this commit, remove the FORM type so people are not confused by getting it
> as BINARY?

Done.

https://codereview.chromium.org/14019002/diff/3007/sdk/lib/io/http_body_impl....
sdk/lib/io/http_body_impl.dart:90: Map get form => type == HttpBody.FORM ? body
: null;
On 2013/04/10 08:20:26, Mads Ager wrote:
> Remove until supported?

Done.

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
File tests/standalone/io/http_body_test.dart (right):

https://codereview.chromium.org/14019002/diff/3007/tests/standalone/io/http_b...
tests/standalone/io/http_body_test.dart:59: test("text/plain; charset=utf-8",
"body".codeUnits, "body", HttpBody.TEXT);
On 2013/04/10 07:09:11, Søren Gjesse wrote:
> Test this with charset iso-8859-1 and us-ascii as well.

Done.

[Mads Ager (google), 2013-04-10 13:01:07]

Still LGTM. But please make sure users cannot add more values to the
HttpBodyType type.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:70: final String name;
_name

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:72: const HttpBodyType(String this.name);
HttpBodyType._(String this.name);

To make sure that you cannot create new ones dynamically.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:124: * responsing to the client.
responding

[Søren Gjesse, 2013-04-10 13:20:45]

lgtm

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:17: *   HttpServer server = ...
I think you need four space indent for dart doc to see this as code.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:74: String toString() => name;
Change toString to

String toString() => "HttpBodyType: $name;"

and add

String get name => _name

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:107: abstract class HttpClientResponseBody extends
HttpBody {
Add dart doc comments to these getters.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:107: abstract class HttpClientResponseBody extends
HttpBody {
Doesn't these getters belong to the HttpRequestBody?

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:126: abstract class HttpRequestBody extends HttpBody {
Add dart doc comments to these getters.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:126: abstract class HttpRequestBody extends HttpBody {
Doesn't these getters belong to the HttpClientResponseBody?

[Anders Johnsen, 2013-04-10 13:46:06]

Ty, landing.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart
File sdk/lib/io/http_body.dart (right):

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:17: *   HttpServer server = ...
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> I think you need four space indent for dart doc to see this as code.

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:70: final String name;
On 2013/04/10 13:01:07, Mads Ager wrote:
> _name

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:72: const HttpBodyType(String this.name);
On 2013/04/10 13:01:07, Mads Ager wrote:
> HttpBodyType._(String this.name);
> 
> To make sure that you cannot create new ones dynamically.

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:74: String toString() => name;
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> Change toString to
> 
> String toString() => "HttpBodyType: $name;"
> 
> and add
> 
> String get name => _name

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:107: abstract class HttpClientResponseBody extends
HttpBody {
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> Add dart doc comments to these getters.

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:107: abstract class HttpClientResponseBody extends
HttpBody {
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> Doesn't these getters belong to the HttpRequestBody?

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:124: * responsing to the client.
On 2013/04/10 13:01:07, Mads Ager wrote:
> responding

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:126: abstract class HttpRequestBody extends HttpBody {
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> Add dart doc comments to these getters.

Done.

https://codereview.chromium.org/14019002/diff/14001/sdk/lib/io/http_body.dart...
sdk/lib/io/http_body.dart:126: abstract class HttpRequestBody extends HttpBody {
On 2013/04/10 13:20:45, Søren Gjesse wrote:
> Doesn't these getters belong to the HttpClientResponseBody?

Done.

[Anders Johnsen, 2013-04-10 13:50:57]

Committed patchset #6 manually as r21202 (presubmit successful).