Create DesignAtVersion model

This adds a new model entity `DesignManagement::DesignAtVersion`, which
wraps a design and an associated version. Mechanisms are provided for
lazy-finding and validation.

These do not have mutable properties, and validate the important
constraint that they share an issue

Finding DesignManagement::DesignAtVersion objects requires querying both
the designs and the versions tables, and is at present more expensive
than a single fetch. It also returns an Array instead of a relation,
since this is not a ActiveModel object. We validate invariants in find,
this ensures we never fetch invalid objects.

* Validations:
  - checking that there is both a design and a version
  - checking that the design and the version have the same issue
  - checking that both the design and the version have an issue

  These validations are checked during `find` and when calling
  `instantiate`

* ID

  It is important for GraphQL and the front-end that the model has an ID
  that can be used to lookup each combination, and be used to store this
  identity on the client.

* deleted? and status methods

  These supplement their analogues in DesignManagement::Design with an
  awareness of the current version. Thus a
  DesignManagement::DesignAtVersion as of a version in which is was
  deleted will respond to `deleted?` appropriately.

  Callers should be be careful to distinguish between `dav.state ==
  :current` and `!dav.deleted?` - as of a version before a design has
  been created it will respond with:
   * `#deleted? == false`
   * `#status == :not_created_yet`
  Reflecting the fact that this is not deleted because it has not been
  created yet.

This requires changes to the design and version factories to be able to
create valid objects with nil issues, a condition that is found when
these objects are being imported.

ee/app/models/design_management/design_at_version.rb

+# frozen_string_literal: true
+
+# Tuple of design and version
+# * has a composite ID, with lazy_find
+module DesignManagement
+  class DesignAtVersion
+    include ActiveModel::Validations
+    include GlobalID::Identification
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :version
+    attr_reader :design
+
+    validates_presence_of :version
+    validates_presence_of :design
+
+    validate :design_and_version_belong_to_the_same_issue
+    validate :design_and_version_have_issue_id
+
+    def initialize(design: nil, version: nil)
+      @design, @version = design, version
+    end
+
+    def self.instantiate(attrs)
+      new(attrs).tap { |obj| obj.validate! }
+    end
+
+    # The ID, needed by GraphQL types and as part of the Lazy-fetch
+    # protocol, includes information about both the design and the version.
+    #
+    # The particular format is not interesting, and should be treated as opaque
+    # by all callers.
+    def id
+      "#{design.id}.#{version.id}"
+    end
+
+    def self.lazy_find(id)
+      BatchLoader.for(id).batch do |ids, callback|
+        find(ids).each do |record|
+          callback.call(record.id, record)
+        end
+      end
+    end
+
+    def self.find(ids)
+      pairs = ids.map { |id| id.split('.').map(&:to_i) }
+
+      design_ids = pairs.map(&:first).uniq
+      version_ids = pairs.map(&:second).uniq
+
+      designs = ::DesignManagement::Design
+        .where(id: design_ids)
+        .index_by(&:id)
+
+      versions = ::DesignManagement::Version
+        .where(id: version_ids)
+        .index_by(&:id)
+
+      pairs.map do |(design_id, version_id)|
+        design = designs[design_id]
+        version = versions[version_id]
+
+        obj = new(design: design, version: version)
+
+        obj if obj.valid?
+      end.compact
+    end
+
+    def status
+      if not_created_yet?
+        :not_created_yet
+      elsif deleted?
+        :deleted
+      else
+        :current
+      end
+    end
+
+    def deleted?
+      action&.deletion?
+    end
+
+    def not_created_yet?
+      action.nil?
+    end
+
+    private
+
+    def action
+      strong_memoize(:most_recent_action) do
+        ::DesignManagement::Action
+          .most_recent.up_to_version(version)
+          .where(design: design)
+          .first
+      end
+    end
+
+    def design_and_version_belong_to_the_same_issue
+      id_a, id_b = [design, version].map { |obj| obj&.issue_id }
+
+      return if id_a == id_b
+
+      errors.add(:issue, "must be the same on design and version")
+    end
+
+    def design_and_version_have_issue_id
+      return if [design, version].all? { |obj| obj.try(:issue_id).present? }
+
+      errors.add(:issue, "must be present on both design and version")
+    end
+  end
+end

ee/app/policies/design_management/design_at_version_policy.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class DesignAtVersionPolicy < ::BasePolicy
+    delegate { @subject.version }
+    delegate { @subject.design }
+  end
+end

