diff options
60 files changed, 837 insertions, 260 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 32b7211cb61..7d47e599800 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.40.0 +1.41.0 @@ -41,8 +41,9 @@ gem 'omniauth-shibboleth', '~> 1.3.0' gem 'omniauth-twitter', '~> 1.4' gem 'omniauth_crowd', '~> 2.2.0' gem 'omniauth-authentiq', '~> 0.3.3' +gem 'omniauth_openid_connect', '~> 0.3.0' +gem "omniauth-ultraauth", '~> 0.0.2' gem 'rack-oauth2', '~> 1.9.3' -gem "omniauth-ultraauth", '~> 0.0.1' gem 'jwt', '~> 2.1.0' # Spam and anti-bot protection diff --git a/Gemfile.lock b/Gemfile.lock index bdcefd2e2b5..9b1a036030a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -561,13 +561,13 @@ GEM omniauth-twitter (1.4.0) omniauth-oauth (~> 1.1) rack - omniauth-ultraauth (0.0.1) - omniauth_openid_connect (~> 0.2.4) + omniauth-ultraauth (0.0.2) + omniauth_openid_connect (~> 0.3.0) omniauth_crowd (2.2.3) activesupport nokogiri (>= 1.4.4) omniauth (~> 1.0) - omniauth_openid_connect (0.2.4) + omniauth_openid_connect (0.3.0) addressable (~> 2.5) omniauth (~> 1.3) openid_connect (~> 1.1) @@ -1130,8 +1130,9 @@ DEPENDENCIES omniauth-saml (~> 1.10) omniauth-shibboleth (~> 1.3.0) omniauth-twitter (~> 1.4) - omniauth-ultraauth (~> 0.0.1) + omniauth-ultraauth (~> 0.0.2) omniauth_crowd (~> 2.2.0) + omniauth_openid_connect (~> 0.3.0) org-ruby (~> 0.9.12) peek (~> 1.0.1) peek-gc (~> 0.0.2) diff --git a/app/assets/javascripts/pages/projects/issues/index/index.js b/app/assets/javascripts/pages/projects/issues/index/index.js index 8bf0c2edc71..c34aff02111 100644 --- a/app/assets/javascripts/pages/projects/issues/index/index.js +++ b/app/assets/javascripts/pages/projects/issues/index/index.js @@ -4,9 +4,9 @@ import IssuableIndex from '~/issuable_index'; import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation'; import UsersSelect from '~/users_select'; import initFilteredSearch from '~/pages/search/init_filtered_search'; -import IssuableFilteredSearchTokenKeys from 'ee_else_ce/filtered_search/issuable_filtered_search_token_keys'; import { FILTERED_SEARCH } from '~/pages/constants'; import { ISSUABLE_INDEX } from '~/pages/projects/constants'; +import IssuableFilteredSearchTokenKeys from 'ee_else_ce/filtered_search/issuable_filtered_search_token_keys'; document.addEventListener('DOMContentLoaded', () => { IssuableFilteredSearchTokenKeys.addExtraTokensForIssues(); diff --git a/app/assets/javascripts/pipelines/components/pipeline_triggerer.vue b/app/assets/javascripts/pipelines/components/pipeline_triggerer.vue new file mode 100644 index 00000000000..740b54cd8e0 --- /dev/null +++ b/app/assets/javascripts/pipelines/components/pipeline_triggerer.vue @@ -0,0 +1,35 @@ +<script> +import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue'; + +export default { + components: { + UserAvatarLink, + }, + props: { + pipeline: { + type: Object, + required: true, + }, + }, + computed: { + user() { + return this.pipeline.user; + }, + }, +}; +</script> +<template> + <div class="table-section section-10 d-none d-sm-none d-md-block pipeline-triggerer"> + <user-avatar-link + v-if="user" + :link-href="user.path" + :img-src="user.avatar_url" + :img-size="26" + :tooltip-text="user.name" + class="prepend-left-default js-pipeline-url-user" + /> + <span v-else class="prepend-left-default js-pipeline-url-api api"> + {{ s__('Pipelines|API') }} + </span> + </div> +</template> diff --git a/app/assets/javascripts/pipelines/components/pipeline_url.vue b/app/assets/javascripts/pipelines/components/pipeline_url.vue index 3e7bf20470c..c41ecab1294 100644 --- a/app/assets/javascripts/pipelines/components/pipeline_url.vue +++ b/app/assets/javascripts/pipelines/components/pipeline_url.vue @@ -59,19 +59,10 @@ export default { }; </script> <template> - <div class="table-section section-15 d-none d-sm-none d-md-block pipeline-tags"> + <div class="table-section section-10 d-none d-sm-none d-md-block pipeline-tags"> <gl-link :href="pipeline.path" class="js-pipeline-url-link"> <span class="pipeline-id">#{{ pipeline.id }}</span> </gl-link> - <span>by</span> - <user-avatar-link - v-if="user" - :link-href="user.path" - :img-src="user.avatar_url" - :tooltip-text="user.name" - class="js-pipeline-url-user" - /> - <span v-if="!user" class="js-pipeline-url-api api"> API </span> <div class="label-container"> <span v-if="pipeline.flags.latest" diff --git a/app/assets/javascripts/pipelines/components/pipelines_table.vue b/app/assets/javascripts/pipelines/components/pipelines_table.vue index fcd1f119df0..03d332cd430 100644 --- a/app/assets/javascripts/pipelines/components/pipelines_table.vue +++ b/app/assets/javascripts/pipelines/components/pipelines_table.vue @@ -1,4 +1,5 @@ <script> +import { GlTooltipDirective } from '@gitlab/ui'; import PipelinesTableRowComponent from './pipelines_table_row.vue'; import PipelineStopModal from './pipeline_stop_modal.vue'; import eventHub from '../event_hub'; @@ -13,6 +14,9 @@ export default { PipelinesTableRowComponent, PipelineStopModal, }, + directives: { + GlTooltip: GlTooltipDirective, + }, props: { pipelines: { type: Array, @@ -62,16 +66,19 @@ export default { <template> <div class="ci-table"> <div class="gl-responsive-table-row table-row-header" role="row"> - <div class="table-section section-10 js-pipeline-status pipeline-status" role="rowheader"> + <div class="table-section section-10 js-pipeline-status" role="rowheader"> {{ s__('Pipeline|Status') }} </div> - <div class="table-section section-15 js-pipeline-info pipeline-info" role="rowheader"> + <div class="table-section section-10 js-pipeline-info pipeline-info" role="rowheader"> {{ s__('Pipeline|Pipeline') }} </div> + <div class="table-section section-10 js-triggerer-info triggerer-info" role="rowheader"> + {{ s__('Pipeline|Triggerer') }} + </div> <div class="table-section section-20 js-pipeline-commit pipeline-commit" role="rowheader"> {{ s__('Pipeline|Commit') }} </div> - <div class="table-section section-20 js-pipeline-stages pipeline-stages" role="rowheader"> + <div class="table-section section-15 js-pipeline-stages pipeline-stages" role="rowheader"> {{ s__('Pipeline|Stages') }} </div> </div> diff --git a/app/assets/javascripts/pipelines/components/pipelines_table_row.vue b/app/assets/javascripts/pipelines/components/pipelines_table_row.vue index 1c44427e720..e32e2f785bd 100644 --- a/app/assets/javascripts/pipelines/components/pipelines_table_row.vue +++ b/app/assets/javascripts/pipelines/components/pipelines_table_row.vue @@ -5,6 +5,7 @@ import PipelinesArtifactsComponent from './pipelines_artifacts.vue'; import CiBadge from '../../vue_shared/components/ci_badge_link.vue'; import PipelineStage from './stage.vue'; import PipelineUrl from './pipeline_url.vue'; +import PipelineTriggerer from './pipeline_triggerer.vue'; import PipelinesTimeago from './time_ago.vue'; import CommitComponent from '../../vue_shared/components/commit.vue'; import LoadingButton from '../../vue_shared/components/loading_button.vue'; @@ -23,6 +24,7 @@ export default { CommitComponent, PipelineStage, PipelineUrl, + PipelineTriggerer, CiBadge, PipelinesTimeago, LoadingButton, @@ -264,8 +266,9 @@ export default { </div> <pipeline-url :pipeline="pipeline" :auto-devops-help-path="autoDevopsHelpPath" /> + <pipeline-triggerer :pipeline="pipeline" /> - <div class="table-section section-20"> + <div class="table-section section-wrap section-20"> <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Commit') }}</div> <div class="table-mobile-content"> <commit-component @@ -281,7 +284,7 @@ export default { </div> </div> - <div class="table-section section-wrap section-20 stage-cell"> + <div class="table-section section-wrap section-15 stage-cell"> <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Stages') }}</div> <div class="table-mobile-content"> <template v-if="pipeline.details.stages.length > 0"> diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ceaa84acaba..4cbab6811bc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -27,6 +27,7 @@ class ApplicationController < ActionController::Base before_action :check_impersonation_availability around_action :set_locale + around_action :set_session_storage after_action :set_page_title_header, if: :json_request? after_action :limit_unauthenticated_session_times @@ -434,6 +435,10 @@ class ApplicationController < ActionController::Base Gitlab::I18n.with_user_locale(current_user, &block) end + def set_session_storage(&block) + Gitlab::Session.with_session(session, &block) + end + def set_page_title_header # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8 response.headers['Page-Title'] = URI.escape(page_title('GitLab')) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c2a1487fc6e..df162e4844c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -66,7 +66,7 @@ class MergeRequest < ApplicationRecord dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue - has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline' + has_many :pipelines_for_merge_request, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline' has_many :suggestions, through: :notes has_many :merge_request_assignees @@ -1157,10 +1157,6 @@ class MergeRequest < ApplicationRecord end end - def merge_request_pipeline_exists? - merge_request_pipelines.exists?(sha: diff_head_sha) - end - def has_test_reports? actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports) end @@ -1379,12 +1375,12 @@ class MergeRequest < ApplicationRecord source_project.repository.squash_in_progress?(id) end - private - def find_actual_head_pipeline all_pipelines.for_sha_or_source_sha(diff_head_sha).first end + private + def source_project_variables Gitlab::Ci::Variables::Collection.new.tap do |variables| break variables unless source_project diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 5f5d92bc2f0..f45bd0e03de 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -51,6 +51,10 @@ class MergeRequestDiff < ApplicationRecord joins(:merge_request_diff_commits).where(merge_request_diff_commits: { sha: sha }).reorder(nil) end + scope :by_project_id, -> (project_id) do + joins(:merge_request).where(merge_requests: { target_project_id: project_id }) + end + scope :recent, -> { order(id: :desc).limit(100) } scope :files_in_database, -> { where(stored_externally: [false, nil]) } diff --git a/app/models/note_diff_file.rb b/app/models/note_diff_file.rb index 9afb94c869a..fcc9e2b3fd8 100644 --- a/app/models/note_diff_file.rb +++ b/app/models/note_diff_file.rb @@ -7,6 +7,10 @@ class NoteDiffFile < ApplicationRecord joins(:diff_note).where("resolved_at IS NULL OR noteable_type = 'Commit'") end + scope :referencing_sha, -> (oids, project_id:) do + joins(:diff_note).where(notes: { project_id: project_id, commit_id: oids }) + end + delegate :original_position, :project, to: :diff_note belongs_to :diff_note, inverse_of: :note_diff_file diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index d412a591fdc..e85397422e6 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -44,7 +44,6 @@ class GlobalPolicy < BasePolicy prevent :access_api prevent :access_git prevent :receive_notifications - prevent :use_quick_actions end rule { required_terms_not_accepted }.policy do diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index a9dd26c02ad..bb9062e9b40 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -81,7 +81,7 @@ module MergeRequests ## # UpdateMergeRequestsWorker could be retried by an exception. # pipelines for merge request should not be recreated in such case. - return false if merge_request.merge_request_pipeline_exists? + return false if merge_request.find_actual_head_pipeline&.triggered_by_merge_request? return false if merge_request.has_no_commits? true diff --git a/app/services/projects/cleanup_service.rb b/app/services/projects/cleanup_service.rb index 12103ea34b5..5972bfd4071 100644 --- a/app/services/projects/cleanup_service.rb +++ b/app/services/projects/cleanup_service.rb @@ -18,9 +18,6 @@ module Projects # per rewritten object, with the old and new SHAs space-separated. It can be # used to update or remove content that references the objects that BFG has # altered - # - # Currently, only the project repository is modified by this service, but we - # may wish to modify other data sources in the future. def execute apply_bfg_object_map! @@ -41,10 +38,52 @@ module Projects raise NoUploadError unless project.bfg_object_map.exists? project.bfg_object_map.open do |io| - repository_cleaner.apply_bfg_object_map(io) + repository_cleaner.apply_bfg_object_map_stream(io) do |response| + cleanup_diffs(response) + end + end + end + + def cleanup_diffs(response) + old_commit_shas = extract_old_commit_shas(response.entries) + + ActiveRecord::Base.transaction do + cleanup_merge_request_diffs(old_commit_shas) + cleanup_note_diff_files(old_commit_shas) end end + def extract_old_commit_shas(batch) + batch.lazy.select { |entry| entry.type == :COMMIT }.map(&:old_oid).force + end + + def cleanup_merge_request_diffs(old_commit_shas) + merge_request_diffs = MergeRequestDiff + .by_project_id(project.id) + .by_commit_sha(old_commit_shas) + + # It's important to run the ActiveRecord callbacks here + merge_request_diffs.destroy_all # rubocop:disable Cop/DestroyAll + + # TODO: ensure the highlight cache is removed immediately. It's too hard + # to calculate the Redis keys at present. + # + # https://gitlab.com/gitlab-org/gitlab-ce/issues/61115 + end + + def cleanup_note_diff_files(old_commit_shas) + # Pluck the IDs instead of running the query twice to ensure we clear the + # cache for exactly the note diffs we remove + ids = NoteDiffFile + .referencing_sha(old_commit_shas, project_id: project.id) + .pluck_primary_key + + NoteDiffFile.id_in(ids).delete_all + + # A highlighted version of the diff is stored in redis. Remove it now. + Gitlab::DiscussionsDiff::HighlightCache.clear_multiple(ids) + end + def repository_cleaner @repository_cleaner ||= Gitlab::Git::RepositoryCleaner.new(repository.raw) end diff --git a/changelogs/unreleased/27383-added_omniauth_openid_connect_startegy.yml b/changelogs/unreleased/27383-added_omniauth_openid_connect_startegy.yml new file mode 100644 index 00000000000..c49b201f0de --- /dev/null +++ b/changelogs/unreleased/27383-added_omniauth_openid_connect_startegy.yml @@ -0,0 +1,5 @@ +--- +title: Added OmniAuth OpenID Connect strategy +merge_request: 27383 +author: Horatiu Eugen Vlad +type: added diff --git a/changelogs/unreleased/30093-apply-bfg-object-map-to-database.yml b/changelogs/unreleased/30093-apply-bfg-object-map-to-database.yml new file mode 100644 index 00000000000..ec851dfcacc --- /dev/null +++ b/changelogs/unreleased/30093-apply-bfg-object-map-to-database.yml @@ -0,0 +1,5 @@ +--- +title: Remove cleaned up OIDs from database and cache +merge_request: 26555 +author: +type: added diff --git a/changelogs/unreleased/58105-pipeline-author-and-commit-author-too-close-together-in-pipeline-list.yml b/changelogs/unreleased/58105-pipeline-author-and-commit-author-too-close-together-in-pipeline-list.yml new file mode 100644 index 00000000000..aef0a5ad53e --- /dev/null +++ b/changelogs/unreleased/58105-pipeline-author-and-commit-author-too-close-together-in-pipeline-list.yml @@ -0,0 +1,5 @@ +--- +title: Improve pipelines table spacing, add triggerer column +merge_request: 26136 +author: +type: changed diff --git a/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml new file mode 100644 index 00000000000..294a665ff3e --- /dev/null +++ b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml @@ -0,0 +1,5 @@ +--- +title: Fix duplicate merge request pipelines created by Sidekiq worker retry +merge_request: 26643 +author: +type: fixed diff --git a/changelogs/unreleased/patch-49.yml b/changelogs/unreleased/patch-49.yml new file mode 100644 index 00000000000..2c8af1e5c48 --- /dev/null +++ b/changelogs/unreleased/patch-49.yml @@ -0,0 +1,5 @@ +--- +title: Remove leading / trailing spaces from heading when generating header ids +merge_request: 27025 +author: Willian Balmant +type: fixed diff --git a/db/migrate/20190418182545_create_merge_request_trains_table.rb b/db/migrate/20190418182545_create_merge_request_trains_table.rb new file mode 100644 index 00000000000..ac927c9c6b9 --- /dev/null +++ b/db/migrate/20190418182545_create_merge_request_trains_table.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateMergeRequestTrainsTable < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :merge_trains, id: :bigserial do |t| + t.references :merge_request, foreign_key: { on_delete: :cascade }, type: :integer, index: false, null: false + t.references :user, foreign_key: { on_delete: :cascade }, type: :integer, null: false + t.references :pipeline, foreign_key: { to_table: :ci_pipelines, on_delete: :nullify }, type: :integer + t.timestamps_with_timezone null: false + + t.index [:merge_request_id], unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index de9e6f0b40d..ef8cb4abf31 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1361,6 +1361,17 @@ ActiveRecord::Schema.define(version: 20190426180107) do t.index ["merge_request_id"], name: "index_merge_requests_closing_issues_on_merge_request_id", using: :btree end + create_table "merge_trains", force: :cascade do |t| + t.integer "merge_request_id", null: false + t.integer "user_id", null: false + t.integer "pipeline_id" + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + t.index ["merge_request_id"], name: "index_merge_trains_on_merge_request_id", unique: true, using: :btree + t.index ["pipeline_id"], name: "index_merge_trains_on_pipeline_id", using: :btree + t.index ["user_id"], name: "index_merge_trains_on_user_id", using: :btree + end + create_table "milestones", id: :serial, force: :cascade do |t| t.string "title", null: false t.integer "project_id" @@ -2514,6 +2525,9 @@ ActiveRecord::Schema.define(version: 20190426180107) do add_foreign_key "merge_requests", "users", column: "updated_by_id", name: "fk_641731faff", on_delete: :nullify add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade + add_foreign_key "merge_trains", "ci_pipelines", column: "pipeline_id", on_delete: :nullify + add_foreign_key "merge_trains", "merge_requests", on_delete: :cascade + add_foreign_key "merge_trains", "users", on_delete: :cascade add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade add_foreign_key "note_diff_files", "notes", column: "diff_note_id", on_delete: :cascade diff --git a/doc/administration/auth/oidc.md b/doc/administration/auth/oidc.md new file mode 100644 index 00000000000..e55f7dbb4df --- /dev/null +++ b/doc/administration/auth/oidc.md @@ -0,0 +1,105 @@ +# OpenID Connect OmniAuth provider + +GitLab can use [OpenID Connect](https://openid.net/specs/openid-connect-core-1_0.html) as an OmniAuth provider. + +To enable the OpenID Connect OmniAuth provider, you must register your application with an OpenID Connect provider. +The OpenID Connect will provide you with a client details and secret for you to use. + +1. On your GitLab server, open the configuration file. + + For Omnibus GitLab: + + ```sh + sudo editor /etc/gitlab/gitlab.rb + ``` + + For installations from source: + + ```sh + cd /home/git/gitlab + sudo -u git -H editor config/gitlab.yml + ``` + + See [Initial OmniAuth Configuration](../../integration/omniauth.md#initial-omniauth-configuration) for initial settings. + +1. Add the provider configuration. + + For Omnibus GitLab: + + ```ruby + gitlab_rails['omniauth_providers'] = [ + { 'name' => 'openid_connect', + 'label' => '<your_oidc_label>', + 'args' => { + 'scope' => ['openid','profile'], + 'response_type' => 'code', + 'issuer' => '<your_oidc_url>', + 'discovery' => true, + 'client_auth_method' => 'query', + 'uid_field' => '<uid_field>', + 'client_options' => { + 'identifier' => '<your_oidc_client_id>', + 'secret' => '<your_oidc_client_secret>', + 'redirect_uri' => '<your_gitlab_url>/users/auth/openid_connect/callback' + } + } + } + ] + ``` + + For installation from source: + + ```yaml + - { name: 'openid_connect', + label: '<your_oidc_label>', + args: { + scope: ['openid','profile'], + response_type: 'code', + issuer: '<your_oidc_url>', + discovery: true, + client_auth_method: 'query', + uid_field: '<uid_field>', + client_options: { + identifier: '<your_oidc_client_id>', + secret: '<your_oidc_client_secret>', + redirect_uri: '<your_gitlab_url>/users/auth/openid_connect/callback' + } + } + } + ``` + + > **Note:** + > + > - For more information on each configuration option refer to + the [OmniAuth OpenID Connect usage documentation](https://github.com/m0n9oose/omniauth_openid_connect#usage) and + the [OpenID Connect Core 1.0 specification](https://openid.net/specs/openid-connect-core-1_0.html). + +1. For the configuration above, change the values for the provider to match your OpenID Connect client setup. Use the following as a guide: + - `<your_oidc_label>` is the label that will be displayed on the login page. + - `<your_oidc_url>` (optional) is the URL that points to the OpenID Connect provider. For example, `https://example.com/auth/realms/your-realm`. + If this value is not provided, the URL is constructed from the `client_options` in the following format: `<client_options.scheme>://<client_options.host>:<client_options.port>`. + - If `discovery` is set to `true`, the OpenID Connect provider will try to auto discover the client options using `<your_oidc_url>/.well-known/openid-configuration`. Defaults to `false`. + - `<uid_field>` (optional) is the field name from the `user_info` details that will be used as `uid` value. For example, `preferred_username`. + If this value is not provided or the field with the configured value is missing from the `user_info` details, the `uid` will use the `sub` field. + - `client_options` are the OpenID Connect client-specific options. Specifically: + + - `identifier` is the client identifier as configured in the OpenID Connect service provider. + - `secret` is the client secret as configured in the OpenID Connect service provider. + - `redirect_uri` is the GitLab URL to redirect the user to after successful login. For example, `http://example.com/users/auth/openid_connect/callback`. + - `end_session_endpoint` (optional) is the URL to the endpoint that end the session (logout). Can be provided if auto-discovery disabled or unsuccessful. + + The following `client_options` are optional unless auto-discovery is disabled or unsuccessful: + + - `authorization_endpoint` is the URL to the endpoint that authorizes the end user. + - `token_endpoint` is the URL to the endpoint that provides Access Token. + - `userinfo_endpoint` is the URL to the endpoint that provides the user information. + - `jwks_uri` is the URL to the endpoint where the Token signer publishes its keys. + +1. Save the configuration file. +1. [Reconfigure](../restart_gitlab.md#omnibus-gitlab-reconfigure) or [restart GitLab](../restart_gitlab.md#installations-from-source) + for the changes to take effect if you installed GitLab via Omnibus or from source respectively. + +On the sign in page, there should now be an OpenID Connect icon below the regular sign in form. +Click the icon to begin the authentication process. The OpenID Connect provider will ask the user to +sign in and authorize the GitLab application (if confirmation required by the client). If everything goes well, the user +will be redirected to GitLab and will be signed in. diff --git a/doc/integration/omniauth.md b/doc/integration/omniauth.md index 7fd39b02fbe..ef1f2df77f8 100644 --- a/doc/integration/omniauth.md +++ b/doc/integration/omniauth.md @@ -33,6 +33,7 @@ contains some settings that are common for all providers. - [Authentiq](../administration/auth/authentiq.md) - [OAuth2Generic](oauth2_generic.md) - [JWT](../administration/auth/jwt.md) +- [OpenID Connect](../administration/auth/oidc.md) - [UltraAuth](ultra_auth.md) ## Initial OmniAuth Configuration diff --git a/doc/user/project/labels.md b/doc/user/project/labels.md index 9003018a521..ac91cd4ea98 100644 --- a/doc/user/project/labels.md +++ b/doc/user/project/labels.md @@ -53,7 +53,7 @@ be able to advance workflow states consistently in issues themselves. ## Creating labels >**Note:** -A permission level of `Developer` or higher is required to create labels. +A permission level of Reporter or higher is required to create labels. ### New project label @@ -91,7 +91,7 @@ From the sidebar of an issue or a merge request, you can create a new **project ## Editing labels NOTE: **Note:** -A permission level of `Developer` or higher is required to edit labels. +A permission level of Reporter or higher is required to edit labels. You can update a label by navigating to **Issues > Labels** in the project or group and clicking the pencil icon. diff --git a/doc/user/project/repository/reducing_the_repo_size_using_git.md b/doc/user/project/repository/reducing_the_repo_size_using_git.md index 672567a8d7d..2339759ecc8 100644 --- a/doc/user/project/repository/reducing_the_repo_size_using_git.md +++ b/doc/user/project/repository/reducing_the_repo_size_using_git.md @@ -98,6 +98,12 @@ up its own internal state, maximizing the space saved. `git gc` against the repository. You will receive an email once it has completed. +This process will remove some copies of the rewritten commits from GitLab's +cache and database, but there are still numerous gaps in coverage - at present, +some of the copies may persist indefinitely. [Clearing the instance cache] +(../../../administration/raketasks/maintenance.md#clear-redis-cache) may help to +remove some of them, but it should not be depended on for security purposes! + ## Using `git filter-branch` 1. Navigate to your repository: diff --git a/lib/api/settings.rb b/lib/api/settings.rb index b064747e5fc..8046acfa397 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -150,6 +150,12 @@ module API given elasticsearch_indexing: ->(val) { val } do optional :elasticsearch_search, type: Boolean, desc: 'Enable Elasticsearch search' requires :elasticsearch_url, type: String, desc: 'The url to use for connecting to Elasticsearch. Use a comma-separated list to support clustering (e.g., "http://localhost:9200, http://localhost:9201")' + optional :elasticsearch_limit_indexing, type: Boolean, desc: 'Limit Elasticsearch to index certain namespaces and projects' + end + + given elasticsearch_limit_indexing: ->(val) { val } do + optional :elasticsearch_namespace_ids, type: Array[Integer], coerce_with: Validations::Types::LabelsList.coerce, desc: 'The namespace ids to index with Elasticsearch.' + optional :elasticsearch_project_ids, type: Array[Integer], coerce_with: Validations::Types::LabelsList.coerce, desc: 'The project ids to index with Elasticsearch.' end optional :email_additional_text, type: String, desc: 'Additional text added to the bottom of every email for legal/auditing/compliance reasons' diff --git a/lib/banzai/filter/table_of_contents_filter.rb b/lib/banzai/filter/table_of_contents_filter.rb index 1a68d773048..ade4d260be1 100644 --- a/lib/banzai/filter/table_of_contents_filter.rb +++ b/lib/banzai/filter/table_of_contents_filter.rb @@ -31,6 +31,7 @@ module Banzai if header_content = node.children.first id = node .text + .strip .downcase .gsub(PUNCTUATION_REGEXP, '') # remove punctuation .tr(' ', '-') # replace spaces with dash diff --git a/lib/gitlab/discussions_diff/highlight_cache.rb b/lib/gitlab/discussions_diff/highlight_cache.rb index 270cfb89488..369c6b87fb4 100644 --- a/lib/gitlab/discussions_diff/highlight_cache.rb +++ b/lib/gitlab/discussions_diff/highlight_cache.rb @@ -52,6 +52,19 @@ module Gitlab end end + # Clears multiple cache keys at once. + # + # raw_keys - An Array of unique cache keys, without namespaces. + # + # It returns the number of cache keys cleared. Ex.: 42 + def clear_multiple(raw_keys) + return [] if raw_keys.empty? + + keys = raw_keys.map { |id| cache_key_for(id) } + + Redis::Cache.with { |redis| redis.del(keys) } + end + def cache_key_for(raw_key) "#{cache_key_prefix}:#{raw_key}" end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 508499f227c..fc9bcbdcca2 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -922,6 +922,12 @@ module Gitlab end end + def disconnect_alternates + wrapped_gitaly_errors do + gitaly_repository_client.disconnect_alternates + end + end + def gitaly_repository Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository, @gl_project_path) end diff --git a/lib/gitlab/git/repository_cleaner.rb b/lib/gitlab/git/repository_cleaner.rb index 2d1d8435cf3..9dd0ddfb44b 100644 --- a/lib/gitlab/git/repository_cleaner.rb +++ b/lib/gitlab/git/repository_cleaner.rb @@ -12,9 +12,9 @@ module Gitlab @repository = repository end - def apply_bfg_object_map(io) + def apply_bfg_object_map_stream(io, &blk) wrapped_gitaly_errors do - gitaly_cleanup_client.apply_bfg_object_map(io) + gitaly_cleanup_client.apply_bfg_object_map_stream(io, &blk) end end diff --git a/lib/gitlab/gitaly_client/cleanup_service.rb b/lib/gitlab/gitaly_client/cleanup_service.rb index 3e8d6a773ca..a56bc35f6d7 100644 --- a/lib/gitlab/gitaly_client/cleanup_service.rb +++ b/lib/gitlab/gitaly_client/cleanup_service.rb @@ -12,25 +12,32 @@ module Gitlab @storage = repository.storage end - def apply_bfg_object_map(io) - first_request = Gitaly::ApplyBfgObjectMapRequest.new(repository: gitaly_repo) + def apply_bfg_object_map_stream(io, &blk) + responses = GitalyClient.call( + storage, + :cleanup_service, + :apply_bfg_object_map_stream, + build_object_map_enum(io), + timeout: GitalyClient.no_timeout + ) + + responses.each(&blk) + end + + private - enum = Enumerator.new do |y| - y.yield first_request + def build_object_map_enum(io) + Enumerator.new do |y| + # First request. For simplicity, doesn't include any object map data + y << Gitaly::ApplyBfgObjectMapStreamRequest.new(repository: gitaly_repo) + # Now stream the BFG object map file to gitaly in chunks while data = io.read(RepositoryService::MAX_MSG_SIZE) - y.yield Gitaly::ApplyBfgObjectMapRequest.new(object_map: data) + y << Gitaly::ApplyBfgObjectMapStreamRequest.new(object_map: data) + break if io&.eof? end end - - GitalyClient.call( - storage, - :cleanup_service, - :apply_bfg_object_map, - enum, - timeout: GitalyClient.no_timeout - ) end end end diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb index 74aae4a8e97..68b17e86608 100644 --- a/lib/gitlab/gitaly_client/repository_service.rb +++ b/lib/gitlab/gitaly_client/repository_service.rb @@ -331,6 +331,14 @@ module Gitlab search_results_from_response(response) end + def disconnect_alternates + request = Gitaly::DisconnectGitAlternatesRequest.new( + repository: @gitaly_repo + ) + + GitalyClient.call(@storage, :object_pool_service, :disconnect_git_alternates, request) + end + private def search_results_from_response(gitaly_response) diff --git a/lib/gitlab/namespaced_session_store.rb b/lib/gitlab/namespaced_session_store.rb new file mode 100644 index 00000000000..34520078bfb --- /dev/null +++ b/lib/gitlab/namespaced_session_store.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Gitlab + class NamespacedSessionStore + delegate :[], :[]=, to: :store + + def initialize(key) + @key = key + end + + def initiated? + !Session.current.nil? + end + + def store + return unless Session.current + + Session.current[@key] ||= {} + Session.current[@key] + end + end +end diff --git a/lib/gitlab/session.rb b/lib/gitlab/session.rb new file mode 100644 index 00000000000..7487ba04a6d --- /dev/null +++ b/lib/gitlab/session.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + class Session + STORE_KEY = :session_storage + + class << self + def with_session(session) + old = self.current + self.current = session + yield + ensure + self.current = old + end + + def current + Thread.current[STORE_KEY] + end + + protected + + def current=(value) + Thread.current[STORE_KEY] = value + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ccf9725e3ff..122f31fec44 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6719,6 +6719,9 @@ msgstr "" msgid "Pipelines settings for '%{project_name}' were successfully updated." msgstr "" +msgid "Pipelines|API" +msgstr "" + msgid "Pipelines|Build with confidence" msgstr "" @@ -6797,6 +6800,9 @@ msgstr "" msgid "Pipeline|Stop pipeline #%{pipelineId}?" msgstr "" +msgid "Pipeline|Triggerer" +msgstr "" + msgid "Pipeline|Variables" msgstr "" diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index abf0e6bccb7..e8df5094b83 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -119,7 +119,7 @@ FactoryBot.define do trait :with_legacy_detached_merge_request_pipeline do after(:create) do |merge_request| - merge_request.merge_request_pipelines << create(:ci_pipeline, + merge_request.pipelines_for_merge_request << create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, project: merge_request.source_project, @@ -130,7 +130,7 @@ FactoryBot.define do trait :with_detached_merge_request_pipeline do after(:create) do |merge_request| - merge_request.merge_request_pipelines << create(:ci_pipeline, + merge_request.pipelines_for_merge_request << create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, project: merge_request.source_project, @@ -147,7 +147,7 @@ FactoryBot.define do end after(:create) do |merge_request, evaluator| - merge_request.merge_request_pipelines << create(:ci_pipeline, + merge_request.pipelines_for_merge_request << create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, project: merge_request.source_project, diff --git a/spec/features/issues/filtered_search/dropdown_hint_spec.rb b/spec/features/issues/filtered_search/dropdown_hint_spec.rb index 096756f19cc..1f4e9e79179 100644 --- a/spec/features/issues/filtered_search/dropdown_hint_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_hint_spec.rb @@ -80,7 +80,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-author', visible: true) - expect_tokens([{ name: 'author' }]) + expect_tokens([{ name: 'Author' }]) expect_filtered_search_input_empty end @@ -89,7 +89,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-assignee', visible: true) - expect_tokens([{ name: 'assignee' }]) + expect_tokens([{ name: 'Assignee' }]) expect_filtered_search_input_empty end @@ -98,7 +98,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-milestone', visible: true) - expect_tokens([{ name: 'milestone' }]) + expect_tokens([{ name: 'Milestone' }]) expect_filtered_search_input_empty end @@ -107,7 +107,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-label', visible: true) - expect_tokens([{ name: 'label' }]) + expect_tokens([{ name: 'Label' }]) expect_filtered_search_input_empty end @@ -116,7 +116,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-my-reaction', visible: true) - expect_tokens([{ name: 'my-reaction' }]) + expect_tokens([{ name: 'My-reaction' }]) expect_filtered_search_input_empty end @@ -125,7 +125,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-confidential', visible: true) - expect_tokens([{ name: 'confidential' }]) + expect_tokens([{ name: 'Confidential' }]) expect_filtered_search_input_empty end end @@ -137,7 +137,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-author', visible: true) - expect_tokens([{ name: 'author' }]) + expect_tokens([{ name: 'Author' }]) expect_filtered_search_input_empty end @@ -147,7 +147,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-assignee', visible: true) - expect_tokens([{ name: 'assignee' }]) + expect_tokens([{ name: 'Assignee' }]) expect_filtered_search_input_empty end @@ -157,7 +157,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-milestone', visible: true) - expect_tokens([{ name: 'milestone' }]) + expect_tokens([{ name: 'Milestone' }]) expect_filtered_search_input_empty end @@ -167,7 +167,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-label', visible: true) - expect_tokens([{ name: 'label' }]) + expect_tokens([{ name: 'Label' }]) expect_filtered_search_input_empty end @@ -177,7 +177,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-my-reaction', visible: true) - expect_tokens([{ name: 'my-reaction' }]) + expect_tokens([{ name: 'My-reaction' }]) expect_filtered_search_input_empty end end @@ -189,7 +189,7 @@ describe 'Dropdown hint', :js do filtered_search.send_keys(:backspace) click_hint('author') - expect_tokens([{ name: 'author' }]) + expect_tokens([{ name: 'Author' }]) expect_filtered_search_input_empty end @@ -199,7 +199,7 @@ describe 'Dropdown hint', :js do filtered_search.send_keys(:backspace) click_hint('assignee') - expect_tokens([{ name: 'assignee' }]) + expect_tokens([{ name: 'Assignee' }]) expect_filtered_search_input_empty end @@ -209,7 +209,7 @@ describe 'Dropdown hint', :js do filtered_search.send_keys(:backspace) click_hint('milestone') - expect_tokens([{ name: 'milestone' }]) + expect_tokens([{ name: 'Milestone' }]) expect_filtered_search_input_empty end @@ -219,7 +219,7 @@ describe 'Dropdown hint', :js do filtered_search.send_keys(:backspace) click_hint('label') - expect_tokens([{ name: 'label' }]) + expect_tokens([{ name: 'Label' }]) expect_filtered_search_input_empty end @@ -229,7 +229,7 @@ describe 'Dropdown hint', :js do filtered_search.send_keys(:backspace) click_hint('my-reaction') - expect_tokens([{ name: 'my-reaction' }]) + expect_tokens([{ name: 'My-reaction' }]) expect_filtered_search_input_empty end end @@ -247,7 +247,7 @@ describe 'Dropdown hint', :js do expect(page).to have_css(js_dropdown_hint, visible: false) expect(page).to have_css('#js-dropdown-wip', visible: true) - expect_tokens([{ name: 'wip' }]) + expect_tokens([{ name: 'WIP' }]) expect_filtered_search_input_empty end end diff --git a/spec/javascripts/pipelines/pipeline_triggerer_spec.js b/spec/javascripts/pipelines/pipeline_triggerer_spec.js new file mode 100644 index 00000000000..8cf290f2663 --- /dev/null +++ b/spec/javascripts/pipelines/pipeline_triggerer_spec.js @@ -0,0 +1,54 @@ +import { mount } from '@vue/test-utils'; +import pipelineTriggerer from '~/pipelines/components/pipeline_triggerer.vue'; + +describe('Pipelines Triggerer', () => { + let wrapper; + + const mockData = { + pipeline: { + user: { + name: 'foo', + avatar_url: '/avatar', + path: '/path', + }, + }, + }; + + const createComponent = () => { + wrapper = mount(pipelineTriggerer, { + propsData: mockData, + }); + }; + + beforeEach(() => { + createComponent(); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + it('should render a table cell', () => { + expect(wrapper.contains('.table-section')).toBe(true); + }); + + it('should render triggerer information when triggerer is provided', () => { + const link = wrapper.find('.js-pipeline-url-user'); + + expect(link.attributes('href')).toEqual(mockData.pipeline.user.path); + expect(link.find('.js-user-avatar-image-toolip').text()).toEqual(mockData.pipeline.user.name); + expect(link.find('img.avatar').attributes('src')).toEqual( + `${mockData.pipeline.user.avatar_url}?width=26`, + ); + }); + + it('should render "API" when no triggerer is provided', () => { + wrapper.setProps({ + pipeline: { + user: null, + }, + }); + + expect(wrapper.find('.js-pipeline-url-api').text()).toEqual('API'); + }); +}); diff --git a/spec/javascripts/pipelines/pipeline_url_spec.js b/spec/javascripts/pipelines/pipeline_url_spec.js index faad49a78b0..aa196af2f33 100644 --- a/spec/javascripts/pipelines/pipeline_url_spec.js +++ b/spec/javascripts/pipelines/pipeline_url_spec.js @@ -42,54 +42,6 @@ describe('Pipeline Url Component', () => { expect(component.$el.querySelector('.js-pipeline-url-link span').textContent).toEqual('#1'); }); - it('should render user information when a user is provided', () => { - const mockData = { - pipeline: { - id: 1, - path: 'foo', - flags: {}, - user: { - web_url: '/', - name: 'foo', - avatar_url: '/', - path: '/', - }, - }, - autoDevopsHelpPath: 'foo', - }; - - const component = new PipelineUrlComponent({ - propsData: mockData, - }).$mount(); - - const image = component.$el.querySelector('.js-pipeline-url-user img'); - const tooltip = component.$el.querySelector( - '.js-pipeline-url-user .js-user-avatar-image-toolip', - ); - - expect(component.$el.querySelector('.js-pipeline-url-user').getAttribute('href')).toEqual( - mockData.pipeline.user.web_url, - ); - - expect(tooltip.textContent.trim()).toEqual(mockData.pipeline.user.name); - expect(image.getAttribute('src')).toEqual(`${mockData.pipeline.user.avatar_url}?width=20`); - }); - - it('should render "API" when no user is provided', () => { - const component = new PipelineUrlComponent({ - propsData: { - pipeline: { - id: 1, - path: 'foo', - flags: {}, - }, - autoDevopsHelpPath: 'foo', - }, - }).$mount(); - - expect(component.$el.querySelector('.js-pipeline-url-api').textContent).toContain('API'); - }); - it('should render latest, yaml invalid, merge request, and stuck flags when provided', () => { const component = new PipelineUrlComponent({ propsData: { diff --git a/spec/javascripts/pipelines/pipelines_table_row_spec.js b/spec/javascripts/pipelines/pipelines_table_row_spec.js index 234fc705a81..d47504d2f54 100644 --- a/spec/javascripts/pipelines/pipelines_table_row_spec.js +++ b/spec/javascripts/pipelines/pipelines_table_row_spec.js @@ -80,13 +80,13 @@ describe('Pipelines Table Row', () => { it('should render user information', () => { expect( component.$el - .querySelector('.table-section:nth-child(2) a:nth-child(3)') + .querySelector('.table-section:nth-child(3) .js-pipeline-url-user') .getAttribute('href'), ).toEqual(pipeline.user.path); expect( component.$el - .querySelector('.table-section:nth-child(2) .js-user-avatar-image-toolip') + .querySelector('.table-section:nth-child(3) .js-user-avatar-image-toolip') .textContent.trim(), ).toEqual(pipeline.user.name); }); diff --git a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb index 7213cd58ea7..4a9880ac85a 100644 --- a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb +++ b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb @@ -58,6 +58,11 @@ describe Banzai::Filter::TableOfContentsFilter do expect(doc.css('h1 a').first.attr('href')).to eq '#this-header-is-filled-with-punctuation' end + it 'removes any leading or trailing spaces' do + doc = filter(header(1, " \r\n\tTitle with spaces\r\n\t ")) + expect(doc.css('h1 a').first.attr('href')).to eq '#title-with-spaces' + end + it 'appends a unique number to duplicates' do doc = filter(header(1, 'One') + header(2, 'One')) diff --git a/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb b/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb index fe26ebb8796..15ee8c40b55 100644 --- a/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb +++ b/spec/lib/gitlab/discussions_diff/highlight_cache_spec.rb @@ -3,31 +3,32 @@ require 'spec_helper' describe Gitlab::DiscussionsDiff::HighlightCache, :clean_gitlab_redis_cache do + def fake_file(offset) + { + text: 'foo', + type: 'new', + index: 2 + offset, + old_pos: 10 + offset, + new_pos: 11 + offset, + line_code: 'xpto', + rich_text: '<blips>blops</blips>' + } + end + + let(:mapping) do + { + 3 => [ + fake_file(0), + fake_file(1) + ], + 4 => [ + fake_file(2) + ] + } + end + describe '#write_multiple' do it 'sets multiple keys serializing content as JSON' do - mapping = { - 3 => [ - { - text: 'foo', - type: 'new', - index: 2, - old_pos: 10, - new_pos: 11, - line_code: 'xpto', - rich_text: '<blips>blops</blips>' - }, - { - text: 'foo', - type: 'new', - index: 3, - old_pos: 11, - new_pos: 12, - line_code: 'xpto', - rich_text: '<blops>blips</blops>' - } - ] - } - described_class.write_multiple(mapping) mapping.each do |key, value| @@ -41,53 +42,16 @@ describe Gitlab::DiscussionsDiff::HighlightCache, :clean_gitlab_redis_cache do describe '#read_multiple' do it 'reads multiple keys and serializes content into Gitlab::Diff::Line objects' do - mapping = { - 3 => [ - { - text: 'foo', - type: 'new', - index: 2, - old_pos: 11, - new_pos: 12, - line_code: 'xpto', - rich_text: '<blips>blops</blips>' - }, - { - text: 'foo', - type: 'new', - index: 3, - old_pos: 10, - new_pos: 11, - line_code: 'xpto', - rich_text: '<blips>blops</blips>' - } - ] - } - described_class.write_multiple(mapping) found = described_class.read_multiple(mapping.keys) - expect(found.size).to eq(1) + expect(found.size).to eq(2) expect(found.first.size).to eq(2) expect(found.first).to all(be_a(Gitlab::Diff::Line)) end it 'returns nil when cached key is not found' do - mapping = { - 3 => [ - { - text: 'foo', - type: 'new', - index: 2, - old_pos: 11, - new_pos: 12, - line_code: 'xpto', - rich_text: '<blips>blops</blips>' - } - ] - } - described_class.write_multiple(mapping) found = described_class.read_multiple([2, 3]) @@ -95,8 +59,30 @@ describe Gitlab::DiscussionsDiff::HighlightCache, :clean_gitlab_redis_cache do expect(found.size).to eq(2) expect(found.first).to eq(nil) - expect(found.second.size).to eq(1) + expect(found.second.size).to eq(2) expect(found.second).to all(be_a(Gitlab::Diff::Line)) end end + + describe '#clear_multiple' do + it 'removes all named keys' do + described_class.write_multiple(mapping) + + described_class.clear_multiple(mapping.keys) + + expect(described_class.read_multiple(mapping.keys)).to all(be_nil) + end + + it 'only removed named keys' do + to_clear, to_leave = mapping.keys + + described_class.write_multiple(mapping) + described_class.clear_multiple([to_clear]) + + cleared, left = described_class.read_multiple([to_clear, to_leave]) + + expect(cleared).to be_nil + expect(left).to all(be_a(Gitlab::Diff::Line)) + end + end end diff --git a/spec/lib/gitlab/git/object_pool_spec.rb b/spec/lib/gitlab/git/object_pool_spec.rb index 6511c2b61bf..ebeb7b7b633 100644 --- a/spec/lib/gitlab/git/object_pool_spec.rb +++ b/spec/lib/gitlab/git/object_pool_spec.rb @@ -7,8 +7,6 @@ describe Gitlab::Git::ObjectPool do let(:pool_repository) { create(:pool_repository) } let(:source_repository) { pool_repository.source_project.repository } - let(:source_repository_path) { File.join(TestEnv.repos_path, source_repository.relative_path) } - let(:source_repository_rugged) { Rugged::Repository.new(source_repository_path) } subject { pool_repository.object_pool } @@ -82,6 +80,8 @@ describe Gitlab::Git::ObjectPool do end describe '#fetch' do + let(:source_repository_path) { File.join(TestEnv.repos_path, source_repository.relative_path) } + let(:source_repository_rugged) { Rugged::Repository.new(source_repository_path) } let(:commit_count) { source_repository.commit_count } context "when the object's pool repository exists" do @@ -98,7 +98,7 @@ describe Gitlab::Git::ObjectPool do it "re-creates the object pool's repository" do subject.fetch - expect(subject.repository.exists?).to be(true) + expect(subject.repository.exists?).to be true end it 'does not raise an error' do diff --git a/spec/lib/gitlab/git/repository_cleaner_spec.rb b/spec/lib/gitlab/git/repository_cleaner_spec.rb index 6602f22843f..7bba0107e58 100644 --- a/spec/lib/gitlab/git/repository_cleaner_spec.rb +++ b/spec/lib/gitlab/git/repository_cleaner_spec.rb @@ -6,55 +6,62 @@ describe Gitlab::Git::RepositoryCleaner do let(:project) { create(:project, :repository) } let(:repository) { project.repository } let(:head_sha) { repository.head_commit.id } - let(:object_map_data) { "#{head_sha} #{'0' * 40}" } + let(:object_map_data) { "#{head_sha} #{Gitlab::Git::BLANK_SHA}" } - subject(:cleaner) { described_class.new(repository.raw) } + let(:clean_refs) { %W[refs/environments/1 refs/merge-requests/1 refs/keep-around/#{head_sha}] } + let(:keep_refs) { %w[refs/heads/_keep refs/tags/_keep] } - describe '#apply_bfg_object_map' do - let(:clean_refs) { %W[refs/environments/1 refs/merge-requests/1 refs/keep-around/#{head_sha}] } - let(:keep_refs) { %w[refs/heads/_keep refs/tags/_keep] } + subject(:cleaner) { described_class.new(repository.raw) } + shared_examples_for '#apply_bfg_object_map_stream' do before do (clean_refs + keep_refs).each { |ref| repository.create_ref(head_sha, ref) } end - context 'from StringIO' do - let(:object_map) { StringIO.new(object_map_data) } + it 'removes internal references' do + entries = [] - it 'removes internal references' do - cleaner.apply_bfg_object_map(object_map) + cleaner.apply_bfg_object_map_stream(object_map) do |rsp| + entries.concat(rsp.entries) + end - aggregate_failures do - clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy } - keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy } - end + aggregate_failures do + clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be(false) } + keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be(true) } + + expect(entries).to contain_exactly( + Gitaly::ApplyBfgObjectMapStreamResponse::Entry.new( + type: :COMMIT, + old_oid: head_sha, + new_oid: Gitlab::Git::BLANK_SHA + ) + ) end end + end - context 'from Gitlab::HttpIO' do - let(:url) { 'http://example.com/bfg_object_map.txt' } - let(:tempfile) { Tempfile.new } - let(:object_map) { Gitlab::HttpIO.new(url, object_map_data.size) } + describe '#apply_bfg_object_map_stream (from StringIO)' do + let(:object_map) { StringIO.new(object_map_data) } - around do |example| - tempfile.write(object_map_data) - tempfile.close + include_examples '#apply_bfg_object_map_stream' + end - example.run - ensure - tempfile.unlink - end + describe '#apply_bfg_object_map_stream (from Gitlab::HttpIO)' do + let(:url) { 'http://example.com/bfg_object_map.txt' } + let(:tempfile) { Tempfile.new } + let(:object_map) { Gitlab::HttpIO.new(url, object_map_data.size) } - it 'removes internal references' do - stub_remote_url_200(url, tempfile.path) + around do |example| + tempfile.write(object_map_data) + tempfile.close - cleaner.apply_bfg_object_map(object_map) + stub_remote_url_200(url, tempfile.path) - aggregate_failures do - clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy } - keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy } - end - end + example.run + ensure + tempfile.unlink end + + include_examples '#apply_bfg_object_map_stream' end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 0f6aac9b6de..7644d83992f 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -2215,4 +2215,43 @@ describe Gitlab::Git::Repository, :seed_helper do line.split("\t").last end end + + describe '#disconnect_alternates' do + let(:project) { create(:project, :repository) } + let(:pool_repository) { create(:pool_repository) } + let(:repository) { project.repository } + let(:repository_path) { File.join(TestEnv.repos_path, repository.relative_path) } + let(:object_pool) { pool_repository.object_pool } + let(:object_pool_path) { File.join(TestEnv.repos_path, object_pool.repository.relative_path) } + let(:object_pool_rugged) { Rugged::Repository.new(object_pool_path) } + + before do + object_pool.create + end + + it 'does not raise an error when disconnecting a non-linked repository' do + expect { repository.disconnect_alternates }.not_to raise_error + end + + it 'removes the alternates file' do + object_pool.link(repository) + + alternates_file = File.join(repository_path, "objects", "info", "alternates") + expect(File.exist?(alternates_file)).to be_truthy + + repository.disconnect_alternates + + expect(File.exist?(alternates_file)).to be_falsey + end + + it 'can still access objects in the object pool' do + object_pool.link(repository) + new_commit = new_commit_edit_old_file(object_pool_rugged) + expect(repository.commit(new_commit.oid).id).to eq(new_commit.oid) + + repository.disconnect_alternates + + expect(repository.commit(new_commit.oid).id).to eq(new_commit.oid) + end + end end diff --git a/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb b/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb index 369deff732a..c42332dc27b 100644 --- a/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/cleanup_service_spec.rb @@ -6,14 +6,14 @@ describe Gitlab::GitalyClient::CleanupService do let(:relative_path) { project.disk_path + '.git' } let(:client) { described_class.new(project.repository) } - describe '#apply_bfg_object_map' do - it 'sends an apply_bfg_object_map message' do + describe '#apply_bfg_object_map_stream' do + it 'sends an apply_bfg_object_map_stream message' do expect_any_instance_of(Gitaly::CleanupService::Stub) - .to receive(:apply_bfg_object_map) + .to receive(:apply_bfg_object_map_stream) .with(kind_of(Enumerator), kind_of(Hash)) - .and_return(double) + .and_return([]) - client.apply_bfg_object_map(StringIO.new) + client.apply_bfg_object_map_stream(StringIO.new) end end end diff --git a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb index 46ca2340389..09de7ca6afd 100644 --- a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb @@ -231,4 +231,34 @@ describe Gitlab::GitalyClient::RepositoryService do client.raw_changes_between('deadbeef', 'deadpork') end end + + describe '#disconnect_alternates' do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:repository_path) { File.join(TestEnv.repos_path, repository.relative_path) } + let(:pool_repository) { create(:pool_repository) } + let(:object_pool) { pool_repository.object_pool } + let(:object_pool_service) { Gitlab::GitalyClient::ObjectPoolService.new(object_pool) } + + before do + object_pool_service.create(repository) + object_pool_service.link_repository(repository) + end + + it 'deletes the alternates file' do + repository.disconnect_alternates + + alternates_file = File.join(repository_path, "objects", "info", "alternates") + + expect(File.exist?(alternates_file)).to be_falsey + end + + context 'when called twice' do + it "doesn't raise an error" do + repository.disconnect_alternates + + expect { repository.disconnect_alternates }.not_to raise_error + end + end + end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 482e9c05da8..2242543daad 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -99,7 +99,7 @@ merge_requests: - timelogs - head_pipeline - latest_merge_request_diff -- merge_request_pipelines +- pipelines_for_merge_request - merge_request_assignees - suggestions - assignees diff --git a/spec/lib/gitlab/namespaced_session_store_spec.rb b/spec/lib/gitlab/namespaced_session_store_spec.rb new file mode 100644 index 00000000000..c0af2ede32a --- /dev/null +++ b/spec/lib/gitlab/namespaced_session_store_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::NamespacedSessionStore do + let(:key) { :some_key } + subject { described_class.new(key) } + + it 'stores data under the specified key' do + Gitlab::Session.with_session({}) do + subject[:new_data] = 123 + + expect(Thread.current[:session_storage][key]).to eq(new_data: 123) + end + end + + it 'retrieves data from the given key' do + Thread.current[:session_storage] = { key => { existing_data: 123 } } + + expect(subject[:existing_data]).to eq 123 + end +end diff --git a/spec/lib/gitlab/session_spec.rb b/spec/lib/gitlab/session_spec.rb new file mode 100644 index 00000000000..8db73f0ec7b --- /dev/null +++ b/spec/lib/gitlab/session_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Session do + it 'uses the current thread as a data store' do + Thread.current[:session_storage] = { a: :b } + + expect(described_class.current).to eq(a: :b) + ensure + Thread.current[:session_storage] = nil + end + + describe '#with_session' do + it 'sets session hash' do + described_class.with_session(one: 1) do + expect(described_class.current).to eq(one: 1) + end + end + + it 'restores current store after' do + described_class.with_session(two: 2) { } + + expect(described_class.current).to eq nil + end + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 8c73f37bd32..9b489baf163 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2817,7 +2817,7 @@ describe Ci::Build do context 'when ref is merge request' do let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } context 'when ref is protected' do @@ -2875,7 +2875,7 @@ describe Ci::Build do context 'when ref is merge request' do let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } context 'when ref is protected' do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index af455a72f50..a0319b3eb0a 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -466,7 +466,7 @@ describe Ci::Pipeline, :mailer do target_branch: 'master') end - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } it 'does not return the pipeline' do is_expected.to be_empty diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb index 6805731fed3..66b25c77430 100644 --- a/spec/models/concerns/has_ref_spec.rb +++ b/spec/models/concerns/has_ref_spec.rb @@ -19,7 +19,7 @@ describe HasRef do context 'when it was triggered by merge request' do let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } let(:build) { create(:ci_build, pipeline: pipeline) } it 'returns false' do @@ -68,7 +68,7 @@ describe HasRef do context 'when it is triggered by a merge request' do let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } let(:build) { create(:ci_build, tag: false, pipeline: pipeline) } it 'returns nil' do diff --git a/spec/models/note_diff_file_spec.rb b/spec/models/note_diff_file_spec.rb index 99eeac8d778..b15bedd257e 100644 --- a/spec/models/note_diff_file_spec.rb +++ b/spec/models/note_diff_file_spec.rb @@ -10,4 +10,31 @@ describe NoteDiffFile do describe 'validations' do it { is_expected.to validate_presence_of(:diff_note) } end + + describe '.referencing_sha' do + let!(:diff_note) { create(:diff_note_on_commit) } + + let(:note_diff_file) { diff_note.note_diff_file } + let(:project) { diff_note.project } + + it 'finds note diff files by project and sha' do + found = described_class.referencing_sha(diff_note.commit_id, project_id: project.id) + + expect(found).to contain_exactly(note_diff_file) + end + + it 'excludes note diff files with the wrong project' do + other_project = create(:project) + + found = described_class.referencing_sha(diff_note.commit_id, project_id: other_project.id) + + expect(found).to be_empty + end + + it 'excludes note diff files with the wrong sha' do + found = described_class.referencing_sha(Gitlab::Git::BLANK_SHA, project_id: project.id) + + expect(found).to be_empty + end + end end diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb index dba7fd91747..47f767ae4ab 100644 --- a/spec/serializers/pipeline_entity_spec.rb +++ b/spec/serializers/pipeline_entity_spec.rb @@ -137,7 +137,7 @@ describe PipelineEntity do context 'when pipeline is detached merge request pipeline' do let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:project) { merge_request.target_project } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } it 'makes detached flag true' do expect(subject[:flags][:detached_merge_request_pipeline]).to be_truthy @@ -185,7 +185,7 @@ describe PipelineEntity do context 'when pipeline is merge request pipeline' do let(:merge_request) { create(:merge_request, :with_merge_request_pipeline, merge_sha: 'abc') } let(:project) { merge_request.target_project } - let(:pipeline) { merge_request.merge_request_pipelines.first } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } it 'makes detached flag false' do expect(subject[:flags][:detached_merge_request_pipeline]).to be_falsy diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index c795176a1e4..ed48f4b1e44 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -195,7 +195,7 @@ describe MergeRequests::CreateService do expect(merge_request).to be_persisted merge_request.reload - expect(merge_request.merge_request_pipelines.count).to eq(1) + expect(merge_request.pipelines_for_merge_request.count).to eq(1) expect(merge_request.actual_head_pipeline).to be_detached_merge_request_pipeline end @@ -247,7 +247,7 @@ describe MergeRequests::CreateService do expect(merge_request).to be_persisted merge_request.reload - expect(merge_request.merge_request_pipelines.count).to eq(0) + expect(merge_request.pipelines_for_merge_request.count).to eq(0) end end @@ -281,7 +281,7 @@ describe MergeRequests::CreateService do expect(merge_request).to be_persisted merge_request.reload - expect(merge_request.merge_request_pipelines.count).to eq(0) + expect(merge_request.pipelines_for_merge_request.count).to eq(0) end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d20b2d81763..7258428589f 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -166,8 +166,8 @@ describe MergeRequests::RefreshService do it 'create detached merge request pipeline with commits' do expect { subject } - .to change { @merge_request.merge_request_pipelines.count }.by(1) - .and change { @another_merge_request.merge_request_pipelines.count }.by(0) + .to change { @merge_request.pipelines_for_merge_request.count }.by(1) + .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0) expect(@merge_request.has_commits?).to be_truthy expect(@another_merge_request.has_commits?).to be_falsy @@ -175,13 +175,13 @@ describe MergeRequests::RefreshService do it 'does not create detached merge request pipeline for forked project' do expect { subject } - .not_to change { @fork_merge_request.merge_request_pipelines.count } + .not_to change { @fork_merge_request.pipelines_for_merge_request.count } end it 'create detached merge request pipeline for non-fork merge request' do subject - expect(@merge_request.merge_request_pipelines.first) + expect(@merge_request.pipelines_for_merge_request.first) .to be_detached_merge_request_pipeline end @@ -190,7 +190,7 @@ describe MergeRequests::RefreshService do it 'does not create detached merge request pipeline' do expect { subject } - .not_to change { @merge_request.merge_request_pipelines.count } + .not_to change { @merge_request.pipelines_for_merge_request.count } end end @@ -199,9 +199,9 @@ describe MergeRequests::RefreshService do it 'creates legacy detached merge request pipeline for fork merge request' do expect { subject } - .to change { @fork_merge_request.merge_request_pipelines.count }.by(1) + .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) - expect(@fork_merge_request.merge_request_pipelines.first) + expect(@fork_merge_request.pipelines_for_merge_request.first) .to be_legacy_detached_merge_request_pipeline end end @@ -214,7 +214,7 @@ describe MergeRequests::RefreshService do it 'create legacy detached merge request pipeline for non-fork merge request' do subject - expect(@merge_request.merge_request_pipelines.first) + expect(@merge_request.pipelines_for_merge_request.first) .to be_legacy_detached_merge_request_pipeline end end @@ -245,11 +245,11 @@ describe MergeRequests::RefreshService do it 'does not re-create a duplicate detached merge request pipeline' do expect do service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') - end.to change { @merge_request.merge_request_pipelines.count }.by(1) + end.to change { @merge_request.pipelines_for_merge_request.count }.by(1) expect do service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') - end.not_to change { @merge_request.merge_request_pipelines.count } + end.not_to change { @merge_request.pipelines_for_merge_request.count } end end end @@ -266,7 +266,7 @@ describe MergeRequests::RefreshService do it 'does not create a detached merge request pipeline' do expect { subject } - .not_to change { @merge_request.merge_request_pipelines.count } + .not_to change { @merge_request.pipelines_for_merge_request.count } end end end diff --git a/spec/services/projects/cleanup_service_spec.rb b/spec/services/projects/cleanup_service_spec.rb index 29eabc86327..5c246854eb7 100644 --- a/spec/services/projects/cleanup_service_spec.rb +++ b/spec/services/projects/cleanup_service_spec.rb @@ -6,13 +6,13 @@ describe Projects::CleanupService do let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) } let(:object_map) { project.bfg_object_map } + let(:cleaner) { service.__send__(:repository_cleaner) } + subject(:service) { described_class.new(project) } describe '#execute' do - it 'runs the apply_bfg_object_map gitaly RPC' do - expect_next_instance_of(Gitlab::Git::RepositoryCleaner) do |cleaner| - expect(cleaner).to receive(:apply_bfg_object_map).with(kind_of(IO)) - end + it 'runs the apply_bfg_object_map_stream gitaly RPC' do + expect(cleaner).to receive(:apply_bfg_object_map_stream).with(kind_of(IO)) service.execute end @@ -37,10 +37,91 @@ describe Projects::CleanupService do expect(object_map.exists?).to be_falsy end + context 'with a tainted merge request diff' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:diff) { merge_request.merge_request_diff } + let(:entry) { build_entry(diff.commits.first.id) } + + before do + allow(cleaner) + .to receive(:apply_bfg_object_map_stream) + .and_yield(Gitaly::ApplyBfgObjectMapStreamResponse.new(entries: [entry])) + end + + it 'removes the tainted commit from the database' do + service.execute + + expect(MergeRequestDiff.exists?(diff.id)).to be_falsy + end + + it 'ignores non-commit responses from Gitaly' do + entry.type = :UNKNOWN + + service.execute + + expect(MergeRequestDiff.exists?(diff.id)).to be_truthy + end + end + + context 'with a tainted diff note' do + let(:diff_note) { create(:diff_note_on_commit, project: project) } + let(:note_diff_file) { diff_note.note_diff_file } + let(:entry) { build_entry(diff_note.commit_id) } + + let(:highlight_cache) { Gitlab::DiscussionsDiff::HighlightCache } + let(:cache_id) { note_diff_file.id } + + before do + allow(cleaner) + .to receive(:apply_bfg_object_map_stream) + .and_yield(Gitaly::ApplyBfgObjectMapStreamResponse.new(entries: [entry])) + end + + it 'removes the tainted commit from the database' do + service.execute + + expect(NoteDiffFile.exists?(note_diff_file.id)).to be_falsy + end + + it 'removes the highlight cache from redis' do + write_cache(highlight_cache, cache_id, [{}]) + + expect(read_cache(highlight_cache, cache_id)).not_to be_nil + + service.execute + + expect(read_cache(highlight_cache, cache_id)).to be_nil + end + + it 'ignores non-commit responses from Gitaly' do + entry.type = :UNKNOWN + + service.execute + + expect(NoteDiffFile.exists?(note_diff_file.id)).to be_truthy + end + end + it 'raises an error if no object map can be found' do object_map.remove! expect { service.execute }.to raise_error(described_class::NoUploadError) end end + + def build_entry(old_oid) + Gitaly::ApplyBfgObjectMapStreamResponse::Entry.new( + type: :COMMIT, + old_oid: old_oid, + new_oid: Gitlab::Git::BLANK_SHA + ) + end + + def read_cache(cache, key) + cache.read_multiple([key]).first + end + + def write_cache(cache, key, value) + cache.write_multiple(key => value) + end end diff --git a/spec/support/helpers/filtered_search_helpers.rb b/spec/support/helpers/filtered_search_helpers.rb index 03057a102c5..34ef185ea27 100644 --- a/spec/support/helpers/filtered_search_helpers.rb +++ b/spec/support/helpers/filtered_search_helpers.rb @@ -78,20 +78,17 @@ module FilteredSearchHelpers # .tokens-container to make sure the correct names and values are rendered def expect_tokens(tokens) page.within '.filtered-search-box .tokens-container' do - page.all(:css, '.tokens-container li .selectable').each_with_index do |el, index| - token_name = tokens[index][:name] - token_value = tokens[index][:value] - token_emoji = tokens[index][:emoji_name] + token_elements = page.all(:css, 'li.filtered-search-token') - expect(el.find('.name')).to have_content(token_name) + tokens.each_with_index do |token, index| + el = token_elements[index] - if token_value - expect(el.find('.value')).to have_content(token_value) - end + expect(el.find('.name')).to have_content(token[:name]) + expect(el.find('.value')).to have_content(token[:value]) if token[:value].present? # gl-emoji content is blank when the emoji unicode is not supported - if token_emoji - selector = %(gl-emoji[data-name="#{token_emoji}"]) + if token[:emoji_name].present? + selector = %(gl-emoji[data-name="#{token[:emoji_name]}"]) expect(el.find('.value')).to have_css(selector) end end |