From 4aeb8a02c506eaa8f0710ee17edd9e35dd68d280 Mon Sep 17 00:00:00 2001
From: GitLab Bot <gitlab-bot@gitlab.com>
Date: Mon, 23 Dec 2019 12:08:18 +0000
Subject: Add latest changes from gitlab-org/gitlab@master

---
 .gitignore                                          |  1 +
 .../javascripts/boards/components/modal/list.vue    |  6 +++---
 .../commit/pipelines/pipelines_table.vue            |  2 +-
 app/assets/javascripts/fly_out_nav.js               |  7 ++-----
 app/assets/javascripts/lib/utils/common_utils.js    |  2 +-
 .../javascripts/pages/projects/wikis/wikis.js       |  4 ++--
 app/controllers/projects/raw_controller.rb          | 12 ++++++++----
 app/graphql/gitlab_schema.rb                        |  9 ++++++++-
 app/graphql/resolvers/base_resolver.rb              | 14 +++++++++++++-
 ...ns-api-doesn-t-reject-an-error-input-in-some.yml |  6 ++++++
 .../sh-skip-findcommit-lookup-rate-limit.yml        |  5 +++++
 locale/gitlab.pot                                   |  3 +++
 spec/controllers/projects/raw_controller_spec.rb    | 13 ++++++++-----
 .../raw/user_interacts_with_raw_endpoint_spec.rb    |  2 --
 spec/javascripts/fly_out_nav_spec.js                |  4 ++--
 spec/javascripts/lib/utils/common_utils_spec.js     |  2 +-
 spec/models/diff_note_spec.rb                       |  8 ++++----
 spec/requests/api/discussions_spec.rb               | 12 ++++++++++++
 spec/support/matchers/graphql_matchers.rb           |  2 +-
 .../requests/api/diff_discussions.rb                | 21 ++++++++++++++++-----
 20 files changed, 97 insertions(+), 38 deletions(-)
 create mode 100644 changelogs/unreleased/27946-merge-request-discussions-api-doesn-t-reject-an-error-input-in-some.yml
 create mode 100644 changelogs/unreleased/sh-skip-findcommit-lookup-rate-limit.yml

diff --git a/.gitignore b/.gitignore
index e1dc80e79b0..7e038bff3b3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -86,3 +86,4 @@ jsdoc/
 .projections.json
 /qa/.rakeTasks
 webpack-dev-server.json
+.nvimrc
diff --git a/app/assets/javascripts/boards/components/modal/list.vue b/app/assets/javascripts/boards/components/modal/list.vue
index 1802b543687..78e3351a79e 100644
--- a/app/assets/javascripts/boards/components/modal/list.vue
+++ b/app/assets/javascripts/boards/components/modal/list.vue
@@ -1,6 +1,6 @@
 <script>
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import Icon from '~/vue_shared/components/icon.vue';
-import bp from '../../../breakpoints';
 import ModalStore from '../../stores/modal_store';
 import IssueCardInner from '../issue_card_inner.vue';
 
