From 18ca66076fc70059f109a1f8eaaa76115db727a8 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 24 Aug 2023 21:08:59 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- Gemfile | 2 +- Gemfile.checksum | 4 +- Gemfile.lock | 6 +- .../list/queries/issue.fragment.graphql | 1 + .../issues/show/components/app.vue | 1 + .../issues/show/components/sticky_header.vue | 9 +- .../super_sidebar/components/create_menu.vue | 2 +- .../list/components/issuable_item.vue | 42 +- .../merge_requests/ff_merge_service.rb | 31 - .../merge_requests/merge_base_service.rb | 36 - app/services/merge_requests/merge_service.rb | 91 +- .../merge_requests/merge_to_ref_service.rb | 8 +- .../optimize_group_template_query.yml | 2 +- .../development/refactor_merge_service.yml | 8 - config/initializers/click_house.rb | 3 +- .../sidekiq/sidekiq_job_migration.md | 2 +- doc/ci/variables/predefined_variables.md | 4 +- .../clickhouse/clickhouse_within_gitlab.md | 198 +++ doc/development/database/index.md | 1 + doc/integration/jira/troubleshooting.md | 2 +- doc/user/project/releases/index.md | 28 +- .../lib/click_house/client.rb | 12 +- .../click_house/client/bind_index_manager.rb | 17 + .../lib/click_house/client/database.rb | 3 +- .../lib/click_house/client/query.rb | 68 ++ .../lib/click_house/client/query_like.rb | 19 + .../client}/bind_index_manager_spec.rb | 2 +- .../spec/click_house/client/database_spec.rb | 1 - .../click_house/client/query_like_spec.rb | 15 + .../spec/click_house/client/query_spec.rb | 125 ++ .../spec/click_house/client_spec.rb | 2 +- lib/click_house/bind_index_manager.rb | 15 - lib/click_house/query_builder.rb | 6 +- lib/click_house/redactor.rb | 3 +- lib/peek/views/click_house.rb | 2 +- spec/factories/users.rb | 4 + spec/features/dashboard/navbar_spec.rb | 2 +- spec/features/groups/navbar_spec.rb | 2 +- spec/features/projects/navbar_spec.rb | 5 +- spec/features/snippets/show_spec.rb | 4 +- spec/features/webauthn_spec.rb | 17 +- spec/frontend/fixtures/issues.rb | 2 +- spec/frontend/fixtures/snippet.rb | 4 +- spec/frontend/issues/dashboard/mock_data.js | 1 + spec/frontend/issues/list/mock_data.js | 1 + .../show/components/sticky_header_spec.js | 8 + .../issues/show/mock_data/mock_data.js | 3 +- .../components/create_menu_spec.js | 4 +- .../list/components/issuable_item_spec.js | 16 +- .../vue_shared/issuable/list/mock_data.js | 2 +- spec/lib/click_house/query_builder_spec.rb | 26 +- .../database/click_house_client_spec.rb | 14 +- .../merge_requests/ff_merge_service_spec.rb | 144 --- .../merge_requests/merge_service_spec.rb | 1075 ++++++++--------- .../project_integrations_shared_context.rb | 2 +- .../features/2fa_shared_examples.rb | 4 +- ...ures_apply_to_issuables_shared_examples.rb | 4 +- .../features/snippets_shared_examples.rb | 2 +- 58 files changed, 1174 insertions(+), 943 deletions(-) delete mode 100644 app/services/merge_requests/ff_merge_service.rb delete mode 100644 config/feature_flags/development/refactor_merge_service.yml create mode 100644 doc/development/database/clickhouse/clickhouse_within_gitlab.md create mode 100644 gems/click_house-client/lib/click_house/client/bind_index_manager.rb create mode 100644 gems/click_house-client/lib/click_house/client/query.rb create mode 100644 gems/click_house-client/lib/click_house/client/query_like.rb rename {spec/lib/click_house => gems/click_house-client/spec/click_house/client}/bind_index_manager_spec.rb (91%) create mode 100644 gems/click_house-client/spec/click_house/client/query_like_spec.rb create mode 100644 gems/click_house-client/spec/click_house/client/query_spec.rb delete mode 100644 lib/click_house/bind_index_manager.rb delete mode 100644 spec/services/merge_requests/ff_merge_service_spec.rb diff --git a/Gemfile b/Gemfile index b1bcdacfff5..5ebf99c2ea9 100644 --- a/Gemfile +++ b/Gemfile @@ -360,7 +360,7 @@ gem 'gitlab-labkit', '~> 0.34.0' gem 'thrift', '>= 0.16.0' # I18n -gem 'ruby_parser', '~> 3.20', require: false +gem 'ruby_parser', '~> 3.20.3', require: false gem 'rails-i18n', '~> 7.0' gem 'gettext_i18n_rails', '~> 1.11.0' gem 'gettext_i18n_rails_js', '~> 1.3' diff --git a/Gemfile.checksum b/Gemfile.checksum index 976fa3a14c4..2a0d3617b1a 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -548,7 +548,7 @@ {"name":"ruby-saml","version":"1.15.0","platform":"ruby","checksum":"3a9dda2b448310f4f90d5cf0967d4b668530fa7994d2a4d9cbfdfa62e35f76a3"}, {"name":"ruby-statistics","version":"3.0.0","platform":"ruby","checksum":"610301370346931cb701e3a8d3d3e28eb65681162cae6066c0c11abf20efdc81"}, {"name":"ruby2_keywords","version":"0.0.5","platform":"ruby","checksum":"ffd13740c573b7301cf7a2e61fc857b2a8e3d3aff32545d6f8300d8bae10e3ef"}, -{"name":"ruby_parser","version":"3.20.0","platform":"ruby","checksum":"17d0c8bbef7fcdf99b1070bb2555d49111758f75d312e8799f66df831ebdcbe3"}, +{"name":"ruby_parser","version":"3.20.3","platform":"ruby","checksum":"8d2289a695dc81ffddcdd5a56e80c9a109806bc0d0b1239a1c852b0c71251c49"}, {"name":"rubyntlm","version":"0.6.3","platform":"ruby","checksum":"5b321456dba3130351f7451f8669f1afa83a0d26fd63cdec285b7b88e667102d"}, {"name":"rubypants","version":"0.2.0","platform":"ruby","checksum":"f07e38eac793655a0323fe91946081052341b9e69807026fcf102346589eedee"}, {"name":"rubyzip","version":"2.3.2","platform":"ruby","checksum":"3f57e3935dc2255c414484fbf8d673b4909d8a6a57007ed754dde39342d2373f"}, @@ -571,7 +571,7 @@ {"name":"sentry-ruby","version":"5.8.0","platform":"ruby","checksum":"caeb121433be379fb94e991a45265a287b13a9a9083e7264f539752369d37110"}, {"name":"sentry-sidekiq","version":"5.8.0","platform":"ruby","checksum":"90d1123d16a9fc5fd99dbad190b766dd189eaf9e2baddad641f1334e1877c779"}, {"name":"set","version":"1.0.2","platform":"ruby","checksum":"02ffa4de1f2621495e05b72326040dd014d7abbcb02fea698bc600a389992c02"}, -{"name":"sexp_processor","version":"4.16.1","platform":"ruby","checksum":"5caadbf4bbe5ab539cb892a5bcf74ca33a2f2a897cecafdee4a63be79b4819dc"}, +{"name":"sexp_processor","version":"4.17.0","platform":"ruby","checksum":"4daa4874ce1838cd801c65e66ed5d4f140024404a3de7482c36d4ef2604dff6f"}, {"name":"shellany","version":"0.0.1","platform":"ruby","checksum":"0e127a9132698766d7e752e82cdac8250b6adbd09e6c0a7fbbb6f61964fedee7"}, {"name":"shoulda-matchers","version":"5.1.0","platform":"ruby","checksum":"a01d20589989e9653ab4a28c67d9db2b82bcf0a2496cf01d5e1a95a4aaaf5b07"}, {"name":"sidekiq","version":"6.5.7","platform":"ruby","checksum":"7d966fd84d42a942615d6874be31e40f8bece841fdd9b96fc53cad22a590555c"}, diff --git a/Gemfile.lock b/Gemfile.lock index fd4ce6feee0..58cc5cc193c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1425,7 +1425,7 @@ GEM rexml ruby-statistics (3.0.0) ruby2_keywords (0.0.5) - ruby_parser (3.20.0) + ruby_parser (3.20.3) sexp_processor (~> 4.16) rubyntlm (0.6.3) rubypants (0.2.0) @@ -1476,7 +1476,7 @@ GEM sentry-ruby (~> 5.8.0) sidekiq (>= 3.0) set (1.0.2) - sexp_processor (4.16.1) + sexp_processor (4.17.0) shellany (0.0.1) shoulda-matchers (5.1.0) activesupport (>= 5.2.0) @@ -1979,7 +1979,7 @@ DEPENDENCIES ruby-openai (~> 3.7) ruby-progressbar (~> 1.10) ruby-saml (~> 1.15.0) - ruby_parser (~> 3.20) + ruby_parser (~> 3.20.3) rubyzip (~> 2.3.2) rugged (~> 1.6) sanitize (~> 6.0) diff --git a/app/assets/javascripts/issues/list/queries/issue.fragment.graphql b/app/assets/javascripts/issues/list/queries/issue.fragment.graphql index 3b49c0efb14..f3173f0e33a 100644 --- a/app/assets/javascripts/issues/list/queries/issue.fragment.graphql +++ b/app/assets/javascripts/issues/list/queries/issue.fragment.graphql @@ -11,6 +11,7 @@ fragment IssueFragment on Issue { moved state title + titleHtml updatedAt closedAt upvotes diff --git a/app/assets/javascripts/issues/show/components/app.vue b/app/assets/javascripts/issues/show/components/app.vue index d31b56c0277..d59692d2a28 100644 --- a/app/assets/javascripts/issues/show/components/app.vue +++ b/app/assets/javascripts/issues/show/components/app.vue @@ -550,6 +550,7 @@ export default { :issuable-type="issuableType" :show="isStickyHeaderShowing" :title="state.titleText" + :title-html="state.titleHtml" @hide="hideStickyHeader" @show="showStickyHeader" /> diff --git a/app/assets/javascripts/issues/show/components/sticky_header.vue b/app/assets/javascripts/issues/show/components/sticky_header.vue index b8e0937d51c..bcf10ee92bb 100644 --- a/app/assets/javascripts/issues/show/components/sticky_header.vue +++ b/app/assets/javascripts/issues/show/components/sticky_header.vue @@ -6,6 +6,7 @@ import { TYPE_EPIC, WORKSPACE_PROJECT, } from '~/issues/constants'; +import SafeHtml from '~/vue_shared/directives/safe_html'; import ConfidentialityBadge from '~/vue_shared/components/confidentiality_badge.vue'; export default { @@ -18,6 +19,7 @@ export default { }, directives: { GlTooltip: GlTooltipDirective, + SafeHtml, }, props: { isConfidential: { @@ -52,6 +54,10 @@ export default { type: String, required: true, }, + titleHtml: { + type: String, + required: true, + }, }, computed: { isClosed() { @@ -112,11 +118,10 @@ export default { - {{ title }} diff --git a/app/assets/javascripts/super_sidebar/components/create_menu.vue b/app/assets/javascripts/super_sidebar/components/create_menu.vue index 7918f8c8840..cea87a70878 100644 --- a/app/assets/javascripts/super_sidebar/components/create_menu.vue +++ b/app/assets/javascripts/super_sidebar/components/create_menu.vue @@ -14,7 +14,7 @@ import { import { DROPDOWN_Y_OFFSET, IMPERSONATING_OFFSET } from '../constants'; // Left offset required for the dropdown to be aligned with the super sidebar -const DROPDOWN_X_OFFSET_BASE = -147; +const DROPDOWN_X_OFFSET_BASE = -179; const DROPDOWN_X_OFFSET_IMPERSONATING = DROPDOWN_X_OFFSET_BASE + IMPERSONATING_OFFSET; export default { diff --git a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue index 31dd49ca415..0c1ad861889 100644 --- a/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue +++ b/app/assets/javascripts/vue_shared/issuable/list/components/issuable_item.vue @@ -8,6 +8,7 @@ import { isExternal, setUrlFragment } from '~/lib/utils/url_utility'; import { __, n__, sprintf } from '~/locale'; import IssuableAssignees from '~/issuable/components/issue_assignees.vue'; import timeagoMixin from '~/vue_shared/mixins/timeago'; +import SafeHtml from '~/vue_shared/directives/safe_html'; import WorkItemTypeIcon from '~/work_items/components/work_item_type_icon.vue'; import { STATE_CLOSED } from '~/work_items/constants'; import { isAssigneesWidget, isLabelsWidget } from '~/work_items/utils'; @@ -24,6 +25,7 @@ export default { }, directives: { GlTooltip: GlTooltipDirective, + SafeHtml, }, mixins: [timeagoMixin], props: { @@ -86,6 +88,9 @@ export default { authorId() { return getIdFromGraphQLId(this.author.id); }, + isIssueTrackerExternal() { + return Boolean(this.issuable.externalTracker); + }, isIssuableUrlExternal() { return isExternal(this.webUrl); }, @@ -259,18 +264,33 @@ export default { :title="__('This issue is hidden because its author has been banned')" :aria-label="__('Hidden')" /> - - {{ issuable.title }} + + e - handle_merge_error(log_message: e.message, save_message_on_model: true) - ensure - exclusive_lease(merge_request.id).cancel - end - - def execute_v2(merge_request, options = {}) return if merge_request.merged? return unless exclusive_lease(merge_request.id).try_obtain @@ -61,7 +29,7 @@ module MergeRequests validate! merge_request.in_locked_state do - if commit_v2 + if commit after_merge clean_merge_jid success @@ -90,28 +58,8 @@ module MergeRequests end end - # Can remove this entire method when :refactor_merge_service is enabled - def error_check! - super - - return if Feature.enabled?(:refactor_merge_service, project) - - check_source - - error = - if @merge_request.should_be_rebased? - 'Only fast-forward merge is allowed for your project. Please update your source branch' - elsif !@merge_request.mergeable?(skip_discussions_check: @options[:skip_discussions_check], check_mergeability_retry_lease: @options[:check_mergeability_retry_lease]) - 'Merge request is not mergeable' - elsif !@merge_request.squash && project.squash_always? - 'This project requires squashing commits when merge requests are accepted.' - end - - raise_error(error) if error - end - def validate_strategy! - @merge_strategy.validate! if Feature.enabled?(:refactor_merge_service, project) + @merge_strategy.validate! end def updated_check! @@ -121,7 +69,7 @@ module MergeRequests end end - def commit_v2 + def commit log_info("Git merge started on JID #{merge_jid}") merge_result = try_merge { @merge_strategy.execute_git_merge! } @@ -140,35 +88,6 @@ module MergeRequests log_info("Merge request marked in progress") end - def commit - log_info("Git merge started on JID #{merge_jid}") - commit_id = try_merge { execute_git_merge } - - if commit_id - log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}") - else - raise_error(GENERIC_ERROR_MESSAGE) - end - - update_merge_sha_metadata(commit_id) - - commit_id - ensure - merge_request.update_and_mark_in_progress_merge_commit_sha(nil) - log_info("Merge request marked in progress") - end - - def update_merge_sha_metadata(commit_id) - data_to_update = merge_success_data(commit_id) - data_to_update[:squash_commit_sha] = source if merge_request.squash_on_merge? - - merge_request.update!(**data_to_update) if data_to_update.present? - end - - def merge_success_data(commit_id) - { merge_commit_sha: commit_id } - end - def try_merge yield rescue Gitlab::Git::PreReceiveError => e @@ -178,10 +97,6 @@ module MergeRequests raise_error(GENERIC_ERROR_MESSAGE) end - def execute_git_merge - repository.merge(current_user, source, merge_request, commit_message) - end - def after_merge log_info("Post merge started on JID #{merge_jid} with state #{state}") MergeRequests::PostMergeService.new(project: project, current_user: current_user).execute(merge_request) diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb index 8b79feb5e0f..6e1b56d9651 100644 --- a/app/services/merge_requests/merge_to_ref_service.rb +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -31,14 +31,13 @@ module MergeRequests private - override :source def source merge_request.diff_head_sha end override :error_check! def error_check! - check_source + raise_error('No source for merge') unless source end ## @@ -55,6 +54,11 @@ module MergeRequests params[:first_parent_ref] || merge_request.target_branch_ref end + def commit_message + params[:commit_message] || + merge_request.default_merge_commit_message(user: current_user) + end + def extracted_merge_to_ref repository.merge_to_ref(current_user, source_sha: source, diff --git a/config/feature_flags/development/optimize_group_template_query.yml b/config/feature_flags/development/optimize_group_template_query.yml index d5dc8b16476..68cd7bb1e03 100644 --- a/config/feature_flags/development/optimize_group_template_query.yml +++ b/config/feature_flags/development/optimize_group_template_query.yml @@ -5,4 +5,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/422390 milestone: '16.4' type: development group: group::source code -default_enabled: false +default_enabled: true diff --git a/config/feature_flags/development/refactor_merge_service.yml b/config/feature_flags/development/refactor_merge_service.yml deleted file mode 100644 index cb0734cf71c..00000000000 --- a/config/feature_flags/development/refactor_merge_service.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: refactor_merge_service -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128177 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/420949 -milestone: '16.3' -type: development -group: group::pipeline execution -default_enabled: false diff --git a/config/initializers/click_house.rb b/config/initializers/click_house.rb index 481942d775e..bd063f1558e 100644 --- a/config/initializers/click_house.rb +++ b/config/initializers/click_house.rb @@ -20,8 +20,9 @@ ClickHouse::Client.configure do |config| config.json_parser = Gitlab::Json config.http_post_proc = ->(url, headers, body) do options = { + multipart: true, headers: headers, - body: ActiveSupport::Gzip.compress(body), + body: body, allow_local_requests: Rails.env.development? || Rails.env.test? } diff --git a/doc/administration/sidekiq/sidekiq_job_migration.md b/doc/administration/sidekiq/sidekiq_job_migration.md index 89da174eb34..10a1faea850 100644 --- a/doc/administration/sidekiq/sidekiq_job_migration.md +++ b/doc/administration/sidekiq/sidekiq_job_migration.md @@ -26,7 +26,7 @@ If the Sidekiq routing rules are changed, administrators need to take care with Step 4 involves rewriting some Sidekiq job data for jobs that are already stored in Redis, but due to run in future. There are two sets of jobs to run in future: scheduled jobs and jobs to be retried. We provide a separate Rake task to migrate each set: - `gitlab:sidekiq:migrate_jobs:retry` for jobs to be retried. -- `gitlab:sidekiq:migrate_jobs:scheduled` for scheduled jobs. +- `gitlab:sidekiq:migrate_jobs:schedule` for scheduled jobs. Queued jobs that are yet to be run can also be migrated with a Rake task ([available in GitLab 15.6](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101348) and later): diff --git a/doc/ci/variables/predefined_variables.md b/doc/ci/variables/predefined_variables.md index 24cca42a04f..9bceeeac02c 100644 --- a/doc/ci/variables/predefined_variables.md +++ b/doc/ci/variables/predefined_variables.md @@ -165,9 +165,9 @@ These variables are available when: | `CI_MERGE_REQUEST_PROJECT_PATH` | 11.6 | all | The path of the project of the merge request. For example `namespace/awesome-project`. | | `CI_MERGE_REQUEST_PROJECT_URL` | 11.6 | all | The URL of the project of the merge request. For example, `http://192.168.10.15:3000/namespace/awesome-project`. | | `CI_MERGE_REQUEST_REF_PATH` | 11.6 | all | The ref path of the merge request. For example, `refs/merge-requests/1/head`. | -| `CI_MERGE_REQUEST_SQUASH_ON_MERGE` | 16.3 | all | `true` when the [squash on merge](../../user/project/merge_requests/squash_and_merge.md) option is set. | +| `CI_MERGE_REQUEST_SQUASH_ON_MERGE` | 16.4 | all | `true` when the [squash on merge](../../user/project/merge_requests/squash_and_merge.md) option is set. | | `CI_MERGE_REQUEST_SOURCE_BRANCH_NAME` | 11.6 | all | The source branch name of the merge request. | -| `CI_MERGE_REQUEST_SOURCE_BRANCH_PROTECTED` | 16.3 | all | `true` when the source branch of the merge request is [protected](../../user/project/protected_branches.md). | +| `CI_MERGE_REQUEST_SOURCE_BRANCH_PROTECTED` | 16.4 | all | `true` when the source branch of the merge request is [protected](../../user/project/protected_branches.md). | | `CI_MERGE_REQUEST_SOURCE_BRANCH_SHA` | 11.9 | all | The HEAD SHA of the source branch of the merge request. The variable is empty in merge request pipelines. The SHA is present only in [merged results pipelines](../pipelines/merged_results_pipelines.md). | | `CI_MERGE_REQUEST_SOURCE_PROJECT_ID` | 11.6 | all | The ID of the source project of the merge request. | | `CI_MERGE_REQUEST_SOURCE_PROJECT_PATH` | 11.6 | all | The path of the source project of the merge request. | diff --git a/doc/development/database/clickhouse/clickhouse_within_gitlab.md b/doc/development/database/clickhouse/clickhouse_within_gitlab.md new file mode 100644 index 00000000000..597b1732abb --- /dev/null +++ b/doc/development/database/clickhouse/clickhouse_within_gitlab.md @@ -0,0 +1,198 @@ +--- +stage: Data Stores +group: Database +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# ClickHouse within GitLab + +This document gives a high-level overview of how to develop features using ClickHouse in the GitLab Rails application. + +NOTE: +Most of the tooling and APIs are considered unstable. + +## GDK setup + +For instructions on how to set up a ClickHouse server locally, see the [ClickHouse installation documentation](https://clickhouse.com/docs/en/install). + +### Configure your Rails application + +1. Copy the example file and configure the credentials: + + ```shell + cp config/click_house.yml.example + config/click_house.yml + ``` + +1. Create the database using the `clickhouse-client` CLI tool: + + ```shell + clickhouse-client --password + ``` + + ```sql + create database gitlab_clickhouse_development; + ``` + +### Validate your setup + +Run the Rails console and invoke a simple query: + +```ruby +ClickHouse::Client.select('SELECT 1', :main) +# => [{"1"=>1}] +``` + +## Database schema and migrations + +For the ClickHouse database there are no established schema migration procedures yet. We have very basic tooling to build up the database schema in the test environment from scratch using timestamp-prefixed SQL files. + +You can create a table by placing a new SQL file in the `db/click_house/main` folder: + +```sql +// 20230811124511_create_issues.sql +CREATE TABLE issues +( + id UInt64 DEFAULT 0, + title String DEFAULT '' +) +ENGINE = MergeTree +PRIMARY KEY (id) +``` + +When you're working locally in your development environment, you can create or re-create your table schema by executing the respective `CREATE TABLE` statement. Alternatively, you can use the following snippet in the Rails console: + +```ruby +require_relative 'spec/support/database/click_house/hooks.rb' + +# Drops and re-creates all tables +ClickHouseTestRunner.new.ensure_schema +``` + +## Writing database queries + +For the ClickHouse database we don't use ORM (Object Relational Mapping). The main reason is that the GitLab application has many customizations for the `ActiveRecord` PostgresSQL adapter and the application generally assumes that all databases are using `PostgreSQL`. Since ClickHouse-related features are still in a very early stage of development, we decided to implement a simple HTTP client to avoid hard to discover bugs and long debugging time when dealing with multiple `ActiveRecord` adapters. + +Additionally, ClickHouse might not be used the same way as other adapters for `ActiveRecord`. The access patterns differ from traditional transactional databases, in that ClickHouse: + +- Uses nested aggregation `SELECT` queries with `GROUP BY` clauses. +- Doesn't use single `INSERT` statements. Data is inserted in batches via background jobs. +- Has different consistency characteristics, no transactions. +- Has very little database-level validations. + +Database queries are written and executed with the help of the `ClickHouse::Client` gem. + +A simple query from the `events` table: + +```ruby +rows = ClickHouse::Client.select('SELECT * FROM events', :main) +``` + +When working with queries with placeholders you can use the `ClickHouse::Query` object where you need to specify the placeholder name and its data type. The actual variable replacement, quoting and escaping will be done by the ClickHouse server. + +```ruby +raw_query = 'SELECT * FROM events WHERE id > {min_id:UInt64}' +placeholders = { min_id: Integer(100) } +query = ClickHouse::Client::Query.new(raw_query: raw_query, placeholders: placeholders) + +rows = ClickHouse::Client.select(query, :main) +``` + +When using placeholders the client can provide the query with redacted placeholder values which can be ingested by our logging system. You can see the redacted version of your query by calling the `to_redacted_sql` method: + +```ruby +puts query.to_redacted_sql +``` + +ClickHouse allows only one statement per request. This means that the common SQL injection vulnerability where the statement is closed with a `;` character and then another query is "injected" cannot be exploited: + +```ruby +ClickHouse::Client.select('SELECT 1; SELECT 2', :main) + +# ClickHouse::Client::DatabaseError: Code: 62. DB::Exception: Syntax error (Multi-statements are not allowed): failed at position 9 (end of query): ; SELECT 2. . (SYNTAX_ERROR) (version 23.4.2.11 (official build)) +``` + +### Subqueries + +You can compose complex queries with the `ClickHouse::Client::Query` class by specifying the query placeholder with the special `Subquery` type. The library will make sure to correctly merge the queries and the placeholders: + +```ruby +subquery = ClickHouse::Client::Query.new(raw_query: 'SELECT id FROM events WHERE id = {id:UInt64}', placeholders: { id: Integer(10) }) + +raw_query = 'SELECT * FROM events WHERE id > {id:UInt64} AND id IN ({q:Subquery})' +placeholders = { id: Integer(10), q: subquery } + +query = ClickHouse::Client::Query.new(raw_query: raw_query, placeholders: placeholders) +rows = ClickHouse::Client.select(query, :main) + +# ClickHouse will replace the placeholders +puts query.to_sql # SELECT * FROM events WHERE id > {id:UInt64} AND id IN (SELECT id FROM events WHERE id = {id:UInt64}) + +puts query.to_redacted_sql # SELECT * FROM events WHERE id > $1 AND id IN (SELECT id FROM events WHERE id = $2) + +puts query.placeholders # { id: 10 } +``` + +In case there are placeholders with the same name but different values the query will raise an error. + +### Writing query conditions + +When working with complex forms where multiple filter conditions are present, building queries by concatenating query fragments as string can get out of hands very quickly. For queries with several conditions you may use the `ClickHouse::QueryBuilder` class. The class uses the `Arel` gem to generate queries and provides a similar query interface like `ActiveRecord`. + +```ruby +builder = ClickHouse::QueryBuilder.new('events') + +query = builder + .where(builder.table[:created_at].lteq(Date.today)) + .where(id: [1,2,3]) + +rows = ClickHouse::Client.select(query, :main) +``` + +## Testing + +ClickHouse is enabled on CI/CD but to avoid significantly affecting the pipeline runtime we've decided to run the ClickHouse server for test cases tagged with `:click_house` only. + +The `:click_house` tag ensures that the database schema is properly set up before every test case. + +```ruby +RSpec.describe MyClickHouseFeature, :click_house do + it 'returns rows' do + rows = ClickHouse::Client.select('SELECT 1', :main) + expect(rows.size).to eq(1) + end +end +``` + +## Multiple databases + +By design, the `ClickHouse::Client` library supports configuring multiple databases. Because we're still at a very early stage of development, we only have one database called `main`. + +Multi database configuration example: + +```yaml +development: + main: + database: gitlab_clickhouse_main_development + url: 'http://localhost:8123' + username: clickhouse + password: clickhouse + + user_analytics: # made up database + database: gitlab_clickhouse_user_analytics_development + url: 'http://localhost:8123' + username: clickhouse + password: clickhouse +``` + +## Observability + +All queries executed via the `ClickHouse::Client` library expose the query with performance metrics (timings, read bytes) via `ActiveSupport::Notifications`. + +```ruby +ActiveSupport::Notifications.subscribe('sql.click_house') do |_, _, _, _, data| + puts data.inspect +end +``` + +Additionally, to view the executed ClickHouse queries in web interactions, on the performance bar, next to the `ch` label select the count. diff --git a/doc/development/database/index.md b/doc/development/database/index.md index 1ee6aeaa213..284c04d5f91 100644 --- a/doc/development/database/index.md +++ b/doc/development/database/index.md @@ -109,6 +109,7 @@ including the major methods: ## ClickHouse - [Introduction](clickhouse/index.md) +- [ClickHouse within GitLab](clickhouse/clickhouse_within_gitlab.md) - [Optimizing query execution](clickhouse/optimization.md) - [Rebuild GitLab features using ClickHouse 1: Activity data](clickhouse/gitlab_activity_data.md) - [Rebuild GitLab features using ClickHouse 2: Merge Request analytics](clickhouse/merge_request_analytics.md) diff --git a/doc/integration/jira/troubleshooting.md b/doc/integration/jira/troubleshooting.md index 70c6a306299..c2b79f116eb 100644 --- a/doc/integration/jira/troubleshooting.md +++ b/doc/integration/jira/troubleshooting.md @@ -55,7 +55,7 @@ To change all Jira projects to use instance-level integration settings: ```ruby jira_integration_instance_id = Integrations::Jira.find_by(instance: true).id - Integrations::Jira.where(active: true, instance: false, template: false, inherit_from_id: nil).find_each do |integration| + Integrations::Jira.where(active: true, instance: false, inherit_from_id: nil).find_each do |integration| integration.update_attribute(:inherit_from_id, jira_integration_instance_id) end ``` diff --git a/doc/user/project/releases/index.md b/doc/user/project/releases/index.md index bbea52299ff..bd20c0bcf1b 100644 --- a/doc/user/project/releases/index.md +++ b/doc/user/project/releases/index.md @@ -287,18 +287,36 @@ are defined as [crontab](https://crontab.guru/) entries. If the job that's executing is in a freeze period, GitLab CI/CD creates an environment variable named `$CI_DEPLOY_FREEZE`. -To prevent the deployment job from executing, create a `rules` entry in your -`.gitlab-ci.yml`, for example: +To prevent the deployment job from executing in multiple projects in a group, +define the `.freezedeployment` job in a file shared across the group. +Use the [`includes`](../../../ci/yaml/includes.md) keyword to incorporate the +template in your project's `.gitlab-ci.yml` file: + +```yaml +.freezedeployment: + stage: deploy + before_script: + - '[[ ! -z "$CI_DEPLOY_FREEZE" ]] && echo "INFRASTRUCTURE OUTAGE WINDOW" && exit 1; ' + rules: + - if: '$CI_DEPLOY_FREEZE' + when: manual + allow_failure: true + - when: on_success +``` + +To prevent the deployment job from executing, use the [`extends`](../../../ci/yaml/index.md#extends) keyword in the `deploy_to_production` job of your `.gitlab-ci.yml` file to inherit the configuration from the `.freezedeployment` template job: ```yaml deploy_to_production: - stage: deploy + extends: .freezedeployment script: deploy_to_prod.sh - rules: - - if: $CI_DEPLOY_FREEZE == null environment: production ``` +This configuration blocks deployment jobs conditionally and maintains pipeline continuity. When a freeze period is defined, the job fails and the pipeline can proceed without deployment. Manual deployment is possible after the freeze period. + +This approach offers deployment control during critical maintenance, and ensures the uninterrupted flow of the CI/CD pipeline. + To set a deploy freeze window in the UI, complete these steps: 1. Sign in to GitLab as a user with the Maintainer role. diff --git a/gems/click_house-client/lib/click_house/client.rb b/gems/click_house-client/lib/click_house/client.rb index abc54f2bce0..53170e9fb1a 100644 --- a/gems/click_house-client/lib/click_house/client.rb +++ b/gems/click_house-client/lib/click_house/client.rb @@ -6,6 +6,9 @@ require 'active_support/time' require 'active_support/notifications' require_relative "client/database" require_relative "client/configuration" +require_relative "client/bind_index_manager" +require_relative "client/query_like" +require_relative "client/query" require_relative "client/formatter" require_relative "client/response" @@ -25,6 +28,7 @@ module ClickHouse Error = Class.new(StandardError) ConfigurationError = Class.new(Error) DatabaseError = Class.new(Error) + QueryError = Class.new(Error) # Executes a SELECT database query def self.select(query, database, configuration = self.configuration) @@ -58,11 +62,17 @@ module ClickHouse private_class_method def self.instrumented_execute(query, database, configuration) db = lookup_database(configuration, database) + query = ClickHouse::Client::Query.new(raw_query: query) unless query.is_a?(ClickHouse::Client::QueryLike) ActiveSupport::Notifications.instrument('sql.click_house', { query: query, database: database }) do |instrument| + # Use a multipart POST request where the placeholders are sent with the param_ prefix + # See: https://github.com/ClickHouse/ClickHouse/issues/8842 + query_with_params = query.placeholders.transform_keys { |key| "param_#{key}" } + query_with_params['query'] = query.to_sql + response = configuration.http_post_proc.call( db.uri.to_s, db.headers, - query + query_with_params ) raise DatabaseError, response.body unless response.success? diff --git a/gems/click_house-client/lib/click_house/client/bind_index_manager.rb b/gems/click_house-client/lib/click_house/client/bind_index_manager.rb new file mode 100644 index 00000000000..618b13f2fd7 --- /dev/null +++ b/gems/click_house-client/lib/click_house/client/bind_index_manager.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module ClickHouse + module Client + class BindIndexManager + def initialize(start_index = 1) + @current_index = start_index + end + + def next_bind_str + bind_str = "$#{@current_index}" + @current_index += 1 + bind_str + end + end + end +end diff --git a/gems/click_house-client/lib/click_house/client/database.rb b/gems/click_house-client/lib/click_house/client/database.rb index faf5a953a12..2ce5fad1d05 100644 --- a/gems/click_house-client/lib/click_house/client/database.rb +++ b/gems/click_house-client/lib/click_house/client/database.rb @@ -28,8 +28,7 @@ module ClickHouse @headers ||= { 'X-ClickHouse-User' => @username, 'X-ClickHouse-Key' => @password, - 'X-ClickHouse-Format' => 'JSON', # always return JSON data - 'Content-Encoding' => 'gzip' # tell the server that we send compressed data + 'X-ClickHouse-Format' => 'JSON' # always return JSON data }.freeze end end diff --git a/gems/click_house-client/lib/click_house/client/query.rb b/gems/click_house-client/lib/click_house/client/query.rb new file mode 100644 index 00000000000..bd2443b1dc1 --- /dev/null +++ b/gems/click_house-client/lib/click_house/client/query.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module ClickHouse + module Client + class Query < QueryLike + SUBQUERY_PLACEHOLDER_REGEX = /{\w+:Subquery}/ # exmaple: {var:Subquery}, special "internal" type for subqueries + PLACEHOLDER_REGEX = /{\w+:\w+}/ # exmaple: {var:UInt8} + PLACEHOLDER_NAME_REGEX = /{(\w+):/ # exmaple: {var:UInt8} => var + + def initialize(raw_query:, placeholders: {}) + raise QueryError, 'Empty query string given' if raw_query.blank? + + @raw_query = raw_query + @placeholders = placeholders || {} + end + + # List of placeholders to be sent to ClickHouse for replacement. + # If there are subqueries, merge their placeholders as well. + def placeholders + all_placeholders = @placeholders.select { |_, v| !v.is_a?(QueryLike) } + @placeholders.each do |_name, value| + next unless value.is_a?(QueryLike) + + all_placeholders.merge!(value.placeholders) do |key, a, b| + raise QueryError, "mismatching values for the '#{key}' placeholder: #{a} vs #{b}" + end + end + + all_placeholders + end + + # Placeholder replacement is handled by ClickHouse, only subquery placeholders + # will be replaced. + def to_sql + raw_query.gsub(SUBQUERY_PLACEHOLDER_REGEX) do |placeholder_in_query| + value = placeholder_value(placeholder_in_query) + + if value.is_a?(QueryLike) + value.to_sql + else + placeholder_in_query + end + end + end + + def to_redacted_sql(bind_index_manager = BindIndexManager.new) + raw_query.gsub(PLACEHOLDER_REGEX) do |placeholder_in_query| + value = placeholder_value(placeholder_in_query) + + if value.is_a?(QueryLike) + value.to_redacted_sql(bind_index_manager) + else + bind_index_manager.next_bind_str + end + end + end + + private + + attr_reader :raw_query + + def placeholder_value(placeholder_in_query) + placeholder = placeholder_in_query[PLACEHOLDER_NAME_REGEX, 1] + @placeholders.fetch(placeholder.to_sym) + end + end + end +end diff --git a/gems/click_house-client/lib/click_house/client/query_like.rb b/gems/click_house-client/lib/click_house/client/query_like.rb new file mode 100644 index 00000000000..9e9ee46a338 --- /dev/null +++ b/gems/click_house-client/lib/click_house/client/query_like.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module ClickHouse + module Client + class QueryLike + # Build a SQL string that can be executed on a ClickHouse database. + def to_sql + raise NotImplementedError + end + + # Redacted version of the SQL query generated by the to_sql method where the + # placeholders are stripped. These queries are meant to be exported to external + # log aggregation systems. + def to_redacted_sql(bind_index_manager = BindIndexManager.new) + raise NotImplementedError + end + end + end +end diff --git a/spec/lib/click_house/bind_index_manager_spec.rb b/gems/click_house-client/spec/click_house/client/bind_index_manager_spec.rb similarity index 91% rename from spec/lib/click_house/bind_index_manager_spec.rb rename to gems/click_house-client/spec/click_house/client/bind_index_manager_spec.rb index 1c659017c63..38e0865676a 100644 --- a/spec/lib/click_house/bind_index_manager_spec.rb +++ b/gems/click_house-client/spec/click_house/client/bind_index_manager_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ClickHouse::BindIndexManager, feature_category: :database do +RSpec.describe ClickHouse::Client::BindIndexManager do describe '#next_bind_str' do context 'when initialized without a start index' do let(:bind_manager) { described_class.new } diff --git a/gems/click_house-client/spec/click_house/client/database_spec.rb b/gems/click_house-client/spec/click_house/client/database_spec.rb index a74d4a119a4..fdb4c72c0cb 100644 --- a/gems/click_house-client/spec/click_house/client/database_spec.rb +++ b/gems/click_house-client/spec/click_house/client/database_spec.rb @@ -24,7 +24,6 @@ RSpec.describe ClickHouse::Client::Database do describe '#headers' do it 'returns the correct headers' do expect(database.headers).to eq({ - "Content-Encoding" => "gzip", "X-ClickHouse-Format" => "JSON", 'X-ClickHouse-User' => 'user', 'X-ClickHouse-Key' => 'pass' diff --git a/gems/click_house-client/spec/click_house/client/query_like_spec.rb b/gems/click_house-client/spec/click_house/client/query_like_spec.rb new file mode 100644 index 00000000000..8b8426bd5fd --- /dev/null +++ b/gems/click_house-client/spec/click_house/client/query_like_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ClickHouse::Client::QueryLike do + subject(:query) { described_class.new } + + describe '#to_sql' do + it { expect { query.to_sql }.to raise_error(NotImplementedError) } + end + + describe '#to_redacted_sql' do + it { expect { query.to_redacted_sql }.to raise_error(NotImplementedError) } + end +end diff --git a/gems/click_house-client/spec/click_house/client/query_spec.rb b/gems/click_house-client/spec/click_house/client/query_spec.rb new file mode 100644 index 00000000000..82733e523b1 --- /dev/null +++ b/gems/click_house-client/spec/click_house/client/query_spec.rb @@ -0,0 +1,125 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ClickHouse::Client::Query do + subject(:query) { described_class.new(raw_query: raw_query, placeholders: placeholders) } + + let(:sql) { query.to_sql } + let(:redacted_sql) { query.to_redacted_sql } + + context 'when using no placeholders' do + let(:raw_query) { 'SELECT * FROM events' } + let(:placeholders) { nil } + + it { expect(sql).to eq(raw_query) } + it { expect(redacted_sql).to eq(raw_query) } + + context 'when placeholders is an empty hash' do + let(:placeholders) { {} } + + it { expect(sql).to eq(raw_query) } + it { expect(redacted_sql).to eq(raw_query) } + end + end + + context 'when placeholders are given' do + let(:raw_query) { 'SELECT * FROM events WHERE id = {id:UInt64}' } + let(:placeholders) { { id: 1 } } + + it { expect(sql).to eq(raw_query) } + it { expect(redacted_sql).to eq('SELECT * FROM events WHERE id = $1') } + end + + context 'when multiple placeholders are given' do + let(:raw_query) do + <<~SQL.squish + SELECT * + FROM events + WHERE + id = {id:UInt64} AND + title = {some_title:String} AND + another_id = {id:UInt64} + SQL + end + + let(:placeholders) { { id: 1, some_title: "'title'" } } + + it do + expect(sql).to eq(raw_query) + end + + it do + expect(redacted_sql).to eq( + <<~SQL.squish + SELECT * + FROM events + WHERE + id = $1 AND + title = $2 AND + another_id = $3 + SQL + ) + end + end + + context 'when dealing with subqueries' do + let(:raw_query) { 'SELECT * FROM events WHERE id < {min_id:UInt64} AND id IN ({q:Subquery})' } + + let(:subquery) do + described_class.new(raw_query: 'SELECT id FROM events WHERE id > {max_id:UInt64}', placeholders: { max_id: 11 }) + end + + let(:placeholders) { { min_id: 100, q: subquery } } + + it 'replaces the subquery but preserves the other placeholders' do + q = 'SELECT * FROM events WHERE id < {min_id:UInt64} AND id IN (SELECT id FROM events WHERE id > {max_id:UInt64})' + expect(sql).to eq(q) + end + + it 'replaces the subquery and replaces the placeholders with indexed values' do + expect(redacted_sql).to eq('SELECT * FROM events WHERE id < $1 AND id IN (SELECT id FROM events WHERE id > $2)') + end + + it 'merges the placeholders' do + expect(query.placeholders).to eq({ min_id: 100, max_id: 11 }) + end + end + + describe 'validation' do + context 'when SQL string is empty' do + let(:raw_query) { '' } + let(:placeholders) { {} } + + it 'raises error' do + expect { query }.to raise_error(ClickHouse::Client::QueryError, /Empty query string given/) + end + end + + context 'when SQL string is nil' do + let(:raw_query) { nil } + let(:placeholders) { {} } + + it 'raises error' do + expect { query }.to raise_error(ClickHouse::Client::QueryError, /Empty query string given/) + end + end + + context 'when same placeholder value does not match' do + let(:raw_query) { 'SELECT id FROM events WHERE id = {id:UInt64} AND id IN ({q:Subquery})' } + + let(:subquery) do + subquery_string = 'SELECT id FROM events WHERE id = {id:UInt64}' + described_class.new(raw_query: subquery_string, placeholders: { id: 10 }) + end + + let(:placeholders) { { id: 5, q: subquery } } + + it 'raises error' do + expect do + query.placeholders + end.to raise_error(ClickHouse::Client::QueryError, /mismatching values for the 'id' placeholder/) + end + end + end +end diff --git a/gems/click_house-client/spec/click_house/client_spec.rb b/gems/click_house-client/spec/click_house/client_spec.rb index 883199198ba..51cc097a228 100644 --- a/gems/click_house-client/spec/click_house/client_spec.rb +++ b/gems/click_house-client/spec/click_house/client_spec.rb @@ -71,7 +71,7 @@ RSpec.describe ClickHouse::Client do end context 'when the DB is not configured' do - it 'raises erro' do + it 'raises error' do expect do described_class.select('SELECT * FROM issues', :different_db, configuration) end.to raise_error(ClickHouse::Client::ConfigurationError, /not configured/) diff --git a/lib/click_house/bind_index_manager.rb b/lib/click_house/bind_index_manager.rb deleted file mode 100644 index 96b0940ce71..00000000000 --- a/lib/click_house/bind_index_manager.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module ClickHouse - class BindIndexManager - def initialize(start_index = 1) - @current_index = start_index - end - - def next_bind_str - bind_str = "$#{@current_index}" - @current_index += 1 - bind_str - end - end -end diff --git a/lib/click_house/query_builder.rb b/lib/click_house/query_builder.rb index a2136420b2f..dc139663e7c 100644 --- a/lib/click_house/query_builder.rb +++ b/lib/click_house/query_builder.rb @@ -2,7 +2,7 @@ # rubocop:disable CodeReuse/ActiveRecord module ClickHouse - class QueryBuilder + class QueryBuilder < ClickHouse::Client::QueryLike attr_reader :table attr_accessor :conditions, :manager @@ -93,8 +93,8 @@ module ClickHouse manager.to_sql end - def to_redacted_sql - ::ClickHouse::Redactor.redact(self) + def to_redacted_sql(bind_index_manager = ClickHouse::Client::BindIndexManager.new) + ::ClickHouse::Redactor.redact(self, bind_index_manager) end private diff --git a/lib/click_house/redactor.rb b/lib/click_house/redactor.rb index 9b8e2bc90d9..6ca7c46e747 100644 --- a/lib/click_house/redactor.rb +++ b/lib/click_house/redactor.rb @@ -14,9 +14,8 @@ module ClickHouse # redacted_query = ClickHouse::Redactor.redact(query_builder) # # The redacted_query will contain the SQL query with values replaced by placeholders. # output: "SELECT * FROM \"users\" WHERE \"users\".\"name\" = $1" - def self.redact(query_builder) + def self.redact(query_builder, bind_manager = ClickHouse::Client::BindIndexManager.new) cloned_query_builder = query_builder.clone - bind_manager = ::ClickHouse::BindIndexManager.new cloned_query_builder.conditions = cloned_query_builder.conditions.map do |condition| redact_condition(condition, bind_manager) diff --git a/lib/peek/views/click_house.rb b/lib/peek/views/click_house.rb index cc109ccea51..97f02ad3dab 100644 --- a/lib/peek/views/click_house.rb +++ b/lib/peek/views/click_house.rb @@ -39,7 +39,7 @@ module Peek { start: start, duration: finish - start, - sql: data[:query].strip, + sql: data[:query].to_sql.strip, backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller), database: "database: #{data[:database]}", statistics: "query stats: #{data[:statistics]}" diff --git a/spec/factories/users.rb b/spec/factories/users.rb index e29ed02297f..17524647669 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -130,6 +130,10 @@ FactoryBot.define do end end + trait :no_super_sidebar do + use_new_navigation { false } + end + trait :two_factor_via_webauthn do transient { registrations_count { 5 } } diff --git a/spec/features/dashboard/navbar_spec.rb b/spec/features/dashboard/navbar_spec.rb index ff0ff899fc2..30e7f2d2e4e 100644 --- a/spec/features/dashboard/navbar_spec.rb +++ b/spec/features/dashboard/navbar_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe '"Your work" navbar', feature_category: :navigation do include_context 'dashboard navbar structure' - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :no_super_sidebar) } it_behaves_like 'verified navigation bar' do before do diff --git a/spec/features/groups/navbar_spec.rb b/spec/features/groups/navbar_spec.rb index a52e2d95fed..034aa6ab020 100644 --- a/spec/features/groups/navbar_spec.rb +++ b/spec/features/groups/navbar_spec.rb @@ -8,7 +8,7 @@ RSpec.describe 'Group navbar', :with_license, feature_category: :navigation do include_context 'group navbar structure' - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :no_super_sidebar) } let(:group) { create(:group) } diff --git a/spec/features/projects/navbar_spec.rb b/spec/features/projects/navbar_spec.rb index b6645e9b710..b7598f05d18 100644 --- a/spec/features/projects/navbar_spec.rb +++ b/spec/features/projects/navbar_spec.rb @@ -8,9 +8,8 @@ RSpec.describe 'Project navbar', :with_license, feature_category: :groups_and_pr include_context 'project navbar structure' - let_it_be(:project) { create(:project, :repository) } - - let(:user) { project.first_owner } + let_it_be(:user) { create(:user, :no_super_sidebar) } + let_it_be(:project) { create(:project, :repository, namespace: user.namespace) } before do sign_in(user) diff --git a/spec/features/snippets/show_spec.rb b/spec/features/snippets/show_spec.rb index 2673ad5e1d7..a354499fa0c 100644 --- a/spec/features/snippets/show_spec.rb +++ b/spec/features/snippets/show_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe 'Snippet', :js, feature_category: :source_code_management do - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :no_super_sidebar) } let_it_be(:snippet) { create(:personal_snippet, :public, :repository, author: user) } it_behaves_like 'show and render proper snippet blob' do @@ -36,7 +36,7 @@ RSpec.describe 'Snippet', :js, feature_category: :source_code_management do end context 'when authenticated as a different user' do - let_it_be(:different_user) { create(:user) } + let_it_be(:different_user) { create(:user, :no_super_sidebar) } before do sign_in(different_user) diff --git a/spec/features/webauthn_spec.rb b/spec/features/webauthn_spec.rb index 5c42facfa8b..52e2b375187 100644 --- a/spec/features/webauthn_spec.rb +++ b/spec/features/webauthn_spec.rb @@ -15,7 +15,7 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor # TODO: it_behaves_like 'hardware device for 2fa', 'WebAuthn' describe 'registration' do - let(:user) { create(:user) } + let(:user) { create(:user, :no_super_sidebar) } before do gitlab_sign_in(user) @@ -58,7 +58,8 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor gitlab_sign_out # Second user - user = gitlab_sign_in(:user) + user = create(:user, :no_super_sidebar) + gitlab_sign_in(user) visit profile_account_path enable_two_factor_authentication webauthn_device_registration(webauthn_device: webauthn_device, name: 'My other device', password: user.password) @@ -125,7 +126,7 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor it_behaves_like 'hardware device for 2fa', 'WebAuthn' describe 'registration' do - let(:user) { create(:user) } + let(:user) { create(:user, :no_super_sidebar) } before do gitlab_sign_in(user) @@ -160,7 +161,8 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor gitlab_sign_out # Second user - user = gitlab_sign_in(:user) + user = create(:user, :no_super_sidebar) + gitlab_sign_in(user) user.update_attribute(:otp_required_for_login, true) visit profile_account_path manage_two_factor_authentication @@ -225,7 +227,7 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor describe 'authentication' do let(:otp_required_for_login) { true } - let(:user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } + let(:user) { create(:user, :no_super_sidebar, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } let!(:webauthn_device) do add_webauthn_device(app_id, user) end @@ -254,7 +256,7 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor describe 'when a given WebAuthn device has already been registered by another user' do describe 'but not the current user' do - let(:other_user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } + let(:other_user) { create(:user, :no_super_sidebar, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } it 'does not allow logging in with that particular device' do # Register other user with a different WebAuthn device @@ -275,7 +277,8 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js, feature_categor it "allows logging in with that particular device" do pending("support for passing credential options in FakeClient") # Register current user with the same WebAuthn device - current_user = gitlab_sign_in(:user) + current_user = create(:user, :no_super_sidebar) + gitlab_sign_in(current_user) visit profile_account_path manage_two_factor_authentication register_webauthn_device(webauthn_device) diff --git a/spec/frontend/fixtures/issues.rb b/spec/frontend/fixtures/issues.rb index 73594ddf686..9e6fcea2d17 100644 --- a/spec/frontend/fixtures/issues.rb +++ b/spec/frontend/fixtures/issues.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Projects::IssuesController, '(JavaScript fixtures)', :with_license, type: :controller do include JavaScriptFixturesHelpers - let(:user) { create(:user, feed_token: 'feedtoken:coldfeed') } + let(:user) { create(:user, :no_super_sidebar, feed_token: 'feedtoken:coldfeed') } let(:namespace) { create(:namespace, name: 'frontend-fixtures') } let(:project) { create(:project_empty_repo, namespace: namespace, path: 'issues-project') } diff --git a/spec/frontend/fixtures/snippet.rb b/spec/frontend/fixtures/snippet.rb index 0510746a944..23df89a244c 100644 --- a/spec/frontend/fixtures/snippet.rb +++ b/spec/frontend/fixtures/snippet.rb @@ -5,9 +5,9 @@ require 'spec_helper' RSpec.describe SnippetsController, '(JavaScript fixtures)', type: :controller do include JavaScriptFixturesHelpers - let(:namespace) { create(:namespace, name: 'frontend-fixtures') } + let(:user) { create(:user, :no_super_sidebar) } + let(:namespace) { create(:namespace, name: 'frontend-fixtures', owner: user) } let(:project) { create(:project, :repository, namespace: namespace, path: 'branches-project') } - let(:user) { project.first_owner } let(:snippet) { create(:personal_snippet, :public, title: 'snippet.md', content: '# snippet', file_name: 'snippet.md', author: user) } render_views diff --git a/spec/frontend/issues/dashboard/mock_data.js b/spec/frontend/issues/dashboard/mock_data.js index adcd4268449..1e3abd5a018 100644 --- a/spec/frontend/issues/dashboard/mock_data.js +++ b/spec/frontend/issues/dashboard/mock_data.js @@ -19,6 +19,7 @@ export const issuesQueryResponse = { reference: 'group/project#123456', state: 'opened', title: 'Issue title', + titleHtml: 'Issue title', type: 'issue', updatedAt: '2021-05-22T04:08:01Z', upvotes: 3, diff --git a/spec/frontend/issues/list/mock_data.js b/spec/frontend/issues/list/mock_data.js index b9a8bc171db..73fda11f38c 100644 --- a/spec/frontend/issues/list/mock_data.js +++ b/spec/frontend/issues/list/mock_data.js @@ -49,6 +49,7 @@ export const getIssuesQueryResponse = { moved: false, state: 'opened', title: 'Issue title', + titleHtml: 'Issue title', updatedAt: '2021-05-22T04:08:01Z', closedAt: null, upvotes: 3, diff --git a/spec/frontend/issues/show/components/sticky_header_spec.js b/spec/frontend/issues/show/components/sticky_header_spec.js index dd41e3034eb..0c54ae45e70 100644 --- a/spec/frontend/issues/show/components/sticky_header_spec.js +++ b/spec/frontend/issues/show/components/sticky_header_spec.js @@ -30,6 +30,7 @@ describe('StickyHeader component', () => { issuableType: TYPE_ISSUE, show: true, title: 'A sticky issue', + titleHtml: '', ...props, }, }); @@ -124,4 +125,11 @@ describe('StickyHeader component', () => { expect(title.text()).toContain('A sticky issue'); expect(title.attributes('href')).toBe('#top'); }); + + it('shows title containing markup', () => { + const titleHtml = 'A sticky issue'; + createComponent({ titleHtml }); + + expect(wrapper.find('a').html()).toContain(titleHtml); + }); }); diff --git a/spec/frontend/issues/show/mock_data/mock_data.js b/spec/frontend/issues/show/mock_data/mock_data.js index ed969a08ac5..37aa18ced8d 100644 --- a/spec/frontend/issues/show/mock_data/mock_data.js +++ b/spec/frontend/issues/show/mock_data/mock_data.js @@ -1,8 +1,9 @@ import { TEST_HOST } from 'helpers/test_constants'; export const initialRequest = { - title: '

