Revert the use of the `BatchLoader`

This commit will restore the original `preload` solution for the
API::Entities::MergerequestBasic and only use the BatchLoader for the
`Gitlab::IssuableMetadata`.

app/models/label_link.rb

@@ -9,9 +9,4 @@ class LabelLink < ApplicationRecord
 
   validates :target, presence: true, unless: :importing?
   validates :label, presence: true, unless: :importing?
-
-  scope :preloaded, -> { preload(:label) }
-
-  scope :for_targets, ->(type:, scope:) { where(target_type: type, target_id: scope) }
-  scope :for_merge_requests, ->(merge_request_scope) { for_targets(type: MergeRequest, scope: merge_request_scope) }
 end

app/models/merge_request.rb

@@ -254,12 +254,18 @@ class MergeRequest < ApplicationRecord
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
 
+  PROJECT_ROUTE_AND_NAMESPACE_ROUTE = [
+    target_project: [:route, { namespace: :route }],
+    source_project: [:route, { namespace: :route }]
+  ].freeze
+
   scope :with_api_entity_associations, -> {
-    preload(
-      target_project: [:route, { namespace: :route }],
-      source_project: [:route, { namespace: :route }]
-    )
+    preload(:assignees, :author, :unresolved_notes, :labels, :milestone,
+            :timelogs, :latest_merge_request_diff,
+            *PROJECT_ROUTE_AND_NAMESPACE_ROUTE,
+            metrics: [:latest_closed_by, :merged_by])
   }
+
   scope :by_target_branch_wildcard, ->(wildcard_branch_name) do
     where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%'))
   end
@@ -1021,7 +1027,7 @@ class MergeRequest < ApplicationRecord
   end
 
   def for_fork?
-    target_project_id != source_project_id
+    target_project != source_project
   end
 
   # If the merge request closes any issues, save this information in the

app/models/merge_request/metrics.rb

@@ -5,11 +5,6 @@ class MergeRequest::Metrics < ApplicationRecord
   belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
   belongs_to :latest_closed_by, class_name: 'User'
   belongs_to :merged_by, class_name: 'User'
-
-  scope :preloaded, -> { preload(:merged_by, :latest_closed_by) }
-  scope :for_merge_requests, -> (merge_request_scope) do
-    where(merge_request: merge_request_scope)
-  end
 end
 
 MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics')

app/models/merge_request_assignee.rb

@@ -6,11 +6,5 @@ class MergeRequestAssignee < ApplicationRecord
 
   validates :assignee, uniqueness: { scope: :merge_request_id }
 
-  scope :preloaded, -> { preload(:assignee) }
-
   scope :in_projects, ->(project_ids) { joins(:merge_request).where("merge_requests.target_project_id in (?)", project_ids) }
-
-  scope :for_merge_requests, ->(merge_request_scope) do
-    where(merge_request: merge_request_scope)
-  end
 end

app/models/merge_request_diff.rb

@@ -100,10 +100,6 @@ class MergeRequestDiff < ApplicationRecord
     joins(merge_request: :metrics).where(condition)
   end
 
-  scope :for_merge_requests, -> (merge_request_scope) do
-    where(merge_request: merge_request_scope)
-  end
-
   def self.ids_for_external_storage_migration(limit:)
     # No point doing any work unless the feature is enabled
     return [] unless Gitlab.config.external_diffs.enabled

app/models/timelog.rb

@@ -20,10 +20,6 @@ class Timelog < ApplicationRecord
     where('spent_at BETWEEN ? AND ?', start_time, end_time)
   end
 
-  scope :for_merge_requests, -> (merge_request_scope) do
-    where(merge_request: merge_request_scope)
-  end
-
   def issuable
     issue || merge_request
   end

lib/api/entities/issuable_time_stats.rb

@@ -7,12 +7,6 @@ module API
         Gitlab::TimeTrackingFormatter.output(time_spent)
       end
 