@@ -105,9 +105,9 @@ export default {
     setColumnCount() {
       const breakpoint = bp.getBreakpointSize();
 
-      if (breakpoint === 'lg' || breakpoint === 'md') {
+      if (breakpoint === 'xl' || breakpoint === 'lg') {
         this.columns = 3;
-      } else if (breakpoint === 'sm') {
+      } else if (breakpoint === 'md') {
         this.columns = 2;
       } else {
         this.columns = 1;
diff --git a/app/assets/javascripts/commit/pipelines/pipelines_table.vue b/app/assets/javascripts/commit/pipelines/pipelines_table.vue
index e5b030d4900..6b0d184faec 100644
--- a/app/assets/javascripts/commit/pipelines/pipelines_table.vue
+++ b/app/assets/javascripts/commit/pipelines/pipelines_table.vue
@@ -1,5 +1,6 @@
 <script>
 import { GlButton, GlLoadingIcon } from '@gitlab/ui';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import PipelinesService from '~/pipelines/services/pipelines_service';
 import PipelineStore from '~/pipelines/stores/pipelines_store';
 import pipelinesMixin from '~/pipelines/mixins/pipelines';
@@ -7,7 +8,6 @@ import eventHub from '~/pipelines/event_hub';
 import TablePagination from '~/vue_shared/components/pagination/table_pagination.vue';
 import { getParameterByName } from '~/lib/utils/common_utils';
 import CIPaginationMixin from '~/vue_shared/mixins/ci_pagination_api_mixin';
-import bp from '~/breakpoints';
 
 export default {
   components: {
diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js
index 2566ed6b47c..b9ce0851585 100644
--- a/app/assets/javascripts/fly_out_nav.js
+++ b/app/assets/javascripts/fly_out_nav.js
@@ -1,4 +1,4 @@
-import bp from './breakpoints';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import { SIDEBAR_COLLAPSED_CLASS } from './contextual_sidebar';
 
 const HIDE_INTERVAL_TIMEOUT = 300;
@@ -40,10 +40,7 @@ export const canShowActiveSubItems = el => {
   return true;
 };
 
-export const canShowSubItems = () =>
-  bp.getBreakpointSize() === 'sm' ||
-  bp.getBreakpointSize() === 'md' ||
-  bp.getBreakpointSize() === 'lg';
+export const canShowSubItems = () => ['md', 'lg', 'xl'].includes(bp.getBreakpointSize());
 
 export const getHideSubItemsInterval = () => {
   if (!currentOpenMenu || !mousePos.length) return 0;
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index 71f73cd5420..18462cc24c7 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -2,12 +2,12 @@
  * @module common-utils
  */
 
+import { GlBreakpointInstance as breakpointInstance } from '@gitlab/ui/dist/utils';
 import $ from 'jquery';
 import axios from './axios_utils';
 import { getLocationHash } from './url_utility';
 import { convertToCamelCase, convertToSnakeCase } from './text_utility';
 import { isObject } from './type_utility';
-import breakpointInstance from '../../breakpoints';
 
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
diff --git a/app/assets/javascripts/pages/projects/wikis/wikis.js b/app/assets/javascripts/pages/projects/wikis/wikis.js
index d41199f6374..80b62859134 100644
--- a/app/assets/javascripts/pages/projects/wikis/wikis.js
+++ b/app/assets/javascripts/pages/projects/wikis/wikis.js
@@ -1,4 +1,4 @@
-import bp from '../../../breakpoints';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import { s__, sprintf } from '~/locale';
 
 export default class Wikis {
@@ -52,7 +52,7 @@ export default class Wikis {
 
   static sidebarCanCollapse() {
     const bootstrapBreakpoint = bp.getBreakpointSize();
-    return bootstrapBreakpoint === 'xs';
+    return bootstrapBreakpoint === 'xs' || bootstrapBreakpoint === 'sm';
   }
 
   renderSidebar() {
diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index f39d98be516..f7bc6898112 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -9,9 +9,9 @@ class Projects::RawController < Projects::ApplicationController
   prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) }
 
   before_action :require_non_empty_project
-  before_action :assign_ref_vars
   before_action :authorize_download_code!
   before_action :show_rate_limit, only: [:show], unless: :external_storage_request?
+  before_action :assign_ref_vars
   before_action :redirect_to_external_storage, only: :show, if: :static_objects_external_storage_enabled?
 
   def show
@@ -23,11 +23,15 @@ class Projects::RawController < Projects::ApplicationController
   private
 
   def show_rate_limit
-    if rate_limiter.throttled?(:show_raw_controller, scope: [@project, @commit, @path], threshold: raw_blob_request_limit)
+    # This bypasses assign_ref_vars to avoid a Gitaly FindCommit lookup.
+    # When rate limiting, we really don't care if a different commit is
+    # being requested.
+    _ref, path = extract_ref(get_id)
+
+    if rate_limiter.throttled?(:show_raw_controller, scope: [@project, path], threshold: raw_blob_request_limit)
       rate_limiter.log_request(request, :raw_blob_request_limit, current_user)
 
-      flash[:alert] = _('You cannot access the raw file. Please wait a minute.')
-      redirect_to project_blob_path(@project, File.join(@ref, @path)), status: :too_many_requests
+      render plain: _('You cannot access the raw file. Please wait a minute.'), status: :too_many_requests
     end
   end
 
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index a5ddf316572..ccbb1d56030 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -57,13 +57,20 @@ class GitlabSchema < GraphQL::Schema
       object.to_global_id
     end
 
-    def object_from_id(global_id, _ctx = nil)
+    def object_from_id(global_id, ctx = {})
+      expected_type = ctx[:expected_type]
       gid = GlobalID.parse(global_id)
 
       unless gid
         raise Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid GitLab id."
       end
 
+      if expected_type && !gid.model_class.ancestors.include?(expected_type)
+        vars = { global_id: global_id, expected_type: expected_type }
+        msg = _('%{global_id} is not a valid id for %{expected_type}.') % vars
+        raise Gitlab::Graphql::Errors::ArgumentError, msg
+      end
+
       if gid.model_class < ApplicationRecord
         Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
       elsif gid.model_class.respond_to?(:lazy_find)
diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb
index 62dcc41dd9c..f2b015edfa1 100644
--- a/app/graphql/resolvers/base_resolver.rb
+++ b/app/graphql/resolvers/base_resolver.rb
@@ -9,6 +9,10 @@ module Resolvers
         def resolve(**args)
           super.first
         end
+
+        def single?
+          true
+        end
       end
     end
 
@@ -17,6 +21,10 @@ module Resolvers
         def resolve(**args)
           super.last
         end
+
+        def single?
+          true
+        end
       end
     end
 
@@ -42,9 +50,13 @@ module Resolvers
     override :object
     def object
       super.tap do |obj|
-        # If the field this resolver is used in is wrapped in a presenter, go back to it's subject
+        # If the field this resolver is used in is wrapped in a presenter, unwrap its subject
         break obj.subject if obj.is_a?(Gitlab::View::Presenter::Base)
       end
     end
+
+    def single?
+      false
+    end
   end
 end
diff --git a/changelogs/unreleased/27946-merge-request-discussions-api-doesn-t-reject-an-error-input-in-some.yml b/changelogs/unreleased/27946-merge-request-discussions-api-doesn-t-reject-an-error-input-in-some.yml
new file mode 100644
index 00000000000..c92ac53d5f8
--- /dev/null
+++ b/changelogs/unreleased/27946-merge-request-discussions-api-doesn-t-reject-an-error-input-in-some.yml
@@ -0,0 +1,6 @@
+---
+title: Resolve "Merge request discussions API doesn't reject an error input in some
+  case"
+merge_request: 21936
+author:
+type: fixed
diff --git a/changelogs/unreleased/sh-skip-findcommit-lookup-rate-limit.yml b/changelogs/unreleased/sh-skip-findcommit-lookup-rate-limit.yml
new file mode 100644
index 00000000000..1e4991d6390
--- /dev/null
+++ b/changelogs/unreleased/sh-skip-findcommit-lookup-rate-limit.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid Gitaly RPCs in rate-limited raw blob requests
+merge_request: 22123
+author:
+type: performance
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 03508e65eaf..c4bfe86363d 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -272,6 +272,9 @@ msgstr ""
 msgid "%{from} to %{to}"
 msgstr ""
 
+msgid "%{global_id} is not a valid id for %{expected_type}."
+msgstr ""
+
 msgid "%{group_docs_link_start}Groups%{group_docs_link_end} allow you to manage and collaborate across multiple projects. Members of a group have access to all of its projects."
 msgstr ""
 
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index bdf1c1a84d3..a570db12d94 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -56,10 +56,13 @@ describe Projects::RawController do
         stub_application_setting(raw_blob_request_limit: 5)
       end
 
-      it 'prevents from accessing the raw file' do
-        execute_raw_requests(requests: 6, project: project, file_path: file_path)
+      it 'prevents from accessing the raw file', :request_store do
+        execute_raw_requests(requests: 5, project: project, file_path: file_path)
+
+        expect { execute_raw_requests(requests: 1, project: project, file_path: file_path) }
+          .to change { Gitlab::GitalyClient.get_request_count }.by(0)
 
-        expect(flash[:alert]).to eq(_('You cannot access the raw file. Please wait a minute.'))
+        expect(response.body).to eq(_('You cannot access the raw file. Please wait a minute.'))
         expect(response).to have_gitlab_http_status(429)
       end
 
@@ -109,7 +112,7 @@ describe Projects::RawController do
 
           execute_raw_requests(requests: 3, project: project, file_path: modified_path)
 
-          expect(flash[:alert]).to eq(_('You cannot access the raw file. Please wait a minute.'))
+          expect(response.body).to eq(_('You cannot access the raw file. Please wait a minute.'))
           expect(response).to have_gitlab_http_status(429)
         end
       end
@@ -137,7 +140,7 @@ describe Projects::RawController do
           # Accessing downcase version of readme
           execute_raw_requests(requests: 6, project: project, file_path: file_path)
 
-          expect(flash[:alert]).to eq(_('You cannot access the raw file. Please wait a minute.'))
+          expect(response.body).to eq(_('You cannot access the raw file. Please wait a minute.'))
           expect(response).to have_gitlab_http_status(429)
 
           # Accessing upcase version of readme
diff --git a/spec/features/projects/raw/user_interacts_with_raw_endpoint_spec.rb b/spec/features/projects/raw/user_interacts_with_raw_endpoint_spec.rb
index 6d587053b4f..673766073a2 100644
--- a/spec/features/projects/raw/user_interacts_with_raw_endpoint_spec.rb
+++ b/spec/features/projects/raw/user_interacts_with_raw_endpoint_spec.rb
@@ -31,8 +31,6 @@ describe 'Projects > Raw > User interacts with raw endpoint' do
         visit project_raw_url(project, file_path)
       end
 
-      expect(source).to have_content('You are being redirected')
-      click_link('redirected')
       expect(page).to have_content('You cannot access the raw file. Please wait a minute.')
     end
   end
diff --git a/spec/javascripts/fly_out_nav_spec.js b/spec/javascripts/fly_out_nav_spec.js
index 4772f754937..afcf132bea3 100644
--- a/spec/javascripts/fly_out_nav_spec.js
+++ b/spec/javascripts/fly_out_nav_spec.js
@@ -1,3 +1,4 @@
+import { GlBreakpointInstance } from '@gitlab/ui/dist/utils';
 import {
   calculateTop,
   showSubLevelItems,
@@ -15,7 +16,6 @@ import {
   subItemsMouseLeave,
 } from '~/fly_out_nav';
 import { SIDEBAR_COLLAPSED_CLASS } from '~/contextual_sidebar';
-import bp from '~/breakpoints';
 
 describe('Fly out sidebar navigation', () => {
   let el;
@@ -26,7 +26,7 @@ describe('Fly out sidebar navigation', () => {
     el.style.position = 'relative';
     document.body.appendChild(el);
 
-    spyOn(bp, 'getBreakpointSize').and.callFake(() => breakpointSize);
+    spyOn(GlBreakpointInstance, 'getBreakpointSize').and.callFake(() => breakpointSize);
 
     setOpenMenu(null);
   });
diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js
index 1d91b508b71..504d4a3e01a 100644
--- a/spec/javascripts/lib/utils/common_utils_spec.js
+++ b/spec/javascripts/lib/utils/common_utils_spec.js
@@ -1,8 +1,8 @@
 import MockAdapter from 'axios-mock-adapter';
+import { GlBreakpointInstance as breakpointInstance } from '@gitlab/ui/dist/utils';
 import axios from '~/lib/utils/axios_utils';
 import * as commonUtils from '~/lib/utils/common_utils';
 import { faviconDataUrl, overlayDataUrl, faviconWithOverlayDataUrl } from './mock_data';
-import breakpointInstance from '~/breakpoints';
 
 const PIXEL_TOLERANCE = 0.2;
 
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index ae493d5c081..b802c8ba506 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -5,11 +5,11 @@ require 'spec_helper'
 describe DiffNote do
   include RepoHelpers
 
-  let!(:merge_request) { create(:merge_request) }
-  let(:project) { merge_request.project }
-  let(:commit) { project.commit(sample_commit.id) }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:project) { merge_request.project }
+  let_it_be(:commit) { project.commit(sample_commit.id) }
 
-  let(:path) { "files/ruby/popen.rb" }
+  let_it_be(:path) { "files/ruby/popen.rb" }
 
   let(:diff_refs) { merge_request.diff_refs }
   let!(:position) do
diff --git a/spec/requests/api/discussions_spec.rb b/spec/requests/api/discussions_spec.rb
index 68f7d407b54..f37a02e7135 100644
--- a/spec/requests/api/discussions_spec.rb
+++ b/spec/requests/api/discussions_spec.rb
@@ -49,6 +49,18 @@ describe API::Discussions do
     it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid', can_reply_to_individual_notes: true
     it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid'
     it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid'
+
+    context "when position is for a previous commit on the merge request" do
+      it "returns a 400 bad request error because the line_code is old" do
+        # SHA taken from an earlier commit listed in spec/factories/merge_requests.rb
+        position = diff_note.position.to_h.merge(new_line: 'c1acaa58bbcbc3eafe538cb8274ba387047b69f8')
+
+        post api("/projects/#{project.id}/merge_requests/#{noteable['iid']}/discussions", user),
+          params: { body: 'hi!', position: position }
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+    end
   end
 
   context 'when noteable is a Commit' do
diff --git a/spec/support/matchers/graphql_matchers.rb b/spec/support/matchers/graphql_matchers.rb
index dbf457a9200..282deea4606 100644
--- a/spec/support/matchers/graphql_matchers.rb
+++ b/spec/support/matchers/graphql_matchers.rb
@@ -8,7 +8,7 @@ end
 
 RSpec::Matchers.define :have_graphql_fields do |*expected|
   def expected_field_names
-    expected.map { |name| GraphqlHelpers.fieldnamerize(name) }
+    Array.wrap(expected).map { |name| GraphqlHelpers.fieldnamerize(name) }
   end
 
   match do |kls|
diff --git a/spec/support/shared_examples/requests/api/diff_discussions.rb b/spec/support/shared_examples/requests/api/diff_discussions.rb
index 76c6c93964a..a7774d17d3c 100644
--- a/spec/support/shared_examples/requests/api/diff_discussions.rb
+++ b/spec/support/shared_examples/requests/api/diff_discussions.rb
@@ -38,13 +38,24 @@ shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name|
       expect(json_response['notes'].first['position']).to eq(position.stringify_keys)
     end
 
-    it "returns a 400 bad request error when position is invalid" do
-      position = diff_note.position.to_h.merge(new_line: '100000')
+    context "when position is invalid" do
+      it "returns a 400 bad request error when position is not plausible" do
+        position = diff_note.position.to_h.merge(new_line: '100000')
 
-      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user),
-        params: { body: 'hi!', position: position }
+        post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user),
+          params: { body: 'hi!', position: position }
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it "returns a 400 bad request error when the position is not valid for this discussion" do
+        position = diff_note.position.to_h.merge(new_line: '588440f66559714280628a4f9799f0c4eb880a4a')
+
+        post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user),
+          params: { body: 'hi!', position: position }
 
-      expect(response).to have_gitlab_http_status(400)
+        expect(response).to have_gitlab_http_status(400)
+      end
     end
   end
 
-- 
cgit v1.2.3