<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=434114) </details> <!--IssueSummary end--> ### About There are often validations and callbacks on models that should be switched off during an import. See [example validation](https://gitlab.com/gitlab-org/gitlab/-/blob/de69f32e5f23d683d10eb2ef4dab7b321551a3ec/app/models/note.rb#L111) and [example callback](https://gitlab.com/gitlab-org/gitlab/-/blob/de69f32e5f23d683d10eb2ef4dab7b321551a3ec/app/models/note.rb#L200). 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`](https://gitlab.com/gitlab-org/gitlab/-/blob/3473ab1f2a2fa774e4d29fcf3a1d46e1501ed7e2/lib/gitlab/markdown_cache/active_record/extension.rb#L15) should be switched off during an import to protect GitLab's database https://gitlab.com/gitlab-org/gitlab/-/issues/336788, and [this touch](https://gitlab.com/gitlab-org/gitlab/-/blob/de69f32e5f23d683d10eb2ef4dab7b321551a3ec/app/models/note.rb#L200) 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!`](https://gitlab.com/gitlab-org/gitlab/-/blob/c7e4ef1b65fc232f89f5b399a45d7b1e5942db25/lib/gitlab/github_import/importer/events/changed_milestone.rb#L20-28) won't [disable this callback](https://gitlab.com/gitlab-org/gitlab/-/blob/1d888461ae7a465b1c83c4d7aa6c75dbd6c43222/app/models/resource_timebox_event.rb#L9) - A [`Note.create!`](https://gitlab.com/gitlab-org/gitlab/-/blob/3709ada936d6c075dbec5e592a4e5d787ed3fe60/lib/gitlab/github_import/importer/events/changed_assignee.rb#L20) won't disable many callbacks switched off for performance reasons. - Another [`Note.create!`](https://gitlab.com/gitlab-org/gitlab/-/blob/f1fa496c959245dacfe0f797f93392912291cdf9/lib/gitlab/github_import/importer/events/changed_reviewer.rb#L20). - [`ResourceStateEvent.create!`](https://gitlab.com/gitlab-org/gitlab/-/blob/c7e4ef1b65fc232f89f5b399a45d7b1e5942db25/lib/gitlab/github_import/importer/events/closed.rb#L37) won't disable [this validation](https://gitlab.com/gitlab-org/gitlab/-/blob/1d888461ae7a465b1c83c4d7aa6c75dbd6c43222/app/models/resource_state_event.rb#L7) ### 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](https://gitlab.com/gitlab-org/gitlab/-/blob/c4a9e01dc6fa517c042143bbfd08c2a3e524a541/lib/gitlab/sidekiq_middleware/request_store_middleware.rb#L7-9). - 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: <details> <summary>Click to see semi-pseudo code</summary> ### Semi-pseudo code (untested): ```ruby 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 ``` </details> `Importable` module would be rewritten: ```diff 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: ```ruby # 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 ```