diff options
20 files changed, 97 insertions, 38 deletions
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 |