Merge pull request #992 from marshmallow-code/dev_raise_on_attribute_…

…or_data_key_collision

Raise ValueError on attribute or data_key collision

CHANGELOG.rst

@@ -12,6 +12,8 @@ Features:
   Thanks :user:`knagra` for implementing these changes.
 - Enforce ISO 8601 when deserializing date and time (:issue:`899`).
   Thanks :user:`dushr` for the report and the work on the PR.
+- *Backwards-incompatible*: Raise ``ValueError`` on ``Schema`` instantiation in
+  case of ``attribute`` or ``data_key`` collision (:pr:`992`).
 
 Bug fixes:
 

docs/upgrading.rst

@@ -667,6 +667,47 @@ The ``Meta`` option ``dateformat`` used to pass format to `DateTime <marshmallow
     MySchema().dump({'x': dt.datetime(2017, 9, 19), 'y': dt.date(2017, 9, 19)})
     # => {{'x': '2017-09', 'y': '09-19'}}
 
+``attribute`` or ``data_key`` collision triggers an exception
+*************************************************************
+
+When a `Schema <marshmallow.Schema>` is instantiated, a check is performed and a ``ValueError`` is triggered if
+
+- several fields have the same ``attribute`` value (or field name if ``attribute`` is not passed), excluding ``dump_only`` fields, or
+- several fields have the same ``data_key`` value (or field name if ``data_key`` is not passed), excluding ``load_only`` fields
+
+In marshmallow 2, it was possible to have multiple fields with the same ``attribute``. It would work provided the ``Schema`` was only used for dumping. When loading, the behaviour was undefined. In marshmallow 3, all but one of those fields must be marked as ``dump_only``. Likewise for ``data_key`` (formerly ``dump_to``) for fields that are not ``load_only``.
+
+.. code-block:: python
+
+    # 2.x
+    class MySchema(Schema):
+        f1 = fields.Field()
+        f2 = fields.Field(attribute='f1')
+        f3 = fields.Field(attribute='f5')
+        f4 = fields.Field(attribute='f5')
+
+    MySchema()
+    # No error
+
+    # 3.x
+    class MySchema(Schema):
+        f1 = fields.Field()
+        f2 = fields.Field(attribute='f1')
+        f3 = fields.Field(attribute='f5')
+        f4 = fields.Field(attribute='f5')
+
+    MySchema()
+    # ValueError: 'Duplicate attributes: ['f1', 'f5]'
+
+    class MySchema(Schema):
+        f1 = fields.Field()
+        f2 = fields.Field(attribute='f1', dump_only=True)
+        f3 = fields.Field(attribute='f5')
+        f4 = fields.Field(attribute='f5', dump_only=True)
+
+    MySchema()
+    # No error
+
 Upgrading to 2.3
 ++++++++++++++++
 

marshmallow/schema.py

@@ -714,6 +714,20 @@ def _init_fields(self):
             self._bind_field(field_name, field_obj)
             fields_dict[field_name] = field_obj
 
+        dump_data_keys = [
+            obj.data_key or name for name, obj in iteritems(fields_dict) if not obj.load_only
+        ]
+        if len(dump_data_keys) != len(set(dump_data_keys)):
+            data_keys_duplicates = {x for x in dump_data_keys if dump_data_keys.count(x) > 1}
+            raise ValueError('Duplicate data_keys: {}'.format(data_keys_duplicates))
+
+        load_attributes = [
+            obj.attribute or name for name, obj in iteritems(fields_dict) if not obj.dump_only
+        ]
+        if len(load_attributes) != len(set(load_attributes)):
+            attributes_duplicates = {x for x in load_attributes if load_attributes.count(x) > 1}
+            raise ValueError('Duplicate attributes: {}'.format(attributes_duplicates))
+
         return fields_dict
 
     def on_bind_field(self, field_name, field_obj):

tests/base.py

@@ -152,13 +152,13 @@ class UserSchema(Schema):
     name = fields.String()
     age = fields.Float()
     created = fields.DateTime()
-    created_formatted = fields.DateTime(format='%Y-%m-%d', attribute='created')
-    created_iso = fields.DateTime(format='iso', attribute='created')
+    created_formatted = fields.DateTime(format='%Y-%m-%d', attribute='created', dump_only=True)
+    created_iso = fields.DateTime(format='iso', attribute='created', dump_only=True)
     updated = fields.DateTime()
-    updated_local = fields.LocalDateTime(attribute='updated')
+    updated_local = fields.LocalDateTime(attribute='updated', dump_only=True)
     species = fields.String(attribute='SPECIES')
     id = fields.String(default='no-id')
-    uppername = Uppercased(attribute='name')
+    uppername = Uppercased(attribute='name', dump_only=True)
     homepage = fields.Url()
     email = fields.Email()
     balance = fields.Decimal()
@@ -197,11 +197,11 @@ def make_user(self, data):
 
 class UserMetaSchema(Schema):
     """The equivalent of the UserSchema, using the ``fields`` option."""
-    uppername = Uppercased(attribute='name')
+    uppername = Uppercased(attribute='name', dump_only=True)
     balance = fields.Decimal()
     is_old = fields.Method('get_is_old')
     lowername = fields.Function(get_lowername)
-    updated_local = fields.LocalDateTime(attribute='updated')
+    updated_local = fields.LocalDateTime(attribute='updated', dump_only=True)
     species = fields.String(attribute='SPECIES')
     homepage = fields.Url()
     email = fields.Email()

tests/test_schema.py

@@ -1172,6 +1172,36 @@ class ParentSchema(Schema):
     assert 'bah' not in grand_child
 
 
+@pytest.mark.parametrize('data_key', ('f1', 'f5', None))
+def test_data_key_collision(data_key):
+
+    class MySchema(Schema):
+        f1 = fields.Field()
+        f2 = fields.Field(data_key=data_key)
+        f3 = fields.Field(data_key='f5')
+        f4 = fields.Field(data_key='f1', load_only=True)
+
+    if data_key is None:
+        MySchema()
+    else:
+        with pytest.raises(ValueError, match=data_key):
+            MySchema()
+
+@pytest.mark.parametrize('attribute', ('f1', 'f5', None))
+def test_attribute_collision(attribute):
+
+    class MySchema(Schema):
+        f1 = fields.Field()
+        f2 = fields.Field(attribute=attribute)
+        f3 = fields.Field(attribute='f5')
+        f4 = fields.Field(attribute='f1', dump_only=True)
+
+    if attribute is None:
+        MySchema()
+    else:
+        with pytest.raises(ValueError, match=attribute):
+            MySchema()
+
 class TestDeeplyNestedLoadOnly:
 
     @pytest.fixture()