Skip to content

Update DynamicModelHelpers to require a connection

Patrick Bair requested to merge pb-dynamic-model-helpers-base-class into master

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.

  1. Create a test object with the module:
    model_builder = Class.new do
      include Gitlab::Database::DynamicModelHelpers
    end.new
  2. Query with a dynamic model pointed at the main database:
    model_builder.define_batchable_model('projects', connection: Project.connection).count # => 8
  3. 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.

Edited by Patrick Bair

Merge request reports