Fix content type within Message #120

donny-dont · 2017-09-22T22:00:40Z

Attempt to fix content-type within Message.

Fixes #101

donny-dont · 2017-09-22T22:04:54Z

@nex3 if this is the approach you want I'll comment up the functions/methods before merging.

donny-dont · 2017-09-27T05:46:15Z

@nex3 so I made change actually look at the value of body to determine if it was set during the change. This makes it so you can skip the internal headers adjustment which was something I was trying to avoid.

nex3 · 2017-09-22T22:50:57Z

lib/src/body.dart

@@ -36,25 +38,20 @@ class Body {
  /// used to convert it to a [Stream<List<int>>].
  factory Body(body, [Encoding encoding]) {
    if (body is Body) return body;
+    if (body == null)
+      return new Body._(new Stream.fromIterable([]), encoding, 0);


new Stream.fromIterable() could be const Stream.empty(). It might be even cleaner to return a static _empty field here, though, to avoid any allocations in the common case of an empty body.

Nit: use curly braces.

nex3 · 2017-09-22T22:54:14Z

lib/src/body.dart

+
+    if (body is Map) {
+      body = mapToQuery(body, encoding: encoding);
+    }


Why isn't this in the if/else chain with the other type tests?

The value of body becomes a String at this point so it will end up in the is String which would then encode it.

nex3 · 2017-09-22T22:54:26Z

lib/src/body.dart

+      body = mapToQuery(body, encoding: encoding);
+    }
+
+    if (body is String) {


What happened to the branch where encoding == null?

It should never be null since the encoding is calculated.

You mean when it's set to UTF8 above? But then you're marking ASCII-only bodies as UTF8 when a text/plain content type would do. The old behavior here was correct, there's no need to change it.

Ok I'll take a look at what 0.11 was doing here.

Not 0.11, just the previous code in master.

nex3 · 2017-09-27T20:44:25Z

lib/src/message.dart


-  Message._(Body body, Map<String, String> headers, Map<String, Object> context)
+  Message.__(body, MediaType contentType, Map<String, String> headers,


__ isn't a good name for a constructor--it doesn't communicate what the difference is between this and the other unnamed constructor.

K I think I need to rename this anyways so it could be used in the multipart implementation.

I also didn't like fromValues but didn't have any better ideas last night.

nex3 · 2017-09-27T20:53:33Z

lib/src/message.dart

-  Message._(Body body, Map<String, String> headers, Map<String, Object> context)
+  Message.__(body, MediaType contentType, Map<String, String> headers,
+      Map<String, Object> context)
+      : this._(new Body(body, encodingForMediaType(contentType, null)), contentType,


This new Body(...) invocation looks wrong to me... the Body's encoding shouldn't ever come from the headers, it should only come from the explicit encoding passed by the user. So this should continue to be new Body(body, encoding), which means you don't have to do this many-layered-constructor thing anymore.

_determineMediaType is what determines the encoding for the body. If encoding is set directly then it should override what is in content-type header so that value can be used when creating the body.

This reflects the behavior of this test https://github.com/dart-lang/http/blob/master/test/request_test.dart#L152-L158

The way this is written currently, the encoding passed to new Body() can come from headers. That's not correct. It should only ever come from the encoding parameter, or from the default value (which should be UTF-8).

So just to be clear if someone were to set content-type: 'application/json; charset=iso-8859-1' and not set encoding to LATIN when creating a Request it should not encode the body with LATIN it should be UTF8 encoded?

Correct, and the header should be updated accordingly.

nex3 · 2017-09-27T20:57:56Z

lib/src/message.dart

+  ///
+  /// This constructor should be used when no computation is required for the
+  /// [body], [headers] or [context].
+  Message.fromValues(


I understand the performance motivation for having this constructor, but I'd like to focus on API shape and correctness for now and leave optimizations like this for later (ideally driven by real data).

Ok I'll rework this accordingly.

donny-dont · 2017-09-28T00:56:45Z

@nex3 should be good to go. I think I addressed all your feedback.

nex3 · 2017-09-28T20:58:02Z

lib/src/body.dart

@@ -27,6 +29,9 @@ class Body {
  /// determined efficiently.
  final int contentLength;

+  /// An empty stream for use with empty bodies.
+  static const _emptyStream = const Stream.empty();


I meant store an instance of Body statically:

static _empty = new Body._(const Stream.empty(), null, 0);

Creating a static field for a const value doesn't do anything since const constructors are already canonicalized.

Was looking at implementing this but the issue is that calling read removes the stream.

Do you instead want an EmptyBody class that lets you call read() multiple times?

Do you instead want an EmptyBody class that lets you call read() multiple times?

Yeah, I think this is the way to go. That matches the behavior of const Stream.empty(), too.

nex3 · 2017-09-28T21:02:37Z

lib/src/body.dart

+      body = mapToQuery(body, encoding: encoding);
+    }
+
+    if (body is String) {


You mean when it's set to UTF8 above? But then you're marking ASCII-only bodies as UTF8 when a text/plain content type would do. The old behavior here was correct, there's no need to change it.

nex3 · 2017-09-28T21:04:33Z

lib/src/message.dart


-  Message._(Body body, Map<String, String> headers, Map<String, Object> context)
+  Message.withContentType(Body body, MediaType contentType,


I don't want this constructor here. The shape of the constructors from before this CL is generally correct; the only change should be adding a {bool bodyIsMap: false} parameter to new Message._() and _adjustHeaders().

I had planned to use it with http.MultiPart since the MediaType requires a boundary which would be randomly generated.

I'd rather have _adjustHeaders() do some logic like

if (body is MultipartBody) { contentType = "multipart/mixed; boundary=${body.boundary}"; }

Fix content type

75be84a

googlebot added the cla: yes label Sep 22, 2017

Add fromValues constructor

928d7a1

nex3 suggested changes Sep 27, 2017

View reviewed changes

Return an emptyStream with null body

5c7b451

donny-dont force-pushed the fix/content-type-message branch from 1fb0ee5 to 5c7b451 Compare September 28, 2017 00:38

Review comments

7da22ce

unused

d05187a

nex3 suggested changes Sep 28, 2017

View reviewed changes

Add EmptyBody implementation

9f5fbf4

donny-dont closed this Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix content type within Message #120

Fix content type within Message #120

donny-dont commented Sep 22, 2017 •

edited

Loading

donny-dont commented Sep 22, 2017

donny-dont commented Sep 27, 2017

nex3 Sep 22, 2017

nex3 Sep 22, 2017

donny-dont Sep 27, 2017

nex3 Sep 22, 2017

donny-dont Sep 27, 2017

nex3 Sep 28, 2017

donny-dont Sep 28, 2017

nex3 Sep 28, 2017

nex3 Sep 27, 2017

donny-dont Sep 27, 2017

nex3 Sep 27, 2017

donny-dont Sep 27, 2017

nex3 Sep 27, 2017

donny-dont Sep 27, 2017

nex3 Sep 27, 2017

nex3 Sep 27, 2017

donny-dont Sep 27, 2017

donny-dont commented Sep 28, 2017

nex3 Sep 28, 2017

donny-dont Sep 28, 2017

nex3 Sep 28, 2017

nex3 Sep 28, 2017

nex3 Sep 28, 2017

donny-dont Sep 28, 2017

nex3 Sep 28, 2017


		Message._(Body body, Map<String, String> headers, Map<String, Object> context)
		Message.__(body, MediaType contentType, Map<String, String> headers,


		Message._(Body body, Map<String, String> headers, Map<String, Object> context)
		Message.withContentType(Body body, MediaType contentType,

Fix content type within Message #120

Fix content type within Message #120

Conversation

donny-dont commented Sep 22, 2017 • edited Loading

donny-dont commented Sep 22, 2017

donny-dont commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donny-dont commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donny-dont commented Sep 22, 2017 •

edited

Loading