Implement multipart forms #113

donny-dont · 2017-09-21T20:45:51Z

Implement multipart form handling.

Fixes #97

donny-dont · 2017-09-21T20:47:03Z

@nex3 currently work in progress for the implementation. I have tests running locally but won't push that until we figure out if this is what you want.

Another option rather than implementing Body is to move a constructor over there and keep the multipart file handling in there or in a utils sort of file with free functions.

nex3

The overall architecture here looks good!

nex3 · 2017-09-21T22:55:15Z

lib/src/boundary_characters.dart

+
+/// The total length of the multipart boundaries used when building the
+/// request body. According to http://tools.ietf.org/html/rfc1341.html, this
+/// can't be longer than 70.


Nit: Documentation comments should start with a single-sentence paragraph.

nex3 · 2017-09-21T22:55:55Z

lib/src/boundary_characters.dart

+/// The total length of the multipart boundaries used when building the
+/// request body. According to http://tools.ietf.org/html/rfc1341.html, this
+/// can't be longer than 70.
+const int BOUNDARY_LENGTH = 70;


Nit: Constants should be lower camel case. Same for BOUNDARY_CHARACTERS.

I'd also omit the int here.

nex3 · 2017-09-21T22:57:14Z

lib/src/boundary_characters.dart

@@ -2,6 +2,8 @@
 // for details. All rights reserved. Use of this source code is governed by a


I'm not sure why this was ever a separate library... I'd just put this stuff in multipart_request.dart.

nex3 · 2017-09-21T22:57:49Z

lib/src/boundary_characters.dart

+
+final Random _random = new Random();
+
+/// Returns a randomly-generated multipart boundary string


Nit: Period at the end of the sentence.

nex3 · 2017-09-21T23:01:47Z

lib/src/multipart_file.dart

