Skip to content
代码片段 群组 项目
提交 4e7f0890 编辑于 作者: Drew Blessing's avatar Drew Blessing 提交者: Lin Jen-Shin
浏览文件

Danger comment when using ActiveRecord bulk update methods

上级 6e448772
No related branches found
No related tags found
无相关合并请求
# frozen_string_literal: true
bulk_database_actions.add_comment_for_bulk_database_action_method_usage
# frozen_string_literal: true
require_relative '../../tooling/danger/bulk_database_actions'
module Danger
class BulkDatabaseActions < ::Danger::Plugin
include Tooling::Danger::BulkDatabaseActions
end
end
......@@ -29,6 +29,8 @@ A database review is required for:
These metrics could have complex queries over large tables.
See the [Analytics Instrumentation Guide](https://about.gitlab.com/handbook/product/analytics-instrumentation-guide/)
for implementation details.
- Changes that use [`update`, `delete`, `update_all`, `delete_all` or `destroy_all`](#preparation-when-using-update-delete-update_all-delete_all-or-destroy_all)
methods on an ActiveRecord object.
A database reviewer is expected to look out for overly complex
queries in the change and review those closer. If the author does not
......@@ -216,6 +218,15 @@ Include in the MR description:
- If you're adding a composite index, another index might become redundant, so remove that in the same migration.
For example adding `index(column_A, column_B, column_C)` makes the indexes `index(column_A, column_B)` and `index(column_A)` redundant.
#### Preparation when using `update`, `delete`, `update_all`, `delete_all` or `destroy_all`
Using these ActiveRecord methods requires extra care because they modify data and can perform poorly, or they
can destroy data if improperly scoped. These methods are also incompatible with Common Table Expression (CTE)
statements. Danger will comment on a Merge Request Diff when these methods are used.
Follow documentation for [preparation when adding or modifying queries](#preparation-when-adding-or-modifying-queries)
to add the raw SQL query and query plan to the Merge Request description, and request a database review.
### How to review for database
- Check migrations
......
# frozen_string_literal: true
require 'gitlab-dangerfiles'
require 'danger'
require 'danger/plugins/internal/helper'
require 'gitlab/dangerfiles/spec_helper'
require 'rspec-parameterized'
require_relative '../../../tooling/danger/bulk_database_actions'
require_relative '../../../tooling/danger/project_helper'
RSpec.describe Tooling::Danger::BulkDatabaseActions, feature_category: :tooling do
include_context "with dangerfile"
let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
let(:mr_url) { 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1' }
let(:doc_link) { described_class::DOCUMENTATION_LINK }
let(:comment_text) { "\n#{described_class::COMMENT_TEXT}" }
let(:file_lines) do
file_diff.map { |line| line.delete_prefix('+') }
end
let(:file_diff) do
[
"+ def execute",
"+ pat_family.active.#{method_call}",
"+",
"+ ServiceResponse.success",
"+ end"
]
end
before do
allow(bulk_database_actions).to receive(:project_helper).and_return(fake_project_helper)
allow(bulk_database_actions.project_helper).to receive(:file_lines).and_return(file_lines)
allow(bulk_database_actions.helper).to receive(:added_files).and_return([filename])
allow(bulk_database_actions.helper).to receive(:changed_lines).with(filename).and_return(file_diff)
allow(bulk_database_actions.helper).to receive(:mr_web_url).and_return(mr_url)
end
subject(:bulk_database_actions) { fake_danger.new(helper: fake_helper) }
describe '#add_comment_for_bulk_database_action_method_usage' do
context 'when file is a non-spec Ruby file' do
let(:filename) { 'app/services/personal_access_tokens/revoke_token_family_service.rb' }
using RSpec::Parameterized::TableSyntax
where(:method_call, :expect_comment?) do
'update_all(revoked: true)' | true
'destroy_all' | true
'delete_all' | true
'update(revoked: true)' | true
'delete' | true
'update_two_factor' | false
'delete_keys(key)' | false
'destroy_hook(hook)' | false
'destroy_all_merged' | false
'update_all_mirrors' | false
end
with_them do
it "correctly handles potential bulk database action" do
if expect_comment?
expect(bulk_database_actions).to receive(:markdown).with(comment_text, file: filename, line: 2)
else
expect(bulk_database_actions).not_to receive(:markdown)
end
bulk_database_actions.add_comment_for_bulk_database_action_method_usage
end
end
end
context 'for spec directories' do
let(:method_call) { 'update_all(revoked: true)' }
shared_examples 'no Danger comment' do
it 'does not comment on the bulk update action usage' do
expect(bulk_database_actions).not_to receive(:markdown)
bulk_database_actions.add_comment_for_bulk_database_action_method_usage
end
end
context 'for FOSS spec file' do
let(:filename) { 'spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
it_behaves_like 'no Danger comment'
end
context 'for EE spec file' do
let(:filename) { 'ee/spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
it_behaves_like 'no Danger comment'
end
context 'for JiHu spec file' do
let(:filename) { 'jh/spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
it_behaves_like 'no Danger comment'
end
end
end
end
# frozen_string_literal: true
require_relative 'suggestor'
module Tooling
module Danger
module BulkDatabaseActions
include ::Tooling::Danger::Suggestor
BULK_UPDATE_METHODS_REGEX = /\W(((update|delete|destroy)_all)|delete|update)(\(|\s+|$)/
DOCUMENTATION_LINK = 'https://docs.gitlab.com/ee/development/database_review.html#preparation-when-using-update-delete-update_all-and-destroy_all'
COMMENT_TEXT =
"When using `update`, `delete`, `update_all`, `delete_all` or `destroy_all` you must include the full " \
"database query and query execution plan in the merge request description, and request a ~database review. " \
"This comment can be ignored if the object is not an ActiveRecord class, since no database query " \
"would be generated. For more information, see [Database Review documentation](#{DOCUMENTATION_LINK}).".freeze
def add_comment_for_bulk_database_action_method_usage
changed_ruby_files.each do |filename|
add_suggestion(
filename: filename,
regex: BULK_UPDATE_METHODS_REGEX,
comment_text: COMMENT_TEXT
)
end
end
def changed_ruby_files
helper.added_files.select { |f| f.end_with?('.rb') && !f.start_with?('spec/', 'ee/spec/', 'jh/spec/') }
end
end
end
end
0% 加载中 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册