songkick / oauth2-provider

Simple OAuth 2.0 provider toolkit
MIT License
528 stars 148 forks source link

Updating to remove deprecation warnings when using ActiveRecord 4. #53

Closed harigopal closed 11 years ago

harigopal commented 11 years ago

Hi, because of deprecation warnings being raised by ActiveRecord (AR) 4, I've made a few changes which will probably neccessitate a major version bump. I'm sorry, but I couldn't think of any way to support the 2.x versions of ActiveRecord. :-(

Deprecations warnings removed:

count(conditions)

client.authorizations.count(:conditions => {:code => code}).zero?

...many instances of above count method were causing:

DEPRECATION WARNING: Relation#calculate with finder options is deprecated. Please build a scope and then call calculate on it instead.
DEPRECATION WARNING: The :distinct option for `Relation#count` is deprecated. Please use `Relation#distinct` instead. (eg. `relation.distinct.count`).

I replaced all instance of this count usage with:

client.authorizations.where(:code => code).count.zero?


find_or_create_by_column(value)

find_or_create_by_name(name)

caused:

DEPRECATION WARNING: This dynamic method is deprecated. Please use e.g. Post.find_or_create_by(name: 'foo') instead.

However, the suggested method works only from AR4 onwards, so I replaced it with:

where(name: name).first || create(name: name)

which works for AR3.0 onwards (where(conditions).find_or_create was only added in AR3.2)

Related changes:

I added AR4 to the Appraisals file, along with the required version of the mysql gem, and protected_attributes gem to allow mass-assignment protection in gem's model code.

RSpec results

AR2.x - Failure. AR3.x - Passes for SQLite and MySQL, and Postgres. AR4 - Passes for MySQL and Postgres. Tests don't start running for SQLite (details below).

I couldn't get AR4 specs to work with SQLite. It complains that an index name is too long:

Index name 'index_oauth2_authorizations_on_client_id_and_refresh_token_hash' on table 'oauth2_authorizations' is too long; the limit is 62 characters

Running the specs with AR4 (other DB-s) does not generate any more deprecation warnings, but the specs still have very noisy output, with AR complaining about attempts to mass-assign protected attributes (spec appears to test this). I haven't made any alterations there since its possible the specs may need to be updated (now that mass-assignment security is being moved out of the model).

I'd really appreciate it if you could take a look at the changes, and incorporate them if it makes sense. Thanks!

Note: I ran all tests with Ruby 2.0.0-p247.

jcoglan commented 11 years ago

Thanks for the patches!

Please fix the build on Travis, and let me know when everything works.

harigopal commented 11 years ago

@jcoglan I'd like you to take a look at the Travis build results. You'll notice that the failures are for ActiveRecord 2.x, and all of them are caused by the use of the where method.

I searched for a method that works with AR2.x, and isn't deprecated by AR4, but couldn't find anything.

The only fix I could think of was to check AR version manually, everywhere the where method is used (five instances in the codebase), and instead use the old count(conditions) form - which, I think, is a pretty ugly thing to do.

if defined?(ActiveRecord::VERSION && ActiveRecord::VERSION::MAJOR >= 3)
  where(:access_token_hash => hash).count.zero?
else
  count(:conditions => {:access_token_hash => hash}).zero?
end

This could also be done by capturing the NoMethodError, though that's probably more dangerous.

Thoughts?

jcoglan commented 11 years ago

We probably need some wrapper functions that we use like:

Helpers.count(Authorization, :access_token_hash => hash)

Those methods would use respond_to? rather than version numbers to detect capabilities and dispatch to whatever API is available.

harigopal commented 11 years ago

@jcoglan Is that OK?

jcoglan commented 11 years ago

The build looks good. Can you add an entry to .travis.yml to make it run the build with ActiveRecord 4.0 on Ruby 1.9.3 with MySQL and check that passes?

harigopal commented 11 years ago

There was a duplicate entry in .travis.yml for 1.9.3 MySQL AR3.2 - I replaced that with the AR4 build.

harigopal commented 11 years ago

@jcoglan Hey, is there something wrong with the patches / anything left to do?

jcoglan commented 11 years ago

I've merged it now. Thanks a lot :)