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

Loading