diff options
-rw-r--r-- | app/assets/javascripts/helpers/avatar_helper.js | 6 | ||||
-rw-r--r-- | app/controllers/boards/issues_controller.rb | 3 | ||||
-rw-r--r-- | app/models/concerns/bulk_insert_safe.rb | 44 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 16 | ||||
-rw-r--r-- | app/models/issue.rb | 6 | ||||
-rw-r--r-- | app/models/label_link.rb | 1 | ||||
-rw-r--r-- | app/models/merge_request_diff_commit.rb | 1 | ||||
-rw-r--r-- | app/models/merge_request_diff_file.rb | 1 | ||||
-rw-r--r-- | app/services/boards/issues/list_service.rb | 22 | ||||
-rw-r--r-- | changelogs/unreleased/loadash_helpers.yml | 5 | ||||
-rw-r--r-- | doc/development/feature_flags/controls.md | 11 | ||||
-rw-r--r-- | locale/gitlab.pot | 6 | ||||
-rw-r--r-- | qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/concerns/bulk_insert_safe_spec.rb | 38 | ||||
-rw-r--r-- | spec/models/label_link_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/merge_request_diff_commit_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/merge_request_diff_file_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb | 33 |
18 files changed, 189 insertions, 12 deletions
diff --git a/app/assets/javascripts/helpers/avatar_helper.js b/app/assets/javascripts/helpers/avatar_helper.js index 35ac7b2629c..7891b44dd27 100644 --- a/app/assets/javascripts/helpers/avatar_helper.js +++ b/app/assets/javascripts/helpers/avatar_helper.js @@ -1,4 +1,4 @@ -import _ from 'underscore'; +import { escape } from 'lodash'; import { getFirstCharacterCapitalized } from '~/lib/utils/text_utility'; export const DEFAULT_SIZE_CLASS = 's40'; @@ -19,7 +19,7 @@ export function renderIdenticon(entity, options = {}) { const bgClass = getIdenticonBackgroundClass(entity.id); const title = getIdenticonTitle(entity.name); - return `<div class="avatar identicon ${_.escape(sizeClass)} ${_.escape(bgClass)}">${_.escape( + return `<div class="avatar identicon ${escape(sizeClass)} ${escape(bgClass)}">${escape( title, )}</div>`; } @@ -31,5 +31,5 @@ export function renderAvatar(entity, options = {}) { const { sizeClass = DEFAULT_SIZE_CLASS } = options; - return `<img src="${_.escape(entity.avatar_url)}" class="avatar ${_.escape(sizeClass)}" />`; + return `<img src="${escape(entity.avatar_url)}" class="avatar ${escape(sizeClass)}" />`; } diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 1d6711e3c22..ed2c39e3fd3 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -20,6 +20,9 @@ module Boards skip_before_action :authenticate_user!, only: [:index] before_action :validate_id_list, only: [:bulk_move] before_action :can_move_issues?, only: [:bulk_move] + before_action do + push_frontend_feature_flag(:board_search_optimization, board.group) + end # rubocop: disable CodeReuse/ActiveRecord def index diff --git a/app/models/concerns/bulk_insert_safe.rb b/app/models/concerns/bulk_insert_safe.rb new file mode 100644 index 00000000000..6d75906b21f --- /dev/null +++ b/app/models/concerns/bulk_insert_safe.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module BulkInsertSafe + extend ActiveSupport::Concern + + # These are the callbacks we think safe when used on models that are + # written to the database in bulk + CALLBACK_NAME_WHITELIST = Set[ + :initialize, + :validate, + :validation, + :find, + :destroy + ].freeze + + MethodNotAllowedError = Class.new(StandardError) + + class_methods do + def set_callback(name, *args) + unless _bulk_insert_callback_allowed?(name, args) + raise MethodNotAllowedError.new( + "Not allowed to call `set_callback(#{name}, #{args})` when model extends `BulkInsertSafe`." \ + "Callbacks that fire per each record being inserted do not work with bulk-inserts.") + end + + super + end + + private + + def _bulk_insert_callback_allowed?(name, args) + _bulk_insert_whitelisted?(name) || _bulk_insert_saved_from_belongs_to?(name, args) + end + + # belongs_to associations will install a before_save hook during class loading + def _bulk_insert_saved_from_belongs_to?(name, args) + args.first == :before && args.second.to_s.start_with?('autosave_associated_records_for_') + end + + def _bulk_insert_whitelisted?(name) + CALLBACK_NAME_WHITELIST.include?(name) + end + end +end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index a6961df1b51..f3e03ed22d7 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -249,7 +249,7 @@ module Issuable Gitlab::Database.nulls_last_order('highest_priority', direction)) end - def order_labels_priority(direction = 'ASC', excluded_labels: [], extra_select_columns: []) + def order_labels_priority(direction = 'ASC', excluded_labels: [], extra_select_columns: [], with_cte: false) params = { target_type: name, target_column: "#{table_name}.id", @@ -265,7 +265,7 @@ module Issuable ] + extra_select_columns select(select_columns.join(', ')) - .group(arel_table[:id]) + .group(issue_grouping_columns(use_cte: with_cte)) .reorder(Gitlab::Database.nulls_last_order('highest_priority', direction)) end @@ -294,6 +294,18 @@ module Issuable grouping_columns end + # Includes all table keys in group by clause when sorting + # preventing errors in postgres when using CTE search optimisation + # + # Returns an array of arel columns + def issue_grouping_columns(use_cte: false) + if use_cte + [arel_table[:state]] + attribute_names.map { |attr| arel_table[attr.to_sym] } + else + arel_table[:id] + end + end + def to_ability_name model_name.singular end diff --git a/app/models/issue.rb b/app/models/issue.rb index fd4a8c90386..684108b1a7a 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -172,8 +172,10 @@ class Issue < ApplicationRecord end end - def self.order_by_position_and_priority - order_labels_priority + # `with_cte` argument allows sorting when using CTE queries and prevents + # errors in postgres when using CTE search optimisation + def self.order_by_position_and_priority(with_cte: false) + order_labels_priority(with_cte: with_cte) .reorder(Gitlab::Database.nulls_last_order('relative_position', 'ASC'), Gitlab::Database.nulls_last_order('highest_priority', 'ASC'), "id DESC") diff --git a/app/models/label_link.rb b/app/models/label_link.rb index ffc0afd8e85..5ae1e88e14e 100644 --- a/app/models/label_link.rb +++ b/app/models/label_link.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class LabelLink < ApplicationRecord + include BulkInsertSafe include Importable belongs_to :target, polymorphic: true, inverse_of: :label_links # rubocop:disable Cop/PolymorphicAssociations diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb index aa41a68f184..460b394f067 100644 --- a/app/models/merge_request_diff_commit.rb +++ b/app/models/merge_request_diff_commit.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequestDiffCommit < ApplicationRecord + include BulkInsertSafe include ShaAttribute include CachedCommit diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb index 14c86ec69da..23319445a38 100644 --- a/app/models/merge_request_diff_file.rb +++ b/app/models/merge_request_diff_file.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequestDiffFile < ApplicationRecord + include BulkInsertSafe include Gitlab::EncodingHelper include DiffFile diff --git a/app/services/boards/issues/list_service.rb b/app/services/boards/issues/list_service.rb index a9240e1d8a0..699fa17cb65 100644 --- a/app/services/boards/issues/list_service.rb +++ b/app/services/boards/issues/list_service.rb @@ -10,7 +10,7 @@ module Boards end def execute - fetch_issues.order_by_position_and_priority + fetch_issues.order_by_position_and_priority(with_cte: can_attempt_search_optimization?) end # rubocop: disable CodeReuse/ActiveRecord @@ -63,6 +63,7 @@ module Boards set_state set_scope set_non_archived + set_attempt_search_optimizations params end @@ -87,6 +88,16 @@ module Boards params[:non_archived] = parent.is_a?(Group) end + def set_attempt_search_optimizations + return unless can_attempt_search_optimization? + + if board.group_board? + params[:attempt_group_search_optimizations] = true + else + params[:attempt_project_search_optimizations] = true + end + end + # rubocop: disable CodeReuse/ActiveRecord def board_label_ids @board_label_ids ||= board.lists.movable.pluck(:label_id) @@ -113,6 +124,15 @@ module Boards .where("label_links.label_id = ?", list.label_id).limit(1)) end # rubocop: enable CodeReuse/ActiveRecord + + def board_group + board.group_board? ? parent : parent.group + end + + def can_attempt_search_optimization? + params[:search].present? && + Feature.enabled?(:board_search_optimization, board_group, default_enabled: false) + end end end end diff --git a/changelogs/unreleased/loadash_helpers.yml b/changelogs/unreleased/loadash_helpers.yml new file mode 100644 index 00000000000..0296ba6665f --- /dev/null +++ b/changelogs/unreleased/loadash_helpers.yml @@ -0,0 +1,5 @@ +--- +title: Replace underscore with lodash in /app/assets/javascripts/helpers +merge_request: 25014 +author: rkpattnaik780 +type: changed diff --git a/doc/development/feature_flags/controls.md b/doc/development/feature_flags/controls.md index d711e49ee8b..922995cb915 100644 --- a/doc/development/feature_flags/controls.md +++ b/doc/development/feature_flags/controls.md @@ -51,7 +51,7 @@ easier to measure the impact of both separately. GitLab's feature library (using [Flipper](https://github.com/jnunemaker/flipper), and covered in the [Feature Flags process](process.md) guide) supports rolling out changes to a percentage of -users. This in turn can be controlled using [GitLab Chatops](../../ci/chatops/README.md). +time to users. This in turn can be controlled using [GitLab Chatops](../../ci/chatops/README.md). For an up to date list of feature flag commands please see [the source code](https://gitlab.com/gitlab-com/chatops/blob/master/lib/chatops/commands/feature.rb). @@ -136,8 +136,13 @@ you run these 2 commands: /chatops run feature set some_feature 25 ``` -Then `some_feature` will be enabled for 25% of the users interacting with -`gitlab-org/gitlab`, and no one else. +Then `some_feature` will be enabled for 25% of the time the users are interacting with +`gitlab-org/gitlab`. Note that the the feature is not enabled to 25% +of the users, rather a simple randomization is made each time the `enabled?` is checked. + +NOTE: **Note:** +**Percentage of time** rollout is not a good idea if what you want is to make sure a feature +is always on or off to the users. ## Cleaning up diff --git a/locale/gitlab.pot b/locale/gitlab.pot index eb949de4f3f..9edcf28e93e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7773,6 +7773,9 @@ msgstr "" msgid "Events" msgstr "" +msgid "Events in %{project_path}" +msgstr "" + msgid "Every %{action} attempt has failed: %{job_error_message}. Please try again." msgstr "" @@ -14593,6 +14596,9 @@ msgstr "" msgid "Project '%{project_name}' will be deleted on %{date}" msgstr "" +msgid "Project Audit Events" +msgstr "" + msgid "Project Badges" msgstr "" diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb index 5baa0e90c47..4a9901f2a84 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb @@ -14,7 +14,7 @@ module QA @merge_request_description = '... to find them, to bring them all, and in the darkness bind them' end - it 'creates a basic merge request', :smoke do + it 'creates a basic merge request' do Resource::MergeRequest.fabricate_via_browser_ui! do |merge_request| merge_request.project = @project merge_request.title = @merge_request_title diff --git a/spec/models/concerns/bulk_insert_safe_spec.rb b/spec/models/concerns/bulk_insert_safe_spec.rb new file mode 100644 index 00000000000..91884680738 --- /dev/null +++ b/spec/models/concerns/bulk_insert_safe_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BulkInsertSafe do + class BulkInsertItem < ApplicationRecord + include BulkInsertSafe + end + + module InheritedUnsafeMethods + extend ActiveSupport::Concern + + included do + after_save -> { "unsafe" } + end + end + + module InheritedSafeMethods + extend ActiveSupport::Concern + + included do + after_initialize -> { "safe" } + end + end + + it_behaves_like 'a BulkInsertSafe model', BulkInsertItem + + context 'when inheriting class methods' do + it 'raises an error when method is not bulk-insert safe' do + expect { BulkInsertItem.include(InheritedUnsafeMethods) }.to( + raise_error(subject::MethodNotAllowedError)) + end + + it 'does not raise an error when method is bulk-insert safe' do + expect { BulkInsertItem.include(InheritedSafeMethods) }.not_to raise_error + end + end +end diff --git a/spec/models/label_link_spec.rb b/spec/models/label_link_spec.rb index b160e72e759..0a5cb5374b0 100644 --- a/spec/models/label_link_spec.rb +++ b/spec/models/label_link_spec.rb @@ -7,4 +7,6 @@ describe LabelLink do it { is_expected.to belong_to(:label) } it { is_expected.to belong_to(:target) } + + it_behaves_like 'a BulkInsertSafe model', LabelLink end diff --git a/spec/models/merge_request_diff_commit_spec.rb b/spec/models/merge_request_diff_commit_spec.rb index a74c0b20642..a296122ae09 100644 --- a/spec/models/merge_request_diff_commit_spec.rb +++ b/spec/models/merge_request_diff_commit_spec.rb @@ -6,6 +6,8 @@ describe MergeRequestDiffCommit do let(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } + it_behaves_like 'a BulkInsertSafe model', MergeRequestDiffCommit + describe '#to_hash' do subject { merge_request.commits.first } diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb index 84f9c9d06ba..6ecbc5bf832 100644 --- a/spec/models/merge_request_diff_file_spec.rb +++ b/spec/models/merge_request_diff_file_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe MergeRequestDiffFile do + it_behaves_like 'a BulkInsertSafe model', MergeRequestDiffFile + describe '#diff' do context 'when diff is not stored' do let(:unpacked) { 'unpacked' } diff --git a/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb b/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb new file mode 100644 index 00000000000..0a2b6616bb4 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'a BulkInsertSafe model' do |target_class| + # We consider all callbacks unsafe for bulk insertions unless we have explicitly + # whitelisted them (esp. anything related to :save, :create, :commit etc.) + let(:callback_method_blacklist) do + ActiveRecord::Callbacks::CALLBACKS.reject do |callback| + cb_name = callback.to_s.gsub(/(before_|after_|around_)/, '').to_sym + BulkInsertSafe::CALLBACK_NAME_WHITELIST.include?(cb_name) + end.to_set + end + + context 'when calling class methods directly' do + it 'raises an error when method is not bulk-insert safe' do + callback_method_blacklist.each do |m| + expect { target_class.send(m, nil) }.to( + raise_error(BulkInsertSafe::MethodNotAllowedError), + "Expected call to #{m} to raise an error, but it didn't" + ) + end + end + + it 'does not raise an error when method is bulk-insert safe' do + BulkInsertSafe::CALLBACK_NAME_WHITELIST.each do |name| + expect { target_class.set_callback(name) {} }.not_to raise_error + end + end + + it 'does not raise an error when the call is triggered by belongs_to' do + expect { target_class.belongs_to(:other_record) }.not_to raise_error + end + end +end |