Merge branch 'id-mr-widget-etag-caching' into 'master'

Split MR widget into etag-cached and non-cached serializers

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

app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -166,6 +166,7 @@ export default {
         ciEnvironmentsStatusPath: store.ciEnvironmentsStatusPath,
         mergeRequestBasicPath: store.mergeRequestBasicPath,
         mergeRequestWidgetPath: store.mergeRequestWidgetPath,
+        mergeRequestCachedWidgetPath: store.mergeRequestCachedWidgetPath,
         mergeActionsContentPath: store.mergeActionsContentPath,
         rebasePath: store.rebasePath,
       };
@@ -176,8 +177,7 @@ export default {
     checkStatus(cb, isRebased) {
       return this.service
         .checkStatus()
-        .then(res => res.data)
-        .then(data => {
+        .then(({ data }) => {
           this.handleNotification(data);
           this.mr.setData(data, isRebased);
           this.setFaviconHelper();

app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js

@@ -34,7 +34,16 @@ export default class MRWidgetService {
   }
 
   checkStatus() {
-    return axios.get(this.endpoints.mergeRequestWidgetPath);
+    // two endpoints are requested in order to get MR info:
+    // one which is etag-cached and invalidated and another one which is not cached
+    // the idea is to move all the fields to etag-cached endpoint and then perform only one request
+    // https://gitlab.com/gitlab-org/gitlab-ce/issues/61559#note_188801390
+    const getData = axios.get(this.endpoints.mergeRequestWidgetPath);
+    const getCachedData = axios.get(this.endpoints.mergeRequestCachedWidgetPath);
+
+    return axios
+      .all([getData, getCachedData])
+      .then(axios.spread((res, cachedRes) => ({ data: Object.assign(res.data, cachedRes.data) })));
   }
 
   fetchMergeActionsContent() {

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -10,6 +10,8 @@ export default class MergeRequestStore {
     this.sha = data.diff_head_sha;
     this.gitlabLogo = data.gitlabLogo;
 
+    this.setPaths(data);
+
     this.setData(data);
   }
 
@@ -18,13 +20,9 @@ export default class MergeRequestStore {
       this.sha = data.diff_head_sha;
     }
 
-    const currentUser = data.current_user;
     const pipelineStatus = data.pipeline ? data.pipeline.details.status : null;
 
     this.squash = data.squash;
-    this.squashBeforeMergeHelpPath =
-      this.squashBeforeMergeHelpPath || data.squash_before_merge_help_path;
-    this.troubleshootingDocsPath = this.troubleshootingDocsPath || data.troubleshooting_docs_path;
     this.enableSquashBeforeMerge = this.enableSquashBeforeMerge || true;
 
     this.iid = data.iid;
@@ -33,8 +31,6 @@ export default class MergeRequestStore {
     this.targetBranchSha = data.target_branch_sha;
     this.sourceBranch = data.source_branch;
     this.sourceBranchProtected = data.source_branch_protected;
-    this.conflictsDocsPath = data.conflicts_docs_path;
-    this.mergeRequestPipelinesHelpPath = data.merge_request_pipelines_docs_path;
     this.mergeStatus = data.merge_status;
     this.commitMessage = data.default_merge_commit_message;
     this.shortMergeCommitSha = data.short_merge_commit_sha;
@@ -48,7 +44,7 @@ export default class MergeRequestStore {
     this.postMergeDeployments = this.postMergeDeployments || [];
     this.commits = data.commits_without_merge_commits || [];
     this.squashCommitMessage = data.default_squash_commit_message;
-    this.initRebase(data);
+    this.rebaseInProgress = data.rebase_in_progress;
 
     if (data.issues_links) {
       const links = data.issues_links;
@@ -66,14 +62,7 @@ export default class MergeRequestStore {
     this.setToAutoMergeBy = MergeRequestStore.formatUserObject(data.merge_user || {});
     this.mergeUserId = data.merge_user_id;
     this.currentUserId = gon.current_user_id;
-    this.sourceBranchPath = data.source_branch_path;
-    this.sourceBranchLink = data.source_branch_with_namespace_link;
     this.mergeError = data.merge_error;
-    this.targetBranchPath = data.target_branch_commits_path;
-    this.targetBranchTreePath = data.target_branch_tree_path;
-    this.conflictResolutionPath = data.conflict_resolution_path;
-    this.cancelAutoMergePath = data.cancel_auto_merge_path;
-    this.removeWIPPath = data.remove_wip_path;
     this.sourceBranchRemoved = !data.source_branch_exists;
     this.shouldRemoveSourceBranch = data.remove_source_branch || false;
     this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false;
@@ -83,46 +72,23 @@ export default class MergeRequestStore {
     this.preferredAutoMergeStrategy = MergeRequestStore.getPreferredAutoMergeStrategy(
       this.availableAutoMergeStrategies,
     );
-    this.mergePath = data.merge_path;
     this.ffOnlyEnabled = data.ff_only_enabled;
     this.shouldBeRebased = Boolean(data.should_be_rebased);
-    this.mergeRequestBasicPath = data.merge_request_basic_path;
-    this.mergeRequestWidgetPath = data.merge_request_widget_path;
-    this.emailPatchesPath = data.email_patches_path;
-    this.plainDiffPath = data.plain_diff_path;
-    this.newBlobPath = data.new_blob_path;
-    this.createIssueToResolveDiscussionsPath = data.create_issue_to_resolve_discussions_path;
-    this.mergeCheckPath = data.merge_check_path;
-    this.mergeActionsContentPath = data.commit_change_content_path;
-    this.mergeCommitPath = data.merge_commit_path;
     this.isRemovingSourceBranch = this.isRemovingSourceBranch || false;
     this.isOpen = data.state === 'opened';
     this.hasMergeableDiscussionsState = data.mergeable_discussions_state === false;
-    this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
-    this.canMerge = Boolean(data.merge_path);
-    this.canCreateIssue = currentUser.can_create_issue || false;
-    this.canCancelAutomaticMerge = Boolean(data.cancel_auto_merge_path);
     this.isSHAMismatch = this.sha !== data.diff_head_sha;
     this.canBeMerged = data.can_be_merged || false;
     this.isMergeAllowed = data.mergeable || false;
     this.mergeOngoing = data.merge_ongoing;
     this.allowCollaboration = data.allow_collaboration;
-    this.targetProjectFullPath = data.target_project_full_path;
-    this.sourceProjectFullPath = data.source_project_full_path;
     this.sourceProjectId = data.source_project_id;
     this.targetProjectId = data.target_project_id;
     this.mergePipelinesEnabled = Boolean(data.merge_pipelines_enabled);
     this.mergeTrainsCount = data.merge_trains_count || 0;
     this.mergeTrainIndex = data.merge_train_index;
 
-    // Cherry-pick and Revert actions related
-    this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false;
-    this.canRevertInCurrentMR = currentUser.can_revert_on_current_merge_request || false;
-    this.cherryPickInForkPath = currentUser.cherry_pick_in_fork_path;
-    this.revertInForkPath = currentUser.revert_in_fork_path;
-
     // CI related
-    this.ciEnvironmentsStatusPath = data.ci_environments_status_path;
     this.hasCI = data.has_ci;
     this.ciStatus = data.ci_status;
     this.isPipelineFailed = this.ciStatus === 'failed' || this.ciStatus === 'canceled';
@@ -133,8 +99,33 @@ export default class MergeRequestStore {
     this.isPipelineActive = data.pipeline ? data.pipeline.active : false;
     this.isPipelineBlocked = pipelineStatus ? pipelineStatus.group === 'manual' : false;
     this.ciStatusFaviconPath = pipelineStatus ? pipelineStatus.favicon : null;
-
     this.testResultsPath = data.test_reports_path;
+    this.cancelAutoMergePath = data.cancel_auto_merge_path;
+    this.canCancelAutomaticMerge = Boolean(data.cancel_auto_merge_path);
+
+    this.newBlobPath = data.new_blob_path;
+    this.sourceBranchPath = data.source_branch_path;
+    this.sourceBranchLink = data.source_branch_with_namespace_link;
+    this.rebasePath = data.rebase_path;
+    this.targetBranchPath = data.target_branch_commits_path;
+    this.targetBranchTreePath = data.target_branch_tree_path;
+    this.conflictResolutionPath = data.conflict_resolution_path;
+    this.removeWIPPath = data.remove_wip_path;
+    this.createIssueToResolveDiscussionsPath = data.create_issue_to_resolve_discussions_path;
+    this.mergePath = data.merge_path;
+    this.canMerge = Boolean(data.merge_path);
+    this.mergeCommitPath = data.merge_commit_path;
+    this.canPushToSourceBranch = data.can_push_to_source_branch;
+
+    const currentUser = data.current_user;
+
+    this.cherryPickInForkPath = currentUser.cherry_pick_in_fork_path;
+    this.revertInForkPath = currentUser.revert_in_fork_path;
+
+    this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
+    this.canCreateIssue = currentUser.can_create_issue || false;
+    this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false;
+    this.canRevertInCurrentMR = currentUser.can_revert_on_current_merge_request || false;
 
     this.setState(data);
   }
@@ -161,6 +152,24 @@ export default class MergeRequestStore {
     }
   }
 
+  setPaths(data) {
+    // Paths are set on the first load of the page and not auto-refreshed
+    this.squashBeforeMergeHelpPath = data.squash_before_merge_help_path;
+    this.troubleshootingDocsPath = data.troubleshooting_docs_path;
+    this.mergeRequestBasicPath = data.merge_request_basic_path;
+    this.mergeRequestWidgetPath = data.merge_request_widget_path;
+    this.mergeRequestCachedWidgetPath = data.merge_request_cached_widget_path;
+    this.emailPatchesPath = data.email_patches_path;
+    this.plainDiffPath = data.plain_diff_path;
+    this.mergeCheckPath = data.merge_check_path;
+    this.mergeActionsContentPath = data.commit_change_content_path;
+    this.targetProjectFullPath = data.target_project_full_path;
+    this.sourceProjectFullPath = data.source_project_full_path;
+    this.mergeRequestPipelinesHelpPath = data.merge_request_pipelines_docs_path;
+    this.conflictsDocsPath = data.conflicts_docs_path;
+    this.ciEnvironmentsStatusPath = data.ci_environments_status_path;
+  }
+
   get isNothingToMergeState() {
     return this.state === stateKey.nothingToMerge;
   }
@@ -169,13 +178,6 @@ export default class MergeRequestStore {
     return this.state === stateKey.merged;
   }
 
-  initRebase(data) {
-    this.canPushToSourceBranch = data.can_push_to_source_branch;
-    this.rebaseInProgress = data.rebase_in_progress;
-    this.approvalsLeft = !data.approved;
-    this.rebasePath = data.rebase_path;
-  }
-
   static buildMetrics(metrics) {
     if (!metrics) {
       return {};

app/controllers/projects/merge_requests/content_controller.rb

@@ -7,16 +7,33 @@ class Projects::MergeRequests::ContentController < Projects::MergeRequests::Appl
   # for other types of serialization
 
   before_action :close_merge_request_if_no_source_project
+  before_action :set_polling_header
   around_action :allow_gitaly_ref_name_caching
 
   def widget
     respond_to do |format|
       format.json do
-        Gitlab::PollingInterval.set_header(response, interval: 10_000)
+        render json: serializer(MergeRequestPollWidgetEntity)
+      end
+    end
+  end
 
-        serializer = MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
-        render json: serializer.represent(merge_request, serializer: 'widget')
+  def cached_widget
+    respond_to do |format|
+      format.json do
+        render json: serializer(MergeRequestPollCachedWidgetEntity)
       end
     end
   end
+
+  private
+
+  def set_polling_header
+    Gitlab::PollingInterval.set_header(response, interval: 10_000)
+  end
+
+  def serializer(entity)
+    serializer = MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
+    serializer.represent(merge_request, {}, entity)
+  end
 end

app/models/issue.rb

@@ -64,7 +64,7 @@ class Issue < ApplicationRecord
   scope :public_only, -> { where(confidential: false) }
   scope :confidential_only, -> { where(confidential: true) }
 
-  after_save :expire_etag_cache
+  after_commit :expire_etag_cache
   after_save :ensure_metrics, unless: :imported?
 
   attr_spammable :title, spam_title: true

app/models/merge_request.rb

@@ -73,6 +73,7 @@ class MergeRequest < ApplicationRecord
   after_update :clear_memoized_shas
   after_update :reload_diff_if_branch_changed
   after_save :ensure_metrics
+  after_commit :expire_etag_cache
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
@@ -389,6 +390,10 @@ class MergeRequest < ApplicationRecord
   def merge_async(user_id, params)
     jid = MergeWorker.perform_async(id, user_id, params.to_h)
     update_column(:merge_jid, jid)
+
+    # merge_ongoing? depends on merge_jid
+    # expire etag cache since the attribute is changed without triggering callbacks
+    expire_etag_cache
   end
 
   # Set off a rebase asynchronously, atomically updating the `rebase_jid` of
@@ -409,6 +414,10 @@ class MergeRequest < ApplicationRecord
 
       update_column(:rebase_jid, jid)
     end
+
+    # rebase_in_progress? depends on rebase_jid
+    # expire etag cache since the attribute is changed without triggering callbacks
+    expire_etag_cache
   end
 
   def merge_participants
@@ -1429,4 +1438,11 @@ class MergeRequest < ApplicationRecord
       variables.append(key: 'CI_MERGE_REQUEST_SOURCE_BRANCH_NAME', value: source_branch.to_s)
     end
   end
+
+  def expire_etag_cache
+    return unless project.namespace
+
+    key = Gitlab::Routing.url_helpers.cached_widget_project_json_merge_request_path(project, self, format: :json)
+    Gitlab::EtagCaching::Store.new.touch(key)
+  end
 end

app/presenters/merge_request_presenter.rb

@@ -208,14 +208,6 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     merge_request.subscribed?(current_user, merge_request.target_project)
   end
 
-  def conflicts_docs_path
-    help_page_path('user/project/merge_requests/resolve_conflicts.md')
-  end
-
-  def merge_request_pipelines_docs_path
-    help_page_path('ci/merge_request_pipelines/index.md')
-  end
-
   def source_branch_link
     if source_branch_exists?
       link_to(source_branch, source_branch_commits_path, class: 'ref-name')

app/serializers/merge_request_poll_cached_widget_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestPollCachedWidgetEntity < IssuableEntity
+  expose :auto_merge_enabled
+  expose :state
+  expose :merge_commit_sha
+  expose :short_merge_commit_sha
+  expose :merge_error
+  expose :merge_status
+  expose :merge_user_id
+  expose :source_branch
+  expose :source_project_id
+  expose :target_branch
+  expose :target_branch_sha
+  expose :target_project_id
+  expose :squash
+  expose :rebase_in_progress?, as: :rebase_in_progress
+  expose :default_squash_commit_message
+  expose :commits_count
+  expose :merge_ongoing?, as: :merge_ongoing
+  expose :work_in_progress?, as: :work_in_progress
+  expose :cannot_be_merged?, as: :has_conflicts
+  expose :can_be_merged?, as: :can_be_merged
+  expose :remove_source_branch?, as: :remove_source_branch
+  expose :source_branch_exists?, as: :source_branch_exists
+  expose :branch_missing?, as: :branch_missing
+
+  expose :commits_without_merge_commits, using: MergeRequestWidgetCommitEntity do |merge_request|
+    merge_request.commits.without_merge_commits
+  end
+  expose :diff_head_sha do |merge_request|
+    merge_request.diff_head_sha.presence
+  end
+  expose :metrics do |merge_request|
+    metrics = build_metrics(merge_request)
+
+    MergeRequestMetricsEntity.new(metrics).as_json
+  end
+
+  expose :diverged_commits_count do |merge_request|
+    if merge_request.open? && merge_request.diverged_from_target_branch?
+      merge_request.diverged_commits_count
+    else
+      0
+    end
+  end
+
+  # Paths
+  #
+  expose :target_branch_commits_path do |merge_request|
+    presenter(merge_request).target_branch_commits_path
+  end
+
+  expose :target_branch_tree_path do |merge_request|
+    presenter(merge_request).target_branch_tree_path
+  end
+
+  expose :merge_commit_path do |merge_request|
+    if merge_request.merge_commit_sha
+      project_commit_path(merge_request.project, merge_request.merge_commit_sha)
+    end
+  end
+
+  expose :source_branch_path do |merge_request|
+    presenter(merge_request).source_branch_path
+  end
+
+  expose :source_branch_with_namespace_link do |merge_request|
+    presenter(merge_request).source_branch_with_namespace_link
+  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
+
+  # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs,
+  # we can remove this method and just serialize MergeRequest#metrics
+  # instead. See https://gitlab.com/gitlab-org/gitlab-ce/issues/41587
+  def build_metrics(merge_request)
+    # There's no need to query and serialize metrics data for merge requests that are not
+    # merged or closed.
+    return unless merge_request.merged? || merge_request.closed?
+    return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id
+    return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id
+
+    build_metrics_from_events(merge_request)
+  end
+
+  def build_metrics_from_events(merge_request)
+    closed_event = merge_request.closed_event
+    merge_event = merge_request.merge_event
+
+    MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
+                              latest_closed_by: closed_event&.author,
+                              merged_at: merge_event&.updated_at,
+                              merged_by: merge_event&.author)
+  end
+end

app/serializers/merge_request_poll_widget_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestPollWidgetEntity < IssuableEntity
+  expose :auto_merge_strategy
+  expose :available_auto_merge_strategies do |merge_request|
+    AutoMergeService.new(merge_request.project, current_user).available_strategies(merge_request) # rubocop: disable CodeReuse/ServiceClass
+  end
+  expose :source_branch_protected do |merge_request|
+    merge_request.source_project.present? && ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch)
+  end
+  expose :allow_collaboration
+  expose :should_be_rebased?, as: :should_be_rebased
+  expose :ff_only_enabled do |merge_request|
+    merge_request.project.merge_requests_ff_only_enabled
+  end
+
+  # User entities
+  expose :merge_user, using: UserEntity
+
+  expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? }
+
+  expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)}
+
+  expose :default_merge_commit_message
+
+  expose :mergeable?, as: :mergeable
+
+  expose :default_merge_commit_message_with_description do |merge_request|
+    merge_request.default_merge_commit_message(include_description: true)
+  end
+
+  # Booleans
+  expose :mergeable_discussions_state?, as: :mergeable_discussions_state do |merge_request|
+    # This avoids calling MergeRequest#mergeable_discussions_state without
+    # considering the state of the MR first. If a MR isn't mergeable, we can
+    # safely short-circuit it.
+    if merge_request.mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
+      merge_request.mergeable_discussions_state?
+    else
+      false
+    end
+  end
+
+  expose :project_archived do |merge_request|
+    merge_request.project.archived?
+  end
+
+  expose :only_allow_merge_if_pipeline_succeeds do |merge_request|
+    merge_request.project.only_allow_merge_if_pipeline_succeeds?
+  end
+
+  # CI related
+  expose :has_ci?, as: :has_ci
+  expose :ci_status do |merge_request|
+    presenter(merge_request).ci_status
+  end
+
+  expose :cancel_auto_merge_path do |merge_request|
+    presenter(merge_request).cancel_auto_merge_path
+  end
+
+  expose :test_reports_path do |merge_request|
+    if merge_request.has_test_reports?
+      test_reports_project_merge_request_path(merge_request.project, merge_request, format: :json)
+    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
+
+  expose :current_user do
+    expose :can_remove_source_branch do |merge_request|
+      presenter(merge_request).can_remove_source_branch?
+    end
+
+    expose :can_revert_on_current_merge_request do |merge_request|
+      presenter(merge_request).can_revert_on_current_merge_request?
+    end
+
+    expose :can_cherry_pick_on_current_merge_request do |merge_request|
+      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|
+    presenter(merge_request).can_push_to_source_branch?
+  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 :rebase_path do |merge_request|
+    presenter(merge_request).rebase_path
+  end
+
+  expose :conflict_resolution_path do |merge_request|
+    presenter(merge_request).conflict_resolution_path
+  end
+
+  expose :remove_wip_path do |merge_request|
+    presenter(merge_request).remove_wip_path
+  end
+
+  expose :merge_path do |merge_request|
+    presenter(merge_request).merge_path
+  end
+
+  expose :cherry_pick_in_fork_path do |merge_request|
+    presenter(merge_request).cherry_pick_in_fork_path
+  end
+
+  expose :revert_in_fork_path do |merge_request|
+    presenter(merge_request).revert_in_fork_path
+  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_serializer.rb

@@ -4,8 +4,8 @@ class MergeRequestSerializer < BaseSerializer
   # This overrided method takes care of which entity should be used
   # to serialize the `merge_request` based on `serializer` key in `opts` param.
   # Hence, `entity` doesn't need to be declared on the class scope.
-  def represent(merge_request, opts = {})
-    entity =
+  def represent(merge_request, opts = {}, entity = nil)
+    entity ||=
       case opts[:serializer]
       when 'sidebar'
         IssuableSidebarBasicEntity

changelogs/unreleased/id-mr-widget-etag-caching.yml

+---
+title: Split MR widget into etag-cached and non-cached serializers
+merge_request: 31354
+author:
+type: performance

config/routes/project.rb

@@ -267,6 +267,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
             get :pipelines
             get :diffs, to: 'merge_requests/diffs#show'
             get :widget, to: 'merge_requests/content#widget'
+            get :cached_widget, to: 'merge_requests/content#cached_widget'
           end
 
           get :diff_for_path, controller: 'merge_requests/diffs'

lib/gitlab/etag_caching/router.rb

@@ -61,6 +61,10 @@ module Gitlab
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/import/gitea/realtime_changes\.json\z),
           'realtime_changes_import_gitea'
+        ),
+        Gitlab::EtagCaching::Router::Route.new(
+          %r(#{RESERVED_WORDS_PREFIX}/merge_requests/\d+/cached_widget\.json\z),
+          'merge_request_widget'
         )
       ].freeze
 

spec/controllers/projects/merge_requests/content_controller_spec.rb

@@ -11,8 +11,8 @@ describe Projects::MergeRequests::ContentController do
     sign_in(user)
   end
 
-  def do_request
-    get :widget, params: {
+  def do_request(action = :cached_widget)
+    get action, params: {
       namespace_id: project.namespace.to_param,
       project_id: project,
       id: merge_request.iid,
@@ -20,41 +20,65 @@ describe Projects::MergeRequests::ContentController do
     }
   end
 
-  describe 'GET widget' do
-    context 'user has access to the project' do
-      before do
-        expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
+  context 'user has access to the project' do
+    before do
+      expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
 
-        project.add_maintainer(user)
-      end
+      project.add_maintainer(user)
+    end
 
+    describe 'GET cached_widget' do
       it 'renders widget MR entity as json' do
         do_request
 
-        expect(response).to match_response_schema('entities/merge_request_widget')
+        expect(response).to match_response_schema('entities/merge_request_poll_cached_widget')
       end
 
+      it 'closes an MR with moved source project' do
+        merge_request.update_column(:source_project_id, nil)
+
+        expect { do_request }.to change { merge_request.reload.open? }.from(true).to(false)
+      end
+    end
+
+    describe 'GET widget' do
       it 'checks whether the MR can be merged' do
         controller.instance_variable_set(:@merge_request, merge_request)
 
         expect(merge_request).to receive(:check_mergeability)
 
-        do_request
+        do_request(:widget)
       end
 
-      it 'closes an MR with moved source project' do
-        merge_request.update_column(:source_project_id, nil)
+      context 'merged merge request' do
+        let(:merge_request) do
+          create(:merged_merge_request, :with_test_reports, target_project: project, source_project: project)
+        end
 
-        expect { do_request }.to change { merge_request.reload.open? }.from(true).to(false)
+        it 'renders widget MR entity as json' do
+          do_request(:widget)
+
+          expect(response).to match_response_schema('entities/merge_request_poll_widget')
+        end
       end
     end
+  end
 
-    context 'user does not have access to the project' do
-      it 'renders widget MR entity as json' do
+  context 'user does not have access to the project' do
+    describe 'GET cached_widget' do
+      it 'returns 404' do
         do_request
 
         expect(response).to have_http_status(:not_found)
       end
     end
+
+    describe 'GET widget' do
+      it 'returns 404' do
+        do_request(:widget)
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
   end
 end

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

+{
+  "type": "object",
+  "properties" : {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "description": { "type": ["string", "null"] },
+    "title": { "type": "string" },
+    "auto_merge_enabled": { "type": "boolean" },
+    "state": { "type": "string" },
+    "merge_commit_sha": { "type": ["string", "null"] },
+    "short_merge_commit_sha": { "type": ["string", "null"] },
+    "merge_error": { "type": ["string", "null"] },
+    "merge_status": { "type": "string" },
+    "merge_user_id": { "type": ["integer", "null"] },
+    "source_branch": { "type": "string" },
+    "source_project_id": { "type": "integer" },
+    "target_branch": { "type": "string" },
+    "target_branch_sha": { "type": "string" },
+    "target_project_id": { "type": "integer" },
+    "squash": { "type": "boolean" },
+    "rebase_in_progress": { "type": "boolean" },
+    "default_squash_commit_message": { "type": "string" },
+    "commits_count": { "type": ["integer", "null"] },
+    "merge_ongoing": { "type": "boolean" },
+    "work_in_progress": { "type": "boolean" },
+    "has_conflicts": { "type": "boolean" },
+    "can_be_merged": { "type": "boolean" },
+    "remove_source_branch": { "type": ["boolean", "null"] },
+    "source_branch_exists": { "type": "boolean" },
+    "branch_missing": { "type": "boolean" },
+    "commits_without_merge_commits": { "type": "array" },
+    "diff_head_sha": { "type": ["string", "null"] },
+    "metrics": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "merge_request_metrics.json" }
+      ]
+    },
+    "diverged_commits_count": { "type": "integer" },
+    "target_branch_commits_path": { "type": "string" },
+    "target_branch_tree_path": { "type": "string" },
+    "merge_commit_path": { "type": ["string", "null"] },
+    "source_branch_with_namespace_link": { "type": "string" },
+    "source_branch_path": { "type": "string" }
+  }
+}

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

+{
+  "type": "object",
+  "properties" : {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "description": { "type": ["string", "null"] },
+    "title": { "type": "string" },
+    "auto_merge_strategy": { "type": ["string", "null"] },
+    "available_auto_merge_strategies": { "type": "array" },
+    "source_branch_protected": { "type": "boolean" },
+    "allow_collaboration": { "type": "boolean"},
+    "should_be_rebased": { "type": "boolean" },
+    "ff_only_enabled": { "type": ["boolean", false] },
+    "merge_user": { "type": ["object", "null"] },
+    "pipeline": { "type": ["object", "null"] },
+    "merge_pipeline": { "type": ["object", "null"] },
+    "default_merge_commit_message": { "type": ["string", "null"] },
+    "mergeable": { "type": "boolean" },
+    "default_merge_commit_message_with_description": { "type": "string" },
+    "mergeable_discussions_state": { "type": "boolean" },
+    "project_archived": { "type": "boolean" },
+    "only_allow_merge_if_pipeline_succeeds": { "type": "boolean" },
+    "has_ci": { "type": "boolean" },
+    "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"
+      ],
+      "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" }
+      },
+      "additionalProperties": false
+    },
+    "can_push_to_source_branch": { "type": "boolean" },
+    "new_blob_path": { "type": ["string", "null"] },
+    "rebase_path": { "type": ["string", "null"] },
+    "conflict_resolution_path": { "type": ["string", "null"] },
+    "remove_wip_path": { "type": ["string", "null"] },
+    "merge_path": { "type": ["string", "null"] },
+    "cherry_pick_in_fork_path": { "type": ["string", "null"] },
+    "revert_in_fork_path": { "type": ["string", "null"] }
+  }
+}

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

 {
   "type": "object",
-  "properties" : {
-    "id": { "type": "integer" },
-    "iid": { "type": "integer" },
-    "author_id": { "type": "integer" },
-    "description": { "type": ["string", "null"] },
-    "lock_version": { "type": ["string", "null"] },
-    "milestone_id": { "type": ["string", "null"] },
-    "position": { "type": "integer" },
-    "state": { "type": "string" },
-    "title": { "type": "string" },
-    "updated_by_id": { "type": ["string", "null"] },
-    "created_at": { "type": "string" },
-    "updated_at": { "type": "string" },
-    "time_estimate": { "type": "integer" },
-    "total_time_spent": { "type": "integer" },
-    "human_time_estimate": { "type": ["integer", "null"] },
-    "human_total_time_spent": { "type": ["integer", "null"] },
-    "milestone": { "type": ["object", "null"] },
-    "labels": { "type": ["array", "null"] },
-    "in_progress_merge_commit_sha": { "type": ["string", "null"] },
-    "merge_error": { "type": ["string", "null"] },
-    "merge_commit_sha": { "type": ["string", "null"] },
-    "short_merge_commit_sha": { "type": ["string", "null"] },
-    "merge_params": { "type": ["object", "null"] },
-    "merge_status": { "type": "string" },
-    "merge_user_id": { "type": ["integer", "null"] },
-    "merge_when_pipeline_succeeds": { "type": "boolean" },
-    "source_branch": { "type": "string" },
-    "source_project_id": { "type": "integer" },
-    "source_project_full_path": { "type": ["string", "null"]},
-    "target_branch": { "type": "string" },
-    "target_project_id": { "type": "integer" },
-    "target_project_full_path": { "type": ["string", "null"]},
-    "allow_collaboration": { "type": "boolean"},
-    "metrics": {
-      "oneOf": [
-        { "type": "null" },
-        { "$ref": "merge_request_metrics.json" }
-      ]
-    },
-    "author": { "type": ["object", "null"] },
-    "merge_user": { "type": ["object", "null"] },
-    "diff_head_sha": { "type": ["string", "null"] },
-    "diff_head_commit_short_id": { "type": ["string", "null"] },
-    "default_merge_commit_message": { "type": ["string", "null"] },
-    "pipeline": { "type": ["object", "null"] },
-    "merge_pipeline": { "type": ["object", "null"] },
-    "work_in_progress": { "type": "boolean" },
-    "source_branch_exists": { "type": "boolean" },
-    "mergeable_discussions_state": { "type": "boolean" },
-    "conflicts_can_be_resolved_in_ui": { "type": "boolean" },
-    "branch_missing": { "type": "boolean" },
-    "commits_count": { "type": ["integer", "null"] },
-    "has_conflicts": { "type": "boolean" },
-    "can_be_merged": { "type": "boolean" },
-    "mergeable": { "type": "boolean" },
-    "project_archived": { "type": "boolean" },
-    "only_allow_merge_if_pipeline_succeeds": { "type": "boolean" },
-    "has_ci": { "type": "boolean" },
-    "ci_status": { "type": ["string", "null"] },
-    "issues_links": {
-      "type": "object",
-      "required": ["closing", "mentioned_but_not_closing", "assign_to_closing"],
+  "allOf": [
+    { "$ref": "merge_request_poll_cached_widget.json" },
+    { "$ref": "merge_request_poll_widget.json" },
+    {
       "properties" : {
-        "closing": { "type": "string" },
-        "mentioned_but_not_closing": { "type": "string" },
-        "assign_to_closing": { "type": ["string", "null"] }
-      },
-      "additionalProperties": false
-    },
-    "source_branch_with_namespace_link": { "type": "string" },
-    "current_user": {
-      "type": "object",
-      "required": [
-        "can_remove_source_branch",
-        "can_revert_on_current_merge_request",
-        "can_cherry_pick_on_current_merge_request"
-      ],
-      "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" }
-      },
-      "additionalProperties": false
-    },
-    "target_branch_commits_path": { "type": "string" },
-    "target_branch_tree_path": { "type": "string" },
-    "source_branch_path": { "type": "string" },
-    "conflict_resolution_path": { "type": ["string", "null"] },
-    "cancel_merge_when_pipeline_succeeds_path": { "type": ["string", "null"] },
-    "create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
-    "merge_path": { "type": ["string", "null"] },
-    "cherry_pick_in_fork_path": { "type": ["string", "null"] },
-    "revert_in_fork_path": { "type": ["string", "null"] },
-    "email_patches_path": { "type": "string" },
-    "plain_diff_path": { "type": "string" },
-    "merge_request_basic_path": { "type": "string" },
-    "merge_request_widget_path": { "type": "string" },
-    "new_blob_path": { "type": ["string", "null"] },
-    "merge_check_path": { "type": "string" },
-    "ci_environments_status_path": { "type": "string" },
-    "default_merge_commit_message_with_description": { "type": "string" },
-    "default_squash_commit_message": { "type": "string" },
-    "commits_without_merge_commits": { "type": "array" },
-    "diverged_commits_count": { "type": "integer" },
-    "commit_change_content_path": { "type": "string" },
-    "merge_commit_path": { "type": ["string", "null"] },
-    "remove_wip_path": { "type": ["string", "null"] },
-    "commits_count": { "type": "integer" },
-    "remove_source_branch": { "type": ["boolean", "null"] },
-    "merge_ongoing": { "type": "boolean" },
-    "ff_only_enabled": { "type": ["boolean", false] },
-    "should_be_rebased": { "type": "boolean" },
-    "create_note_path": { "type": ["string", "null"] },
-    "preview_note_path": { "type": ["string", "null"] },
-    "rebase_commit_sha": { "type": ["string", "null"] },
-    "rebase_in_progress": { "type": "boolean" },
-    "can_push_to_source_branch": { "type": "boolean" },
-    "rebase_path": { "type": ["string", "null"] },
-    "squash": { "type": "boolean" },
-    "test_reports_path": { "type": ["string", "null"] },
-    "can_receive_suggestion": { "type": "boolean" },
-    "source_branch_protected": { "type": "boolean" },
-    "conflicts_docs_path": { "type": ["string", "null"] },
-    "merge_request_pipelines_docs_path": { "type": ["string", "null"] }
-  }
+        "merge_params": { "type": ["object", "null"] },
+        "source_project_full_path": { "type": ["string", "null"]},
+        "target_project_full_path": { "type": ["string", "null"]},
+        "email_patches_path": { "type": "string" },
+        "plain_diff_path": { "type": "string" },
+        "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" },
+        "issues_links": {
+          "type": "object",
+          "required": ["closing", "mentioned_but_not_closing", "assign_to_closing"],
+          "properties" : {
+            "closing": { "type": "string" },
+            "mentioned_but_not_closing": { "type": "string" },
+            "assign_to_closing": { "type": ["string", "null"] }
+          },
+          "additionalProperties": false
+        }
+      }
+    }
+  ]
 }

spec/models/merge_request_spec.rb

@@ -1988,6 +1988,7 @@ describe MergeRequest do
       params = {}
       merge_jid = 'hash-123'
 
+      expect(merge_request).to receive(:expire_etag_cache)
       expect(MergeWorker).to receive(:perform_async).with(merge_request.id, user_id, params) do
         merge_jid
       end
@@ -2011,6 +2012,7 @@ describe MergeRequest do
         .with(merge_request.id, user_id)
         .and_return(rebase_jid)
 
+      expect(merge_request).to receive(:expire_etag_cache)
       expect(merge_request).to receive(:lock!).and_call_original
 
       execute