ee/spec/factories/design_management/design_at_version.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :design_at_version, class: DesignManagement::DesignAtVersion do
+    skip_create # This is not an Active::Record model.
+
+    design { nil }
+
+    version { nil }
+
+    transient do
+      issue { design&.issue || version&.issue || create(:issue) }
+    end
+
+    initialize_with do
+      attrs = attributes.dup
+      attrs[:design] ||= create(:design, issue: issue)
+      attrs[:version] ||= create(:design_version, issue: issue)
+
+      new(attrs)
+    end
+  end
+end

ee/spec/factories/design_management/designs.rb

@@ -3,13 +3,25 @@
 FactoryBot.define do
   factory :design, class: DesignManagement::Design do
     issue { create(:issue) }
-    project { issue.project }
+    project { issue&.project || create(:project) }
     sequence(:filename) { |n| "homescreen-#{n}.jpg" }
 
     transient do
       author { issue.author }
     end
 
+    trait :importing do
+      issue { nil }
+
+      importing { true }
+      imported { false }
+    end
+
+    trait :imported do
+      importing { false }
+      imported { true }
+    end
+
     create_versions = ->(design, evaluator, commit_version) do
       unless evaluator.versions_count.zero?
         project = design.project

ee/spec/factories/design_management/versions.rb

@@ -4,7 +4,7 @@ FactoryBot.define do
   factory :design_version, class: DesignManagement::Version do
     sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
     issue { designs.first&.issue || create(:issue) }
-    author { issue.author || create(:user) }
+    author { issue&.author || create(:user) }
 
     transient do
       designs_count { 1 }
@@ -18,6 +18,19 @@ FactoryBot.define do
       designs_count { 0 }
     end
 
