Update DynamicModelHelpers to require a connection
What does this MR do and why?
Related issue: #345624 (closed)
Update DynamicModelHelpers
methods to set the model's database connection to the given connection rather than using the default connection from ActiveRecord::Base
. This was the suggested solution in the discussion at !77717 (comment 812876439)
Since these methods are commonly used in migrations, I added wrappers which use the migration connection. I wanted to keep the connection:
argument in DynamicModelHelpers
required, so it's explicit which connection non-migration code is using.
It was also mentioned there that the DynamicModelHelpers
might cause a memory leak when it creates anonymous ActiveRecord models. It seems like that issue was fixed in https://github.com/rails/rails/pull/31442. Locally it seems to work alright:
[1] pry(main)> ActiveRecord::Base.descendants.size
=> 140
[2] pry(main)> 100.times { Class.new(ActiveRecord::Base) }
=> 100
[3] pry(main)> ActiveRecord::Base.descendants.size
=> 240
[4] pry(main)> GC.start
=> nil
[5] pry(main)> ActiveRecord::Base.descendants.size
=> 140
How to set up and validate locally
You can validate this locally if you have your environment setup with multiple databases.
- Create a test object with the module:
model_builder = Class.new do include Gitlab::Database::DynamicModelHelpers end.new
- Query with a dynamic model pointed at the
main
database:model_builder.define_batchable_model('projects', connection: Project.connection).count # => 8
- Query with a dynamic model pointed at the
ci
database:model_builder.define_batchable_model('projects', connection: Ci::ApplicationRecord.connection).count # => 0
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.