@@ -44,20 +40,20 @@ class MultipartFile {
  /// future may be inferred from [filename].
  MultipartFile(this.field, Stream<List<int>> stream, this.length,


What do you think about consolidating these constructors? I'm thinking:

new MultipartFile(String field, body, ...) which takes either a List<int> or a String,

new MultipartFile.fromStream(String field, stream, int length) which takes either a Stream<List<int>> or a Stream<String>,

static Future<MultipartFile> loadStream(String field, stream) which takes either a Stream<List<int>> or a Stream<String> and drains it to determine the length.

I've always kinda preferred the explicit constructors rather than using dynamic since there are no union types. I do like the loadStream idea rather than fromPath.

Would fromStream be possible with a Stream<String> though as there would need to be some kind of encoding?

I've always kinda preferred the explicit constructors rather than using dynamic since there are no union types. I do like the loadStream idea rather than fromPath.

I disagree. That imposes more writing on the user for minimal static analysis gains. Also, union types will almost certainly happen eventually so it's a good idea to plan APIs around them now where possible.

Would fromStream be possible with a Stream<String> though as there would need to be some kind of encoding?

I suppose we'd need an optional Encoding parameter like we do for the Request constructors.

nex3 · 2017-09-21T23:12:52Z

lib/src/multipart_request.dart

+    }
+    void writeLine() {
+      controller.add([13, 10]); // \r\n
+      contentLength += 2;


Since you're only using these for fields now, you might as well just add everything to a single Uint8List rather than adding separate chunks to a controller. The typed_data package can help with that.

nex3 · 2017-09-21T23:19:45Z

lib/src/multipart_request.dart

+      header.addAll(UTF8.encode(_headerForFile(file)));
+      contentLength += header.length + file.length + 2;
+      fileHeaders[i] = header;
+    }


Add a comment explaining that we compute the headers ahead of time so we can synchronously access the length.

nex3 · 2017-09-21T23:21:19Z

lib/src/multipart_request.dart

+    // used to get the fileHeaders
+    var i = 0;
+
+    Future.forEach(files, (file) {


Consider pulling this into a static method so you can make it async and just use a normal for loop.

nex3 · 2017-09-21T23:22:08Z

lib/src/multipart_request.dart

-    // characters). We follow their behavior.
-    return value.replaceAll(_newlineRegExp, "%0D%0A").replaceAll('"', "%22");
+/// Returns the header string for a field.
+String _headerForField(String name, String value) {


I generally make private methods that are only used by one class members of that class, even if they don't access its state directly.

These are accessed from a factory constructor so I put them below. Do you want them as static class methods?

nex3 · 2017-09-21T23:22:56Z

lib/src/request.dart

+    var boundary = boundaryString();
+
+    headers ??= <String, String>{};
+    headers['content-type'] = 'multipart/form-data; boundary=$boundary';


We shouldn't destructively modify the user's headers map.

donny-dont · 2017-10-20T19:50:44Z

@nex3 https://pub.dartlang.org/packages/mime yay or nay?

nex3 · 2017-10-20T22:43:47Z

lib/src/multipart_file.dart

@@ -44,20 +40,20 @@ class MultipartFile {
  /// future may be inferred from [filename].
  MultipartFile(this.field, Stream<List<int>> stream, this.length,


I've always kinda preferred the explicit constructors rather than using dynamic since there are no union types. I do like the loadStream idea rather than fromPath.

I disagree. That imposes more writing on the user for minimal static analysis gains. Also, union types will almost certainly happen eventually so it's a good idea to plan APIs around them now where possible.

Would fromStream be possible with a Stream<String> though as there would need to be some kind of encoding?

I suppose we'd need an optional Encoding parameter like we do for the Request constructors.

nex3 · 2017-10-20T22:57:43Z

lib/src/multipart_body.dart

+import 'multipart_file.dart';
+import 'utils.dart';
+
+class MultipartBody implements Body {


Document this

nex3 · 2017-10-20T22:57:46Z

lib/src/multipart_body.dart

@@ -0,0 +1,147 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file


nex3 · 2017-10-20T23:01:06Z

lib/src/multipart_body.dart

+
+  /// The length of the stream returned by [read].
+  ///
+  /// This is calculated from the fields and files passed into the body.


You generally don't have to document inherited members, since they're documented in the parent class.

nex3 · 2017-10-20T23:01:21Z

lib/src/multipart_body.dart

+
+  /// Creates a [MultipartBody] from the given [fields] and [files].
+  ///
+  /// The [boundary] is used to


Unfinished docs

nex3 · 2017-10-25T05:18:26Z

test/utils.dart

@@ -66,6 +67,40 @@ class _Parse extends Matcher {
  }
 }

+/// A matcher that validates the body of a multipart request after finalization.


Nit: First sentence should be its own paragraph.

nex3 · 2017-10-25T05:19:48Z

test/utils.dart

+/// The string "{{boundary}}" in [pattern] will be replaced by the boundary
+/// string for the request, and LF newlines will be replaced with CRLF.
+/// Indentation will be normalized.
+Matcher bodyMatches(String pattern) => new _BodyMatches(pattern);


This name is pretty generic for something that's specifically designed to work with multipart requests. Maybe multipartBodyMatches()?

nex3 · 2017-10-25T05:23:57Z

test/utils.dart

+  bool matches(item, Map matchState) {
+    if (item is! http.Request) return false;
+
+    var future = collectBytes(item.read()).then((bodyBytes) {


Why not just use Request.readAsString() here?

Because of this check. The string length isn't necessarily the same as the contentLength at this point.

expect(item.contentLength, equals(bodyBytes.length));

Hmm... it might be nice to add Request.readAsBytes().

nex3 · 2017-10-25T05:35:24Z

test/multipart_test.dart

@@ -0,0 +1,249 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file


nex3 · 2017-10-25T05:35:52Z

test/multipart_test.dart

+        --{{boundary}}--
+        '''));
+  });
+}


Are these ported directly from the old API?

Yep they're from the 0.11.x branch.

donny-dont

Hopefully this commit resolves everything but the constructors.

mime was just pushed so I'll integrate that when going through that.

donny-dont · 2017-10-25T18:29:31Z

test/utils.dart

+  bool matches(item, Map matchState) {
+    if (item is! http.Request) return false;
+
+    var future = collectBytes(item.read()).then((bodyBytes) {


Because of this check. The string length isn't necessarily the same as the contentLength at this point.

expect(item.contentLength, equals(bodyBytes.length));

donny-dont · 2017-10-25T18:31:48Z

test/multipart_test.dart

+        --{{boundary}}--
+        '''));
+  });
+}


Yep they're from the 0.11.x branch.

donny-dont · 2017-10-25T18:32:43Z

pubspec.yaml

@@ -8,6 +8,7 @@ dependencies:
  collection: "^1.5.0"
  http_parser: ">=0.0.1 <4.0.0"
  path: ">=0.9.0 <2.0.0"
+  typed_data: "^1.1.5"


Just for my own reference are we going to want to tighten the restriction for strong mode compliance?

donny-dont · 2017-10-25T18:34:04Z

lib/src/request.dart

+        Iterable<MultipartFile> files}) {
+    fields ??= <String, String>{};
+    files ??= <MultipartFile>[];
+    headers ??= <String, String>{};


It will end up getting set when a HttpUnmodifiableMap is created within the Message constructor so no it doesn't need one.

nex3 · 2017-10-25T21:02:52Z

test/utils.dart

+  bool matches(item, Map matchState) {
+    if (item is! http.Request) return false;
+
+    var future = collectBytes(item.read()).then((bodyBytes) {


Hmm... it might be nice to add Request.readAsBytes().

nex3 · 2017-10-25T21:04:21Z

pubspec.yaml

@@ -8,6 +8,7 @@ dependencies:
  collection: "^1.5.0"
  http_parser: ">=0.0.1 <4.0.0"
  path: ">=0.9.0 <2.0.0"
+  typed_data: "^1.1.5"


We generally don't do that. This package doesn't depend on the fact that its dependencies are strong-mode compliant; that's a property of the root package, if anything.

nex3 · 2017-10-26T01:33:15Z

lib/src/multipart_body.dart

+      controller.add(fileHeaders[i]);
+      try {
+        await writeStreamToSink(files[i].read(), controller);
+      } on Exception catch (exception, stackTrace) {


Leave off the on clause here. We want to catch anything that gets thrown.

nex3 · 2017-10-26T01:33:35Z

lib/src/multipart_body.dart

+      try {
+        await writeStreamToSink(files[i].read(), controller);
+      } on Exception catch (exception, stackTrace) {
+        controller.addError(exception, stackTrace);


Document that we're catching here because file.read() can throw synchronously.

nex3 · 2017-10-26T01:43:29Z

lib/src/multipart_file.dart

-  bool get isFinalized => _isFinalized;
-  bool _isFinalized = false;
+    if (value is String) {
+      encoding ??= encodingForMediaType(contentType) ?? UTF8;


Like with the request body, I'd like the value encoding to come only from the encoding parameter, not from the contentType. This ensures that filling in the defaults flows only one way: contentType always looks at encoding for its default value, not the other way around.

Also, we should only set the encoding to UTF8—and thus add a charset parameter to the content type—if the value isn't plain ASCII.

nex3 · 2017-10-26T18:44:14Z

lib/src/multipart_file.dart

+
+  /// Looks up the [MediaType] from the [filename]'s extension or from
+  /// magic numbers contained within a file header's [bytes].
+  static MediaType _lookupMediaType(String filename, [List<int> bytes]) {


Nit: _lookUpMediaType

"Lookup" is a word, but it's only a noun; the verb is always two words.

nex3 · 2017-10-26T18:45:17Z

lib/src/multipart_file.dart

+  /// Looks up the [MediaType] from the [filename]'s extension or from
+  /// magic numbers contained within a file header's [bytes].
+  static MediaType _lookupMediaType(String filename, [List<int> bytes]) {
+    if ((filename == null) && (bytes == null)) return null;


Nit: unnecessary parens.

nex3 · 2017-10-26T18:45:45Z

lib/src/multipart_file.dart

+    if ((filename == null) && (bytes == null)) return null;
+
+    // lookupMimeType expects filename to be non-null but its possible that
+    // this can be called with bytes but no filename.


Can you file an issue against the mime package for this?

nex3 · 2017-10-26T18:47:49Z

lib/src/request.dart

-  Request._(this.method, this.url,
-      body,
-      Encoding encoding,
+  /// Creates a new `multipart/form-data` [Request] to [url], which can be a


Consider linking multipart/form-data to an explanation of the format (maybe Wikipedia).

nex3 · 2017-10-26T18:49:35Z

pubspec.yaml

@@ -7,7 +7,8 @@ dependencies:
  async: "^1.13.0"
  collection: "^1.5.0"
  http_parser: ">=0.0.1 <4.0.0"
-  path: ">=0.9.0 <2.0.0"
+  mime: ">=0.9.0 < 1.0.0"


^0.9.0

With this version constraint, mime could release a totally breaking 0.10.0 release and we'd still declare compatibility with it.

nex3 · 2017-10-26T20:02:53Z

Looks good! Thanks for all your hard work on this 😄.

googlebot added the cla: yes label Sep 21, 2017

nex3 reviewed Sep 21, 2017

View reviewed changes

donny-dont force-pushed the fix/multipart branch from 52b79bc to 6029e43 Compare October 20, 2017 00:07

donny-dont force-pushed the fix/multipart branch from 2097129 to 6029e43 Compare October 20, 2017 22:56

donny-dont added 8 commits October 20, 2017 15:57

Update MultipartFile

83a94f9

Update MultipartRequest

a949063

Fixing some nits

4f75ac7

Additional nits

8592a0e

Formatting

19dcfde

Rename

69f6d95

Use updateMap to add content type

26e7bf9

Add comment to control dartfmt of boundary characters

20afe26

donny-dont force-pushed the fix/multipart branch from 6029e43 to 20afe26 Compare October 20, 2017 22:57

donny-dont added 5 commits October 20, 2017 16:32

nits

7788518

Add and pass tests

55b1666

dartfmt

4866d51

Use Uint8Buffer

7db4f18

Use async function for writing files

9609d56

nex3 suggested changes Oct 25, 2017

View reviewed changes

Fixing review comments

39667b7

donny-dont commented Oct 25, 2017

View reviewed changes

donny-dont added 3 commits October 25, 2017 12:01

nits

28208ae

Update with modified constructors

83bfc8d

Formating

04d482f

nex3 suggested changes Oct 26, 2017

View reviewed changes

donny-dont added 3 commits October 26, 2017 12:32

Add readAsBytes to Message

7246b9a

Fix nits

05350c1

More nits

d6be804

nex3 approved these changes Oct 26, 2017

View reviewed changes

nex3 merged commit 5983d3f into dart-lang:master Oct 26, 2017

donny-dont deleted the fix/multipart branch October 26, 2017 20:03

donny-dont mentioned this pull request Oct 26, 2017

strong mode errors #33

Closed

aliaafreen mentioned this pull request Nov 24, 2022

SocketException: Connection timed out (OS Error: Connection timed out, errno = 110) #831

Closed

		@@ -2,6 +2,8 @@
		// for details. All rights reserved. Use of this source code is governed by a


		final Random _random = new Random();

		/// Returns a randomly-generated multipart boundary string

		@@ -44,20 +40,20 @@ class MultipartFile {
		/// future may be inferred from [filename].
		MultipartFile(this.field, Stream<List<int>> stream, this.length,

		@@ -0,0 +1,147 @@
		// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file

		@@ -0,0 +1,249 @@
		// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file

Implement multipart forms #113

Implement multipart forms #113

Conversation

donny-dont commented Sep 21, 2017

donny-dont commented Sep 21, 2017

nex3 left a 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 Oct 20, 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

donny-dont left a 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

nex3 commented Oct 26, 2017