this is a title

', + title: 'this is a title', title_text: 'this is a title', + title_html: 'this is a title', description: '

this is a description!

', description_text: 'this is a description', task_completion_status: { completed_count: 2, count: 4 }, diff --git a/spec/frontend/super_sidebar/components/create_menu_spec.js b/spec/frontend/super_sidebar/components/create_menu_spec.js index 510a3f5b913..c3c7b2e6c65 100644 --- a/spec/frontend/super_sidebar/components/create_menu_spec.js +++ b/spec/frontend/super_sidebar/components/create_menu_spec.js @@ -45,7 +45,7 @@ describe('CreateMenu component', () => { createWrapper(); expect(findGlDisclosureDropdown().props('dropdownOffset')).toEqual({ - crossAxis: -147, + crossAxis: -179, mainAxis: 4, }); }); @@ -99,7 +99,7 @@ describe('CreateMenu component', () => { createWrapper({ provide: { isImpersonating: true } }); expect(findGlDisclosureDropdown().props('dropdownOffset')).toEqual({ - crossAxis: -115, + crossAxis: -147, mainAxis: 4, }); }); diff --git a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js index 77333a878d1..06a74a381e3 100644 --- a/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js +++ b/spec/frontend/vue_shared/issuable/list/components/issuable_item_spec.js @@ -1,5 +1,6 @@ import { GlLink, GlLabel, GlIcon, GlFormCheckbox, GlSprintf } from '@gitlab/ui'; import { nextTick } from 'vue'; +import { escape } from 'lodash'; import { useFakeDate } from 'helpers/fake_date'; import { shallowMountExtended as shallowMount } from 'helpers/vue_test_utils_helper'; import IssuableItem from '~/vue_shared/issuable/list/components/issuable_item.vue'; @@ -279,10 +280,23 @@ describe('IssuableItem', () => { expect(titleEl.exists()).toBe(true); expect(titleEl.findComponent(GlLink).attributes('href')).toBe(expectedHref); expect(titleEl.findComponent(GlLink).attributes('target')).toBe(expectedTarget); - expect(titleEl.findComponent(GlLink).text()).toBe(mockIssuable.title); + expect(titleEl.findComponent(GlLink).html()).toContain(mockIssuable.titleHtml); }, ); + it('renders issuable title with escaped markup when issue tracker is external', () => { + const mockTitle = ''; + wrapper = createComponent({ + issuable: { + ...mockIssuable, + title: mockTitle, + externalTracker: 'jira', + }, + }); + + expect(wrapper.findByTestId('issuable-title').html()).toContain(escape(mockTitle)); + }); + it('renders checkbox when `showCheckbox` prop is true', async () => { wrapper = createComponent({ showCheckbox: true, diff --git a/spec/frontend/vue_shared/issuable/list/mock_data.js b/spec/frontend/vue_shared/issuable/list/mock_data.js index f8cf3ba5271..b39d177f292 100644 --- a/spec/frontend/vue_shared/issuable/list/mock_data.js +++ b/spec/frontend/vue_shared/issuable/list/mock_data.js @@ -42,7 +42,7 @@ export const mockCurrentUserTodo = { export const mockIssuable = { iid: '30', title: 'Dismiss Cipher with no integrity', - titleHtml: 'Dismiss Cipher with no integrity', + titleHtml: 'Dismiss Cipher with no integrity', description: 'fortitudinis _fomentis_ dolor mitigari solet.', descriptionHtml: 'fortitudinis fomentis dolor mitigari solet.', state: 'opened', diff --git a/spec/lib/click_house/query_builder_spec.rb b/spec/lib/click_house/query_builder_spec.rb index 9e3f1118eeb..f5e1d53e7c1 100644 --- a/spec/lib/click_house/query_builder_spec.rb +++ b/spec/lib/click_house/query_builder_spec.rb @@ -288,7 +288,8 @@ RSpec.describe ClickHouse::QueryBuilder, feature_category: :database do describe '#to_redacted_sql' do it 'calls ::ClickHouse::Redactor correctly' do - expect(::ClickHouse::Redactor).to receive(:redact).with(builder) + expect(::ClickHouse::Redactor).to receive(:redact).with(builder, + an_instance_of(ClickHouse::Client::BindIndexManager)) builder.to_redacted_sql end @@ -331,4 +332,27 @@ RSpec.describe ClickHouse::QueryBuilder, feature_category: :database do expect(sql).to eq(expected_sql) end end + + context 'when combining with a raw query' do + it 'correctly generates the SQL query' do + raw_query = 'SELECT * FROM isues WHERE title = {title:String} AND id IN ({query:Subquery})' + placeholders = { + title: "'test'", + query: builder.select(:id).where(column1: 'value1', column2: 'value2') + } + + query = ClickHouse::Client::Query.new(raw_query: raw_query, placeholders: placeholders) + expected_sql = "SELECT * FROM isues WHERE title = {title:String} AND id IN (SELECT \"test_table\".\"id\" " \ + "FROM \"test_table\" WHERE \"test_table\".\"column1\" = 'value1' AND " \ + "\"test_table\".\"column2\" = 'value2')" + + expect(query.to_sql).to eq(expected_sql) + + expected_redacted_sql = "SELECT * FROM isues WHERE title = $1 AND id IN (SELECT \"test_table\".\"id\" " \ + "FROM \"test_table\" WHERE \"test_table\".\"column1\" = $2 AND " \ + "\"test_table\".\"column2\" = $3)" + + expect(query.to_redacted_sql).to eq(expected_redacted_sql) + end + end end diff --git a/spec/lib/gitlab/database/click_house_client_spec.rb b/spec/lib/gitlab/database/click_house_client_spec.rb index 50086795b2b..06a3536b57c 100644 --- a/spec/lib/gitlab/database/click_house_client_spec.rb +++ b/spec/lib/gitlab/database/click_house_client_spec.rb @@ -89,9 +89,19 @@ RSpec.describe 'ClickHouse::Client', feature_category: :database do 'target_type' => event3.target_type )) - ClickHouse::Client.execute("DELETE FROM events WHERE id = #{event3.id}", :main) + delete_query = ClickHouse::Client::Query.new( + raw_query: 'DELETE FROM events WHERE id = {id:UInt64}', + placeholders: { id: event3.id } + ) - results = ClickHouse::Client.select("SELECT * FROM events WHERE id = #{event3.id}", :main) + ClickHouse::Client.execute(delete_query, :main) + + select_query = ClickHouse::Client::Query.new( + raw_query: 'SELECT * FROM events WHERE id = {id:UInt64}', + placeholders: { id: event3.id } + ) + + results = ClickHouse::Client.select(select_query, :main) expect(results).to be_empty end end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb deleted file mode 100644 index c48ed19e40d..00000000000 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ /dev/null @@ -1,144 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe MergeRequests::FfMergeService, feature_category: :code_review_workflow do - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:merge_request) do - create( - :merge_request, - source_branch: 'flatten-dir', - target_branch: 'improve/awesome', - assignees: [user2], - author: create(:user) - ) - end - - let(:project) { merge_request.project } - let(:valid_merge_params) { { sha: merge_request.diff_head_sha } } - - before do - stub_feature_flags(refactor_merge_service: false) - project.add_maintainer(user) - project.add_developer(user2) - end - - describe '#execute' do - context 'valid params' do - let(:service) { described_class.new(project: project, current_user: user, params: valid_merge_params) } - - def execute_ff_merge - perform_enqueued_jobs do - service.execute(merge_request) - end - end - - before do - allow(service).to receive(:execute_hooks) - end - - it "does not create merge commit" do - execute_ff_merge - - source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha - target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha - - expect(source_branch_sha).to eq(target_branch_sha) - end - - it 'keeps the merge request valid' do - expect { execute_ff_merge } - .not_to change { merge_request.valid? } - end - - it 'updates the merge request to merged' do - expect { execute_ff_merge } - .to change { merge_request.merged? } - .from(false) - .to(true) - end - - it 'sends email to user2 about merge of new merge_request' do - execute_ff_merge - - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end - - it 'creates resource event about merge_request merge' do - execute_ff_merge - - event = merge_request.resource_state_events.last - expect(event.state).to eq('merged') - end - - it 'does not update squash_commit_sha if it is not a squash' do - expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - - expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - - it 'updates squash_commit_sha if it is a squash' do - expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - - merge_request.update!(squash: true) - - expect { execute_ff_merge } - .to change { merge_request.squash_commit_sha } - .from(nil) - - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - end - - context 'error handling' do - let(:service) { described_class.new(project: project, current_user: user, params: valid_merge_params.merge(commit_message: 'Awesome message')) } - - before do - allow(Gitlab::AppLogger).to receive(:error) - end - - it 'logs and saves error if there is an exception' do - error_message = 'error message' - - allow(service).to receive(:repository).and_raise("error message") - allow(service).to receive(:execute_hooks) - - service.execute(merge_request) - - expect(Gitlab::AppLogger).to have_received(:error) - .with(hash_including(message: a_string_matching(error_message))) - end - - it 'logs and saves error if there is an PreReceiveError exception' do - error_message = 'error message' - raw_message = 'The truth is out there' - - pre_receive_error = Gitlab::Git::PreReceiveError.new(raw_message, fallback_message: error_message) - allow(service).to receive(:repository).and_raise(pre_receive_error) - allow(service).to receive(:execute_hooks) - - service.execute(merge_request) - - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error) - .with(hash_including(message: a_string_matching(error_message))) - end - - it 'does not update squash_commit_sha if squash merge is not successful' do - merge_request.update!(squash: true) - - expect(project.repository.raw).to receive(:ff_merge) do - raise 'Merge error' - end - - expect { service.execute(merge_request) }.not_to change { merge_request.squash_commit_sha } - end - end - end -end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 1faa1fd3644..6917feaeaff 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -8,437 +8,528 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf let_it_be(:user) { create(:user) } let_it_be(:user2) { create(:user) } - where(:ff_refactor_merge_service_enabled) { [true, false] } + let(:merge_request) { create(:merge_request, :simple, author: user2, assignees: [user2]) } + let(:project) { merge_request.project } - with_them do - let(:merge_request) { create(:merge_request, :simple, author: user2, assignees: [user2]) } - let(:project) { merge_request.project } + before do + project.add_maintainer(user) + project.add_developer(user2) + end - before do - stub_feature_flags(refactor_merge_service: ff_refactor_merge_service_enabled) - - project.add_maintainer(user) - project.add_developer(user2) + describe '#execute' do + let(:service) { described_class.new(project: project, current_user: user, params: merge_params) } + let(:merge_params) do + { commit_message: 'Awesome message', sha: merge_request.diff_head_sha } end - describe '#execute' do - let(:service) { described_class.new(project: project, current_user: user, params: merge_params) } + let(:lease_key) { "merge_requests_merge_service:#{merge_request.id}" } + let!(:lease) { stub_exclusive_lease(lease_key) } + + shared_examples 'with valid params' do + before do + allow(service).to receive(:execute_hooks) + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original + + perform_enqueued_jobs do + service.execute(merge_request) + end + end + + it { expect(merge_request).to be_valid } + it { expect(merge_request).to be_merged } + + it 'does not update squash_commit_sha if it is not a squash' do + expect(merge_request.squash_commit_sha).to be_nil + end + + it 'sends email to user2 about merge of new merge_request' do + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end + + context 'note creation' do + it 'creates resource state event about merge_request merge' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + end + end + end + + shared_examples 'squashing' do + # A merge request with 5 commits + let(:merge_request) do + create( + :merge_request, + :simple, + author: user2, + assignees: [user2], + squash: true, + source_branch: 'improve/awesome', + target_branch: 'fix' + ) + end + let(:merge_params) do - { commit_message: 'Awesome message', sha: merge_request.diff_head_sha } + { commit_message: 'Merge commit message', + squash_commit_message: 'Squash commit message', + sha: merge_request.diff_head_sha } end - let(:lease_key) { "merge_requests_merge_service:#{merge_request.id}" } - let!(:lease) { stub_exclusive_lease(lease_key) } - - shared_examples 'with valid params' do - before do - allow(service).to receive(:execute_hooks) - expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - - perform_enqueued_jobs do - service.execute(merge_request) - end - end - - it { expect(merge_request).to be_valid } - it { expect(merge_request).to be_merged } - - it 'does not update squash_commit_sha if it is not a squash' do - expect(merge_request.squash_commit_sha).to be_nil - end - - it 'sends email to user2 about merge of new merge_request' do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end - - context 'note creation' do - it 'creates resource state event about merge_request merge' do - event = merge_request.resource_state_events.last - expect(event.state).to eq('merged') - end - end - end - - shared_examples 'squashing' do - # A merge request with 5 commits - let(:merge_request) do - create( - :merge_request, - :simple, - author: user2, - assignees: [user2], - squash: true, - source_branch: 'improve/awesome', - target_branch: 'fix' - ) - end - - let(:merge_params) do - { commit_message: 'Merge commit message', - squash_commit_message: 'Squash commit message', - sha: merge_request.diff_head_sha } - end - - before do - allow(service).to receive(:execute_hooks) - expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - - perform_enqueued_jobs do - service.execute(merge_request) - end - end - - it 'merges the merge request with squashed commits' do - expect(merge_request).to be_merged - - merge_commit = merge_request.merge_commit - squash_commit = merge_request.merge_commit.parents.last - - expect(merge_commit.message).to eq('Merge commit message') - expect(squash_commit.message).to eq("Squash commit message\n") - end - - it 'persists squash_commit_sha' do - squash_commit = merge_request.merge_commit.parents.last - - expect(merge_request.squash_commit_sha).to eq(squash_commit.id) - end - end - - context 'when merge strategy is merge commit' do - it 'persists merge_commit_sha and nullifies in_progress_merge_commit_sha' do - service.execute(merge_request) - - expect(merge_request.merge_commit_sha).not_to be_nil - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - - it_behaves_like 'with valid params' - - it_behaves_like 'squashing' - end - - context 'when merge strategy is fast forward' do - before do - project.update!(merge_requests_ff_only_enabled: true) - end - - let(:merge_request) do - create( - :merge_request, - source_branch: 'flatten-dir', - target_branch: 'improve/awesome', - assignees: [user2], - author: create(:user) - ) - end - - it 'does not create merge_commit_sha and nullifies in_progress_merge_commit_sha' do - service.execute(merge_request) - - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - - it_behaves_like 'with valid params' - - it 'updates squash_commit_sha if it is a squash' do - expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - - merge_request.update!(squash: true) - - expect { service.execute(merge_request) } - .to change { merge_request.squash_commit_sha } - .from(nil) - - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end - end - - context 'running the service once' do - let(:ref) { merge_request.to_reference(full: true) } - let(:jid) { SecureRandom.hex } - - let(:messages) do - [ - /#{ref} - Git merge started on JID #{jid}/, - /#{ref} - Git merge finished on JID #{jid}/, - /#{ref} - Post merge started on JID #{jid}/, - /#{ref} - Post merge finished on JID #{jid}/, - /#{ref} - Merge process finished on JID #{jid}/ - ] - end - - before do - merge_request.update!(merge_jid: jid) - ::Gitlab::ApplicationContext.push(caller_id: 'MergeWorker') - end - - it 'logs status messages' do - allow(Gitlab::AppLogger).to receive(:info).and_call_original - - messages.each do |message| - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - 'meta.caller_id' => 'MergeWorker', - message: message, - merge_request_info: ref - ) - ).and_call_original - end + before do + allow(service).to receive(:execute_hooks) + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original + perform_enqueued_jobs do service.execute(merge_request) end end - context 'running the service multiple time' do - it 'is idempotent' do - 2.times { service.execute(merge_request) } + it 'merges the merge request with squashed commits' do + expect(merge_request).to be_merged - expect(merge_request.merge_error).to be_falsey - expect(merge_request).to be_valid - expect(merge_request).to be_merged + merge_commit = merge_request.merge_commit + squash_commit = merge_request.merge_commit.parents.last - commit_messages = project.repository.commits('master', limit: 2).map(&:message) - expect(commit_messages.uniq.size).to eq(2) - expect(merge_request.in_progress_merge_commit_sha).to be_nil - end + expect(merge_commit.message).to eq('Merge commit message') + expect(squash_commit.message).to eq("Squash commit message\n") end - context 'when an invalid sha is passed' do - let(:merge_request) do - create( - :merge_request, - :simple, - author: user2, - assignees: [user2], - squash: true, - source_branch: 'improve/awesome', - target_branch: 'fix' - ) - end + it 'persists squash_commit_sha' do + squash_commit = merge_request.merge_commit.parents.last - let(:merge_params) do - { sha: merge_request.commits.second.sha } - end + expect(merge_request.squash_commit_sha).to eq(squash_commit.id) + end + end - it 'does not merge the MR' do - service.execute(merge_request) + context 'when merge strategy is merge commit' do + it 'persists merge_commit_sha and nullifies in_progress_merge_commit_sha' do + service.execute(merge_request) - expect(merge_request).not_to be_merged - expect(merge_request.merge_error).to match(/Branch has been updated/) - end + expect(merge_request.merge_commit_sha).not_to be_nil + expect(merge_request.in_progress_merge_commit_sha).to be_nil end - context 'when the `sha` param is missing' do - let(:merge_params) { {} } + it_behaves_like 'with valid params' - it 'returns the error' do - merge_error = 'Branch has been updated since the merge was requested. '\ - 'Please review the changes.' + it_behaves_like 'squashing' + end - expect { service.execute(merge_request) } - .to change { merge_request.merge_error } - .from(nil).to(merge_error) - end + context 'when merge strategy is fast forward' do + before do + project.update!(merge_requests_ff_only_enabled: true) end - context 'closes related issues' do + let(:merge_request) do + create( + :merge_request, + source_branch: 'flatten-dir', + target_branch: 'improve/awesome', + assignees: [user2], + author: create(:user) + ) + end + + it 'does not create merge_commit_sha and nullifies in_progress_merge_commit_sha' do + service.execute(merge_request) + + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end + + it_behaves_like 'with valid params' + + it 'updates squash_commit_sha if it is a squash' do + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original + + merge_request.update!(squash: true) + + expect { service.execute(merge_request) } + .to change { merge_request.squash_commit_sha } + .from(nil) + + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end + end + + context 'running the service once' do + let(:ref) { merge_request.to_reference(full: true) } + let(:jid) { SecureRandom.hex } + + let(:messages) do + [ + /#{ref} - Git merge started on JID #{jid}/, + /#{ref} - Git merge finished on JID #{jid}/, + /#{ref} - Post merge started on JID #{jid}/, + /#{ref} - Post merge finished on JID #{jid}/, + /#{ref} - Merge process finished on JID #{jid}/ + ] + end + + before do + merge_request.update!(merge_jid: jid) + ::Gitlab::ApplicationContext.push(caller_id: 'MergeWorker') + end + + it 'logs status messages' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + messages.each do |message| + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + 'meta.caller_id' => 'MergeWorker', + message: message, + merge_request_info: ref + ) + ).and_call_original + end + + service.execute(merge_request) + end + end + + context 'running the service multiple time' do + it 'is idempotent' do + 2.times { service.execute(merge_request) } + + expect(merge_request.merge_error).to be_falsey + expect(merge_request).to be_valid + expect(merge_request).to be_merged + + commit_messages = project.repository.commits('master', limit: 2).map(&:message) + expect(commit_messages.uniq.size).to eq(2) + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end + end + + context 'when an invalid sha is passed' do + let(:merge_request) do + create( + :merge_request, + :simple, + author: user2, + assignees: [user2], + squash: true, + source_branch: 'improve/awesome', + target_branch: 'fix' + ) + end + + let(:merge_params) do + { sha: merge_request.commits.second.sha } + end + + it 'does not merge the MR' do + service.execute(merge_request) + + expect(merge_request).not_to be_merged + expect(merge_request.merge_error).to match(/Branch has been updated/) + end + end + + context 'when the `sha` param is missing' do + let(:merge_params) { {} } + + it 'returns the error' do + merge_error = 'Branch has been updated since the merge was requested. '\ + 'Please review the changes.' + + expect { service.execute(merge_request) } + .to change { merge_request.merge_error } + .from(nil).to(merge_error) + end + end + + context 'closes related issues' do + before do + allow(project).to receive(:default_branch).and_return(merge_request.target_branch) + end + + it 'closes GitLab issue tracker issues', :sidekiq_inline do + issue = create :issue, project: project + commit = double('commit', safe_message: "Fixes #{issue.to_reference}", date: Time.current, authored_date: Time.current) + allow(merge_request).to receive(:commits).and_return([commit]) + merge_request.cache_merge_request_closes_issues! + + service.execute(merge_request) + + expect(issue.reload.closed?).to be_truthy + end + + context 'with Jira integration' do + include JiraIntegrationHelpers + + let(:jira_tracker) { project.create_jira_integration } + let(:jira_issue) { ExternalIssue.new('JIRA-123', project) } + let(:commit) { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") } + before do - allow(project).to receive(:default_branch).and_return(merge_request.target_branch) - end - - it 'closes GitLab issue tracker issues', :sidekiq_inline do - issue = create :issue, project: project - commit = double('commit', safe_message: "Fixes #{issue.to_reference}", date: Time.current, authored_date: Time.current) + stub_jira_integration_test + project.update!(has_external_issue_tracker: true) + jira_integration_settings + stub_jira_urls(jira_issue.id) allow(merge_request).to receive(:commits).and_return([commit]) - merge_request.cache_merge_request_closes_issues! - - service.execute(merge_request) - - expect(issue.reload.closed?).to be_truthy end - context 'with Jira integration' do - include JiraIntegrationHelpers + it 'closes issues on Jira issue tracker' do + jira_issue = ExternalIssue.new('JIRA-123', project) + stub_jira_urls(jira_issue) + commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") + allow(merge_request).to receive(:commits).and_return([commit]) - let(:jira_tracker) { project.create_jira_integration } - let(:jira_issue) { ExternalIssue.new('JIRA-123', project) } - let(:commit) { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") } + expect_any_instance_of(Integrations::Jira).to receive(:close_issue).with(merge_request, jira_issue, user).once - before do - stub_jira_integration_test - project.update!(has_external_issue_tracker: true) - jira_integration_settings - stub_jira_urls(jira_issue.id) - allow(merge_request).to receive(:commits).and_return([commit]) - end + service.execute(merge_request) + end - it 'closes issues on Jira issue tracker' do - jira_issue = ExternalIssue.new('JIRA-123', project) + context 'wrong issue markdown' do + it 'does not close issues on Jira issue tracker' do + jira_issue = ExternalIssue.new('#JIRA-123', project) stub_jira_urls(jira_issue) commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") allow(merge_request).to receive(:commits).and_return([commit]) - expect_any_instance_of(Integrations::Jira).to receive(:close_issue).with(merge_request, jira_issue, user).once + expect_any_instance_of(Integrations::Jira).not_to receive(:close_issue) service.execute(merge_request) end + end + end + end - context 'wrong issue markdown' do - it 'does not close issues on Jira issue tracker' do - jira_issue = ExternalIssue.new('#JIRA-123', project) - stub_jira_urls(jira_issue) - commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") - allow(merge_request).to receive(:commits).and_return([commit]) + context 'closes related todos' do + let(:merge_request) { create(:merge_request, assignees: [user], author: user) } + let(:project) { merge_request.project } - expect_any_instance_of(Integrations::Jira).not_to receive(:close_issue) + let!(:todo) do + create(:todo, :assigned, + project: project, + author: user, + user: user, + target: merge_request) + end - service.execute(merge_request) - end - end + before do + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + service.execute(merge_request) + todo.reload end end - context 'closes related todos' do - let(:merge_request) { create(:merge_request, assignees: [user], author: user) } - let(:project) { merge_request.project } + it { expect(todo).to be_done } + end - let!(:todo) do - create(:todo, :assigned, - project: project, - author: user, - user: user, - target: merge_request) + context 'source branch removal' do + context 'when the source branch is protected' do + let(:service) do + described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) end before do - allow(service).to receive(:execute_hooks) + create(:protected_branch, project: project, name: merge_request.source_branch) + end + + it 'does not delete the source branch' do + expect(::Branches::DeleteService).not_to receive(:new) + + service.execute(merge_request) + end + end + + context 'when the source branch is the default branch' do + let(:service) do + described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) + end + + before do + allow(project).to receive(:root_ref?).with(merge_request.source_branch).and_return(true) + end + + it 'does not delete the source branch' do + expect(::Branches::DeleteService).not_to receive(:new) + service.execute(merge_request) + end + end + + context 'when the source branch can be removed' do + context 'when MR author set the source branch to be removed' do + before do + merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' }) + end + + # Not a real use case. When a merger merges a MR , merge param 'should_remove_source_branch' is defined + it 'removes the source branch using the author user' do + expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, merge_request.author.id) - perform_enqueued_jobs do service.execute(merge_request) - todo.reload + + expect(merge_request.reload.should_remove_source_branch?).to be nil + end + + context 'when the merger set the source branch not to be removed' do + let(:service) { described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => false)) } + + it 'does not delete the source branch' do + expect(::MergeRequests::DeleteSourceBranchWorker).not_to receive(:perform_async) + + service.execute(merge_request) + + expect(merge_request.reload.should_remove_source_branch?).to be false + end end end - it { expect(todo).to be_done } - end - - context 'source branch removal' do - context 'when the source branch is protected' do + context 'when MR merger set the source branch to be removed' do let(:service) do described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) end - before do - create(:protected_branch, project: project, name: merge_request.source_branch) - end - - it 'does not delete the source branch' do - expect(::Branches::DeleteService).not_to receive(:new) + it 'removes the source branch using the current user' do + expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, user.id) service.execute(merge_request) - end - end - context 'when the source branch is the default branch' do - let(:service) do - described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) - end - - before do - allow(project).to receive(:root_ref?).with(merge_request.source_branch).and_return(true) - end - - it 'does not delete the source branch' do - expect(::Branches::DeleteService).not_to receive(:new) - service.execute(merge_request) - end - end - - context 'when the source branch can be removed' do - context 'when MR author set the source branch to be removed' do - before do - merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' }) - end - - # Not a real use case. When a merger merges a MR , merge param 'should_remove_source_branch' is defined - it 'removes the source branch using the author user' do - expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, merge_request.author.id) - - service.execute(merge_request) - - expect(merge_request.reload.should_remove_source_branch?).to be nil - end - - context 'when the merger set the source branch not to be removed' do - let(:service) { described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => false)) } - - it 'does not delete the source branch' do - expect(::MergeRequests::DeleteSourceBranchWorker).not_to receive(:perform_async) - - service.execute(merge_request) - - expect(merge_request.reload.should_remove_source_branch?).to be false - end - end - end - - context 'when MR merger set the source branch to be removed' do - let(:service) do - described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) - end - - it 'removes the source branch using the current user' do - expect(::MergeRequests::DeleteSourceBranchWorker).to receive(:perform_async).with(merge_request.id, merge_request.source_branch_sha, user.id) - - service.execute(merge_request) - - expect(merge_request.reload.should_remove_source_branch?).to be true - end + expect(merge_request.reload.should_remove_source_branch?).to be true end end end + end - context 'error handling' do - before do - allow(Gitlab::AppLogger).to receive(:error) - end + context 'error handling' do + before do + allow(Gitlab::AppLogger).to receive(:error) + end - context 'when source is missing' do - it 'logs and saves error' do - allow(merge_request).to receive(:diff_head_sha) { nil } + context 'when source is missing' do + it 'logs and saves error' do + allow(merge_request).to receive(:diff_head_sha) { nil } - error_message = 'No source for merge' - - service.execute(merge_request) - - expect(merge_request.merge_error).to eq(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - end - - it 'logs and saves error if there is an exception' do - error_message = 'error message' - - allow_next_instance_of(MergeRequests::MergeStrategies::FromSourceBranch) do |strategy| - allow(strategy).to receive(:execute_git_merge!).and_raise(error_message) - end - # we can remove these allows upon refactor_merge_service cleanup - allow(service).to receive(:repository).and_raise(error_message) - allow(service).to receive(:execute_hooks) + error_message = 'No source for merge' service.execute(merge_request) - expect(merge_request.merge_error).to eq(described_class::GENERIC_ERROR_MESSAGE) + expect(merge_request.merge_error).to eq(error_message) + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end + end + + it 'logs and saves error if there is an exception' do + error_message = 'error message' + + allow_next_instance_of(MergeRequests::MergeStrategies::FromSourceBranch) do |strategy| + allow(strategy).to receive(:execute_git_merge!).and_raise(error_message) + end + + service.execute(merge_request) + + expect(merge_request.merge_error).to eq(described_class::GENERIC_ERROR_MESSAGE) + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end + + it 'logs and saves error if user is not authorized' do + stub_exclusive_lease + + unauthorized_user = create(:user) + project.add_reporter(unauthorized_user) + + service = described_class.new(project: project, current_user: unauthorized_user) + + service.execute(merge_request) + + expect(merge_request.merge_error) + .to eq('You are not allowed to merge this merge request') + end + + it 'logs and saves error if there is an PreReceiveError exception' do + error_message = 'error message' + + allow_next_instance_of(MergeRequests::MergeStrategies::FromSourceBranch) do |strategy| + allow(strategy).to receive(:execute_git_merge!).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") + end + + service.execute(merge_request) + + expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end + + it 'logs and saves error if commit is not created' do + allow_any_instance_of(Repository).to receive(:merge).and_return(false) + allow(service).to receive(:execute_hooks) + + service.execute(merge_request) + + expect(merge_request).to be_open + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.merge_error).to include(described_class::GENERIC_ERROR_MESSAGE) + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(described_class::GENERIC_ERROR_MESSAGE) + ) + ) + end + + context 'when squashing is required' do + before do + merge_request.update!(source_branch: 'master', target_branch: 'feature') + merge_request.target_project.project_setting.squash_always! + end + + it 'raises an error if squashing is not done' do + error_message = 'requires squashing commits' + + service.execute(merge_request) + + expect(merge_request).to be_open + + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil + expect(merge_request.merge_error).to include(error_message) + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end + end + + context 'when squashing' do + before do + merge_request.update!(source_branch: 'master', target_branch: 'feature') + end + + it 'logs and saves error if there is an error when squashing' do + error_message = 'Squashing failed: Squash the commits locally, resolve any conflicts, then push the branch.' + + allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) + merge_request.update!(squash: true) + + service.execute(merge_request) + + expect(merge_request).to be_open + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil + expect(merge_request.merge_error).to include(error_message) expect(Gitlab::AppLogger).to have_received(:error).with( hash_including( merge_request_info: merge_request.to_reference(full: true), @@ -447,32 +538,19 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf ) end - it 'logs and saves error if user is not authorized' do - stub_exclusive_lease - - unauthorized_user = create(:user) - project.add_reporter(unauthorized_user) - - service = described_class.new(project: project, current_user: unauthorized_user) - - service.execute(merge_request) - - expect(merge_request.merge_error) - .to eq('You are not allowed to merge this merge request') - end - it 'logs and saves error if there is an PreReceiveError exception' do error_message = 'error message' allow_next_instance_of(MergeRequests::MergeStrategies::FromSourceBranch) do |strategy| allow(strategy).to receive(:execute_git_merge!).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") end - # we can remove these allows upon refactor_merge_service cleanup - allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") - allow(service).to receive(:execute_hooks) + merge_request.update!(squash: true) service.execute(merge_request) + expect(merge_request).to be_open + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') expect(Gitlab::AppLogger).to have_received(:error).with( hash_including( @@ -482,139 +560,25 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf ) end - it 'logs and saves error if commit is not created' do - allow_any_instance_of(Repository).to receive(:merge).and_return(false) - allow(service).to receive(:execute_hooks) - - service.execute(merge_request) - - expect(merge_request).to be_open - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.merge_error).to include(described_class::GENERIC_ERROR_MESSAGE) - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(described_class::GENERIC_ERROR_MESSAGE) - ) - ) - end - - context 'when squashing is required' do + context 'when fast-forward merge is not allowed' do before do - merge_request.update!(source_branch: 'master', target_branch: 'feature') - merge_request.target_project.project_setting.squash_always! + allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) end - it 'raises an error if squashing is not done' do - error_message = 'requires squashing commits' + %w(semi-linear ff).each do |merge_method| + it "logs and saves error if merge is #{merge_method} only" do + merge_method = 'rebase_merge' if merge_method == 'semi-linear' + merge_request.project.update!(merge_method: merge_method) + error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch' + allow(service).to receive(:execute_hooks) + expect(lease).to receive(:cancel) - service.execute(merge_request) - - expect(merge_request).to be_open - - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.squash_commit_sha).to be_nil - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - end - - context 'when squashing' do - before do - merge_request.update!(source_branch: 'master', target_branch: 'feature') - end - - it 'logs and saves error if there is an error when squashing' do - error_message = 'Squashing failed: Squash the commits locally, resolve any conflicts, then push the branch.' - - allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) - merge_request.update!(squash: true) - - service.execute(merge_request) - - expect(merge_request).to be_open - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.squash_commit_sha).to be_nil - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - - it 'logs and saves error if there is an PreReceiveError exception' do - error_message = 'error message' - - allow_next_instance_of(MergeRequests::MergeStrategies::FromSourceBranch) do |strategy| - allow(strategy).to receive(:execute_git_merge!).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") - end - # we can remove these allows upon refactor_merge_service cleanup - allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") - allow(service).to receive(:execute_hooks) - merge_request.update!(squash: true) - - service.execute(merge_request) - - expect(merge_request).to be_open - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.squash_commit_sha).to be_nil - expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - - context 'when fast-forward merge is not allowed' do - before do - allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) - end - - %w(semi-linear ff).each do |merge_method| - it "logs and saves error if merge is #{merge_method} only" do - merge_method = 'rebase_merge' if merge_method == 'semi-linear' - merge_request.project.update!(merge_method: merge_method) - error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch' - allow(service).to receive(:execute_hooks) - expect(lease).to receive(:cancel) - - service.execute(merge_request) - - expect(merge_request).to be_open - expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.squash_commit_sha).to be_nil - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - end - end - end - - context 'when not mergeable' do - let!(:error_message) { 'Merge request is not mergeable' } - - context 'with failing CI' do - before do - allow(merge_request).to receive(:mergeable_ci_state?) { false } - end - - it 'logs and saves error' do service.execute(merge_request) + expect(merge_request).to be_open + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil + expect(merge_request.merge_error).to include(error_message) expect(Gitlab::AppLogger).to have_received(:error).with( hash_including( merge_request_info: merge_request.to_reference(full: true), @@ -623,54 +587,75 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf ) end end - - context 'with unresolved discussions' do - before do - allow(merge_request).to receive(:mergeable_discussions_state?) { false } - end - - it 'logs and saves error' do - service.execute(merge_request) - - expect(Gitlab::AppLogger).to have_received(:error).with( - hash_including( - merge_request_info: merge_request.to_reference(full: true), - message: a_string_matching(error_message) - ) - ) - end - - context 'when passing `skip_discussions_check: true` as `options` parameter' do - it 'merges the merge request' do - service.execute(merge_request, skip_discussions_check: true) - - expect(merge_request).to be_valid - expect(merge_request).to be_merged - end - end - end - end - - context 'when passing `check_mergeability_retry_lease: true` as `options` parameter' do - it 'call mergeable? with check_mergeability_retry_lease' do - expect(merge_request).to receive(:mergeable?).with(hash_including(check_mergeability_retry_lease: true)).and_call_original - - service.execute(merge_request, check_mergeability_retry_lease: true) - end end end - context 'when the other sidekiq worker has already been running' do - before do - stub_exclusive_lease_taken(lease_key) + context 'when not mergeable' do + let!(:error_message) { 'Merge request is not mergeable' } + + context 'with failing CI' do + before do + allow(merge_request).to receive(:mergeable_ci_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end end - it 'does not execute service' do - expect(service).not_to receive(:commit) + context 'with unresolved discussions' do + before do + allow(merge_request).to receive(:mergeable_discussions_state?) { false } + end - service.execute(merge_request) + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with( + hash_including( + merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message) + ) + ) + end + + context 'when passing `skip_discussions_check: true` as `options` parameter' do + it 'merges the merge request' do + service.execute(merge_request, skip_discussions_check: true) + + expect(merge_request).to be_valid + expect(merge_request).to be_merged + end + end end end + + context 'when passing `check_mergeability_retry_lease: true` as `options` parameter' do + it 'call mergeable? with check_mergeability_retry_lease' do + expect(merge_request).to receive(:mergeable?).with(hash_including(check_mergeability_retry_lease: true)).and_call_original + + service.execute(merge_request, check_mergeability_retry_lease: true) + end + end + end + + context 'when the other sidekiq worker has already been running' do + before do + stub_exclusive_lease_taken(lease_key) + end + + it 'does not execute service' do + expect(service).not_to receive(:commit) + + service.execute(merge_request) + end end end end diff --git a/spec/support/shared_contexts/features/integrations/project_integrations_shared_context.rb b/spec/support/shared_contexts/features/integrations/project_integrations_shared_context.rb index a9b9a5246e6..e123469fc8f 100644 --- a/spec/support/shared_contexts/features/integrations/project_integrations_shared_context.rb +++ b/spec/support/shared_contexts/features/integrations/project_integrations_shared_context.rb @@ -4,7 +4,7 @@ RSpec.shared_context 'project integration activation' do include_context 'with integration activation' let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :no_super_sidebar) } before do project.add_maintainer(user) diff --git a/spec/support/shared_examples/features/2fa_shared_examples.rb b/spec/support/shared_examples/features/2fa_shared_examples.rb index 6c4e98c9989..f50874b6b05 100644 --- a/spec/support/shared_examples/features/2fa_shared_examples.rb +++ b/spec/support/shared_examples/features/2fa_shared_examples.rb @@ -14,7 +14,7 @@ RSpec.shared_examples 'hardware device for 2fa' do |device_type| end describe "registration" do - let(:user) { create(:user) } + let(:user) { create(:user, :no_super_sidebar) } before do gitlab_sign_in(user) @@ -67,7 +67,7 @@ RSpec.shared_examples 'hardware device for 2fa' do |device_type| end describe 'fallback code authentication' do - let(:user) { create(:user) } + let(:user) { create(:user, :no_super_sidebar) } before do # Register and logout diff --git a/spec/support/shared_examples/features/project_features_apply_to_issuables_shared_examples.rb b/spec/support/shared_examples/features/project_features_apply_to_issuables_shared_examples.rb index d410653ca43..58bf461c733 100644 --- a/spec/support/shared_examples/features/project_features_apply_to_issuables_shared_examples.rb +++ b/spec/support/shared_examples/features/project_features_apply_to_issuables_shared_examples.rb @@ -4,8 +4,8 @@ RSpec.shared_examples 'project features apply to issuables' do |klass| let(:described_class) { klass } let(:group) { create(:group) } - let(:user_in_group) { create(:group_member, :developer, user: create(:user), group: group ).user } - let(:user_outside_group) { create(:user) } + let(:user_in_group) { create(:group_member, :developer, user: create(:user, :no_super_sidebar), group: group ).user } + let(:user_outside_group) { create(:user, :no_super_sidebar) } let(:project) { create(:project, :public, project_args) } diff --git a/spec/support/shared_examples/features/snippets_shared_examples.rb b/spec/support/shared_examples/features/snippets_shared_examples.rb index bf870b3ce66..383f81d048f 100644 --- a/spec/support/shared_examples/features/snippets_shared_examples.rb +++ b/spec/support/shared_examples/features/snippets_shared_examples.rb @@ -52,7 +52,7 @@ RSpec.shared_examples 'tabs with counts' do end RSpec.shared_examples 'does not show New Snippet button' do - let(:user) { create(:user, :external) } + let(:user) { create(:user, :external, :no_super_sidebar) } specify do sign_in(user)