Merge branch '1756-set-iid-via-api' into 'master'

Resolve "Allow issue's Internal ID (`iid`) to be set when creating via the API"

Closes #1756

See merge request gitlab-org/gitlab-ce!20626

app/models/concerns/atomic_internal_id.rb

@@ -35,16 +35,21 @@ module AtomicInternalId
 
       define_method("ensure_#{scope}_#{column}!") do
         scope_value = association(scope).reader
+        value = read_attribute(column)
+
+        return value unless scope_value
 
-        if read_attribute(column).blank? && scope_value
         scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value }
         usage = self.class.table_name.to_sym
 
-          new_iid = InternalId.generate_next(self, scope_attrs, usage, init)
-          write_attribute(column, new_iid)
+        if value.present?
+          InternalId.track_greatest(self, scope_attrs, usage, value, init)
+        else
+          value = InternalId.generate_next(self, scope_attrs, usage, init)
+          write_attribute(column, value)
         end
 
-        read_attribute(column)
+        value
       end
     end
   end

app/models/internal_id.rb

 # An InternalId is a strictly monotone sequence of integers
 # generated for a given scope and usage.
 #
+# The monotone sequence may be broken if an ID is explicitly provided
+# to `.track_greatest_and_save!` or `#track_greatest`.
+#
 # For example, issues use their project to scope internal ids:
 # In that sense, scope is "project" and usage is "issues".
 # Generated internal ids for an issue are unique per project.
@@ -25,13 +28,34 @@ class InternalId < ActiveRecord::Base
   # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
   # As such, the increment is atomic and safe to be called concurrently.
   def increment_and_save!
+    update_and_save { self.last_value = (last_value || 0) + 1 }
+  end
+
+  # Increments #last_value with new_value if it is greater than the current,
+  # and saves the record
+  #
+  # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
+  # As such, the increment is atomic and safe to be called concurrently.
+  def track_greatest_and_save!(new_value)
+    update_and_save { self.last_value = [last_value || 0, new_value].max }
+  end
+
+  private
+
+  def update_and_save(&block)
     lock!
-    self.last_value = (last_value || 0) + 1
+    yield
     save!
     last_value
   end
 
   class << self
+    def track_greatest(subject, scope, usage, new_value, init)
+      return new_value unless available?
+
+      InternalIdGenerator.new(subject, scope, usage, init).track_greatest(new_value)
+    end
+
     def generate_next(subject, scope, usage, init)
       # Shortcut if `internal_ids` table is not available (yet)
       # This can be the case in other (unrelated) migration specs
@@ -94,6 +118,16 @@ class InternalId < ActiveRecord::Base
       end
     end
 
+    # Create a record in internal_ids if one does not yet exist
+    # and set its new_value if it is higher than the current last_value
+    #
+    # Note this will acquire a ROW SHARE lock on the InternalId record
+    def track_greatest(new_value)
+      subject.transaction do
+        (lookup || create_record).track_greatest_and_save!(new_value)
+      end
+    end
+
     private
 
     # Retrieve InternalId record for (project, usage) combination, if it exists

changelogs/unreleased/1756-set-iid-via-api.yml

+---
+title: Allow issues API to receive an internal ID (iid) on create
+merge_request: 20626
+author: Jamie Schembri
+type: fixed

doc/api/issues.md

@@ -463,6 +463,7 @@ POST /projects/:id/issues
 | Attribute                                 | Type           | Required | Description  |
 |-------------------------------------------|----------------|----------|--------------|
 | `id`                                      | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `iid`                                     | integer/string | no       | The internal ID of the project's issue (requires admin or project owner rights) |
 | `title`                                   | string  | yes      | The title of an issue |
 | `description`                             | string  | no       | The description of an issue  |
 | `confidential`                            | boolean | no       | Set an issue to be confidential. Default is `false`.  |

lib/api/issues.rb

@@ -162,6 +162,9 @@ module API
                                                            desc: 'The IID of a merge request for which to resolve discussions'
         optional :discussion_to_resolve, type: String,
                                          desc: 'The ID of a discussion to resolve, also pass `merge_request_to_resolve_discussions_of`'
+        optional :iid, type: Integer,
+                       desc: 'The internal ID of a project issue. Available only for admins and project owners.'
+
         use :issue_params
       end
       post ':id/issues' do
