StripeObject: only add changed values to _unsaved_values

[blueyed]

No description provided.

[blueyed]

It would still trigger an update for metadata, which is mentioned in

stripe-python/tests/api_resources/abstract/test_updateable_api_resource.py

Lines 137 to 144 in 1751c91

	# 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
	)

.

I've also found https://github.com/blueyed/stripe-python/blob/1751c911b497c37cb60bdf74eeb4073b04ef423c/tests/api_resources/abstract/test_updateable_api_resource.py#L301-L306, which seems even to be incorrect?! Doesn't it empty legal_entity.address?

[ob-stripe]

Hey @blueyed, thanks a lot for the contribution! This looks very promising.

Doesn't it empty legal_entity.address?

So, no, although I can see why you'd think so! We mock APIRequestor, which can receive empty dicts. Those will be ignored in the outgoing request though:

>>> list(stripe.api_requestor._api_encode({'metadata': {}}))
[]

I agree it would be better and clearer if APIRequestor would not receive empty dicts in the first place! I'll take a closer look at your patch today.

[ob-stripe]

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.

[blueyed]

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

[blueyed]

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?!

[ob-stripe]

Yes, exactly. Thanks again!

[ob-stripe]

I left a comment, but generally this looks pretty great to me!

r? @deontologician-stripe Do you mind taking a look at this?

[deontologician-stripe]

I am +1 on this. I wish we could do the checking for _unsaved_values in __setstate__ instead of just in time when it's needed, but that is unrelated to this PR.

[deontologician-stripe]

r? @ob-stripe To approve after the test you mentioned in the comment is added

[ob-stripe]

lgtm

[ob-stripe]

@dpetrovics-stripe (dev-platform run, feel free to re-assign) Mind merging this and releasing a version? Thanks!