diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb index 96120d9412f0d4ce7e82d0d4950b57cc2f32610b..64903c67573964e35d3eb1cee7435b38101241f6 100644 --- a/app/finders/user_recent_events_finder.rb +++ b/app/finders/user_recent_events_finder.rb @@ -73,10 +73,20 @@ def execute_multi return Event.none if users.empty? - if event_filter.filter == EventFilter::ALL - execute_optimized_multi(users) + if Feature.enabled?(:optimized_followed_users_queries, current_user) + query_builder_params = event_filter.in_operator_query_builder_params(users) + + Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder + .new(**query_builder_params) + .execute + .limit(limit) + .offset(params[:offset] || 0) else - event_filter.apply_filter(Event.where(author: users).limit_recent(limit, params[:offset] || 0)) + if event_filter.filter == EventFilter::ALL + execute_optimized_multi(users) + else + event_filter.apply_filter(Event.where(author: users).limit_recent(limit, params[:offset] || 0)) + end end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/models/event.rb b/app/models/event.rb index 38740dc89d6bf78c7996b0f021157610d6e6ef61..e9a98c06b59685381b18c8993012e01f691dbb7d 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -31,8 +31,9 @@ class Event < ApplicationRecord private_constant :ACTIONS WIKI_ACTIONS = [:created, :updated, :destroyed].freeze - DESIGN_ACTIONS = [:created, :updated, :destroyed].freeze + TEAM_ACTIONS = [:joined, :left, :expired].freeze + ISSUE_ACTIONS = [:created, :updated, :closed, :reopened].freeze TARGET_TYPES = HashWithIndifferentAccess.new( issue: Issue, diff --git a/config/feature_flags/development/optimized_followed_users_queries.yml b/config/feature_flags/development/optimized_followed_users_queries.yml new file mode 100644 index 0000000000000000000000000000000000000000..514c3c91829a76a59df5abbf5feb02915a571272 --- /dev/null +++ b/config/feature_flags/development/optimized_followed_users_queries.yml @@ -0,0 +1,8 @@ +--- +name: optimized_followed_users_queries +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84856 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/358649 +milestone: '14.10' +type: development +group: group::optimize +default_enabled: false diff --git a/db/post_migrate/20220409160628_add_async_index_for_events_followed_users.rb b/db/post_migrate/20220409160628_add_async_index_for_events_followed_users.rb new file mode 100644 index 0000000000000000000000000000000000000000..fb858248b1917415b90df96e1b0ddd1be588f600 --- /dev/null +++ b/db/post_migrate/20220409160628_add_async_index_for_events_followed_users.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddAsyncIndexForEventsFollowedUsers < Gitlab::Database::Migration[1.0] + INDEX_NAME = 'index_events_for_followed_users' + + def up + prepare_async_index :events, %I[author_id target_type action id], name: INDEX_NAME + end + + def down + unprepare_async_index :events, %I[author_id target_type action id], name: INDEX_NAME + end +end diff --git a/db/schema_migrations/20220409160628 b/db/schema_migrations/20220409160628 new file mode 100644 index 0000000000000000000000000000000000000000..29b46427dd24fb87a4c5c32ebbc84e9bbe4adc9e --- /dev/null +++ b/db/schema_migrations/20220409160628 @@ -0,0 +1 @@ +7952024a6a8df98842fa23ca9a4c328b83816ded3071e7597dbab431a5561e1a \ No newline at end of file diff --git a/ee/app/models/ee/event.rb b/ee/app/models/ee/event.rb index 3cc2a722c26a27fdecf4a6ad6cf04f6cf7ed9c28..31042be99d7e8dace04d61b660caa0311b004b5c 100644 --- a/ee/app/models/ee/event.rb +++ b/ee/app/models/ee/event.rb @@ -13,6 +13,8 @@ module Event scope :epics, -> { where(target_type: 'Epic') } end + EPIC_ACTIONS = [:created, :closed, :reopened].freeze + override :capabilities def capabilities super.merge(read_epic: %i[epic? epic_note?]) diff --git a/ee/lib/ee/event_filter.rb b/ee/lib/ee/event_filter.rb index 7ade068997b504fc73dd3d1a821ca18d4fd04ca3..ca7a3a5586315ff366ef7df65789d3c15c959148 100644 --- a/ee/lib/ee/event_filter.rb +++ b/ee/lib/ee/event_filter.rb @@ -16,6 +16,20 @@ def apply_filter(events) end end + def in_operator_query_builder_params(user_ids) + case filter + when EPIC + in_operator_params( + array_scope_ids: user_ids, + scope: ::Event.epics, + in_column: :action, + in_values: ::Event.actions.values_at(*::Event::EPIC_ACTIONS) + ) + else + super + end + end + private override :filters diff --git a/ee/spec/finders/ee/user_recent_events_finder_spec.rb b/ee/spec/finders/ee/user_recent_events_finder_spec.rb index d46f6d4f5227ce92e9cb3878c5959fa7e858af61..32bb663cf588e61e194e25a5435000b0ef8ff037 100644 --- a/ee/spec/finders/ee/user_recent_events_finder_spec.rb +++ b/ee/spec/finders/ee/user_recent_events_finder_spec.rb @@ -5,28 +5,69 @@ RSpec.describe UserRecentEventsFinder do describe '#execute' do let_it_be(:current_user) { create(:user) } - let_it_be(:user) { create(:user) } + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } let_it_be(:epic) { create(:epic) } + let_it_be(:issue) { create(:issue) } let_it_be(:private_epic) { create(:epic, group: create(:group, :private)) } let_it_be(:note) { create(:note_on_epic, noteable: epic) } - let_it_be(:public_event) { create(:event, :commented, target: note, author: user, project: nil) } - let_it_be(:private_event) { create(:event, :closed, target: private_epic, author: user, project: nil) } + let_it_be(:public_event) { create(:event, :commented, target: note, author: user1, project: nil) } + let_it_be(:private_epic_event1) { create(:event, :closed, target: private_epic, author: user1, project: nil) } - subject { described_class.new(current_user, user, nil, {}).execute } + let(:event_filter) { nil } + let(:target_users) { user1 } - context 'epic related activities' do - context 'when profile is public' do - it { is_expected.to match_array([public_event, private_event]) } - end + subject { described_class.new(current_user, target_users, event_filter, {}).execute } + + shared_examples 'UserRecentEventsFinder examples' do + context 'epic related activities' do + context 'when profile is public' do + it { is_expected.to match_array([public_event, private_epic_event1]) } + end + + context 'when profile is private' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, user1).and_return(false) + end + + it { is_expected.to be_empty } + end + + context 'wehen fetching events from multiple users' do + let_it_be(:private_issue_event) { create(:event, :created, target: issue, author: user1, project: nil) } + let_it_be(:private_epic_event2) do + create(:event, :created, target: private_epic, author: user2, project: nil) + end + + let_it_be(:private_epic_event3) do + create(:event, :reopened, target: private_epic, author: user2, project: nil) + end - context 'when profile is private' do - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, user).and_return(false) + let(:event_filter) { EventFilter.new(EventFilter::EPIC) } + let(:target_users) { [user1, user2] } + + context 'when filtering for epic events' do + it { is_expected.to eq([private_epic_event3, private_epic_event2, private_epic_event1]) } + end end + end + end + + context 'when the optimized_followed_users_queries FF is on' do + before do + stub_feature_flags(optimized_followed_users_queries: true) + end + + it_behaves_like 'UserRecentEventsFinder examples' + end - it { is_expected.to be_empty } + context 'when the optimized_followed_users_queries FF is off' do + before do + stub_feature_flags(optimized_followed_users_queries: false) end + + it_behaves_like 'UserRecentEventsFinder examples' end end end diff --git a/lib/event_filter.rb b/lib/event_filter.rb index 915ab3555081bc2d2dcef9cdfefcde8dab0ac45a..8833207dd1d2057336a04ff34e8661b0e7d24ab7 100644 --- a/lib/event_filter.rb +++ b/lib/event_filter.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +# rubocop: disable CodeReuse/ActiveRecord class EventFilter include Gitlab::Utils::StrongMemoize @@ -24,7 +25,6 @@ def active?(key) filter == key.to_s end - # rubocop: disable CodeReuse/ActiveRecord def apply_filter(events) case filter when PUSH @@ -34,9 +34,9 @@ def apply_filter(events) when COMMENTS events.commented_action when TEAM - events.where(action: [:joined, :left, :expired]) + events.where(action: Event::TEAM_ACTIONS) when ISSUE - events.where(action: [:created, :updated, :closed, :reopened], target_type: 'Issue') + events.where(action: Event::ISSUE_ACTIONS, target_type: 'Issue') when WIKI wiki_events(events) when DESIGNS @@ -45,10 +45,157 @@ def apply_filter(events) events end end - # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable Metrics/CyclomaticComplexity + # This method build specialized in-operator optimized queries based on different + # filter parameters. All queries will benefit from the index covering the following columns: + # author_id target_type action id + # + # More context: https://docs.gitlab.com/ee/development/database/efficient_in_operator_queries.html#the-inoperatoroptimization-module + def in_operator_query_builder_params(user_ids) + case filter + when ALL + in_operator_params(array_scope_ids: user_ids) + when PUSH + # Here we need to add an order hint column to force the correct index usage. + # Without the order hint, the following conditions will use the `index_events_on_author_id_and_id` + # index which is not as efficient as the `index_events_for_followed_users` index. + # > target_type IS NULL AND action = 5 AND author_id = X ORDER BY id DESC + # + # The order hint adds an extra order by column which doesn't affect the result but forces the planner + # to use the correct index: + # > target_type IS NULL AND action = 5 AND author_id = X ORDER BY target_type DESC, id DESC + in_operator_params( + array_scope_ids: user_ids, + scope: Event.where(target_type: nil).pushed_action, + order_hint_column: :target_type + ) + when MERGED + in_operator_params( + array_scope_ids: user_ids, + scope: Event.where(target_type: MergeRequest.to_s).merged_action + ) + when COMMENTS + in_operator_params( + array_scope_ids: user_ids, + scope: Event.commented_action, + in_column: :target_type, + in_values: [Note, *Note.descendants].map(&:name) # To make the query efficient we need to list all Note classes + ) + when TEAM + in_operator_params( + array_scope_ids: user_ids, + scope: Event.where(target_type: nil), + order_hint_column: :target_type, + in_column: :action, + in_values: Event.actions.values_at(*Event::TEAM_ACTIONS) + ) + when ISSUE + in_operator_params( + array_scope_ids: user_ids, + scope: Event.where(target_type: Issue.name), + in_column: :action, + in_values: Event.actions.values_at(*Event::ISSUE_ACTIONS) + ) + when WIKI + in_operator_params( + array_scope_ids: user_ids, + scope: Event.for_wiki_page, + in_column: :action, + in_values: Event.actions.values_at(*Event::WIKI_ACTIONS) + ) + when DESIGNS + in_operator_params( + array_scope_ids: user_ids, + scope: Event.for_design, + in_column: :action, + in_values: Event.actions.values_at(*Event::DESIGN_ACTIONS) + ) + else + in_operator_params(array_scope_ids: user_ids) + end + end + # rubocop: enable Metrics/CyclomaticComplexity private + def in_operator_params(array_scope_ids:, scope: nil, in_column: nil, in_values: nil, order_hint_column: nil) + base_scope = Event.all + base_scope = base_scope.merge(scope) if scope + + order = { id: :desc } + finder_query = -> (id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) } + + if order_hint_column.present? + order = Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: order_hint_column, + order_expression: Event.arel_table[order_hint_column].desc, + nullable: :nulls_last, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: :id, + order_expression: Event.arel_table[:id].desc + ) + ]) + + finder_query = -> (_order_hint, id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) } + end + + base_scope = base_scope.reorder(order) + + array_params = in_operator_array_params( + array_scope_ids: array_scope_ids, + scope: base_scope, + in_column: in_column, + in_values: in_values + ) + + array_params.merge( + scope: base_scope, + finder_query: finder_query + ) + end + + # This method builds the array_ parameters + # without in_column parameter: uses one IN filter: author_id + # with in_column: two IN filters: author_id, (target_type OR action) + def in_operator_array_params(scope:, array_scope_ids:, in_column: nil, in_values: nil) + if in_column + # Builds Carthesian product of the in_values and the array_scope_ids (in this case: user_ids). + # The process is described here: https://docs.gitlab.com/ee/development/database/efficient_in_operator_queries.html#multiple-in-queries + # VALUES ((array_scope_ids[0], in_values[0]), (array_scope_ids[1], in_values[0]) ...) + cartesian = array_scope_ids.product(in_values) + user_with_column_list = Arel::Nodes::ValuesList.new(cartesian) + + as = "array_ids(id, #{Event.connection.quote_column_name(in_column)})" + from = Arel::Nodes::Grouping.new(user_with_column_list).as(as) + { + array_scope: User.select(:id, in_column).from(from), + array_mapping_scope: -> (author_id_expression, in_column_expression) do + Event + .merge(scope) + .where(Event.arel_table[:author_id].eq(author_id_expression)) + .where(Event.arel_table[in_column].eq(in_column_expression)) + end + } + else + # Builds a simple query to represent the array_scope_ids + # VALUES ((array_scope_ids[0]), (array_scope_ids[2])...) + array_ids_list = Arel::Nodes::ValuesList.new(array_scope_ids.map { |id| [id] }) + from = Arel::Nodes::Grouping.new(array_ids_list).as('array_ids(id)') + { + array_scope: User.select(:id).from(from), + array_mapping_scope: -> (author_id_expression) do + Event + .merge(scope) + .where(Event.arel_table[:author_id].eq(author_id_expression)) + end + } + end + end + def wiki_events(events) events.for_wiki_page end @@ -61,5 +208,6 @@ def filters [ALL, PUSH, MERGED, ISSUE, COMMENTS, TEAM, WIKI, DESIGNS] end end +# rubocop: enable CodeReuse/ActiveRecord EventFilter.prepend_mod_with('EventFilter') diff --git a/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder.rb b/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder.rb index 065a3a0cf20ebd2762b9b4da5ad6cfda773338ec..8c0f082f61c57f19dac3392cf06995a09be4a656 100644 --- a/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder.rb +++ b/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder.rb @@ -120,7 +120,7 @@ def build_column_arrays_query .from(array_cte) .join(Arel.sql("LEFT JOIN LATERAL (#{initial_keyset_query.to_sql}) #{table_name} ON TRUE")) - order_by_columns.each { |column| q.where(column.column_expression.not_eq(nil)) } + order_by_columns.each { |c| q.where(c.column_expression.not_eq(nil)) unless c.column.nullable? } q.as('array_scope_lateral_query') end @@ -200,7 +200,7 @@ def array_order_query .project([*order_by_columns.original_column_names_as_arel_string, Arel.sql('position')]) .from("UNNEST(#{list(order_by_columns.array_aggregated_column_names)}) WITH ORDINALITY AS u(#{list(order_by_columns.original_column_names)}, position)") - order_by_columns.each { |column| q.where(Arel.sql(column.original_column_name).not_eq(nil)) } # ignore rows where all columns are NULL + order_by_columns.each { |c| q.where(Arel.sql(c.original_column_name).not_eq(nil)) unless c.column.nullable? } # ignore rows where all columns are NULL q.order(Arel.sql(order_by_without_table_references)).take(1) end diff --git a/spec/factories/events.rb b/spec/factories/events.rb index d182dc9f95fd6d8492b6922702fc841767dff824..403165a393510df00df25839bed8b33c050f70fa 100644 --- a/spec/factories/events.rb +++ b/spec/factories/events.rb @@ -59,6 +59,11 @@ target { design } end + factory :design_updated_event, traits: [:has_design] do + action { :updated } + target { design } + end + factory :project_created_event do project factory: :project action { :created } diff --git a/spec/finders/user_recent_events_finder_spec.rb b/spec/finders/user_recent_events_finder_spec.rb index 6019d22059de6fd2ee85f24e96dc7a0a4ae9b0da..d7f7bb9cebed009c5a8374b8a2974aa03790ea11 100644 --- a/spec/finders/user_recent_events_finder_spec.rb +++ b/spec/finders/user_recent_events_finder_spec.rb @@ -8,9 +8,9 @@ let_it_be(:private_project) { create(:project, :private, creator: project_owner) } let_it_be(:internal_project) { create(:project, :internal, creator: project_owner) } let_it_be(:public_project) { create(:project, :public, creator: project_owner) } - let!(:private_event) { create(:event, project: private_project, author: project_owner) } - let!(:internal_event) { create(:event, project: internal_project, author: project_owner) } - let!(:public_event) { create(:event, project: public_project, author: project_owner) } + let_it_be(:private_event) { create(:event, project: private_project, author: project_owner) } + let_it_be(:internal_event) { create(:event, project: internal_project, author: project_owner) } + let_it_be(:public_event) { create(:event, project: public_project, author: project_owner) } let_it_be(:issue) { create(:issue, project: public_project) } let(:limit) { nil } @@ -18,210 +18,266 @@ subject(:finder) { described_class.new(current_user, project_owner, nil, params) } - describe '#execute' do - context 'when profile is public' do - it 'returns all the events' do - expect(finder.execute).to include(private_event, internal_event, public_event) + shared_examples 'UserRecentEventsFinder examples' do + describe '#execute' do + context 'when profile is public' do + it 'returns all the events' do + expect(finder.execute).to include(private_event, internal_event, public_event) + end end - end - context 'when profile is private' do - it 'returns no event' do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, project_owner).and_return(false) + context 'when profile is private' do + it 'returns no event' do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, project_owner).and_return(false) - expect(finder.execute).to be_empty + expect(finder.execute).to be_empty + end end - end - it 'does not include the events if the user cannot read cross project' do - allow(Ability).to receive(:allowed?).and_call_original - expect(Ability).to receive(:allowed?).with(current_user, :read_cross_project) { false } + it 'does not include the events if the user cannot read cross project' do + allow(Ability).to receive(:allowed?).and_call_original + expect(Ability).to receive(:allowed?).with(current_user, :read_cross_project) { false } - expect(finder.execute).to be_empty - end + expect(finder.execute).to be_empty + end - context 'events from multiple users' do - let_it_be(:second_user, reload: true) { create(:user) } - let_it_be(:private_project_second_user) { create(:project, :private, creator: second_user) } + context 'events from multiple users' do + let_it_be(:second_user, reload: true) { create(:user) } + let_it_be(:private_project_second_user) { create(:project, :private, creator: second_user) } - let(:internal_project_second_user) { create(:project, :internal, creator: second_user) } - let(:public_project_second_user) { create(:project, :public, creator: second_user) } - let!(:private_event_second_user) { create(:event, project: private_project_second_user, author: second_user) } - let!(:internal_event_second_user) { create(:event, project: internal_project_second_user, author: second_user) } - let!(:public_event_second_user) { create(:event, project: public_project_second_user, author: second_user) } + let_it_be(:internal_project_second_user) { create(:project, :internal, creator: second_user) } + let_it_be(:public_project_second_user) { create(:project, :public, creator: second_user) } + let_it_be(:private_event_second_user) { create(:event, project: private_project_second_user, author: second_user) } + let_it_be(:internal_event_second_user) { create(:event, project: internal_project_second_user, author: second_user) } + let_it_be(:public_event_second_user) { create(:event, project: public_project_second_user, author: second_user) } - it 'includes events from all users', :aggregate_failures do - events = described_class.new(current_user, [project_owner, second_user], nil, params).execute + it 'includes events from all users', :aggregate_failures do + events = described_class.new(current_user, [project_owner, second_user], nil, params).execute - expect(events).to include(private_event, internal_event, public_event) - expect(events).to include(private_event_second_user, internal_event_second_user, public_event_second_user) - expect(events.size).to eq(6) - end + expect(events).to include(private_event, internal_event, public_event) + expect(events).to include(private_event_second_user, internal_event_second_user, public_event_second_user) + expect(events.size).to eq(6) + end - context 'selected events' do - let!(:push_event) { create(:push_event, project: public_project, author: project_owner) } - let!(:push_event_second_user) { create(:push_event, project: public_project_second_user, author: second_user) } + context 'selected events' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:push_event1) { create(:push_event, project: public_project, author: project_owner) } + let_it_be(:push_event2) { create(:push_event, project: public_project_second_user, author: second_user) } + let_it_be(:merge_event1) { create(:event, :merged, target_type: MergeRequest.to_s, project: public_project, author: project_owner) } + let_it_be(:merge_event2) { create(:event, :merged, target_type: MergeRequest.to_s, project: public_project_second_user, author: second_user) } + let_it_be(:comment_event1) { create(:event, :commented, target_type: Note.to_s, project: public_project, author: project_owner) } + let_it_be(:comment_event2) { create(:event, :commented, target_type: DiffNote.to_s, project: public_project, author: project_owner) } + let_it_be(:comment_event3) { create(:event, :commented, target_type: DiscussionNote.to_s, project: public_project_second_user, author: second_user) } + let_it_be(:issue_event1) { create(:event, :created, project: public_project, target: issue, author: project_owner) } + let_it_be(:issue_event2) { create(:event, :updated, project: public_project, target: issue, author: project_owner) } + let_it_be(:issue_event3) { create(:event, :closed, project: public_project_second_user, target: issue, author: second_user) } + let_it_be(:wiki_event1) { create(:wiki_page_event, project: public_project, author: project_owner) } + let_it_be(:wiki_event2) { create(:wiki_page_event, project: public_project_second_user, author: second_user) } + let_it_be(:design_event1) { create(:design_event, project: public_project, author: project_owner) } + let_it_be(:design_event2) { create(:design_updated_event, project: public_project_second_user, author: second_user) } + + where(:event_filter, :ordered_expected_events) do + EventFilter.new(EventFilter::PUSH) | lazy { [push_event1, push_event2] } + EventFilter.new(EventFilter::MERGED) | lazy { [merge_event1, merge_event2] } + EventFilter.new(EventFilter::COMMENTS) | lazy { [comment_event1, comment_event2, comment_event3] } + EventFilter.new(EventFilter::TEAM) | lazy { [private_event, internal_event, public_event, private_event_second_user, internal_event_second_user, public_event_second_user] } + EventFilter.new(EventFilter::ISSUE) | lazy { [issue_event1, issue_event2, issue_event3] } + EventFilter.new(EventFilter::WIKI) | lazy { [wiki_event1, wiki_event2] } + EventFilter.new(EventFilter::DESIGNS) | lazy { [design_event1, design_event2] } + end - it 'only includes selected events (PUSH) from all users', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::PUSH) - events = described_class.new(current_user, [project_owner, second_user], event_filter, params).execute + with_them do + it 'only returns selected events from all users (id DESC)' do + events = described_class.new(current_user, [project_owner, second_user], event_filter, params).execute - expect(events).to contain_exactly(push_event, push_event_second_user) + expect(events).to eq(ordered_expected_events.reverse) + end + end end - end - it 'does not include events from users with private profile', :aggregate_failures do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, second_user).and_return(false) + it 'does not include events from users with private profile', :aggregate_failures do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, second_user).and_return(false) - events = described_class.new(current_user, [project_owner, second_user], nil, params).execute + events = described_class.new(current_user, [project_owner, second_user], nil, params).execute - expect(events).to contain_exactly(private_event, internal_event, public_event) - end + expect(events).to contain_exactly(private_event, internal_event, public_event) + end - context 'with pagination params' do - using RSpec::Parameterized::TableSyntax + context 'with pagination params' do + using RSpec::Parameterized::TableSyntax - where(:limit, :offset, :ordered_expected_events) do - nil | nil | lazy { [public_event_second_user, internal_event_second_user, private_event_second_user, public_event, internal_event, private_event] } - 2 | nil | lazy { [public_event_second_user, internal_event_second_user] } - nil | 4 | lazy { [internal_event, private_event] } - 2 | 2 | lazy { [private_event_second_user, public_event] } - end + where(:limit, :offset, :ordered_expected_events) do + nil | nil | lazy { [public_event_second_user, internal_event_second_user, private_event_second_user, public_event, internal_event, private_event] } + 2 | nil | lazy { [public_event_second_user, internal_event_second_user] } + nil | 4 | lazy { [internal_event, private_event] } + 2 | 2 | lazy { [private_event_second_user, public_event] } + end - with_them do - let(:params) { { limit: limit, offset: offset }.compact } + with_them do + let(:params) { { limit: limit, offset: offset }.compact } - it 'returns paginated events sorted by id (DESC)' do - events = described_class.new(current_user, [project_owner, second_user], nil, params).execute + it 'returns paginated events sorted by id (DESC)' do + events = described_class.new(current_user, [project_owner, second_user], nil, params).execute - expect(events).to eq(ordered_expected_events) + expect(events).to eq(ordered_expected_events) + end end end end - end - context 'filter activity events' do - let!(:push_event) { create(:push_event, project: public_project, author: project_owner) } - let!(:merge_event) { create(:event, :merged, project: public_project, author: project_owner) } - let!(:issue_event) { create(:event, :closed, project: public_project, target: issue, author: project_owner) } - let!(:comment_event) { create(:event, :commented, project: public_project, author: project_owner) } - let!(:wiki_event) { create(:wiki_page_event, project: public_project, author: project_owner) } - let!(:design_event) { create(:design_event, project: public_project, author: project_owner) } - let!(:team_event) { create(:event, :joined, project: public_project, author: project_owner) } - - it 'includes all events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::ALL) - events = described_class.new(current_user, project_owner, event_filter, params).execute - - expect(events).to include(private_event, internal_event, public_event) - expect(events).to include(push_event, merge_event, issue_event, comment_event, wiki_event, design_event, team_event) - expect(events.size).to eq(10) - end + context 'filter activity events' do + let_it_be(:push_event) { create(:push_event, project: public_project, author: project_owner) } + let_it_be(:merge_event) { create(:event, :merged, project: public_project, author: project_owner) } + let_it_be(:issue_event) { create(:event, :closed, project: public_project, target: issue, author: project_owner) } + let_it_be(:comment_event) { create(:event, :commented, project: public_project, author: project_owner) } + let_it_be(:wiki_event) { create(:wiki_page_event, project: public_project, author: project_owner) } + let_it_be(:design_event) { create(:design_event, project: public_project, author: project_owner) } + let_it_be(:team_event) { create(:event, :joined, project: public_project, author: project_owner) } + + it 'includes all events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::ALL) + events = described_class.new(current_user, project_owner, event_filter, params).execute + + expect(events).to include(private_event, internal_event, public_event) + expect(events).to include(push_event, merge_event, issue_event, comment_event, wiki_event, design_event, team_event) + expect(events.size).to eq(10) + end - it 'only includes push events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::PUSH) - events = described_class.new(current_user, project_owner, event_filter, params).execute + context 'when unknown filter is given' do + it 'includes returns all events', :aggregate_failures do + event_filter = EventFilter.new('unknown') + allow(event_filter).to receive(:filter).and_return('unknown') - expect(events).to include(push_event) - expect(events.size).to eq(1) - end + events = described_class.new(current_user, [project_owner], event_filter, params).execute - it 'only includes merge events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::MERGED) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(private_event, internal_event, public_event) + expect(events).to include(push_event, merge_event, issue_event, comment_event, wiki_event, design_event, team_event) + expect(events.size).to eq(10) + end + end - expect(events).to include(merge_event) - expect(events.size).to eq(1) - end + it 'only includes push events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::PUSH) + events = described_class.new(current_user, project_owner, event_filter, params).execute - it 'only includes issue events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::ISSUE) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(push_event) + expect(events.size).to eq(1) + end - expect(events).to include(issue_event) - expect(events.size).to eq(1) - end + it 'only includes merge events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::MERGED) + events = described_class.new(current_user, project_owner, event_filter, params).execute - it 'only includes comments events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::COMMENTS) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(merge_event) + expect(events.size).to eq(1) + end - expect(events).to include(comment_event) - expect(events.size).to eq(1) - end + it 'only includes issue events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::ISSUE) + events = described_class.new(current_user, project_owner, event_filter, params).execute - it 'only includes wiki events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::WIKI) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(issue_event) + expect(events.size).to eq(1) + end - expect(events).to include(wiki_event) - expect(events.size).to eq(1) - end + it 'only includes comments events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::COMMENTS) + events = described_class.new(current_user, project_owner, event_filter, params).execute - it 'only includes design events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::DESIGNS) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(comment_event) + expect(events.size).to eq(1) + end - expect(events).to include(design_event) - expect(events.size).to eq(1) - end + it 'only includes wiki events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::WIKI) + events = described_class.new(current_user, project_owner, event_filter, params).execute - it 'only includes team events', :aggregate_failures do - event_filter = EventFilter.new(EventFilter::TEAM) - events = described_class.new(current_user, project_owner, event_filter, params).execute + expect(events).to include(wiki_event) + expect(events.size).to eq(1) + end - expect(events).to include(private_event, internal_event, public_event, team_event) - expect(events.size).to eq(4) - end - end + it 'only includes design events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::DESIGNS) + events = described_class.new(current_user, project_owner, event_filter, params).execute - describe 'issue activity events' do - let(:issue) { create(:issue, project: public_project) } - let(:note) { create(:note_on_issue, noteable: issue, project: public_project) } - let!(:event_a) { create(:event, :commented, target: note, author: project_owner) } - let!(:event_b) { create(:event, :closed, target: issue, author: project_owner) } + expect(events).to include(design_event) + expect(events.size).to eq(1) + end - it 'includes all issue related events', :aggregate_failures do - events = finder.execute + it 'only includes team events', :aggregate_failures do + event_filter = EventFilter.new(EventFilter::TEAM) + events = described_class.new(current_user, project_owner, event_filter, params).execute - expect(events).to include(event_a) - expect(events).to include(event_b) + expect(events).to include(private_event, internal_event, public_event, team_event) + expect(events.size).to eq(4) + end end - end - context 'limits' do - before do - stub_const("#{described_class}::DEFAULT_LIMIT", 1) - stub_const("#{described_class}::MAX_LIMIT", 3) - end + describe 'issue activity events' do + let(:issue) { create(:issue, project: public_project) } + let(:note) { create(:note_on_issue, noteable: issue, project: public_project) } + let!(:event_a) { create(:event, :commented, target: note, author: project_owner) } + let!(:event_b) { create(:event, :closed, target: issue, author: project_owner) } - context 'when limit is not set' do - it 'returns events limited to DEFAULT_LIMIT' do - expect(finder.execute.size).to eq(described_class::DEFAULT_LIMIT) + it 'includes all issue related events', :aggregate_failures do + events = finder.execute + + expect(events).to include(event_a) + expect(events).to include(event_b) end end - context 'when limit is set' do - let(:limit) { 2 } + context 'limits' do + before do + stub_const("#{described_class}::DEFAULT_LIMIT", 1) + stub_const("#{described_class}::MAX_LIMIT", 3) + end - it 'returns events limited to specified limit' do - expect(finder.execute.size).to eq(limit) + context 'when limit is not set' do + it 'returns events limited to DEFAULT_LIMIT' do + expect(finder.execute.size).to eq(described_class::DEFAULT_LIMIT) + end end - end - context 'when limit is set to a number that exceeds maximum limit' do - let(:limit) { 4 } + context 'when limit is set' do + let(:limit) { 2 } - before do - create(:event, project: public_project, author: project_owner) + it 'returns events limited to specified limit' do + expect(finder.execute.size).to eq(limit) + end end - it 'returns events limited to MAX_LIMIT' do - expect(finder.execute.size).to eq(described_class::MAX_LIMIT) + context 'when limit is set to a number that exceeds maximum limit' do + let(:limit) { 4 } + + before do + create(:event, project: public_project, author: project_owner) + end + + it 'returns events limited to MAX_LIMIT' do + expect(finder.execute.size).to eq(described_class::MAX_LIMIT) + end end end end end + + context 'when the optimized_followed_users_queries FF is on' do + before do + stub_feature_flags(optimized_followed_users_queries: true) + end + + it_behaves_like 'UserRecentEventsFinder examples' + end + + context 'when the optimized_followed_users_queries FF is off' do + before do + stub_feature_flags(optimized_followed_users_queries: false) + end + + it_behaves_like 'UserRecentEventsFinder examples' + end end diff --git a/spec/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder_spec.rb b/spec/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder_spec.rb index 832e269a78c9f4bcc032ca9c4fd2f7c70b9884ce..9f2ac9a953d41a33b324f55bc49597574f7dd688 100644 --- a/spec/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder_spec.rb +++ b/spec/lib/gitlab/pagination/keyset/in_operator_optimization/query_builder_spec.rb @@ -24,12 +24,12 @@ let_it_be(:issues) do [ create(:issue, project: project_1, created_at: three_weeks_ago, relative_position: 5), - create(:issue, project: project_1, created_at: two_weeks_ago), + create(:issue, project: project_1, created_at: two_weeks_ago, relative_position: nil), create(:issue, project: project_2, created_at: two_weeks_ago, relative_position: 15), - create(:issue, project: project_2, created_at: two_weeks_ago), - create(:issue, project: project_3, created_at: four_weeks_ago), + create(:issue, project: project_2, created_at: two_weeks_ago, relative_position: nil), + create(:issue, project: project_3, created_at: four_weeks_ago, relative_position: nil), create(:issue, project: project_4, created_at: five_weeks_ago, relative_position: 10), - create(:issue, project: project_5, created_at: four_weeks_ago) + create(:issue, project: project_5, created_at: four_weeks_ago, relative_position: nil) ] end @@ -155,6 +155,31 @@ it_behaves_like 'correct ordering examples' end + + context 'with condition "relative_position IS NULL"' do + let(:base_scope) { Issue.where(relative_position: nil) } + let(:scope) { base_scope.order(order) } + + let(:in_operator_optimization_options) do + { + array_scope: Project.where(namespace_id: top_level_group.self_and_descendants.select(:id)).select(:id), + array_mapping_scope: -> (id_expression) { Issue.merge(base_scope.dup).where(Issue.arel_table[:project_id].eq(id_expression)) }, + finder_query: -> (_relative_position_expression, id_expression) { Issue.where(Issue.arel_table[:id].eq(id_expression)) } + } + end + + context 'when iterating records one by one' do + let(:batch_size) { 1 } + + it_behaves_like 'correct ordering examples' + end + + context 'when iterating records with LIMIT 3' do + let(:batch_size) { 3 } + + it_behaves_like 'correct ordering examples' + end + end end context 'when ordering by issues.created_at DESC, issues.id ASC' do