From 991bf24ec8890eca248a00deb4f33f309c9ffb83 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 15 Nov 2017 17:22:18 +0000
Subject: Use latest_merge_request_diff association

Compared to the merge_request_diff association:

1. It's simpler to query. The query uses a foreign key to the
   merge_request_diffs table, so no ordering is necessary.
2. It's faster for preloading. The merge_request_diff association has to load
   every diff for the MRs in the set, then discard all but the most recent for
   each. This association means that Rails can just query for N diffs from N
   MRs.
3. It's more complicated to update. This is a bidirectional foreign key, so we
   need to update two tables when adding a diff record. This also means we need
   to handle this as a special case when importing a GitLab project.

There is some juggling with this association in the merge request model:

* `MergeRequest#latest_merge_request_diff` is _always_ the latest diff.
* `MergeRequest#merge_request_diff` reuses
  `MergeRequest#latest_merge_request_diff` unless:
    * Arguments are passed. These are typically to force-reload the association.
    * It doesn't exist. That means we might be trying to implicitly create a
      diff. This only seems to happen in specs.
    * The association is already loaded. This is important for the reasons
      explained in the comment, which I'll reiterate here: if we a) load a
      non-latest diff, then b) get its `merge_request`, then c) get that MR's
      `merge_request_diff`, we should get the diff we loaded in c), even though
      that's not the latest diff.

Basically, `MergeRequest#merge_request_diff` is the latest diff in most cases,
but not quite all.
---
 .../concerns/manual_inverse_association_spec.rb    | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 spec/models/concerns/manual_inverse_association_spec.rb

(limited to 'spec/models/concerns/manual_inverse_association_spec.rb')

diff --git a/spec/models/concerns/manual_inverse_association_spec.rb b/spec/models/concerns/manual_inverse_association_spec.rb
new file mode 100644
index 00000000000..aad40883854
--- /dev/null
+++ b/spec/models/concerns/manual_inverse_association_spec.rb
@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+describe ManualInverseAssociation do
+  let(:model) do
+    Class.new(MergeRequest) do
+      belongs_to :manual_association, class_name: 'MergeRequestDiff', foreign_key: :latest_merge_request_diff_id
+      manual_inverse_association :manual_association, :merge_request
+    end
+  end
+
+  before do
+    stub_const("#{described_class}::Model", model)
+  end
+
+  let(:instance) { create(:merge_request).becomes(model) }
+
+  describe '.manual_inverse_association' do
+    context 'when the relation exists' do
+      before do
+        instance.create_merge_request_diff
+        instance.reload
+      end
+
+      it 'loads the relation' do
+        expect(instance.manual_association).to be_an_instance_of(MergeRequestDiff)
+      end
+
+      it 'does not perform extra queries after loading' do
+        instance.manual_association
+
+        expect { instance.manual_association.merge_request }
+          .not_to exceed_query_limit(0)
+      end
+
+      it 'passes arguments to the default association method, to allow reloading' do
+        query_count = ActiveRecord::QueryRecorder.new do
+          instance.manual_association
+          instance.manual_association(true)
+        end.count
+
+        expect(query_count).to eq(2)
+      end
+    end
+
+    context 'when the relation does not return a value' do
+      it 'does not try to set an inverse' do
+        expect(instance.manual_association).to be_nil
+      end
+    end
+  end
+end
-- 
cgit v1.2.1