+    trait :importing do
+      issue { nil }
+
+      designs_count { 0 }
+      importing { true }
+      imported { false }
+    end
+
+    trait :imported do
+      importing { false }
+      imported { true }
+    end
+
     after(:build) do |version, evaluator|
       # By default all designs are created_designs, so just add them.
       specific_designs = [].concat(

ee/spec/models/design_management/design_at_version_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::DesignAtVersion do
+  include DesignManagementTestHelpers
+
+  set(:issue) { create(:issue) }
+  set(:issue_b) { create(:issue) }
+  set(:design) { create(:design, issue: issue) }
+  set(:version) { create(:design_version, designs: [design]) }
+
+  describe '#id' do
+    subject { described_class.new(design: design, version: version) }
+
+    it 'combines design.id and version.id' do
+      expect(subject.id).to include(design.id.to_s, version.id.to_s)
+    end
+  end
+
+  describe 'status methods' do
+    let!(:design_a) { create(:design, issue: issue) }
+    let!(:design_b) { create(:design, issue: issue) }
+
+    let!(:version_a) do
+      create(:design_version, designs: [design_a])
+    end
+    let!(:version_b) do
+      create(:design_version, designs: [design_b])
+    end
+    let!(:version_mod) do
+      create(:design_version, modified_designs: [design_a, design_b])
+    end
+    let!(:version_c) do
+      create(:design_version, deleted_designs: [design_a])
+    end
+    let!(:version_d) do
+      create(:design_version, deleted_designs: [design_b])
+    end
+    let!(:version_e) do
+      create(:design_version, designs: [design_a])
+    end
+
+    describe 'a design before it has been created' do
+      subject { build(:design_at_version, design: design_b, version: version_a) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :not_created_yet' do
+        expect(subject).to have_attributes(status: :not_created_yet)
+      end
+    end
+
+    describe 'a design as of its creation' do
+      subject { build(:design_at_version, design: design_a, version: version_a) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design after it has been created, but before deletion' do
+      subject { build(:design_at_version, design: design_b, version: version_c) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design as of its modification' do
+      subject { build(:design_at_version, design: design_a, version: version_mod) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design as of its deletion' do
+      subject { build(:design_at_version, design: design_a, version: version_c) }
+
+      it 'is deleted' do
+        expect(subject).to be_deleted
+      end
+
+      it 'has the status :deleted' do
+        expect(subject).to have_attributes(status: :deleted)
+      end
+    end
+
+    describe 'a design after its deletion' do
+      subject { build(:design_at_version, design: design_b, version: version_e) }
+
+      it 'is deleted' do
+        expect(subject).to be_deleted
+      end
+
+      it 'has the status :deleted' do
+        expect(subject).to have_attributes(status: :deleted)
+      end
+    end
+
+    describe 'a design on its recreation' do
+      subject { build(:design_at_version, design: design_a, version: version_e) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+  end
+
+  describe 'validations' do
+    subject(:design_at_version) { build(:design_at_version) }
+
+    it { is_expected.to be_valid }
+
+    describe 'a design-at-version without a design' do
+      subject { described_class.new(design: nil, version: build(:design_version)) }
+
+      it { is_expected.to be_invalid }
+
+      it 'mentions the design in the errors' do
+        subject.valid?
+
+        expect(subject.errors[:design]).to be_present
+      end
+    end
+
+    describe 'a design-at-version without a version' do
+      subject { described_class.new(design: build(:design), version: nil) }
+
+      it { is_expected.to be_invalid }
+
+      it 'mentions the version in the errors' do
+        subject.valid?
+
+        expect(subject.errors[:version]).to be_present
+      end
+    end
+
+    describe 'design_and_version_belong_to_the_same_issue' do
+      context 'both design and version are supplied' do
+        subject(:design_at_version) { build(:design_at_version, design: design, version: version) }
+
+        context 'the design belongs to the same issue as the version' do
+          it { is_expected.to be_valid }
+        end
+
+        context 'the design does not belong to the same issue as the version' do
+          let(:design) { create(:design) }
+          let(:version) { create(:design_version) }
+
+          it { is_expected.to be_invalid }
+        end
+      end
+
+      context 'the factory is just supplied with a design' do
+        let(:design) { create(:design) }
+
+        subject(:design_at_version) { build(:design_at_version, design: design) }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'the factory is just supplied with a version' do
+        let(:version) { create(:design_version) }
+
+        subject(:design_at_version) { build(:design_at_version, version: version) }
+
+        it { is_expected.to be_valid }
+      end
+    end
+
+    describe 'design_and_version_have_issue_id' do
+      subject(:design_at_version) { build(:design_at_version, design: design, version: version) }
+
+      context 'the design has no issue_id, because it is being imported' do
+        let(:design) { create(:design, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+
+      context 'the version has no issue_id, because it is being imported' do
+        let(:version) { create(:design_version, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+
+      context 'both the design and the version are being imported' do
+        let(:version) { create(:design_version, :importing) }
+        let(:design) { create(:design, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+    end
+  end
+
+  def id_of(design, version)
+    build(:design_at_version, design: design, version: version).id
+  end
+
+  describe '.instantiate' do
+    context 'when attrs are valid' do
+      subject do
+        described_class.instantiate(design: design, version: version)
+      end
+
+      it { is_expected.to be_a(described_class).and(be_valid) }
+    end
+
+    context 'when attrs are invalid' do
+      subject do
+        described_class.instantiate(
+          design: create(:design),
+          version: create(:design_version)
+        )
+      end
+
+      it 'raises a validation error' do
+        expect { subject }.to raise_error(ActiveModel::ValidationError)
+      end
+    end
+  end
+
+  describe '.lazy_find' do
+    let!(:version_a) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue))
+    end
+    let!(:version_b) do
+      create(:design_version, designs: create_list(:design, 1, issue: issue))
+    end
+    let!(:version_c) do
+      create(:design_version, designs: create_list(:design, 1, issue: issue_b))
+    end
+
+    let(:id_a)   { id_of(version_a.designs.first,  version_a) }
+    let(:id_b)   { id_of(version_a.designs.second, version_a) }
+    let(:id_c)   { id_of(version_a.designs.last,   version_a) }
+    let(:id_d)   { id_of(version_b.designs.first,  version_b) }
+    let(:id_e)   { id_of(version_c.designs.first,  version_c) }
+    let(:bad_id) { id_of(version_c.designs.first,  version_a) }
+
+    def find(the_id)
+      described_class.lazy_find(the_id)
+    end
+
+    let(:db_calls) { 2 }
+
+    it 'issues fewer queries than the naive approach would' do
+      expect do
+        dav_a = find(id_a)
+        dav_b = find(id_b)
+        dav_c = find(id_c)
+        dav_d = find(id_d)
+        dav_e = find(id_e)
+        should_not_exist = find(bad_id)
+
+        expect(dav_a.version).to eq(version_a)
+        expect(dav_b.version).to eq(version_a)
+        expect(dav_c.version).to eq(version_a)
+        expect(dav_d.version).to eq(version_b)
+        expect(dav_e.version).to eq(version_c)
+        expect(should_not_exist).not_to be_present
+
+        expect(version_a.designs).to include(dav_a.design, dav_b.design, dav_c.design)
+        expect(version_b.designs).to include(dav_d.design)
+        expect(version_c.designs).to include(dav_e.design)
+      end.not_to exceed_query_limit(db_calls)
+    end
+  end
+
+  describe '.find' do
+    let(:results) { described_class.find(ids) }
+
+    # 2 versions, with 5 total designs on issue A, so 2*5 = 10
+    let!(:version_a) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue))
+    end
+    let!(:version_b) do
+      create(:design_version, designs: create_list(:design, 2, issue: issue))
+    end
+    # 1 version, with 3 designs on issue B, so 1*3 = 3
+    let!(:version_c) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue_b))
+    end
+
+    context 'invalid ids' do
+      let(:ids) do
+        version_b.designs.map { |d| id_of(d, version_c) }
+      end
+
+      describe '#count' do
+        it 'counts 0 records' do
+          expect(results.count).to eq(0)
+        end
+      end
+
+      describe '#empty?' do
+        it 'is empty' do
+          expect(results).to be_empty
+        end
+      end
+
+      describe '#to_a' do
+        it 'finds no records' do
+          expect(results.to_a).to eq([])
+        end
+      end
+    end
+
+    context 'valid ids' do
+      let(:red_herrings) { issue_b.designs.sample(2).map { |d| id_of(d, version_a) } }
+
+      let(:ids) do
+        a_ids = issue.designs.sample(2).map { |d| id_of(d, version_a) }
+        b_ids = issue.designs.sample(2).map { |d| id_of(d, version_b) }
+        c_ids = issue_b.designs.sample(2).map { |d| id_of(d, version_c) }
+
+        a_ids + b_ids + c_ids + red_herrings
+      end
+
+      before do
+        ids.size # force IDs
+      end
+
+      describe '#count' do
+        it 'counts 2 records' do
+          expect(results.count).to eq(6)
+        end
+
+        it 'issues at most two queries' do
+          expect { results.count }.not_to exceed_query_limit(2)
+        end
+      end
+
+      describe '#to_a' do
+        it 'finds 6 records' do
+          expect(results.size).to eq(6)
+          expect(results).to all(be_a(described_class))
+        end
+
+        it 'only returns records with matching IDs' do
+          expect(results.map(&:id)).to match_array(ids - red_herrings)
+        end
+
+        it 'only returns valid records' do
+          expect(results).to all(be_valid)
+        end
+
+        it 'issues at most two queries' do
+          expect { results.to_a }.not_to exceed_query_limit(2)
+        end
+      end
+    end
+  end
+end

ee/spec/models/design_management/version_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe DesignManagement::Version do
+  let_it_be(:issue) { create(:issue) }
+
   describe 'relations' do
     it { is_expected.to have_many(:actions) }
     it { is_expected.to have_many(:designs).through(:actions) }
@@ -82,7 +84,6 @@ describe DesignManagement::Version do
     end
 
     let_it_be(:author) { create(:user) }
-    let_it_be(:issue) { create(:issue) }
     let_it_be(:design_a) { create(:design, issue: issue) }
     let_it_be(:design_b) { create(:design, issue: issue) }
     let_it_be(:designs) { [design_a, design_b] }
@@ -291,4 +292,51 @@ describe DesignManagement::Version do
       expect(version.author).to eq(commit_user)
     end
   end
+
+  describe '#diff_refs' do
+    let(:project) { issue.project }
+
+    before do
+      expect(project.design_repository).to receive(:commit)
+        .once
+        .with(sha)
+        .and_return(commit)
+    end
+
+    subject { create(:design_version, issue: issue, sha: sha) }
+
+    context 'there is a commit in the repo by the SHA' do
+      let(:commit) { build(:commit) }
+      let(:sha) { commit.id }
+
+      it { is_expected.to have_attributes(diff_refs: commit.diff_refs) }
+
+      it 'memoizes calls to #diff_refs' do
+        expect(subject.diff_refs).to eq(subject.diff_refs)
+      end
+    end
+
+    context 'there is no commit in the repo by the SHA' do
+      let(:commit) { nil }
+      let(:sha) { Digest::SHA1.hexdigest("points to nothing") }
+
+      it { is_expected.to have_attributes(diff_refs: be_nil) }
+    end
+  end
+
+  describe '#reset' do
+    subject { create(:design_version, issue: issue) }
+
+    it 'removes memoized values' do
+      expect(subject).to receive(:commit).twice.and_return(nil)
+
+      subject.diff_refs
+      subject.diff_refs
+
+      subject.reset
+
+      subject.diff_refs
+      subject.diff_refs
+    end
+  end
 end