@@ -169,9 +172,10 @@ module API
 
         authorize! :create_issue, user_project
 
-        # Setting created_at time only allowed for admins and project owners
+        # Setting created_at time or iid only allowed for admins and project owners
         unless current_user.admin? || user_project.owner == current_user
           params.delete(:created_at)
+          params.delete(:iid)
         end
 
         issue_params = declared_params(include_missing: false)

spec/models/internal_id_spec.rb

@@ -79,6 +79,46 @@ describe InternalId do
     end
   end
 
+  describe '.track_greatest' do
+    let(:value) { 9001 }
+    subject { described_class.track_greatest(issue, scope, usage, value, init) }
+
+    context 'in the absence of a record' do
+      it 'creates a record if not yet present' do
+        expect { subject }.to change { described_class.count }.from(0).to(1)
+      end
+    end
+
+    it 'stores record attributes' do
+      subject
+
+      described_class.first.tap do |record|
+        expect(record.project).to eq(project)
+        expect(record.usage).to eq(usage.to_s)
+        expect(record.last_value).to eq(value)
+      end
+    end
+
+    context 'with existing issues' do
+      before do
+        create(:issue, project: project)
+        described_class.delete_all
+      end
+
+      it 'still returns the last value to that of the given value' do
+        expect(subject).to eq(value)
+      end
+    end
+
+    context 'when value is less than the current last_value' do
+      it 'returns the current last_value' do
+        described_class.create!(**scope, usage: usage, last_value: 10_001)
+
+        expect(subject).to eq 10_001
+      end
+    end
+  end
+
   describe '#increment_and_save!' do
     let(:id) { create(:internal_id) }
     subject { id.increment_and_save! }
@@ -103,4 +143,30 @@ describe InternalId do
       end
     end
   end
+
+  describe '#track_greatest_and_save!' do
+    let(:id) { create(:internal_id) }
+    let(:new_last_value) { 9001 }
+    subject { id.track_greatest_and_save!(new_last_value) }
+
+    it 'returns new last value' do
+      expect(subject).to eq new_last_value
+    end
+
+    it 'saves the record' do
+      subject
+
+      expect(id.changed?).to be_falsey
+    end
+
+    context 'when new last value is lower than the max' do
+      it 'does not update the last value' do
+        id.update!(last_value: 10_001)
+
+        subject
+
+        expect(id.reload.last_value).to eq 10_001
+      end
+    end
+  end
 end

spec/requests/api/issues_spec.rb

@@ -1002,6 +1002,38 @@ describe API::Issues do
       end
     end
 
+    context 'an internal ID is provided' do
+      context 'by an admin' do
+        it 'sets the internal ID on the new issue' do
+          post api("/projects/#{project.id}/issues", admin),
+            title: 'new issue', iid: 9001
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['iid']).to eq 9001
+        end
+      end
+
+      context 'by an owner' do
+        it 'sets the internal ID on the new issue' do
+          post api("/projects/#{project.id}/issues", user),
+            title: 'new issue', iid: 9001
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['iid']).to eq 9001
+        end
+      end
+
+      context 'by another user' do
+        it 'ignores the given internal ID' do
+          post api("/projects/#{project.id}/issues", user2),
+            title: 'new issue', iid: 9001
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['iid']).not_to eq 9001
+        end
+      end
+    end
+
     it 'creates a new project issue' do
       post api("/projects/#{project.id}/issues", user),
         title: 'new issue', labels: 'label, label2', weight: 3,

spec/support/shared_examples/models/atomic_internal_id_spec.rb

@@ -60,6 +60,20 @@ shared_examples_for 'AtomicInternalId' do |validate_presence: true|
 
         expect { subject }.not_to change { instance.public_send(internal_id_attribute) }
       end
+
+      context 'when the instance has an internal ID set' do
+        let(:internal_id) { 9001 }
+
+        it 'calls InternalId.update_last_value and sets the `last_value` to that of the instance' do
+          instance.send("#{internal_id_attribute}=", internal_id)
+
+          expect(InternalId)
+            .to receive(:track_greatest)
+            .with(instance, scope_attrs, usage, internal_id, any_args)
+            .and_return(internal_id)
+          subject
+        end
+      end
     end
   end
 end