[Discuss] Extending Doorkeeper & working with Zeitwerk

[jasl]

There's a technique about extending engine's models

https://edgeguides.rubyonrails.org/engines.html#overriding-models-and-controllers

It works well in Rails 5, but break in Rails 6, I guess it relates to our lazy constructing models
to support different ORMs (this was introduced by me).

Extending Doorkeeper's models is useful, for example, I want to add some description fields such as icon, website to doorkeeper's application model.

I want to find a way to extending doorkeeper's models, I think I can ensure not break user side, but I'm not sure not to break Doorkeeper's extensions, is this acceptable? any suggestion?

[nbulaj]

Hi @jasl . Well, what is the problem with something like:

# config/initializers/doorkeeper.rb 

Doorkeeper.configure do
 # ...
end

Doorkeeper::Application.prepend(MyCustomizations)

# lib/mny_customizations.rb

module MyCustomizations
  extend ActiveSupport::Concern

  included do
    validates :icon_path, :help_url, presence: true # don't forget to replace default views
    
    def icon
      # ...
    end
  end
end

?

It plays nice with lazy loading and so on. Or you wanna to have an ability to configure it somehow using Doorkeeper configuration?

Also could you please provide more info on issue with Zeitwerk? Don't see any reasons to not to work with it 😕

[jasl]

@nbulaj

Unfortunately, included block won't work with prepend, you can simply try

[1] pry(main)> require "active_support/concern"
=> true
[2] pry(main)> 
[3] pry(main)> class Foo
[3] pry(main)*   def bar  
[3] pry(main)*     puts "hello"    
[3] pry(main)*   end    
[3] pry(main)* end  
=> :bar
[4] pry(main)> 
[5] pry(main)> module Baz
[5] pry(main)*   extend ActiveSupport::Concern  
[5] pry(main)*   
[5] pry(main)*   included do  
[5] pry(main)*     def bar    
[5] pry(main)*       puts "baz"      
[5] pry(main)*       super      
[5] pry(main)*     end      
[5] pry(main)*   end    
[5] pry(main)* end  
=> #<Proc:0x00005626470ceb58@(pry):10>
[6] pry(main)> 
[7] pry(main)> Foo.prepend Baz
=> Foo
[8] pry(main)> 
[9] pry(main)> Foo.new.bar
hello
=> nil

without included, we can't call DSL to model

To let included block working with prepend, you need a hack, see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/patch/prependable.rb

[nbulaj]

Well, it was just an idea. You can use any other methods to extend the model, like class_eval, pure modules and so on. Prepend it just the case when you need to override default behavior, which I personally recommend to not to do (or do in a very special cases)

[jasl]

Our ORM-specifics models don't follow path convention of Zeitwerk, and after I read doc & did research, I give up to hacking Zeitwerk, too complicated.

Instead, there is a simpler way, wrapping Doorkeeper::Application.class_eval into ActiveSupport.on_load(:active_record).

I tested roughly and it works, but didn't know if it has any side effect. I will keep watching on this.

But I'm thinking what about we allow configure orm :none to skipping load ORM-specifics that let pro-developer do their own models?

[nbulaj]

You are talking about this long-term existing in gem code, right?

doorkeeper/lib/doorkeeper/orm/active_record.rb

Lines 33 to 35 in 7242515

	def self.lazy_load(&block)
	ActiveSupport.on_load(:active_record, {}, &block)
	end

We already uses hooks for AR, Sequel and Mongoid/MongoMapper, read #913

[nbulaj]

But I'm thinking what about we allow configure orm :none to skipping load ORM-specifics that let pro-developer do their own models?

I think we can achieve this by creating doorkeeper-null-orm extension 😃 I don't think adding this to gem core would be a good idea.

[jasl]

You are talking about this long-term existing in gem code, right?

doorkeeper/lib/doorkeeper/orm/active_record.rb

Lines 33 to 35 in 7242515

def self.lazy_load(&block)
ActiveSupport.on_load(:active_record, {}, &block)
end
We already uses hooks for AR, Sequel and Mongoid/MongoMapper, read #913

No, I mean before Rails 6.0.0.beta2 we don't need to wrap the override in ActiveSupport.on_load(:active_record)

[jasl]

But I'm thinking what about we allow configure orm :none to skipping load ORM-specifics that let pro-developer do their own models?

I think we can achieve this by creating doorkeeper-null-orm extension I don't think adding this to gem core would be a good idea.

Sound good to me, I will do this later, could you accept this to Doorkeeper-gem organization and publish it? I think I don't have enough time to maintain gems, but if something breaks, I will attach PR.

[jasl]

Also could you please provide more info on issue with Zeitwerk? Don't see any reasons to not to work with it

Whatever I use require or Zeitwerk's preload to load Doorkeeper models in my Rails app, Ruby always tells you Uninitialized constant until Doorkeeper initialized.