-      def presented
-        lazy_timelogs
-
-        super
-      end
-
       expose :time_estimate
       expose :total_time_spent
       expose :human_time_estimate
@@ -21,19 +15,12 @@ module API
         expose :total_time_spent, as: :human_total_time_spent
       end
 
-      private
-
-      def lazy_timelogs
-        BatchLoader.for(object.id).batch(key: :timelogs, default_value: []) do |ids, loader|
-          Timelog.for_merge_requests(ids).find_each do |timelog|
-            loader.call(timelog.merge_request_id) { |acc| acc << timelog }
-          end
-        end
-      end
-
+      # rubocop: disable CodeReuse/ActiveRecord
       def total_time_spent
-        lazy_timelogs.sum(&:time_spent) # rubocop:disable CodeReuse/ActiveRecord
+        # Avoids an N+1 query since timelogs are preloaded
+        object.timelogs.map(&:time_spent).sum
       end
+      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

lib/api/entities/merge_request_basic.rb

@@ -3,35 +3,17 @@
 module API
   module Entities
     class MergeRequestBasic < IssuableEntity
-      # Evaluate the lazy exposures to trigger the BatchLoader
-      # before any object is serialized.
-      def presented
-        lazy_merge_request_metrics
-        lazy_diff
-        lazy_assignees
-        lazy_author
-        lazy_milestone
-        lazy_labels
-
-        # TODO: we could have a `:batch` exposure option to automatically scan
-        # exposures and evaluate the block so that the BatchLoader is primed
-        time_stats = self.class.find_exposure(:time_stats)
-        Object.const_get(time_stats.using_class_name, false).new(object).presented
-
-        super
-      end
-
       expose :merged_by, using: Entities::UserBasic do |merge_request, _options|
-        lazy_merge_request_metrics&.merged_by
+        merge_request.metrics&.merged_by
       end
       expose :merged_at do |merge_request, _options|
-        lazy_merge_request_metrics&.merged_at
+        merge_request.metrics&.merged_at
       end
       expose :closed_by, using: Entities::UserBasic do |merge_request, _options|
-        lazy_merge_request_metrics&.latest_closed_by
+        merge_request.metrics&.latest_closed_by
       end
       expose :closed_at do |merge_request, _options|
-        lazy_merge_request_metrics&.latest_closed_at
+        merge_request.metrics&.latest_closed_at
       end
       expose :title_html, if: -> (_, options) { options[:render_html] } do |entity|
         MarkupHelper.markdown_field(entity, :title)
@@ -45,25 +27,23 @@ module API
       expose(:downvotes)        { |merge_request, options| issuable_metadata.downvotes }
 
       with_options using: Entities::UserBasic do
-        expose :lazy_author, as: :author
-        expose :lazy_assignees, as: :assignees
-        expose :lazy_assignee, as: :assignee do |merge_request, options|
-          lazy_assignees.first
+        expose :author, as: :author
+        expose :assignees
+        expose :assignee do |merge_request, options|
+          merge_request.assignees.first
         end
       end
 
       expose :source_project_id, :target_project_id
       expose :labels do |merge_request, options|
-        lazy_labels do |label|
-          if options[:with_labels_details]
-            Entities::LabelBasic.new(label)
-          else
-            label.title
-          end
+        if options[:with_labels_details]
+          ::API::Entities::LabelBasic.represent(merge_request.labels.sort_by(&:title))
+        else
+          merge_request.labels.map(&:title).sort
         end
       end
       expose :work_in_progress?, as: :work_in_progress
-      expose :lazy_milestone, as: :milestone, using: Entities::Milestone
+      expose :milestone, using: Entities::Milestone
       expose :merge_when_pipeline_succeeds
 
       # Ideally we should deprecate `MergeRequest#merge_status` exposure and
@@ -76,9 +56,7 @@ module API
         merge_request.check_mergeability(async: true) unless options[:skip_merge_status_recheck]
         merge_request.public_merge_status
       end
