balanced / balanced-ruby

Balanced API library in ruby.
MIT License
111 stars 47 forks source link

Associating a card with an account fails on version 0.5.3 #69

Closed ajsharp closed 10 years ago

ajsharp commented 11 years ago

If you attempt to assign an account_uri to a Balanced::Card object, this fails with a 400 error:

Balanced::BadRequest: Balanced::BadRequest(400)::Bad Request:: PUT https://api.balancedpayments.com/v1/marketplaces/XXX/cards/YYY: request: Only one of [account_uri|account] can be specified Your request id is OHMZZZ.

The problem here is that I didn't set the account object. This seems like a pretty straightforward client error.

ajsharp commented 11 years ago

So, the problem here is that the account key is set in the attributes hash by default, with a nil value. I removed the key from the hash, but unfortunately, a related side effect here is that the uri property gets cleared from the object when the first exception is raised, so you can't attempt to save the object again.

ajsharp commented 11 years ago

I was thinking that a general approach to this problem could be viewed as a "filtering" problem: before any payload is sent over to the API, clear out any keys that point at nil values. But, this could have unintended side effects for attributes where a false-ey value is accepted in the api.

balanced-rss-bot commented 11 years ago

The better approach may be to observe which keys are set and only send those.

On Apr 2, 2013, at 14:56, Alex Sharp notifications@github.com wrote:

I was thinking that a general approach to this problem could be viewed as a "filtering" problem: before any payload is sent over to the API, clear out any keys that point at nil values. But, this could have unintended side effects for attributes where a false-ey value is accepted in the api.

— Reply to this email directly or view it on GitHub.

ajsharp commented 11 years ago

The better approach may be to observe which keys are set and only send those.

Would you do this by maintaining an explicit hash of attributes somewhere that are to be sent over to the server, or would you do this by filtering out nil values? The latter approach would require some care to explicitly remove only nil values, and not false values.

Also, does Balanced support the PATCH http verb? Or would we keep using PUT for these types of requests?

ajsharp commented 11 years ago

A more surgical approach may work too, approaching it on the other end. In this case, the 'account' key gets set on the attributes hash when it's read from the server (Balanced::BankAccount.find(...), and the server is returning an "account" key with a nil value. The construct_from_response method could be modified to not set keys with a nil value on the attributes hash, and not generate methods for such values: https://github.com/balanced/balanced-ruby/blob/abe4284d9184b11a56f12f7a66d3f377a5e59d98/lib/balanced/resources/resource.rb#L141.

mahmoudimus commented 11 years ago

Travis failed!

ajsharp commented 11 years ago

Unrelated: there's some weird global state stuff going on. I'm getting different failures depending on if I run the whole suite, or just run the specs for that file. We should consider using rspec's random spec ordering feature to find these problems.