Multiple attrs update #194

yedpodtrzitko · 2016-11-08T15:19:14Z

this allows to update multiple attributes at once using a new function Model.update (code based on the existing method Model.update_item()

This fixes #157 and #195

coveralls · 2016-11-08T15:21:22Z

Coverage increased (+0.05%) to 97.816% when pulling 751c9da on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T15:21:22Z

Coverage increased (+0.05%) to 97.816% when pulling 751c9da on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T15:21:22Z

Coverage increased (+0.05%) to 97.816% when pulling 751c9da on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T16:52:59Z

Coverage increased (+0.05%) to 97.816% when pulling 6da2455 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T16:52:59Z

Coverage increased (+0.05%) to 97.816% when pulling 6da2455 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T16:52:59Z

Coverage increased (+0.05%) to 97.816% when pulling 6da2455 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T16:52:59Z

Coverage increased (+0.05%) to 97.816% when pulling 6da2455 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

danielhochman · 2016-11-08T18:17:31Z

i think i'd prefer to move multiple update functionality to a method called update() and deprecate update_item(). update_item() never works how anyone would expect it to.

thoughts?

yedpodtrzitko · 2016-11-08T18:53:20Z

Ok, the code will be a wee bit simpler without the backward compatibility + I'll also try to implement a fix for #195 there.

coveralls · 2016-11-08T22:01:09Z

Coverage decreased (-0.07%) to 97.702% when pulling cc4b9ea on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-08T22:18:57Z

Coverage decreased (-0.07%) to 97.702% when pulling ae39b87 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

yedpodtrzitko · 2016-11-08T22:19:43Z

@danielhochman

update() implemented (at least in the Model)
saving null value for attributes with null=True is now handled properly

let me know if you have any suggestions about the code. Otherwise I'll just add docs & it's good to go.

danielhochman · 2016-11-09T23:47:05Z

pynamodb/models.py

@@ -323,6 +323,8 @@ def update_item(self, attribute, value=None, action=None, conditional_operator=N
        :param action: The action to take if this item already exists.
            See: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_UpdateItem.html#DDB-UpdateItem-request-AttributeUpdate
        """
+        DeprecationWarning("`Model.update_item` is deprecated in favour of `Model.update` now")


does this do anything?

coveralls · 2016-11-10T10:09:10Z

Coverage decreased (-0.07%) to 97.703% when pulling d1f272e on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

jmphilli · 2016-11-11T17:06:07Z

pynamodb/constants.py

@@ -162,7 +166,7 @@
 # These are the valid select values for the Scan operation
 # See: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Scan.html#DDB-Scan-request-Select
 NOT_NULL = 'NOT_NULL'
-NULL = 'NULL'
+#NULL = 'NULL' # declared as a type above


can we just delete this maybe?

We can. The reason I've done it this way was if someone would be thinking 'so here NOT_NULL, but where's NULL?'

Fixed in the last commit.

coveralls · 2016-11-11T19:10:44Z

Coverage decreased (-0.05%) to 97.716% when pulling 495f340 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-14T19:33:12Z

Coverage decreased (-0.05%) to 97.716% when pulling 70fe12e on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

jmphilli · 2016-11-16T22:42:21Z

pynamodb/models.py

+
+        :param value: a value to serialize
+        """
+        serialized = attr.serialize(value)


the passed value is already serialized on 1210 right?

we should either remove that or skip this

This will require a few more changes (which I wanted to avoid), but I can give it a try.

jmphilli · 2016-11-16T22:43:09Z

pynamodb/models.py

+        if serialized is None:
+            if not attr.null:
+                raise ValueError("Attribute '{0}': cannot be None".format(attr.attr_name))
+            return {NULL: True}


can we just set serialized = True and let the ATTR_TYPE_MAP on line 1286 handle the null bit? seems nicer maybe? don't really know tbh

No, this is a fix for #195

jmphilli · 2016-11-16T22:44:01Z

pynamodb/models.py

+            kwargs['expected'] = self._build_expected_values(expected_values, UPDATE_FILTER_OPERATOR_MAP)
+
+        for attr, params in attributes.items():
+            attribute_cls = self._get_attributes()[attr]


might move the self._get_attributes() call outside the loop?

jmphilli · 2016-11-16T22:44:27Z

pynamodb/models.py

+
+        for attr, params in attributes.items():
+            attribute_cls = self._get_attributes()[attr]
+            action = params['action'] and params['action'].upper()


won't this return a bool ?

no, either False (in case params['action'] is evaluated to False) or the value of the second statement)

>>> a = {'none': False, 'one': 'yes'} >>> a['none'] and a['none'].upper() False >>> a['one'] and a['one'].upper() 'YES'

jmphilli · 2016-11-16T22:44:59Z

pynamodb/models.py

+            attribute_cls = self._get_attributes()[attr]
+            action = params['action'] and params['action'].upper()
+            attr_values = {ACTION: action}
+            if action != DELETE:


can we add a test for DELETE

coveralls · 2016-11-17T01:28:22Z

Coverage decreased (-0.2%) to 97.561% when pulling 2d89666 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

coveralls · 2016-11-17T01:36:08Z

Coverage decreased (-0.03%) to 97.738% when pulling d0eb3c8 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

jmphilli · 2016-11-18T16:53:43Z

pynamodb/models.py

@@ -357,6 +361,52 @@ def update_item(self, attribute, value=None, action=None, conditional_operator=N
                setattr(self, name, attr.deserialize(value.get(ATTR_TYPE_MAP[attr.attr_type])))
        return data

+
+    def update(self, attributes,  conditional_operator=None, **expected_values):


would it be better to take kwargs that map from the model_field_name to the new value? i see we've already got **expected_values filling that role though... maybe we could make that a keyword argument

would be good to do now since this is new...

cc @danielhochman

@jmphilli you might need to pass action for the attributes too. That would mean a bunch of small dictionaries like attribute_name={'value': 1, 'action': 'add'}.

jmphilli · 2016-11-18T17:04:40Z

pynamodb/models.py

            else:
                if attr.is_hash_key:
-                    attrs[HASH] = serialized
+                    attrs[HASH] = list(serialized.values())[0]


I replaced serialized = attr.serialize(value) above with _serialize_value() in order to avoid a double call of attr.serialize() you did not like (I could not remove it inside of serialize_value() since it's used elsewhere too)

Since _serialize_value() returns value in the DynamoDB serialization format (ie. {TYPE: VALUE}), I need to extract the value here.

List conversion is done because Python 3 does not support indexing of the type returned by values()

coveralls · 2016-11-18T20:01:01Z

Coverage decreased (-0.03%) to 97.738% when pulling 2018285 on yedpodtrzitko:multiple-attrs-update into ce741ab on jlafon:devel.

jmphilli · 2016-11-21T23:46:59Z

daniel is out for a week. i'll look at this again today, but i can only give my seal of approval which doesn't really do much for you unfortunately lol.

yedpodtrzitko · 2016-11-22T07:15:39Z

np, I appreciate your feedback.

jmphilli · 2016-11-23T18:45:39Z

pynamodb/models.py

-            serialized = attr.serialize(value)
-            if serialized is None:
+
+            serialized = Model._serialize_value(attr, value, null_check)


self._serialize_value

jmphilli · 2016-11-23T18:45:58Z

pynamodb/models.py

+            action = params['action'] and params['action'].upper()
+            attr_values = {ACTION: action}
+            if action != DELETE:
+                attr_values[VALUE] = Model._serialize_value(attribute_cls, params['value'])


self._serialize_value

coveralls · 2016-11-23T19:40:54Z

Coverage decreased (-0.04%) to 97.76% when pulling c56c8da on yedpodtrzitko:multiple-attrs-update into 3d986bb on jlafon:devel.

add support for multiple attrs update, fix null saving

cc4b9ea

yedpodtrzitko force-pushed the multiple-attrs-update branch from 6da2455 to cc4b9ea Compare November 8, 2016 21:59

NULL expects bool value

ae39b87

danielhochman reviewed Nov 9, 2016

View reviewed changes

properly warn about deprecation

d1f272e

jmphilli reviewed Nov 11, 2016

View reviewed changes

add Model.update into docs

495f340

update docs info about update_item

70fe12e

jmphilli reviewed Nov 16, 2016

View reviewed changes

remove double serialization, minor CR improvements

2d89666

python3 fix

d0eb3c8

jmphilli reviewed Nov 18, 2016

View reviewed changes

nicer value unwrapping

2018285

Merge branch 'devel' into multiple-attrs-update

6b7cf22

jmphilli reviewed Nov 23, 2016

View reviewed changes

call serialization from instance directly

c56c8da

danielhochman merged commit cd705cc into pynamodb:devel Nov 30, 2016

yedpodtrzitko mentioned this pull request Jan 10, 2017

Delete should not validate non-key attributes #216

Closed

Multiple attrs update #194

Multiple attrs update #194

Conversation

yedpodtrzitko commented Nov 8, 2016 • edited

coveralls commented Nov 8, 2016

coveralls commented Nov 8, 2016 • edited

coveralls commented Nov 8, 2016

coveralls commented Nov 8, 2016 • edited

coveralls commented Nov 8, 2016

coveralls commented Nov 8, 2016

coveralls commented Nov 8, 2016

danielhochman commented Nov 8, 2016

yedpodtrzitko commented Nov 8, 2016 • edited

coveralls commented Nov 8, 2016 • edited

coveralls commented Nov 8, 2016 • edited

yedpodtrzitko commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2016 • edited

Choose a reason for hiding this comment

yedpodtrzitko Nov 11, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Nov 11, 2016 • edited

coveralls commented Nov 14, 2016 • edited

Choose a reason for hiding this comment

yedpodtrzitko Nov 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yedpodtrzitko Nov 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 17, 2016 • edited

coveralls commented Nov 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yedpodtrzitko Nov 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yedpodtrzitko Nov 18, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Nov 18, 2016 • edited

jmphilli commented Nov 21, 2016

yedpodtrzitko commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 23, 2016 • edited

yedpodtrzitko commented Nov 8, 2016 •

edited

coveralls commented Nov 8, 2016 •

edited

coveralls commented Nov 8, 2016 •

edited

yedpodtrzitko commented Nov 8, 2016 •

edited

coveralls commented Nov 8, 2016 •

edited

coveralls commented Nov 8, 2016 •

edited

coveralls commented Nov 10, 2016 •

edited

yedpodtrzitko Nov 11, 2016 •

edited

coveralls commented Nov 11, 2016 •

edited

coveralls commented Nov 14, 2016 •

edited

yedpodtrzitko Nov 16, 2016 •

edited

yedpodtrzitko Nov 16, 2016 •

edited

coveralls commented Nov 17, 2016 •

edited

coveralls commented Nov 17, 2016 •

edited

yedpodtrzitko Nov 18, 2016 •

edited

yedpodtrzitko Nov 18, 2016 •

edited

coveralls commented Nov 18, 2016 •

edited

coveralls commented Nov 23, 2016 •

edited