urbanairship / ruby-library

A Ruby wrapper for the Urban Airship API.
Other
200 stars 118 forks source link

Allow :provider => :android to also be passed as option "provider" => "android" String pair. #32

Closed thrillcall closed 11 years ago

thrillcall commented 11 years ago

This allows the UrbanAirship gem to be used inside a Resque/Sidekiq job which will silently convert the options Hash with Symbol values to String values when the job args are serialized through Redis.

Without this, when we passed arguments to a Sidekiq job to indicate that the job should be processed in the context of an Android request it would fail due to the UrbanAirship gem was very strict in only accepting symbols. This was a tricky to diagnose issue which would cause registration/unregister calls to fail since the iOS API was being incorrectly tried for Android tokens.

schoblaska commented 11 years ago

Won't this be a problem for most of the library's methods that expect symbolized hash arguments?

I don't think this should be the responsibility of the library. Instead, there should be code in the resque/sidekiq job that makes sure that any arguments passed to third-party libraries are properly constructed.

thrillcall commented 11 years ago

@joeyschoblaska

I don't think any of the other options that are passed upstream to Urban Airship are sent as Symbols, they are sent as Strings in JSON.

It is only this parameter, which is only read by the client library itself and never passed upstream for use, that is so strict. I am just relaxing the args that the library will accept to loosen the restriction. It does not otherwise effect any functionality so I only see upside here.

Cheers,

Glenn

schoblaska commented 11 years ago

The :alias option sent to Urbanairship::register and the :aliases and :schedule_for options for the various push methods all have some parsing done on them that won't get executed if the arguments are not passed as symbols.

Regardless, I think the responsibility should lie with the calling code to make sure that the arguments are passed correctly, not with the library to try all possibilities it might be given. I recommend using a HashWithIndifferentAccess in your delayed job code, as well as fixing any values that may have been modified from symbols to strings.

schoblaska commented 11 years ago

Actually, if you could modify this PR to just look for :android, or "android" as values, but not to look at string keys, I'd be cool with that. Then you'd just have to use a HashWithIndifferentAccess and it would work.

thrillcall commented 11 years ago

This commit will now allow the values to be a string for the :provider option hash key or the .provider instance variable.

https://github.com/thrillcall/urbanairship/commit/17b0ee2466ad8a387d91a936039b9ed234f71a51

schoblaska commented 11 years ago

Cool, thanks for the edit! I'll merge this in and do a version bump.

thrillcall commented 11 years ago

Thanks Joey, I saw you just commited the version bump. If you could comment here when that new version is on Rubygems we'll start using it. Thx

schoblaska commented 11 years ago

I just pushed version 2.3.2, which contains your commit.

thrillcall commented 11 years ago

Great. Thx.

thrillcall commented 11 years ago

FWIW, when we call this from within an Async job, we ended up taking the options Hash that we pass as an arg to the async job and calling this method on it to make sure all of the keys in the hash are symbols.

options.symbolize_keys!

If we instantiated a HashWithIndifferentAccess with the original options Hash the default was always to present all of the keys as Strings so it didn't help too much. I still think it would be better if the options Hash could contain any combination of Strings and Symbols and be gracefully handled by the Gem (e.g. maybe if you internally called .symbolize_keys! on the options Hash? But that would require bringing in some Rails components. You could also call hash.map and do a .to_sym on each key I suppose). But we can live with it the way it is for now.

We put the calls to UA behind an Async job since we found we had lots of mini-brownouts in our app where UA was temp down or slow and having a sync API call embedded in our app would cause a bit of a storm with lots of device registration requests/updates coming in and clogging up our app servers while the app workers waited for the sync requests to finish. This approach has worked really well for us so far.

Cheers.

Glenn