Repository: pynamodb/PynamoDB

Issue:
update_item delete item from set fails
Here is a simple test case that attempts to remove an value from a UnicodeSet attribute:

``` python
from pynamodb.models import Model
from pynamodb.attributes import NumberAttribute, UnicodeSetAttribute


class UserModel(Model):

    class Meta:
        table_name = 'User'
        region = 'us-west-2'
        read_capacity_units = 1
        write_capacity_units = 1
        host = 'http://localhost:8000'

    id = NumberAttribute(hash_key=True)
    nicknames = UnicodeSetAttribute(null=True)


UserModel.create_table(wait=True)
try:
    original_nicknames = {'name1', 'name2', 'name3'}
    nicknames_to_remove = {'name2'}
    expected_result_nicknames = original_nicknames - nicknames_to_remove
    user = UserModel(id=1, nicknames=original_nicknames)
    user.save()
    user = UserModel.get(1)
    print('original nicknames:', user.nicknames)
    assert original_nicknames == user.nicknames
    print('nicknames to remove:', nicknames_to_remove)
    user.update_item('nicknames', value=nicknames_to_remove, action='delete')
    user = UserModel.get(1)
    print('expected result nicknames:', expected_result_nicknames)
    print('  actual result nicknames:', user.nicknames)
    print()
    assert expected_result_nicknames == user.nicknames
finally:
    UserModel.delete_table()
```

When running this test case, the output is:

```
original nicknames: {'name2', 'name3', 'name1'}
nicknames to remove: {'name2'}
expected result nicknames: {'name3', 'name1'}
  actual result nicknames: None

Traceback (most recent call last):
  File "mytest.py", line 34, in <module>
    assert expected_result_nicknames == user.nicknames
AssertionError
```

I expected 'name2' to be removed from the `{'name1', 'name2', 'name3'}` set, but instead all the values were removed.

This bug exists since 1.4.4 (1.4.3 works fine), and is still present in 1.6.0. The associated pull request has been updated to be be based on the post 1.6.0 devel branch, and it includes a unit test that fails with only the 'pynamodb/tests' commits applied but succeeds with the changes to connection/base.py and models.py (for update_item of connection and model, respectively).


Discussion:

Patch:
diff --git a/pynamodb/models.py b/pynamodb/models.py
index 2425106..1e539de 100644
--- a/pynamodb/models.py
+++ b/pynamodb/models.py
@@ -344,7 +344,6 @@ class Model(AttributeContainer):
         attribute_cls = None
         for attr_name, attr_cls in self._get_attributes().items():
             if attr_name == attribute:
-                value = attr_cls.serialize(value)
                 attribute_cls = attr_cls
                 break
         if not attribute_cls:
@@ -360,8 +359,10 @@ class Model(AttributeContainer):
                 ACTION: action.upper() if action else None,
             }
         }
-        if action is not None and action.upper() != DELETE:
-            kwargs[pythonic(ATTR_UPDATES)][attribute_cls.attr_name][VALUE] = {ATTR_TYPE_MAP[attribute_cls.attr_type]: value}
+        if value is not None:
+            kwargs[pythonic(ATTR_UPDATES)][attribute_cls.attr_name][VALUE] = {
+                ATTR_TYPE_MAP[attribute_cls.attr_type]: attribute_cls.serialize(value)
+            }
         kwargs[pythonic(RETURN_VALUES)] = ALL_NEW
         kwargs.update(conditional_operator=conditional_operator)
         kwargs.update(condition=condition)
@@ -415,7 +416,7 @@ class Model(AttributeContainer):
             attribute_cls = attrs[attr]
             action = params['action'] and params['action'].upper()
             attr_values = {ACTION: action}
-            if action != DELETE:
+            if 'value' in params:
                 attr_values[VALUE] = self._serialize_value(attribute_cls, params['value'])
 
             kwargs[pythonic(ATTR_UPDATES)][attribute_cls.attr_name] = attr_values


Test patch:
diff --git a/pynamodb/tests/test_model.py b/pynamodb/tests/test_model.py
index 571c1e3..2688fb4 100644
--- a/pynamodb/tests/test_model.py
+++ b/pynamodb/tests/test_model.py
@@ -1080,6 +1080,44 @@ class ModelTestCase(TestCase):
             assert item.views is None
             self.assertEquals(set(['bob']), item.custom_aliases)
 
+        # Reproduces https://github.com/pynamodb/PynamoDB/issues/132
+        with patch(PATCH_METHOD) as req:
+            req.return_value = {
+                ATTRIBUTES: {
+                    "aliases": {
+                        "SS": set(["alias1", "alias3"])
+                    }
+                }
+            }
+            item.update({
+                'custom_aliases': {'value': set(['alias2']), 'action': 'delete'},
+            })
+
+            args = req.call_args[0][1]
+            params = {
+                'TableName': 'SimpleModel',
+                'ReturnValues': 'ALL_NEW',
+                'Key': {
+                    'user_name': {
+                        'S': 'foo'
+                    }
+                },
+                'UpdateExpression': 'DELETE #0 :0',
+                'ExpressionAttributeNames': {
+                    '#0': 'aliases'
+                },
+                'ExpressionAttributeValues': {
+                    ':0': {
+                        'SS': set(['alias2'])
+                    }
+                },
+                'ReturnConsumedCapacity': 'TOTAL'
+            }
+            deep_eq(args, params, _assert=True)
+
+            assert item.views is None
+            self.assertEquals(set(['alias1', 'alias3']), item.custom_aliases)
+
     def test_update_item(self):
         """
         Model.update_item
@@ -1577,6 +1615,38 @@ class ModelTestCase(TestCase):
             }
             deep_eq(args, params, _assert=True)
 
+        # Reproduces https://github.com/pynamodb/PynamoDB/issues/132
+        with patch(PATCH_METHOD) as req:
+            req.return_value = {
+                ATTRIBUTES: {
+                    "aliases": {
+                        "SS": set(["alias1", "alias3"])
+                    }
+                }
+            }
+            item.update_item('custom_aliases', set(['alias2']), action='delete')
+            args = req.call_args[0][1]
+            params = {
+                'TableName': 'SimpleModel',
+                'ReturnValues': 'ALL_NEW',
+                'Key': {
+                    'user_name': {
+                        'S': 'foo'
+                    }
+                },
+                'UpdateExpression': 'DELETE #0 :0',
+                'ExpressionAttributeNames': {
+                    '#0': 'aliases'
+                },
+                'ExpressionAttributeValues': {
+                    ':0': {
+                        'SS': set(['alias2'])
+                    }
+                },
+                'ReturnConsumedCapacity': 'TOTAL'
+            }
+            deep_eq(args, params, _assert=True)
+            self.assertEqual(set(["alias1", "alias3"]), item.custom_aliases)
 
     def test_save(self):
         """