diff --git a/CHANGELOG.rst b/CHANGELOG.rst index eee192ae8..1fb1474c0 100644 --- a/CHANGELOG.rst +++ b/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: diff --git a/docs/upgrading.rst b/docs/upgrading.rst index ff71d6d4b..12b585e92 100644 --- a/docs/upgrading.rst +++ b/docs/upgrading.rst @@ -667,6 +667,47 @@ The ``Meta`` option ``dateformat`` used to pass format to `DateTime {{'x': '2017-09', 'y': '09-19'}} +``attribute`` or ``data_key`` collision triggers an exception +************************************************************* + +When a `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 ++++++++++++++++ diff --git a/marshmallow/schema.py b/marshmallow/schema.py index 730ed77c3..a7c1fda04 100644 --- a/marshmallow/schema.py +++ b/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): diff --git a/tests/base.py b/tests/base.py index ee212c42a..431da3f73 100644 --- a/tests/base.py +++ b/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() diff --git a/tests/test_schema.py b/tests/test_schema.py index cb5dbb715..79eca5e01 100755 --- a/tests/test_schema.py +++ b/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()