Repository: maximkulkin/lollipop

Issue:
Asymmetry in load/dump for Dict with key_type and dict as values_type
I think there is a problem with the Dict type when using both key_type verification and a values_type dictionary:

In Dict.dump, the key is used in its original / non-dumped form to lookup the value type. However, in Dict.load, the dumped key is used to lookup the value type.

This works fine when using native types as key type such as String and Integer since they map to the same loaded/dumped value. But it's causing a problem when using a more complex key type (e.g. in my case an Enum that dumps to a string).

I believe, in Dict.load, key_type.load should be called **before** the lookup of the value type, so that the lookup is again performed with the original / non-dumped value. 


Discussion:

Patch:
diff --git a/lollipop/types.py b/lollipop/types.py
index 3ec8a50..acb7f3b 100644
--- a/lollipop/types.py
+++ b/lollipop/types.py
@@ -679,16 +679,22 @@ class Dict(Type):
         errors_builder = ValidationErrorBuilder()
         result = {}
         for k, v in iteritems(data):
-            value_type = self.value_types.get(k)
-            if value_type is None:
-                continue
             try:
                 k = self.key_type.load(k, *args, **kwargs)
             except ValidationError as ve:
                 errors_builder.add_error(k, ve.messages)
 
+            if k is MISSING:
+                continue
+
+            value_type = self.value_types.get(k)
+            if value_type is None:
+                continue
+
             try:
-                result[k] = value_type.load(v, *args, **kwargs)
+                value = value_type.load(v, *args, **kwargs)
+                if value is not MISSING:
+                    result[k] = value
             except ValidationError as ve:
                 errors_builder.add_error(k, ve.messages)
 
@@ -715,8 +721,13 @@ class Dict(Type):
             except ValidationError as ve:
                 errors_builder.add_error(k, ve.messages)
 
+            if k is MISSING:
+                continue
+
             try:
-                result[k] = value_type.dump(v, *args, **kwargs)
+                value = value_type.dump(v, *args, **kwargs)
+                if value is not MISSING:
+                    result[k] = value
             except ValidationError as ve:
                 errors_builder.add_error(k, ve.messages)
 


Test patch:
diff --git a/tests/test_types.py b/tests/test_types.py
index e652fb6..6489dbf 100644
--- a/tests/test_types.py
+++ b/tests/test_types.py
@@ -641,6 +641,30 @@ class TestDict(NameDescriptionTestsMixin, RequiredTestsMixin, ValidationTestsMix
         assert Dict(Any(), key_type=Integer())\
             .load({'123': 'foo', '456': 'bar'}) == {123: 'foo', 456: 'bar'}
 
+    def test_loading_dict_with_custom_key_type_and_values_of_different_types(self):
+        assert Dict({1: Integer(), 2: String()}, key_type=Integer())\
+            .load({'1': '123', '2': 'bar'}) == {1: 123, 2: 'bar'}
+
+    def test_loading_skips_key_value_if_custom_key_type_loads_to_missing(self):
+        class CustomKeyType(String):
+            def load(self, data, *args, **kwargs):
+                if data == 'foo':
+                    return MISSING
+                return super(CustomKeyType, self).load(data, *args, **kwargs)
+
+        assert Dict(String(), key_type=CustomKeyType())\
+            .load({'foo': 'hello', 'bar': 'goodbye'}) == {'bar': 'goodbye'}
+
+    def test_loading_skips_key_value_if_value_type_loads_to_missing(self):
+        class CustomValueType(String):
+            def load(self, data, *args, **kwargs):
+                if data == 'foo':
+                    return MISSING
+                return super(CustomValueType, self).load(data, *args, **kwargs)
+
+        assert Dict(CustomValueType())\
+            .load({'key1': 'foo', 'key2': 'bar'}) == {'key2': 'bar'}
+
     def test_loading_accepts_any_key_if_key_type_is_not_specified(self):
         assert Dict(Any())\
             .load({'123': 'foo', 456: 'bar'}) == {'123': 'foo', 456: 'bar'}
@@ -719,7 +743,27 @@ class TestDict(NameDescriptionTestsMixin, RequiredTestsMixin, ValidationTestsMix
     def test_dumping_dict_with_values_of_different_types(self):
         value = {'foo': 1, 'bar': 'hello', 'baz': True}
         assert Dict({'foo': Integer(), 'bar': String(), 'baz': Boolean()})\
-            .load(value) == value
+            .dump(value) == value
+
+    def test_dumping_skips_key_value_if_custom_key_type_loads_to_missing(self):
+        class CustomKeyType(String):
+            def dump(self, data, *args, **kwargs):
+                if data == 'foo':
+                    return MISSING
+                return super(CustomKeyType, self).load(data, *args, **kwargs)
+
+        assert Dict(String(), key_type=CustomKeyType())\
+            .dump({'foo': 'hello', 'bar': 'goodbye'}) == {'bar': 'goodbye'}
+
+    def test_dumping_skips_key_value_if_value_type_loads_to_missing(self):
+        class CustomValueType(String):
+            def dump(self, data, *args, **kwargs):
+                if data == 'foo':
+                    return MISSING
+                return super(CustomValueType, self).load(data, *args, **kwargs)
+
+        assert Dict(CustomValueType())\
+            .dump({'key1': 'foo', 'key2': 'bar'}) == {'key2': 'bar'}
 
     def test_dumping_accepts_any_value_if_value_types_are_not_specified(self):
         assert Dict()\