Skip to content

Commit

Permalink
Properly give defaults for DatabaseSelector options
Browse files Browse the repository at this point in the history 
The initializer receives `nil` for these options when no cofigurations were given:
https://github.com/rails/rails/blob/v6.0.0.rc1/activerecord/lib/active_record/railtie.rb#L91-L97
  • Loading branch information
@amatsuda
amatsuda committed May 6, 2019
1 parent 01f8abd commit cecbc23
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/middleware/database_selector.rb
View file
Expand Up @@ -35,10 +35,10 @@ module Middleware
# config.active_record.database_resolver = MyResolver
# config.active_record.database_resolver_context = MyResolver::MySession
class DatabaseSelector
def initialize(app, resolver_klass = Resolver, context_klass = Resolver::Session, options = {})
def initialize(app, resolver_klass = nil, context_klass = nil, options = {})
@app = app
@resolver_klass = resolver_klass
@context_klass = context_klass
@resolver_klass = resolver_klass || Resolver
@context_klass = context_klass || Resolver::Session
@options = options
end

Expand Down

5 comments on commit cecbc23

@amatsuda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileencodes I pushed this commit because I firstly found this to be a tiny careless bug, but I'm now confused seeing this documentation.

rails/activerecord/lib/active_record/middleware/database_selector.rb

Lines 22 to 27 in cecbc23

# To use the DatabaseSelector in your application with default settings add
# the following options to your environment config:
#
# config.active_record.database_selector = { delay: 2.seconds }
# config.active_record.database_resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver
# config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session

Was this totally intentional that we have to explicitly configure all three options here? We were getting a nil error when the klasses were not specified, but was that a designed behavior?
If so, I'll revert this commit (and maybe I'll add a warning message or something that tells the users to explicitly configure these klasses).

@eileencodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. Thanks for letting me know.

@amatsuda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😌

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amatsuda did you apply this commit to 6-0-stable?

@amatsuda
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca Yes. Shouldn't I?

Please sign in to comment.