Merge branch 'id-optimize-sql-requests-mr-show' into 'master'

Reduce the number of SQL requests on MR-show

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

app/controllers/projects/merge_requests_controller.rb

@@ -46,6 +46,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
         @noteable = @merge_request
         @commits_count = @merge_request.commits_count
         @issuable_sidebar = serializer.represent(@merge_request, serializer: 'sidebar')
+        @current_user_data = UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json
 
         set_pipeline_variables
 

app/serializers/merge_request_noteable_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestNoteableEntity < Grape::Entity
+  include RequestAwareEntity
+
+  # Currently this attr is exposed to be used in app/assets/javascripts/notes/stores/getters.js
+  # in order to determine whether a noteable is an issue or an MR
+  expose :merge_params
+
+  expose :state
+  expose :source_branch
+  expose :target_branch
+  expose :diff_head_sha
+
+  expose :create_note_path do |merge_request|
+    project_notes_path(merge_request.project, target_type: 'merge_request', target_id: merge_request.id)
+  end
+
+  expose :preview_note_path do |merge_request|
+    preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid)
+  end
+
+  expose :supports_suggestion?, as: :can_receive_suggestion
+
+  expose :create_issue_to_resolve_discussions_path do |merge_request|
+    presenter(merge_request).create_issue_to_resolve_discussions_path
+  end
+
+  expose :new_blob_path do |merge_request|
+    if presenter(merge_request).can_push_to_source_branch?
+      project_new_blob_path(merge_request.source_project, merge_request.source_branch)
+    end
+  end
+
+  expose :current_user do
+    expose :can_create_note do |merge_request|
+      can?(current_user, :create_note, merge_request)
+    end
+
+    expose :can_update do |merge_request|
+      can?(current_user, :update_merge_request, merge_request)
+    end
+  end
+
+  private
+
+  delegate :current_user, to: :request
+
+  def presenter(merge_request)
+    @presenters ||= {}
+    @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter
+  end
+end

app/serializers/merge_request_poll_widget_entity.rb

@@ -65,8 +65,6 @@ class MergeRequestPollWidgetEntity < IssuableEntity
     end
   end
 
-  expose :supports_suggestion?, as: :can_receive_suggestion
-
   expose :create_issue_to_resolve_discussions_path do |merge_request|
     presenter(merge_request).create_issue_to_resolve_discussions_path
   end
@@ -84,17 +82,9 @@ class MergeRequestPollWidgetEntity < IssuableEntity
       presenter(merge_request).can_cherry_pick_on_current_merge_request?
     end
 
-    expose :can_create_note do |merge_request|
-      can?(current_user, :create_note, merge_request)
-    end
-
     expose :can_create_issue do |merge_request|
       can?(current_user, :create_issue, merge_request.project)
     end
-
-    expose :can_update do |merge_request|
-      can?(current_user, :update_merge_request, merge_request)
-    end
   end
 
   expose :can_push_to_source_branch do |merge_request|

app/serializers/merge_request_serializer.rb

@@ -13,6 +13,8 @@ class MergeRequestSerializer < BaseSerializer
         MergeRequestSidebarExtrasEntity
       when 'basic'
         MergeRequestBasicEntity
+      when 'noteable'
+        MergeRequestNoteableEntity
       else
         # fallback to widget for old poll requests without `serializer` set
         MergeRequestWidgetEntity

app/serializers/merge_request_widget_entity.rb

@@ -3,10 +3,6 @@
 class MergeRequestWidgetEntity < Grape::Entity
   include RequestAwareEntity
 
-  # Currently this attr is exposed to be used in app/assets/javascripts/notes/stores/getters.js
-  # in order to determine whether a noteable is an issue or an MR
-  expose :merge_params
-
   expose :source_project_full_path do |merge_request|
     merge_request.source_project&.full_path
   end
@@ -35,18 +31,10 @@ class MergeRequestWidgetEntity < Grape::Entity
     cached_widget_project_json_merge_request_path(merge_request.target_project, merge_request, format: :json)
   end
 
-  expose :create_note_path do |merge_request|
-    project_notes_path(merge_request.project, target_type: 'merge_request', target_id: merge_request.id)
-  end
-
   expose :commit_change_content_path do |merge_request|
     commit_change_content_project_merge_request_path(merge_request.project, merge_request)
   end
 
-  expose :preview_note_path do |merge_request|
-    preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid)
-  end
-
   expose :conflicts_docs_path do |merge_request|
     help_page_path('user/project/merge_requests/resolve_conflicts.md')
   end

app/views/projects/merge_requests/show.html.haml

@@ -6,6 +6,7 @@
 - page_description @merge_request.description
 - page_card_attributes @merge_request.card_attributes
 - suggest_changes_help_path = help_page_path('user/discussions/index.md', anchor: 'suggest-changes')
+- number_of_pipelines = @pipelines.size
 
 .merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project), lock_version: @merge_request.lock_version } }
   = render "projects/merge_requests/mr_title"
@@ -41,11 +42,11 @@
               = tab_link_for @merge_request, :commits do
                 = _("Commits")
                 %span.badge.badge-pill= @commits_count
