Skip to content

Commit

Permalink
Fix replacement of non-metadata embedded StripeObjects
Browse files Browse the repository at this point in the history
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:

``` ruby
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 stripe#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:

``` ruby
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 stripe#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 stripe#631.
  • Loading branch information
brandur committed Apr 3, 2018
1 parent 3bc4256 commit 256556e
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 10 deletions.
4 changes: 2 additions & 2 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Metrics/ClassLength:

# Offense count: 11
Metrics/CyclomaticComplexity:
Max: 13
Max: 15

# Offense count: 259
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
Expand All @@ -47,7 +47,7 @@ Metrics/ParameterLists:

# Offense count: 5
Metrics/PerceivedComplexity:
Max: 15
Max: 17

# Offense count: 2
Style/ClassVars:
Expand Down
12 changes: 8 additions & 4 deletions lib/stripe/stripe_object.rb
Original file line number Diff line number Diff line change
Expand Up @@ -410,10 +410,14 @@ def serialize_params_value(value, original, unsaved, force, key: nil)
elsif value.is_a?(StripeObject)
update = value.serialize_params(force: force)

# If the entire object was replaced, then we need blank each field of
# the old object that held a value. The new serialized values will
# override any of these empty values.
update = empty_values(original).merge(update) if original && unsaved
# If the entire object was replaced and this is a `metadata` hash, then
# we need blank each field of the old object that held a value because
# otherwise the update to `metadata` will be additive instead of a full
# replacement. The new serialized values will override any of these
# empty values.
if original && unsaved && key && key == :metadata
update = empty_values(original).merge(update)
end

update

Expand Down
19 changes: 15 additions & 4 deletions test/stripe/stripe_object_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,23 @@ def to_hash
assert_equal({ foo: "bar" }, serialized[:metadata])
end

should "#serialize_params with a StripeObject that's been replaced" do
should "#serialize_params with StripeObject that's been replaced" do
obj = Stripe::StripeObject.construct_from(source: Stripe::StripeObject.construct_from(bar: "foo"))

# Here we replace the object wholesale.
obj.source =
Stripe::StripeObject.construct_from(baz: "foo")

serialized = obj.serialize_params
assert_equal({ baz: "foo" }, serialized[:source])
end

should "#serialize_params with StripeObject that's been replaced which is `metadata`" do
obj = Stripe::StripeObject.construct_from(metadata: Stripe::StripeObject.construct_from(bar: "foo"))

# Here we replace the object wholesale which means that the client must
# be able to blank out the values that were in the old object, but which
# are no longer present in the new one.
# Here we replace the object wholesale. Because it's `metadata`, the
# client must be able to blank out the values that were in the old
# object, but which are no longer present in the new one.
obj.metadata =
Stripe::StripeObject.construct_from(baz: "foo")

Expand Down

0 comments on commit 256556e

Please sign in to comment.