Remove null constraint from GroupImportState#jid

In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/31731 we are
displaying the import status to the user. We need to be able to show the
user that an import has been scheduled. As this happens asynchronously,
the running of the job will likely happen after the request returns to
the client.

Currently our group import state is created in the job, but we need to
create it before the job is triggered so that we can have an in progress
import state when we return the HTTP request. If we create the import
state after scheduling the job, we end up with a race condition between
the job wanting to use the import state and the import state being
created in the request.

For this reason, we remove the requirement for the jid to be present.
This also follows the same pattern that the ProjectImportState takes.
This will also mean that if we fail to schedule an import job (no jid is
returned) we can still mark the import as failed properly.

MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33181
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/211808

app/models/group_import_state.rb

@@ -5,7 +5,8 @@ class GroupImportState < ApplicationRecord
 
   belongs_to :group, inverse_of: :import_state
 
-  validates :group, :status, :jid, presence: true
+  validates :group, :status, presence: true
+  validates :jid, presence: true, if: -> { started? || finished? }
 
   state_machine :status, initial: :created do
     state :created, value: 0

changelogs/unreleased/jh-drop_jid_null_constraint.yml

+---
+title: Remove null constraint for JID in GroupImportState
+merge_request: 33181
+author:
+type: changed

db/migrate/20200526142550_drop_null_constraint_on_group_import_state_jid.rb

+# frozen_string_literal: true
+
+class DropNullConstraintOnGroupImportStateJid < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    change_column_null :group_import_states, :jid, true
+  end
+
+  def down
+    # No-op -- null values could have been added after this this constraint was removed.
+  end
+end

db/structure.sql

@@ -3186,7 +3186,7 @@ CREATE TABLE public.group_import_states (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     status smallint DEFAULT 0 NOT NULL,
-    jid text NOT NULL,
+    jid text,
     last_error text,
     CONSTRAINT check_87b58f6b30 CHECK ((char_length(last_error) <= 255)),
     CONSTRAINT check_96558fff96 CHECK ((char_length(jid) <= 100))
@@ -13788,6 +13788,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200526000407
 20200526013844
 20200526120714
+20200526142550
 20200526153844
 20200526164946
 20200526164947

spec/factories/group_import_states.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :group_import_state, class: 'GroupImportState', traits: %i[created] do
+    association :group, factory: :group
+
+    trait :created do
+      status { 0 }
+    end
+
+    trait :started do
+      status { 1 }
+      sequence(:jid) { |n| "group_import_state_#{n}" }
+    end
+
+    trait :finished do
+      status { 2 }
+      sequence(:jid) { |n| "group_import_state_#{n}" }
+    end
+
+    trait :failed do
+      status { -1 }
+    end
+  end
+end

spec/models/group_import_state_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupImportState do
+  describe 'validations' do
+    let_it_be(:group) { create(:group) }
+
+    it { is_expected.to validate_presence_of(:group) }
+    it { is_expected.to validate_presence_of(:status) }
+
+    it 'can be created without a jid' do
+      import_state = build(:group_import_state, :created, group: group, jid: nil)
+
+      expect(import_state).to be_valid
+    end
+
+    it 'cannot be started without a jid' do
+      import_state = build(:group_import_state, :started, group: group, jid: nil)
+
+      expect(import_state).not_to be_valid
+      expect(import_state.errors[:jid]).to include "can't be blank"
+    end
+
+    it 'cannot be finished without a jid' do
+      import_state = build(:group_import_state, :finished, group: group, jid: nil)
+
+      expect(import_state).not_to be_valid
+      expect(import_state.errors[:jid]).to include "can't be blank"
+    end
+
+    it 'can fail without a jid' do
+      import_state = build(:group_import_state, :failed, group: group, jid: nil)
+
+      expect(import_state).to be_valid
+    end
+  end
+end