Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--.gitpod.yml2
-rw-r--r--.rubocop.yml3
-rw-r--r--.rubocop_todo/background_migration/avoid_silent_rescue_exceptions.yml9
-rw-r--r--Gemfile2
-rw-r--r--Gemfile.checksum2
-rw-r--r--Gemfile.lock4
-rw-r--r--doc/development/testing_guide/best_practices.md28
-rw-r--r--doc/user/application_security/secret_detection/index.md2
-rw-r--r--lib/gitlab/background_migration/.rubocop.yml5
-rw-r--r--rubocop/cop/background_migration/avoid_silent_rescue_exceptions.rb84
-rw-r--r--spec/frontend/vue_shared/alert_details/alert_status_spec.js120
-rw-r--r--spec/lib/gitlab/i18n/pluralization_spec.rb1
-rw-r--r--spec/rubocop/cop/background_migration/avoid_silent_rescue_exceptions_spec.rb107
13 files changed, 293 insertions, 76 deletions
diff --git a/.gitpod.yml b/.gitpod.yml
index 19785aba125..81e08a98674 100644
--- a/.gitpod.yml
+++ b/.gitpod.yml
@@ -53,7 +53,7 @@ tasks:
cd /workspace/gitlab-development-kit
# update GDK
echo "$(date) – Updating GDK" | tee -a /workspace/startup.log
- export DEFAULT_BRANCH=$(git branch --show-current)
+ export DEFAULT_BRANCH=$(git --git-dir=gitlab/.git branch --show-current)
gdk config set gitlab.default_branch "$DEFAULT_BRANCH"
gdk update
# ensure gdk.yml has correct instance settings
diff --git a/.rubocop.yml b/.rubocop.yml
index a7ebc87f4b3..7c6979935e5 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -925,6 +925,9 @@ Performance/ActiveRecordSubtransactionMethods:
Migration/BackgroundMigrationBaseClass:
Enabled: false
+BackgroundMigration/AvoidSilentRescueExceptions:
+ Enabled: false
+
Style/ClassAndModuleChildren:
Enabled: true
diff --git a/.rubocop_todo/background_migration/avoid_silent_rescue_exceptions.yml b/.rubocop_todo/background_migration/avoid_silent_rescue_exceptions.yml
new file mode 100644
index 00000000000..8a781903b03
--- /dev/null
+++ b/.rubocop_todo/background_migration/avoid_silent_rescue_exceptions.yml
@@ -0,0 +1,9 @@
+---
+BackgroundMigration/AvoidSilentRescueExceptions:
+ Details: grace period
+ Exclude:
+ - 'lib/gitlab/background_migration/backfill_imported_issue_search_data.rb'
+ - 'lib/gitlab/background_migration/migrate_links_for_vulnerability_findings.rb'
+ - 'lib/gitlab/background_migration/populate_vulnerability_dismissal_fields.rb'
+ - 'lib/gitlab/background_migration/reset_status_on_container_repositories.rb'
+ - 'lib/gitlab/background_migration/migrate_remediations_for_vulnerability_findings.rb'
diff --git a/Gemfile b/Gemfile
index f14f85066b4..c21743f9e24 100644
--- a/Gemfile
+++ b/Gemfile
@@ -346,7 +346,7 @@ gem 'thrift', '>= 0.16.0'
# I18n
gem 'ruby_parser', '~> 3.20', require: false
gem 'rails-i18n', '~> 7.0'
-gem 'gettext_i18n_rails', '~> 1.8.0'
+gem 'gettext_i18n_rails', '~> 1.11.0'
gem 'gettext_i18n_rails_js', '~> 1.3'
gem 'gettext', '~> 3.3', require: false, group: :development
diff --git a/Gemfile.checksum b/Gemfile.checksum
index b2c07e5a201..6ae52adfb06 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -201,7 +201,7 @@
{"name":"gemoji","version":"3.0.1","platform":"ruby","checksum":"80553f2f4932a7a95fb1b3c7c63f7dd937e7c8c610164bbdea28fd06eba5f36d"},
{"name":"get_process_mem","version":"0.2.7","platform":"ruby","checksum":"4afd3c3641dd6a817c09806c7d6d509d8a9984512ac38dea8b917426bbf77eba"},
{"name":"gettext","version":"3.3.6","platform":"ruby","checksum":"ee6bbd1b2f833ee52d7797fa68acbfecc4726aec6b6280fd7eab92aa0190b413"},
-{"name":"gettext_i18n_rails","version":"1.8.0","platform":"ruby","checksum":"95e5cf8440b1e08705b27f2bccb56143272c5a7a0dabcf54ea1bd701140a496f"},
+{"name":"gettext_i18n_rails","version":"1.11.0","platform":"ruby","checksum":"e19c7e4a256c500f7f38396dca44a282b9838ae278f57c362993a54964b22bbe"},
{"name":"gettext_i18n_rails_js","version":"1.3.0","platform":"ruby","checksum":"5d10afe4be3639bff78c50a56768c20f39aecdabc580c08aa45573911c2bd687"},
{"name":"git","version":"1.11.0","platform":"ruby","checksum":"7e95ba4da8298a0373ef1a6862aa22007d761f3c8274b675aa787966fecea0f1"},
{"name":"gitaly","version":"16.1.0.pre.rc2","platform":"ruby","checksum":"0007db161bd3f12321813e7095c8d3266d74b0a22b84d7cba8df864c0b88e3af"},
diff --git a/Gemfile.lock b/Gemfile.lock
index 26594b29704..06a3d1e70e3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -572,7 +572,7 @@ GEM
gettext (3.3.6)
locale (>= 2.0.5)
text (>= 1.3.0)
- gettext_i18n_rails (1.8.0)
+ gettext_i18n_rails (1.11.0)
fast_gettext (>= 0.9.0)
gettext_i18n_rails_js (1.3.0)
gettext (>= 3.0.2)
@@ -1753,7 +1753,7 @@ DEPENDENCIES
fugit (~> 1.8.1)
fuubar (~> 2.2.0)
gettext (~> 3.3)
- gettext_i18n_rails (~> 1.8.0)
+ gettext_i18n_rails (~> 1.11.0)
gettext_i18n_rails_js (~> 1.3)
gitaly (~> 16.1.0.pre.rc2)
gitlab-chronic (~> 0.10.5)
diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md
index 90267a517b8..02c8ad44033 100644
--- a/doc/development/testing_guide/best_practices.md
+++ b/doc/development/testing_guide/best_practices.md
@@ -1470,19 +1470,17 @@ under `spec/support/helpers/`. Helpers can be placed in a subfolder if they appl
to a certain type of specs only (such as features or requests) but shouldn't be
if they apply to multiple type of specs.
-Helpers should follow the Rails naming / namespacing convention. For instance
-`spec/support/helpers/cycle_analytics_helpers.rb` should define:
+Helpers should follow the Rails naming / namespacing convention, where
+`spec/support/helpers/` is the root. For instance
+`spec/support/helpers/features/iteration_helpers.rb` should define:
```ruby
-module Spec
- module Support
- module Helpers
- module CycleAnalyticsHelpers
- def create_commit_referencing_issue(issue, branch_name: random_git_name)
- project.repository.add_branch(user, branch_name, 'main')
- create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name)
- end
- end
+# frozen_string_literal: true
+
+module Features
+ module IterationHelpers
+ def iteration_period(iteration)
+ "#{iteration.start_date.to_s(:medium)} - #{iteration.due_date.to_s(:medium)}"
end
end
end
@@ -1492,8 +1490,14 @@ Helpers should not change the RSpec configuration. For instance, the helpers mod
described above should not include:
```ruby
+# bad
RSpec.configure do |config|
- config.include Spec::Support::Helpers::CycleAnalyticsHelpers
+ config.include Features::IterationHelpers
+end
+
+# good, include in specific spec
+RSpec.describe 'Issue Sidebar', feature_category: :team_planning do
+ include Features::IterationHelpers
end
```
diff --git a/doc/user/application_security/secret_detection/index.md b/doc/user/application_security/secret_detection/index.md
index 2e6de222ec3..c6928d3679b 100644
--- a/doc/user/application_security/secret_detection/index.md
+++ b/doc/user/application_security/secret_detection/index.md
@@ -83,7 +83,7 @@ Secret Detection can detect if a secret was added in one commit and removed in a
- Commit range
- If the `SECRET_DETECTION_LOG_OPTS` variable is set, the secrets analyzer fetches the entire
+ If the `SECRET_DETECTION_LOG_OPTIONS` variable is set, the secrets analyzer fetches the entire
history of the branch or reference the pipeline is being run for. Secret Detection then runs,
scanning the commit range specified.
diff --git a/lib/gitlab/background_migration/.rubocop.yml b/lib/gitlab/background_migration/.rubocop.yml
index 116c84c3759..9424686340f 100644
--- a/lib/gitlab/background_migration/.rubocop.yml
+++ b/lib/gitlab/background_migration/.rubocop.yml
@@ -59,3 +59,8 @@ Migration/BackgroundMigrationBaseClass:
- 'base_job.rb'
- 'batched_migration_job.rb'
- 'logger.rb'
+
+BackgroundMigration/AvoidSilentRescueExceptions:
+ Enabled: true
+ Description: >-
+ Rescuing errors in batched background migration jobs can lead to undesired results
diff --git a/rubocop/cop/background_migration/avoid_silent_rescue_exceptions.rb b/rubocop/cop/background_migration/avoid_silent_rescue_exceptions.rb
new file mode 100644
index 00000000000..0bd5c587205
--- /dev/null
+++ b/rubocop/cop/background_migration/avoid_silent_rescue_exceptions.rb
@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+module RuboCop
+ module Cop
+ module BackgroundMigration
+ # Checks for rescuing errors inside Batched Background Migration Job Classes.
+ #
+ # @example
+ #
+ # # bad
+ # def perform
+ # do_something
+ # rescue StandardError => error
+ # logger.error(error.message)
+ # end
+ #
+ # # bad
+ # def perform
+ # do_something
+ # rescue JSON::ParserError, ActiveRecord::StatementTimeout => error
+ # logger.error(error.message)
+ # end
+ #
+ # # good
+ # def perform
+ # do_something
+ # rescue StandardError => error
+ # logger.error(error.message)
+ #
+ # raise
+ # end
+ #
+ # # good
+ # def perform
+ # do_something
+ # rescue JSON::ParserError, ActiveRecord::StatementTimeout => error
+ # logger.error(error.message)
+ #
+ # raise MyCustomException
+ # end
+ class AvoidSilentRescueExceptions < RuboCop::Cop::Base
+ MSG = 'Avoid rescuing exceptions inside job classes. See ' \
+ 'https://docs.gitlab.com/ee/development/database/batched_background_migrations.html#best-practices'
+
+ def_node_matcher :batched_migration_job_class?, <<~PATTERN
+ (class
+ const
+ (const {nil? cbase const} :BatchedMigrationJob)
+ ...
+ )
+ PATTERN
+
+ # Matches rescued exceptions that were not re-raised
+ #
+ # @example
+ # rescue => error
+ # rescue Exception => error
+ # rescue JSON::ParserError => e
+ # rescue ActiveRecord::StatementTimeout => error
+ # rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled => error
+ def_node_matcher :rescue_timeout_error, <<~PATTERN
+ (resbody $_ _ (... !(send nil? :raise ...)))
+ PATTERN
+
+ def on_class(node)
+ @batched_migration_job_class ||= batched_migration_job_class?(node)
+ end
+
+ def on_resbody(node)
+ return unless batched_migration_job_class
+
+ rescue_timeout_error(node) do |error|
+ range = error ? node.loc.keyword.join(error.loc.expression) : node.loc.keyword
+ add_offense(range)
+ end
+ end
+
+ private
+
+ attr_reader :batched_migration_job_class
+ end
+ end
+ end
+end
diff --git a/spec/frontend/vue_shared/alert_details/alert_status_spec.js b/spec/frontend/vue_shared/alert_details/alert_status_spec.js
index 98cb2f5cb0b..90d29f0bfd4 100644
--- a/spec/frontend/vue_shared/alert_details/alert_status_spec.js
+++ b/spec/frontend/vue_shared/alert_details/alert_status_spec.js
@@ -1,7 +1,9 @@
+import Vue from 'vue';
+import VueApollo from 'vue-apollo';
import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
-import { nextTick } from 'vue';
import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
import waitForPromises from 'helpers/wait_for_promises';
+import createMockApollo from 'helpers/mock_apollo_helper';
import updateAlertStatusMutation from '~/graphql_shared//mutations/alert_status_update.mutation.graphql';
import Tracking from '~/tracking';
import AlertManagementStatus from '~/vue_shared/alert_details/components/alert_status.vue';
@@ -11,6 +13,27 @@ const mockAlert = mockAlerts[0];
describe('AlertManagementStatus', () => {
let wrapper;
+ let requestHandler;
+
+ const iid = '1527542';
+ const mockUpdatedMutationResult = ({ errors = [], nodes = [] } = {}) =>
+ jest.fn().mockResolvedValue({
+ data: {
+ updateAlertStatus: {
+ errors,
+ alert: {
+ id: '1',
+ iid,
+ status: 'acknowledged',
+ endedAt: 'endedAt',
+ notes: {
+ nodes,
+ },
+ },
+ },
+ },
+ });
+
const findStatusDropdown = () => wrapper.findComponent(GlDropdown);
const findFirstStatusOption = () => findStatusDropdown().findComponent(GlDropdownItem);
const findAllStatusOptions = () => findStatusDropdown().findAllComponents(GlDropdownItem);
@@ -22,8 +45,20 @@ describe('AlertManagementStatus', () => {
return waitForPromises();
};
- function mountComponent({ props = {}, provide = {}, loading = false, stubs = {} } = {}) {
+ const createMockApolloProvider = (handler) => {
+ Vue.use(VueApollo);
+ requestHandler = handler;
+
+ return createMockApollo([[updateAlertStatusMutation, handler]]);
+ };
+
+ function mountComponent({
+ props = {},
+ provide = {},
+ handler = mockUpdatedMutationResult(),
+ } = {}) {
wrapper = shallowMountExtended(AlertManagementStatus, {
+ apolloProvider: createMockApolloProvider(handler),
propsData: {
alert: { ...mockAlert },
projectPath: 'gitlab-org/gitlab',
@@ -31,17 +66,6 @@ describe('AlertManagementStatus', () => {
...props,
},
provide,
- mocks: {
- $apollo: {
- mutate: jest.fn(),
- queries: {
- alert: {
- loading,
- },
- },
- },
- },
- stubs,
});
}
@@ -63,43 +87,32 @@ describe('AlertManagementStatus', () => {
});
describe('updating the alert status', () => {
- const iid = '1527542';
- const mockUpdatedMutationResult = {
- data: {
- updateAlertStatus: {
- errors: [],
- alert: {
- iid,
- status: 'acknowledged',
- },
- },
- },
- };
-
beforeEach(() => {
- mountComponent({});
+ mountComponent();
});
- it('calls `$apollo.mutate` with `updateAlertStatus` mutation and variables containing `iid`, `status`, & `projectPath`', () => {
- jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue(mockUpdatedMutationResult);
+ it('calls `$apollo.mutate` with `updateAlertStatus` mutation and variables containing `iid`, `status`, & `projectPath`', async () => {
findFirstStatusOption().vm.$emit('click');
+ await waitForPromises();
- expect(wrapper.vm.$apollo.mutate).toHaveBeenCalledWith({
- mutation: updateAlertStatusMutation,
- variables: {
- iid,
- status: 'TRIGGERED',
- projectPath: 'gitlab-org/gitlab',
- },
+ expect(requestHandler).toHaveBeenCalledWith({
+ iid,
+ status: 'TRIGGERED',
+ projectPath: 'gitlab-org/gitlab',
});
});
describe('when a request fails', () => {
- beforeEach(() => {
- jest.spyOn(wrapper.vm.$apollo, 'mutate').mockReturnValue(Promise.reject(new Error()));
+ beforeEach(async () => {
+ mountComponent({
+ handler: mockUpdatedMutationResult({ errors: ['<span data-testid="htmlError" />'] }),
+ });
+ await waitForPromises();
});
it('emits an error', async () => {
+ mountComponent({ handler: jest.fn().mockRejectedValue({}) });
+ await waitForPromises();
await selectFirstStatusOption();
expect(wrapper.emitted('alert-error')[0]).toEqual([
@@ -116,7 +129,6 @@ describe('AlertManagementStatus', () => {
it('emits an error when triggered a second time', async () => {
await selectFirstStatusOption();
- await nextTick();
await selectFirstStatusOption();
// Should emit two errors [0,1]
expect(wrapper.emitted('alert-error').length > 1).toBe(true);
@@ -124,19 +136,9 @@ describe('AlertManagementStatus', () => {
});
it('shows an error when response includes HTML errors', async () => {
- const mockUpdatedMutationErrorResult = {
- data: {
- updateAlertStatus: {
- errors: ['<span data-testid="htmlError" />'],
- alert: {
- iid,
- status: 'acknowledged',
- },
- },
- },
- };
-
- jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue(mockUpdatedMutationErrorResult);
+ mountComponent({
+ handler: mockUpdatedMutationResult({ errors: ['<span data-testid="htmlError" />'] }),
+ });
await selectFirstStatusOption();
@@ -160,7 +162,7 @@ describe('AlertManagementStatus', () => {
mountComponent({
props: { alert: { ...mockAlert, status }, statuses: { [status]: translatedStatus } },
});
- expect(findAllStatusOptions().length).toBe(1);
+ expect(findAllStatusOptions()).toHaveLength(1);
expect(findFirstStatusOption().text()).toBe(translatedStatus);
});
});
@@ -173,10 +175,10 @@ describe('AlertManagementStatus', () => {
it('should not track alert status updates when the tracking options do not exist', async () => {
mountComponent({});
Tracking.event.mockClear();
- jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue({});
+
findFirstStatusOption().vm.$emit('click');
- await nextTick();
+ await waitForPromises();
expect(Tracking.event).not.toHaveBeenCalled();
});
@@ -187,12 +189,14 @@ describe('AlertManagementStatus', () => {
action: 'update_alert_status',
label: 'Status',
};
- mountComponent({ provide: { trackAlertStatusUpdateOptions } });
+ mountComponent({
+ provide: { trackAlertStatusUpdateOptions },
+ handler: mockUpdatedMutationResult({ nodes: mockAlerts }),
+ });
Tracking.event.mockClear();
- jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue({});
findFirstStatusOption().vm.$emit('click');
- await nextTick();
+ await waitForPromises();
const status = findFirstStatusOption().text();
const { category, action, label } = trackAlertStatusUpdateOptions;
diff --git a/spec/lib/gitlab/i18n/pluralization_spec.rb b/spec/lib/gitlab/i18n/pluralization_spec.rb
index 857562d549c..6bfc500c8b8 100644
--- a/spec/lib/gitlab/i18n/pluralization_spec.rb
+++ b/spec/lib/gitlab/i18n/pluralization_spec.rb
@@ -2,6 +2,7 @@
require 'fast_spec_helper'
require 'rspec-parameterized'
+require 'rails/version'
require 'gettext_i18n_rails'
RSpec.describe Gitlab::I18n::Pluralization, feature_category: :internationalization do
diff --git a/spec/rubocop/cop/background_migration/avoid_silent_rescue_exceptions_spec.rb b/spec/rubocop/cop/background_migration/avoid_silent_rescue_exceptions_spec.rb
new file mode 100644
index 00000000000..ea4f7d5fca8
--- /dev/null
+++ b/spec/rubocop/cop/background_migration/avoid_silent_rescue_exceptions_spec.rb
@@ -0,0 +1,107 @@
+# frozen_string_literal: true
+
+require 'rubocop_spec_helper'
+require_relative '../../../../rubocop/cop/background_migration/avoid_silent_rescue_exceptions'
+
+RSpec.describe RuboCop::Cop::BackgroundMigration::AvoidSilentRescueExceptions, feature_category: :database do
+ shared_examples 'expecting offense when' do |node|
+ it 'throws offense when rescuing exceptions without re-raising them' do
+ %w[Gitlab::BackgroundMigration::BatchedMigrationJob BatchedMigrationJob].each do |base_class|
+ expect_offense(<<~RUBY)
+ module Gitlab
+ module BackgroundMigration
+ class MyJob < #{base_class}
+ #{node}
+ end
+ end
+ end
+ RUBY
+ end
+ end
+ end
+
+ shared_examples 'not expecting offense when' do |node|
+ it 'does not throw any offense if exception is re-raised' do
+ %w[Gitlab::BackgroundMigration::BatchedMigrationJob BatchedMigrationJob].each do |base_class|
+ expect_no_offenses(<<~RUBY)
+ module Gitlab
+ module BackgroundMigration
+ class MyJob < #{base_class}
+ #{node}
+ end
+ end
+ end
+ RUBY
+ end
+ end
+ end
+
+ context "when the migration class doesn't inherits from BatchedMigrationJob" do
+ it 'does not throw any offense' do
+ expect_no_offenses(<<~RUBY)
+ module Gitlab
+ module BackgroundMigration
+ class MyClass < ::Gitlab::BackgroundMigration::Logger
+ def my_method
+ execute
+ rescue StandardError => error
+ puts error.message
+ end
+ end
+ end
+ end
+ RUBY
+ end
+ end
+
+ context 'when the migration class inherits from BatchedMigrationJob' do
+ context 'when specifying an error class' do
+ it_behaves_like 'expecting offense when', <<~RUBY
+ def perform
+ connection.execute('SELECT 1;')
+ rescue JSON::ParserError
+ ^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+ logger.error(message: error.message, class: self.class.name)
+ end
+ RUBY
+
+ it_behaves_like 'expecting offense when', <<~RUBY
+ def perform
+ connection.execute('SELECT 1;')
+ rescue StandardError, ActiveRecord::StatementTimeout => error
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+ logger.error(message: error.message, class: self.class.name)
+ end
+ RUBY
+
+ it_behaves_like 'not expecting offense when', <<~RUBY
+ def perform
+ connection.execute('SELECT 1;')
+ rescue StandardError, ActiveRecord::StatementTimeout => error
+ logger.error(message: error.message, class: self.class.name)
+ raise error
+ end
+ RUBY
+ end
+
+ context 'without specifying an error class' do
+ it_behaves_like 'expecting offense when', <<~RUBY
+ def perform
+ connection.execute('SELECT 1;')
+ rescue => error
+ ^^^^^^ #{described_class::MSG}
+ logger.error(message: error.message, class: self.class.name)
+ end
+ RUBY
+
+ it_behaves_like 'not expecting offense when', <<~RUBY
+ def perform
+ connection.execute('SELECT 1;')
+ rescue => error
+ logger.error(message: error.message, class: self.class.name)
+ raise error
+ end
+ RUBY
+ end
+ end
+end
generated by cgit v1.2.3 (git 2.25.1) at 2024-08-06 23:38:38 +0300