Skip to content

Null request payload/body values not handled properly #75

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.

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

Closed
bobjackman opened this issue Apr 6, 2017 · 5 comments
Closed

Null request payload/body values not handled properly #75

bobjackman opened this issue Apr 6, 2017 · 5 comments
Labels
closed-as-intended Closed as the reported issue is expected behavior

Comments

@bobjackman
Copy link

bobjackman commented Apr 6, 2017

It appears that http/src/utils.dart's mapToQuery() method doesn't properly handle objects with null values causing unhandled exceptions:

Unhandled exception:
NoSuchMethodError: The getter 'length' was called on null.
Receiver: null
Tried calling: length
...
#3      Uri.encodeQueryComponent (dart:core/uri.dart:1055)
...

Repro code:

import 'package:http/http.dart' as http;

main(List<String> args) async {
  // ======== Works
  try {
    // -- load model
    var foo = loadExistingModel("123");
    // -- modify
    foo.message += " modified";
    // -- save
    var postResult = await http.put('http://www.example.com/myModel', body: foo.asMap);
    print("++update succeeded");

  } catch(e) {
    print("!!update failed");
    rethrow;
  }

  // ======== Broken (due to `null` value for `id` property)
  try {
    // -- create model
    var foo = new MyModel("brand new model");
    // -- save new
    var postResult = await http.post('http://www.example.com/myModel', body: foo.asMap);
    print("++create succeeded");

  } catch(e) {
    print("!!create failed");
    rethrow;
  }
}

class MyModel {
  final String id; // -- only populated once saved
  String message;

  MyModel(this.message, [this.id]);

  Map<String, dynamic> get asMap {
    return {
      "id"     : id,
      "message": message
    };
  }
}

loadExistingModel(String id) {
  // fetch model by id
  // getResult = await http.get('http://www.example.com/myModel/$id');
  return new MyModel("existing model", id);
}

Suggested fix:

http-0.11.3+9/lib/src/utils.dart
 String mapToQuery(Map<String, String> map, {Encoding encoding}) {
   var pairs = <List<String>>[];
   map.forEach((key, value) =>
+      if (value == null) {
+        return; // -- skip null elements
+      }
+
       pairs.add([Uri.encodeQueryComponent(key, encoding: encoding),
                  Uri.encodeQueryComponent(value, encoding: encoding)]));
+  }
   return pairs.map((pair) => "${pair[0]}=${pair[1]}").join("&");
 }

Alternative fix:

http-0.11.3+9/lib/src/utils.dart
 String mapToQuery(Map<String, String> map, {Encoding encoding}) {
+  map.keys
+      .where((k) => (map[k] == null)).toList() // -- keys for null elements
+      .forEach(map.remove);
+
   var pairs = <List<String>>[];
   map.forEach((key, value) =>
       pairs.add([Uri.encodeQueryComponent(key, encoding: encoding),
                  Uri.encodeQueryComponent(value, encoding: encoding)]));

   return pairs.map((pair) => "${pair[0]}=${pair[1]}").join("&");
 }
@nex3
Copy link
Member

nex3 commented May 15, 2017

This behavior is intentional. It's generally the case in Dart that null values aren't allowed unless it's specifically indicated that they are—once we have support for explicitly declaring the nullability of a type, this will have non-nullable string values. It's the caller's responsibility to make sure that nulls don't get passed where they don't belong, not the callee's responsibility to clean them up.

@nex3 nex3 closed this as completed May 15, 2017
@nex3 nex3 added the closed-as-intended Closed as the reported issue is expected behavior label May 15, 2017
@bobjackman
Copy link
Author

bobjackman commented May 16, 2017

In that case, I feel like we could benefit from a better error message. Maybe something like this?

http-0.11.3+9/lib/src/utils.dart
 String mapToQuery(Map<String, String> map, {Encoding encoding}) {
+  if (map.values.any((v) => (v == null))) { // -- null values found
+      throw new ArgumentError(map, "map", "NULL values not supported in querystring");
+  }
+
   var pairs = <List<String>>[];
   map.forEach((key, value) =>
       pairs.add([Uri.encodeQueryComponent(key, encoding: encoding),
                  Uri.encodeQueryComponent(value, encoding: encoding)]));

   return pairs.map((pair) => "${pair[0]}=${pair[1]}").join("&");
 }

@jonkirkman
Copy link

I definitely appreciate the principles that @nex3 mentions and agree that non-nullable types will help clarify this in the future.
However without a specific error, such as @kogi suggests, this looks like an implementation mistake on the part of the library author.

@nex3
Copy link
Member

nex3 commented May 17, 2017

For the same reason, we generally don't include explicit null checks in places where null isn't expected—otherwise we'd have them all over the place.

@vmihalachi
Copy link

Hi, so if I need to update a record on the server and set the value of one of the fields to be null, how can I accomplish this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-as-intended Closed as the reported issue is expected behavior
Projects
None yet
Development

No branches or pull requests

4 participants