Fix replacement of non-metadata embedded StripeObjects

[brandur]

So we have a bit of a problem right now when it comes to replacing a
StripeObject that's embedded in an API resource.

Most of the time when someone does this, they want to replace an
object embedded in another object. Take setting a source on a
subscription for example:

subscription.source = {
  object: 'card',
  number: 123,
}
subscription.save

In the case above, the serialized parameters should come out as:

source[object]=card&source[number]=123

That should apply even if the previous source had something else set on
it which we're not going to set this time -- say an optional parameter
like source[address_state]. Those should not be present at all in the
final serialized parameters.

(Another example is setting a payout_schedule as seen in #631 which is
PR is intended to address.)

There is an exception to this rule in the form of metadata though.
Metadata is a bit of a strange case in that the API will treat it as
additive, so if we send metadata[foo], that will set the foo key,
but it won't overwrite any other keys that were already present.

This is a problem because when a user fully sets metadata to a new
object in Ruby, what they're probably trying to do is replace it
rather than add to it. For example:

subscription.metadata
=> { old: 'bar' }

subscription.metadata = {
  new: 'baz'
}
subscription.save

To accomplish what the user is probably trying to do, we actually need
to send metadata[old]=&metadata[new]=baz so that we empty the value of
old while simultaneously setting new to baz.

In summary, metadata behaves different from other embedded objects in a
fairly fundamental way, and because the code is currently only set up to
handle the metadata case, it's not behaving correctly when other types
of objects are being set. A lot of the time emptying values like we do
for metadata is benign, but as we've seen in #631, sometimes it's not.

In this patch, I modify serialization to only empty out object values
when we see that parameter is metadata.

I'm really not crazy about the implementation here at all, but I'm
having trouble thinking of a better way to do it. One possibility is to
introduce a new class annotation like empty_embedded_object :metadata,
but that will have to go everywhere and might be error-prone in case
someone forgets it on a new resource type. If anyone has a suggestion
for an alternative (or can let me know if I'm missing something), I'd
love to hear it.

This PR is an alternate to #631.

@ob-stripe Thoughts? Thanks!
@remi-stripe Do I have the API semantics stated above right? Thanks!
cc @stripe/api-libraries

[ob-stripe]

If I understand correctly, the issue you described is the same one that I fixed already for the specific case of items on subscription objects. The API returns items as a list object, so if a user set items to a new hash, the library would try to unset the values of the list object (by setting data, has_more, etc. to null values). (It's the second issue described in #596.)

With your fix, it should no longer be necessary to handle this specifically in Subscription.serialize_params (though we'll still need to handle the array to hash conversion).

Also, in stripe-php 6.0 I reused ~all of the encoding logic from stripe-ruby, so stripe-php might also be affected by this issue. I'll check and open an issue over there if needed.

One possibility is to
introduce a new class annotation like empty_embedded_object :metadata,
but that will have to go everywhere and might be error-prone in case
someone forgets it on a new resource type.

Nearly all updatable objects have metadata, so maybe we could bundle this in the APIOperations::Save mixin, and have an explicit opt-out annotation for updatable models that don't have metadata? (Are there even any?)

[stripe-ci]

Gerald Rule: Rubocop Core File Change

cc @pt-stripe

[brandur]

f I understand correctly, the issue you described is the same one that I fixed already for the specific case of items on subscription objects. The API returns items as a list object, so if a user set items to a new hash, the library would try to unset the values of the list object (by setting data, has_more, etc. to null values). (It's the second issue described in #596.)

Ah yes, I'd totally forgotten about that, but it's definitely the same problem.

Also, in stripe-php 6.0 I reused ~all of the encoding logic from stripe-ruby, so stripe-php might also be affected by this issue. I'll check and open an issue over there if needed.

Yep, and thanks!

Nearly all updatable objects have metadata, so maybe we could bundle this in the APIOperations::Save mixin, and have an explicit opt-out annotation for updatable models that don't have metadata? (Are there even any?)

Ah, good idea. I ended up changing this to add an explicit additive_object_param descriptor that we can use with metadata along with corresponding documentation. It strikes me as maybe a little too abstract for what we actually need, but I think overall it's okay.

And yeah, I think pretty much everything gets metadata — I didn't explicitly opt any API resources out because it's not a big deal if we set additive_object_param and there isn't actually a metadata field. It'll just be a no-op.

r? @ob-stripe Mind taking a look at the new diff and letting me know what you think? Thanks!

[ob-stripe]

Awesome! Thanks for the very detailed comment :)

[ob-stripe]

"through"

[brandur-stripe]

Doh! That's what I get for making a last second tweak. Thanks.

[ob-stripe]

@brandur You should also be able to replace https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/subscription.rb#L27-L35 with just:

def serialize_params(options = {})
  @values[:items] = Util.array_to_hash(@values[:items]) if @values[:items]
  super
end

[brandur-stripe]

@ob-stripe Ah of course! I tried that and the subscriptions test suite still seems to work. Committed in 39b80e5. Thanks.