Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StripeObject: only add changed values to _unsaved_values #372

Merged
merged 3 commits into from
Nov 29, 2017
Merged

StripeObject: only add changed values to _unsaved_values #372

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1751c91
StripeObject: only add changed values to _unsaved_values
blueyed Nov 13, 2017
39a40c3
StripeObject.serialize: omit empty childs
blueyed Nov 13, 2017
3c50f68
compare to dict with test
blueyed Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions stripe/stripe_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,14 @@ def __setitem__(self, k, v):
"You may set %s.%s = None to delete the property" % (
k, str(self), k))

super(StripeObject, self).__setitem__(k, v)
if not hasattr(self, k) or v != getattr(self, k):
# Allows for unpickling in Python 3.x
if not hasattr(self, '_unsaved_values'):
self._unsaved_values = set()

# Allows for unpickling in Python 3.x
if not hasattr(self, '_unsaved_values'):
self._unsaved_values = set()
self._unsaved_values.add(k)

self._unsaved_values.add(k)
super(StripeObject, self).__setitem__(k, v)

def __getitem__(self, k):
try:
Expand Down Expand Up @@ -237,7 +238,9 @@ def serialize(self, previous):
elif isinstance(v, stripe.api_resources.abstract.APIResource):
continue
elif hasattr(v, 'serialize'):
params[k] = v.serialize(previous.get(k, None))
child = v.serialize(previous.get(k, None))
if child:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's possible in practice, but in theory serialize could return an empty string "" to unset the entire attribute, and because empty strings are falsey in Python this would get ignored. It might be better to explicitly compare the returned value to an empty dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.
Should have a test for it then, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's possible in practice

The two internal serialize methods will return a dict always, so it is unlikely to come from there.

Added a test with a specialized StripeObject, which is likely what you had in mind?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Thanks again!

params[k] = child
elif k in unsaved_keys:
params[k] = _compute_diff(v, previous.get(k, None))
elif k == 'additional_owners' and v is not None:
Expand Down
42 changes: 26 additions & 16 deletions tests/api_resources/abstract/test_updateable_api_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ def test_idempotent_save(self):
'post',
'/v1/myupdateables/myid',
{
'metadata': {},
'baz': 'updated',
},
{
Expand Down Expand Up @@ -75,6 +74,30 @@ def test_save(self):
None
)

# Saving again should not cause any request.
self.requestor_mock.request.reset_mock()
self.checkSave()
self.assertFalse(self.requestor_mock.request.called)

# Setting the same value should not cause any request.
self.obj.thats = 'it'
self.checkSave()
self.assertFalse(self.requestor_mock.request.called)

# Changing the value should cause a request.
self.obj.id = 'myid'
self.obj.thats = 'updated'
self.checkSave()

self.requestor_mock.request.assert_called_with(
'post',
'/v1/myupdateables/myid',
{
'thats': 'updated',
},
None
)

def test_add_key_to_nested_object(self):
acct = MyUpdateable.construct_from({
'id': 'myid',
Expand Down Expand Up @@ -110,14 +133,7 @@ def test_save_nothing(self):

self.assertTrue(acct is acct.save())

# Note: ideally, we'd want the library to NOT issue requests in this
# case (i.e. the assert should actually be `assert_not_called()`).
self.requestor_mock.request.assert_called_with(
'post',
'/v1/myupdateables/myid',
{'metadata': {}},
None
)
self.requestor_mock.request.assert_not_called()

def test_replace_nested_object(self):
acct = MyUpdateable.construct_from({
Expand Down Expand Up @@ -185,7 +201,6 @@ def test_array_none(self):
'/v1/myupdateables/myid',
{
'foo': 'bar',
'legal_entity': {},
},
None
)
Expand Down Expand Up @@ -274,12 +289,7 @@ def test_hash_noop(self):

self.assertTrue(acct is acct.save())

self.requestor_mock.request.assert_called_with(
'post',
'/v1/myupdateables/myid',
{'legal_entity': {'address': {}}},
None
)
self.requestor_mock.request.assert_not_called()

def test_save_replace_metadata_with_number(self):
self.obj.baz = 'updated'
Expand Down