Commit 20d40228 authored by Ash McKenzie's avatar Ash McKenzie

Merge branch 'fix-group-error-in-vsa-label-queries' into 'master'

Fix GROUP BY in VSA label queries

See merge request gitlab-org/gitlab!34381
parents 0efbe282 eac04612
...@@ -7,7 +7,7 @@ module EE ...@@ -7,7 +7,7 @@ module EE
module DataCollector module DataCollector
def duration_chart_data def duration_chart_data
strong_memoize(:duration_chart) do strong_memoize(:duration_chart) do
::Gitlab::Analytics::CycleAnalytics::DataForDurationChart.new(stage: stage, query: query).load ::Gitlab::Analytics::CycleAnalytics::DataForDurationChart.new(stage: stage, params: params, query: query).load
end end
end end
end end
......
...@@ -8,23 +8,23 @@ module Gitlab ...@@ -8,23 +8,23 @@ module Gitlab
MAX_RESULTS = 500 MAX_RESULTS = 500
def initialize(stage:, query:) def initialize(stage:, params:, query:)
@stage = stage @stage = stage
@params = params
@query = query @query = query
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def load def load
query order_by_end_event(query)
.select(round_duration_to_seconds.as('duration_in_seconds'), stage.end_event.timestamp_projection.as('finished_at')) .select(round_duration_to_seconds.as('duration_in_seconds'), stage.end_event.timestamp_projection.as('finished_at'))
.reorder(stage.end_event.timestamp_projection.desc)
.limit(MAX_RESULTS) .limit(MAX_RESULTS)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
private private
attr_reader :stage, :query attr_reader :stage, :query, :params
end end
end end
end end
......
...@@ -24,6 +24,10 @@ module Gitlab ...@@ -24,6 +24,10 @@ module Gitlab
Arel.sql("#{join_expression_name}.created_at") Arel.sql("#{join_expression_name}.created_at")
end end
def column_list
[timestamp_projection]
end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def apply_query_customization(query) def apply_query_customization(query)
query query
......
...@@ -28,7 +28,8 @@ module Gitlab ...@@ -28,7 +28,8 @@ module Gitlab
def issues_count def issues_count
issues = IssuesFinder.new(current_user, finder_params).execute issues = IssuesFinder.new(current_user, finder_params).execute
issues = issues.where(projects: { id: options[:projects] }) if options[:projects].present? issues = issues.where(projects: { id: options[:projects] }) if options[:projects].present?
issues.count
::Issue.where(id: issues.select(:id)).count
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
...@@ -210,7 +210,24 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do ...@@ -210,7 +210,24 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do
).execute(issue) ).execute(issue)
end end
it_behaves_like 'custom cycle analytics stage' it_behaves_like 'custom cycle analytics stage' do
context 'when filtering for two labels' do
let(:params) do
{
from: Time.new(2019),
to: Time.new(2020),
current_user: user,
label_name: [label.name, other_label.name]
}
end
subject { described_class.new(stage: stage, params: params) }
it 'does not raise query syntax error' do
expect { subject.records_fetcher.serialized_records }.not_to raise_error(ActiveRecord::StatementInvalid)
end
end
end
end end
describe 'between issue creation time and issue label added time' do describe 'between issue creation time and issue label added time' do
...@@ -491,6 +508,23 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do ...@@ -491,6 +508,23 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do
it_behaves_like 'filter examples' it_behaves_like 'filter examples'
end end
context 'when two labels are given' do
let(:label1) { create(:group_label, group: group) }
let(:label2) { create(:group_label, group: group) }
before do
MergeRequests::UpdateService.new(
merge_request.project,
user,
label_ids: [label1.id, label2.id]
).execute(merge_request)
data_collector_params[:label_name] = [label1.name, label2.name]
end
it_behaves_like 'filter examples'
end
context 'when `milestone_title` is given' do context 'when `milestone_title` is given' do
let(:milestone) { create(:milestone, group: group) } let(:milestone) { create(:milestone, group: group) }
......
...@@ -80,6 +80,27 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d ...@@ -80,6 +80,27 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::Summary::Group::StageSummary d
end end
end end
context 'with `label_name` filter' do
let(:label1) { create(:group_label, group: group) }
let(:label2) { create(:group_label, group: group) }
before do
issue = project.issues.last
Issues::UpdateService.new(
issue.project,
user,
label_ids: [label1.id, label2.id]
).execute(issue)
end
subject { described_class.new(group, options: { from: Time.now, current_user: user, label_name: [label1.name, label2.name] }).data }
it 'finds issue with two labels' do
expect(subject.first[:value]).to eq('1')
end
end
context 'when `from` and `to` parameters are provided' do context 'when `from` and `to` parameters are provided' do
subject { described_class.new(group, options: { from: 10.days.ago, to: Time.now, current_user: user }).data } subject { described_class.new(group, options: { from: 10.days.ago, to: Time.now, current_user: user }).data }
......
...@@ -90,9 +90,7 @@ module Gitlab ...@@ -90,9 +90,7 @@ module Gitlab
end end
def ordered_and_limited_query def ordered_and_limited_query
query order_by_end_event(query).limit(MAX_RECORDS)
.reorder(stage.end_event.timestamp_projection.desc)
.limit(MAX_RECORDS)
end end
def records def records
......
...@@ -18,10 +18,14 @@ module Gitlab ...@@ -18,10 +18,14 @@ module Gitlab
end end
def timestamp_projection def timestamp_projection
Arel::Nodes::NamedFunction.new('COALESCE', [ Arel::Nodes::NamedFunction.new('COALESCE', column_list)
end
def column_list
[
issue_metrics_table[:first_associated_with_milestone_at], issue_metrics_table[:first_associated_with_milestone_at],
issue_metrics_table[:first_added_to_board_at] issue_metrics_table[:first_added_to_board_at]
]) ]
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
......
...@@ -10,6 +10,10 @@ module Gitlab ...@@ -10,6 +10,10 @@ module Gitlab
query.joins(:metrics) query.joins(:metrics)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def column_list
[timestamp_projection]
end
end end
end end
end end
......
...@@ -18,10 +18,14 @@ module Gitlab ...@@ -18,10 +18,14 @@ module Gitlab
end end
def timestamp_projection def timestamp_projection
Arel::Nodes::NamedFunction.new('COALESCE', [ Arel::Nodes::NamedFunction.new('COALESCE', column_list)
end
def column_list
[
issue_metrics_table[:first_associated_with_milestone_at], issue_metrics_table[:first_associated_with_milestone_at],
issue_metrics_table[:first_added_to_board_at] issue_metrics_table[:first_added_to_board_at]
]) ]
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
......
...@@ -21,6 +21,10 @@ module Gitlab ...@@ -21,6 +21,10 @@ module Gitlab
mr_metrics_table[:first_deployed_to_production_at] mr_metrics_table[:first_deployed_to_production_at]
end end
def column_list
[timestamp_projection]
end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def apply_query_customization(query) def apply_query_customization(query)
query.joins(merge_requests_closing_issues: { merge_request: [:metrics] }).where(mr_metrics_table[:first_deployed_to_production_at].gteq(mr_table[:created_at])) query.joins(merge_requests_closing_issues: { merge_request: [:metrics] }).where(mr_metrics_table[:first_deployed_to_production_at].gteq(mr_table[:created_at]))
......
...@@ -32,6 +32,13 @@ module Gitlab ...@@ -32,6 +32,13 @@ module Gitlab
raise NotImplementedError raise NotImplementedError
end end
# List of columns that are referenced in the `timestamp_projection` expression
# Example timestamp projection: COALESCE(issue_metrics.created_at, issue_metrics.updated_at)
# Expected column list: issue_metrics.created_at, issue_metrics.updated_at
def column_list
[]
end
# Optionally a StageEvent may apply additional filtering or join other tables on the base query. # Optionally a StageEvent may apply additional filtering or join other tables on the base query.
def apply_query_customization(query) def apply_query_customization(query)
query query
......
...@@ -22,6 +22,29 @@ module Gitlab ...@@ -22,6 +22,29 @@ module Gitlab
stage.start_event.timestamp_projection stage.start_event.timestamp_projection
) )
end end
# rubocop: disable CodeReuse/ActiveRecord
def order_by_end_event(query)
ordered_query = query.reorder(stage.end_event.timestamp_projection.desc)
# When filtering for more than one label, postgres requires the columns in ORDER BY to be present in the GROUP BY clause
if requires_grouping?
column_list = [
ordered_query.arel_table[:id],
*stage.end_event.column_list,
*stage.start_event.column_list
]
ordered_query = ordered_query.group(column_list)
end
ordered_query
end
# rubocop: enable CodeReuse/ActiveRecord
def requires_grouping?
Array(params[:label_name]).size > 1
end
end end
end end
end end
......
...@@ -8,6 +8,7 @@ RSpec.shared_examples_for 'cycle analytics event' do ...@@ -8,6 +8,7 @@ RSpec.shared_examples_for 'cycle analytics event' do
it { expect(described_class.identifier).to be_a_kind_of(Symbol) } it { expect(described_class.identifier).to be_a_kind_of(Symbol) }
it { expect(instance.object_type.ancestors).to include(ApplicationRecord) } it { expect(instance.object_type.ancestors).to include(ApplicationRecord) }
it { expect(instance).to respond_to(:timestamp_projection) } it { expect(instance).to respond_to(:timestamp_projection) }
it { expect(instance.column_list).to be_a_kind_of(Array) }
describe '#apply_query_customization' do describe '#apply_query_customization' do
it 'expects an ActiveRecord::Relation object as argument and returns a modified version of it' do it 'expects an ActiveRecord::Relation object as argument and returns a modified version of it' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment