Breaking change in 4.3.0

[stevenharman]

A change with #1029 extracted a new Doorkeeper::BaseRecord class and was released as part of 4.3.0. This is a breaking change for anyone that's had to re-open the Doorkeeper::Application, Doorkeeper::AccessGrant, or Doorkeeper::AccessToken classes.

One might do this to setup, for example, inverse relations between the resource owner and the Grant or Token. Or to add relationships between a token and a Device (for APIs with native client devices, for example). These objects are part of the public API, and thus such usage, while not documented, seems reasonable. And it's certainly something that's done in the wild - I nearly always end up doing something along those lines when using Doorkeeper.

I would recommend rolling back that extraction and duplicating the following method out to each of the Application, AccessGrant, and AccessToken classes:

def self.ordered_by(attribute, direction = :asc)
  order(attribute => direction)
end

We could then re-introduce the BaseResource as part of Doorkeeper 5.0 as a breaking change.

If y'all are happy with that approach, I'll get a PR together today.

[nbulaj]

Hi @stevenharman . Why does this changes is breaking for you? What code now is not working properly? class_eval? Need more info

[stevenharman]

Hello @nbulaj,

As I mentioned, the breaking change is for anyone who's re-opened the Doorkeeper::Application, Doorkeeper::AccessGrant, or Doorkeeper::AccessToken classes. An example of code that worked in 4.2.6, but breaks with a TypeError in 4.3.0`

require 'doorkeeper/orm/activerecord'

module Doorkeeper
  class AccessToken < ActiveRecord::Base
    belongs_to :account, foreign_key: :resource_owner_id, inverse_of: :access_tokens
  end
end

Because the base class of Doorkeeper::AccessToken has changed in 4.3.0, this re-opened class has a different base class, and thus is a TypeError. Meaning everyone has to change their re-opened class definitions to match:

require 'doorkeeper/orm/active_record'

module Doorkeeper
  class AccessToken < BaseRecord
    belongs_to :account, foreign_key: :resource_owner_id, inverse_of: :access_tokens
  end
end

This change is totally fine, and a good idea in the long term. But should be reserved for a breaking change release, like 5.0. Does that help?

[nbulaj]

Yep, now it make sense. Can't say thay I encourage this type of patching (class_eval will work without any problem), but you are right. I will refactor this and release a new patch version.

[grosser]

FYI (and a bit off-topic, but wanted to post it somewhere) I'm disabling all model loading since I want no AR things defined before the app starts and had to add an autoload for BaseReord to fix my different error which had the same error message
zendesk/samson#2612 (review)

[nbulaj]

@grosser can you please explain what do you mean? In 4.3 Doorkeeper models use ActiveSupport lazy loading for ActiveRecord models, so any initializer can configure ActiveRecord before models loading. But it has some troubles (like #1043), so currently I thinking about to revert it..

[grosser]

I mean it should not load these models at all during initialization like a real app would do.

…

On Thu, Mar 1, 2018 at 12:09 AM, Nikita Bulai ***@***.***> wrote: @grosser <https://github.com/grosser> can you please explain what do you mean? In 4.3 Doorkeeper models use ActiveSupport lazy loading for ActiveRecord models, so any initializer can configure ActiveRecord before models loading. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1044 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZ6h1nLcln6NnRtDRqjYrGkiJezbWks5tZ6yxgaJpZM4SW915> .

[nbulaj]

Hi @stevenharman. Could you please check a master branch if it helps to solve your problem? Also if you have some specs with Doorkeeper objects, could you run you specs and report if there any problems?

[nbulaj]

Fix released as a part of 4.3.1

[stevenharman]

@nbulaj Thanks! Sorry I didn't get back to you earlier - I was out of town for an event. The change looks good. I'll verify it 🔜, but I expect it will be 👍.