Set `importing` on objects through global context
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