-          - if @pipelines.any?
+          - if number_of_pipelines.nonzero?
             %li.pipelines-tab
               = tab_link_for @merge_request, :pipelines do
                 = _("Pipelines")
-                %span.badge.badge-pill.js-pipelines-mr-count= @pipelines.size
+                %span.badge.badge-pill.js-pipelines-mr-count= number_of_pipelines
           %li.diffs-tab.qa-diffs-tab
             = tab_link_for @merge_request, :diffs do
               = _("Changes")
@@ -63,21 +64,21 @@
             %script.js-notes-data{ type: "application/json" }= initial_notes_data(true).to_json.html_safe
             .issuable-discussion.js-vue-notes-event
               #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request).to_json,
-                noteable_data: serialize_issuable(@merge_request),
+                noteable_data: serialize_issuable(@merge_request, serializer: 'noteable'),
                 noteable_type: 'MergeRequest',
                 target_type: 'merge_request',
                 help_page_path: suggest_changes_help_path,
-                current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json} }
+                current_user_data: @current_user_data} }
 
       #commits.commits.tab-pane
         -# This tab is always loaded via AJAX
       #pipelines.pipelines.tab-pane
-        - if @pipelines.any?
+        - if number_of_pipelines.nonzero?
           = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request)
       #js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?,
         endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
         help_page_path: suggest_changes_help_path,
-        current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json,
+        current_user_data: @current_user_data,
         project_path: project_path(@merge_request.project),
         changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg'),
         is_fluid_layout: fluid_layout.to_s,

changelogs/unreleased/id-optimize-sql-requests-mr-show.yml

+---
+title: Reduce the number of SQL requests on MR-show
+merge_request: 32192
+author:
+type: performance

spec/fixtures/api/schemas/entities/merge_request_noteable.json

+{
+  "type": "object",
+  "properties" : {
+    "merge_params": { "type": ["object", "null"] },
+    "state": { "type": "string" },
+    "source_branch": { "type": "string" },
+    "target_branch": { "type": "string" },
+    "diff_head_sha": { "type": "string" },
+    "create_note_path": { "type": ["string", "null"] },
+    "preview_note_path": { "type": ["string", "null"] },
+    "create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
+    "new_blob_path": { "type": ["string", "null"] },
+    "can_receive_suggestion": { "type": "boolean" },
+    "current_user": {
+      "type": "object",
+      "required": [
+        "can_create_note",
+        "can_update"
+      ],
+      "properties": {
+        "can_create_note": { "type": "boolean" },
+        "can_update": { "type": "boolean" }
+      },
+      "additionalProperties": false
+    }
+  },
+  "additionalProperties": false
+}

spec/fixtures/api/schemas/entities/merge_request_poll_widget.json

@@ -24,22 +24,20 @@
     "ci_status": { "type": ["string", "null"] },
     "cancel_auto_merge_path": { "type": ["string", "null"] },
     "test_reports_path": { "type": ["string", "null"] },
-    "can_receive_suggestion": { "type": "boolean" },
     "create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
     "current_user": {
       "type": "object",
       "required": [
         "can_remove_source_branch",
         "can_revert_on_current_merge_request",
-        "can_cherry_pick_on_current_merge_request"
+        "can_cherry_pick_on_current_merge_request",
+        "can_create_issue"
       ],
       "properties": {
         "can_remove_source_branch": { "type": "boolean" },
         "can_revert_on_current_merge_request": { "type": ["boolean", "null"] },
         "can_cherry_pick_on_current_merge_request": { "type": ["boolean", "null"] },
-        "can_create_note": { "type": "boolean" },
-        "can_create_issue": { "type": "boolean" },
-        "can_update": { "type": "boolean" }
+        "can_create_issue": { "type": "boolean" }
       },
       "additionalProperties": false
     },

spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -5,7 +5,6 @@
     { "$ref": "merge_request_poll_widget.json" },
     {
       "properties" : {
-        "merge_params": { "type": ["object", "null"] },
         "source_project_full_path": { "type": ["string", "null"]},
         "target_project_full_path": { "type": ["string", "null"]},
         "email_patches_path": { "type": "string" },
@@ -13,9 +12,7 @@
         "merge_request_basic_path": { "type": "string" },
         "merge_request_widget_path": { "type": "string" },
         "merge_request_cached_widget_path": { "type": "string" },
-        "create_note_path": { "type": ["string", "null"] },
         "commit_change_content_path": { "type": "string" },
-        "preview_note_path": { "type": ["string", "null"] },
         "conflicts_docs_path": { "type": ["string", "null"] },
         "merge_request_pipelines_docs_path": { "type": ["string", "null"] },
         "ci_environments_status_path": { "type": "string" },

spec/serializers/merge_request_serializer_spec.rb

@@ -41,6 +41,14 @@ describe MergeRequestSerializer do
     end
   end
 
+  context 'noteable merge request serialization' do
+    let(:serializer) { 'noteable' }
+
+    it 'matches noteable merge request json schema' do
+      expect(json_entity).to match_schema('entities/merge_request_noteable', strict: true)
+    end
+  end
+
   context 'no serializer' do
     let(:serializer) { nil }