Set importing on objects through global context

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

About

There are often validations and callbacks on models that should be switched off during an import. See example validation and example callback.

We do this:

• To optimise away performance problems in callbacks (example: https://gitlab.com/gitlab-org/gitlab/-/issues/336788).
• Because order of data created might make an object invalid.

Problem

To disable callbacks on an object we create during an import, we must always remember to set importing: true on the object before saving, if that object's class includes the Importable module.

We often forget to set importing: true on all objects before we save them.

So the current method is error-prone and brittle.

Some problems when we forget to set this property:

• Callbacks that should be disabled for performance reasons trigger. For example, this callback on Note should be switched off during an import to protect GitLab's database https://gitlab.com/gitlab-org/gitlab/-/issues/336788, and this touch among others.
• Object might fail to import due to validations.

This seems to be extensive, examples spotted in just github_import/importer/events are:

• ResourceMilestoneEvent.create! won't disable this callback
• A Note.create! won't disable many callbacks switched off for performance reasons.
• Another Note.create!.
• ResourceStateEvent.create! won't disable this validation

Proposal

Globals are an anti-pattern, but I think we could:

• Implement a controlled form of a global where we set state within Gitlab::SafeRequestStore once. This is available to Sidekiq workers.
• This would be read by the Importable module to know that we're importing.

So we would "set and forget".

Example code

Just to sketch out the proposal in code:

Click to see semi-pseudo code

Semi-pseudo code (untested):

module Import
  # Wrap the importing code in a block that ensured the state flag was unset
  def importing(&block)
    Gitlab::SafeRequestStore[:importing] = true
    yield
  ensure
    Gitlab::SafeRequestStore.delete(:importing)
  end

  # From Sidekiq, generally a worker will always be creating import data,
  # so we could probably just set it without needing to unset it
  def importing!
    raise "..." unless Gitlab::Runtime.sidekiq?

    Gitlab::SafeRequestStore[:importing] = true
  end
end

Importable module would be rewritten:

diff --git a/app/models/concerns/importable.rb b/app/models/concerns/importable.rb
index 4d2707b08abd..1ccc432e9820 100644
--- a/app/models/concerns/importable.rb
+++ b/app/models/concerns/importable.rb
@@ -4,8 +4,11 @@ module Importable
   extend ActiveSupport::Concern

   attr_accessor :importing
-  alias_method :importing?, :importing

   attr_accessor :imported
   alias_method :imported?, :imported
+
+  def importing?
+    Gitlab::SafeRequestStore[:importing] || importing
+  end
 end

And we would use it like this:

# Rails/rake task:
Import.importing do
  # ... create the import data
end

# Otherwise, from Sidekiq, generally a worker will always be creating 
# import data, so we could probably just set it within `#perform` so it's
# not part of every stack trace.
# We can block non-Sidekiq context code from being able to use this method.
Import.importing!
# ... create the import data