-      expose :diff_head_sha, as: :sha do |_, options|
-        lazy_diff.read_attribute(:head_commit_sha)
-      end
+      expose :diff_head_sha, as: :sha
       expose :merge_commit_sha
       expose :squash_commit_sha
       expose :discussion_locked
@@ -113,72 +91,6 @@ module API
       expose :task_completion_status
       expose :cannot_be_merged?, as: :has_conflicts
       expose :mergeable_discussions_state?, as: :blocking_discussions_resolved
-
-      private
-
-      def lazy_merge_request_metrics
-        BatchLoader.for(object.id).batch(key: :merge_request_metrics) do |models, loader|
-          ::MergeRequest::Metrics
-            .preloaded
-            .for_merge_requests(models)
-            .find_each do |metric|
-            loader.call(metric.merge_request_id, metric)
-          end
-        end
-      end
-
-      def lazy_diff
-        BatchLoader.for(object.id).batch(key: :merge_request_diff) do |ids, loader|
-          ::MergeRequestDiff
-            .for_merge_requests(ids)
-            .find_each do |diff|
-            loader.call(diff.merge_request_id, diff)
-          end
-        end
-      end
-
-      def lazy_assignees
-        BatchLoader.for(object.id).batch(key: :assignees, default_value: []) do |ids, loader|
-          ::MergeRequestAssignee
-            .preloaded
-            .for_merge_requests(ids)
-            .find_each do |assignment|
-            loader.call(assignment.merge_request_id) { |acc| acc << assignment.assignee }
-          end
-        end
-      end
-
-      def lazy_author
-        BatchLoader.for(object.author_id).batch(key: :author) do |ids, loader|
-          ::User.id_in(ids).find_each do |author|
-            loader.call(author.id, author)
-          end
-        end
-      end
-
-      def lazy_milestone
-        BatchLoader.for(object.milestone_id).batch(key: :milestone) do |ids, loader|
-          ::Milestone
-            .with_api_entity_associations
-            .id_in(ids)
-            .find_each do |milestone|
-            loader.call(milestone.id, milestone)
-          end
-        end
-      end
-
-      def lazy_labels(&block)
-        BatchLoader.for(object.id).batch(key: :labels, default_value: []) do |ids, loader|
-          ::LabelLink
-            .preloaded
-            .for_merge_requests(ids)
-            .find_each do |link|
-            loader.call(link.target_id) do |memo|
-              memo << yield(link.label)
-            end
-          end
-        end
-      end
     end
   end
 end

spec/lib/api/entities/merge_request_basic_spec.rb

@@ -9,14 +9,14 @@ RSpec.describe ::API::Entities::MergeRequestBasic do
   let_it_be(:labels) { create_list(:label, 3) }
   let_it_be(:merge_requests) { create_list(:labeled_merge_request, 10, :unique_branches, :with_diffs, labels: labels) }
 
-  let(:scope) { MergeRequest.with_api_entity_associations }
-
   # This mimics the behavior of the `Grape::Entity` serializer
   def present(obj)
     described_class.new(obj).presented
   end
 
-  describe "#with_api_entity_associations" do
+  context "with :with_api_entity_associations scope" do
+    let(:scope) { MergeRequest.with_api_entity_associations }
+
     it "avoids N+1 queries" do
       query = scope.find(merge_request.id)
 
@@ -24,6 +24,11 @@ RSpec.describe ::API::Entities::MergeRequestBasic do
         present(query).to_json
       end
 
+      # stub the `head_commit_sha` as it will trigger a
+      # backward compatibility query that is out-of-scope
+      # for this test whenever it is `nil`
+      allow_any_instance_of(MergeRequestDiff).to receive(:head_commit_sha).and_return(Gitlab::Git::BLANK_SHA)
+
       query = scope.all
       batch = ActiveRecord::QueryRecorder.new do
         entities = query.map(&method(:present))