diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-04-10 00:10:04 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-04-10 00:10:04 +0000 |
commit | 1bf4fece121298260663c6ca73d39716d3548a03 (patch) | |
tree | 5c82dd2fa63552ecb67fcb034740ec7e8fdba25a | |
parent | 254ec28f5448f6f353cd98f637985de3d1405854 (diff) | |
download | gitlab-ce-1bf4fece121298260663c6ca73d39716d3548a03.tar.gz |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | app/models/blob.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/where_composite.rb | 81 | ||||
-rw-r--r-- | app/models/issue.rb | 21 | ||||
-rw-r--r-- | lib/api/internal/base.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/ci/templates/Terraform.gitlab-ci.yml | 5 | ||||
-rw-r--r-- | spec/models/concerns/where_composite_spec.rb | 184 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 16 | ||||
-rw-r--r-- | spec/support/matchers/exceed_query_limit.rb | 36 | ||||
-rw-r--r-- | spec/support/shared_examples/models/concerns/composite_id_shared_examples.rb | 47 |
9 files changed, 389 insertions, 7 deletions
diff --git a/app/models/blob.rb b/app/models/blob.rb index d8282c918b7..cdc5838797b 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -254,5 +254,3 @@ class Blob < SimpleDelegator classes.find { |viewer_class| viewer_class.can_render?(self, verify_binary: verify_binary) } end end - -Blob.prepend_if_ee('EE::Blob') diff --git a/app/models/concerns/where_composite.rb b/app/models/concerns/where_composite.rb new file mode 100644 index 00000000000..3b66efc1c77 --- /dev/null +++ b/app/models/concerns/where_composite.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +module WhereComposite + extend ActiveSupport::Concern + + class TooManyIds < ArgumentError + LIMIT = 100 + + def initialize(no_of_ids) + super(<<~MSG) + At most #{LIMIT} identifier sets at a time please! Got #{no_of_ids}. + Have you considered splitting your request into batches? + MSG + end + + def self.guard(collection) + n = collection.size + return collection if n <= LIMIT + + raise self, n + end + end + + class_methods do + # Apply a set of constraints that function as composite IDs. + # + # This is the plural form of the standard ActiveRecord idiom: + # `where(foo: x, bar: y)`, except it allows multiple pairs of `x` and + # `y` to be specified, with the semantics that translate to: + # + # ```sql + # WHERE + # (foo = x_0 AND bar = y_0) + # OR (foo = x_1 AND bar = y_1) + # OR ... + # ``` + # + # or the equivalent: + # + # ```sql + # WHERE + # (foo, bar) IN ((x_0, y_0), (x_1, y_1), ...) + # ``` + # + # @param permitted_keys [Array<Symbol>] The keys each hash must have. There + # must be at least one key (but really, + # it ought to be at least two) + # @param hashes [Array<#to_h>|#to_h] The constraints. Each parameter must have a + # value for the keys named in `permitted_keys` + # + # e.g.: + # ``` + # where_composite(%i[foo bar], [{foo: 1, bar: 2}, {foo: 1, bar: 3}]) + # ``` + # + def where_composite(permitted_keys, hashes) + raise ArgumentError, 'no permitted_keys' unless permitted_keys.present? + + # accept any hash-like thing, such as Structs + hashes = TooManyIds.guard(Array.wrap(hashes)).map(&:to_h) + + return none if hashes.empty? + + case permitted_keys.size + when 1 + key = permitted_keys.first + where(key => hashes.map { |hash| hash.fetch(key) }) + else + clauses = hashes.map do |hash| + permitted_keys.map do |key| + arel_table[key].eq(hash.fetch(key)) + end.reduce(:and) + end + + where(clauses.reduce(:or)) + end + rescue KeyError + raise ArgumentError, "all arguments must contain #{permitted_keys}" + end + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index f414be51065..cdd7429bc58 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -16,6 +16,7 @@ class Issue < ApplicationRecord include LabelEventable include IgnorableColumns include MilestoneEventable + include WhereComposite DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze @@ -78,6 +79,26 @@ class Issue < ApplicationRecord scope :counts_by_state, -> { reorder(nil).group(:state_id).count } + # An issue can be uniquely identified by project_id and iid + # Takes one or more sets of composite IDs, expressed as hash-like records of + # `{project_id: x, iid: y}`. + # + # @see WhereComposite::where_composite + # + # e.g: + # + # .by_project_id_and_iid({project_id: 1, iid: 2}) + # .by_project_id_and_iid([]) # returns ActiveRecord::NullRelation + # .by_project_id_and_iid([ + # {project_id: 1, iid: 1}, + # {project_id: 2, iid: 1}, + # {project_id: 1, iid: 2} + # ]) + # + scope :by_project_id_and_iid, ->(composites) do + where_composite(%i[project_id iid], composites) + end + after_commit :expire_etag_cache, unless: :importing? after_save :ensure_metrics, unless: :importing? diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index a926706f513..f1e33b27d2b 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -203,10 +203,6 @@ module API { reference_counter_increased: reference_counter_increased } end - post '/notify_post_receive' do - status 200 - end - post '/post_receive' do status 200 diff --git a/lib/gitlab/ci/templates/Terraform.gitlab-ci.yml b/lib/gitlab/ci/templates/Terraform.gitlab-ci.yml index dc70b977452..83483108fde 100644 --- a/lib/gitlab/ci/templates/Terraform.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Terraform.gitlab-ci.yml @@ -12,6 +12,7 @@ image: # Default output file for Terraform plan variables: PLAN: plan.tfplan + JSON_PLAN_FILE: tfplan.json cache: paths: @@ -37,11 +38,13 @@ plan: stage: build script: - terraform plan -out=$PLAN - - "terraform show --json $PLAN | convert_report > tfplan.json" + - "terraform show --json $PLAN | convert_report > $JSON_PLAN_FILE" artifacts: name: plan paths: - $PLAN + reports: + terraform: $JSON_PLAN_FILE # Separate apply job for manual launching Terraform as it can be destructive # action. diff --git a/spec/models/concerns/where_composite_spec.rb b/spec/models/concerns/where_composite_spec.rb new file mode 100644 index 00000000000..1c0951d90d0 --- /dev/null +++ b/spec/models/concerns/where_composite_spec.rb @@ -0,0 +1,184 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe WhereComposite do + describe '.where_composite' do + let_it_be(:test_table_name) { "test_table_#{SecureRandom.hex(10)}" } + + let(:model) do + tbl_name = test_table_name + Class.new(ActiveRecord::Base) do + self.table_name = tbl_name + + include WhereComposite + end + end + + def connection + ActiveRecord::Base.connection + end + + before_all do + connection.create_table(test_table_name) do |t| + t.integer :foo + t.integer :bar + t.string :wibble + end + end + + it 'requires at least one permitted key' do + expect { model.where_composite([], nil) } + .to raise_error(ArgumentError) + end + + it 'requires all arguments to match the permitted_keys' do + expect { model.where_composite([:foo], [{ foo: 1 }, { bar: 2 }]) } + .to raise_error(ArgumentError) + end + + it 'attaches a key error as cause if a key is missing' do + expect { model.where_composite([:foo], [{ foo: 1 }, { bar: 2 }]) } + .to raise_error(have_attributes(cause: KeyError)) + end + + it 'returns an empty relation if there are no arguments' do + expect(model.where_composite([:foo, :bar], nil)) + .to be_empty + + expect(model.where_composite([:foo, :bar], [])) + .to be_empty + end + + it 'permits extra arguments' do + a = model.where_composite([:foo, :bar], { foo: 1, bar: 2 }) + b = model.where_composite([:foo, :bar], { foo: 1, bar: 2, baz: 3 }) + + expect(a.to_sql).to eq(b.to_sql) + end + + it 'can handle multiple fields' do + fields = [:foo, :bar, :wibble] + args = { foo: 1, bar: 2, wibble: 'wobble' } + pattern = %r{ + WHERE \s+ + \(? + \s* "#{test_table_name}"\."foo" \s* = \s* 1 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 2 + \s+ AND + \s+ "#{test_table_name}"\."wibble" \s* = \s* 'wobble' + \s* + \)? + }x + + expect(model.where_composite(fields, args).to_sql).to match(pattern) + end + + it 'is equivalent to ids.map { |attrs| model.find_by(attrs) }' do + 10.times do |i| + 10.times do |j| + model.create!(foo: i, bar: j, wibble: generate(:filename)) + end + end + + ids = [{ foo: 1, bar: 9 }, { foo: 9, bar: 1 }] + + found = model.where_composite(%i[foo bar], ids) + + expect(found).to match_array(ids.map { |attrs| model.find_by!(attrs) }) + end + + it 'constructs (A&B) for one argument' do + fields = [:foo, :bar] + args = [ + { foo: 1, bar: 2 } + ] + pattern = %r{ + WHERE \s+ + \(? + \s* "#{test_table_name}"\."foo" \s* = \s* 1 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 2 + \s* + \)? + }x + + expect(model.where_composite(fields, args).to_sql).to match(pattern) + expect(model.where_composite(fields, args[0]).to_sql).to match(pattern) + end + + it 'constructs (A&B) OR (C&D) for two arguments' do + args = [ + { foo: 1, bar: 2 }, + { foo: 3, bar: 4 } + ] + pattern = %r{ + WHERE \s+ + \( \s* "#{test_table_name}"\."foo" \s* = \s* 1 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 2 + \s* \)? + \s* OR \s* + \(? \s* "#{test_table_name}"\."foo" \s* = \s* 3 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 4 + \s* \) + }x + + q = model.where_composite([:foo, :bar], args) + + expect(q.to_sql).to match(pattern) + end + + it 'constructs (A&B) OR (C&D) OR (E&F) for three arguments' do + args = [ + { foo: 1, bar: 2 }, + { foo: 3, bar: 4 }, + { foo: 5, bar: 6 } + ] + pattern = %r{ + WHERE \s+ + \({2} + \s* "#{test_table_name}"\."foo" \s* = \s* 1 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 2 + \s* \)? + \s* OR \s* + \(? \s* "#{test_table_name}"\."foo" \s* = \s* 3 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 4 + \s* \)? + \s* OR \s* + \(? \s* "#{test_table_name}"\."foo" \s* = \s* 5 + \s+ AND + \s+ "#{test_table_name}"\."bar" \s* = \s* 6 + \s* \) + }x + + q = model.where_composite([:foo, :bar], args) + + expect(q.to_sql).to match(pattern) + end + + describe 'large sets of IDs' do + def query(size) + args = (0..).lazy.take(size).map { |n| { foo: n, bar: n * n, wibble: 'x' * n } }.to_a + model.where_composite([:foo, :bar, :wibble], args) + end + + it 'constructs correct trees of constraints' do + n = described_class::TooManyIds::LIMIT + q = query(n) + sql = q.to_sql + + expect(sql.scan(/OR/).count).to eq(n - 1) + expect(sql.scan(/AND/).count).to eq(2 * n) + end + + it 'raises errors if too many IDs are passed' do + expect { query(described_class::TooManyIds::LIMIT + 1) }.to raise_error(described_class::TooManyIds) + end + end + end +end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 9797e0a0472..e8103be0682 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -900,6 +900,22 @@ describe Issue do end end + describe '.by_project_id_and_iid' do + let_it_be(:issue_a) { create(:issue) } + let_it_be(:issue_b) { create(:issue, iid: issue_a.iid) } + let_it_be(:issue_c) { create(:issue, project: issue_a.project) } + let_it_be(:issue_d) { create(:issue, project: issue_a.project) } + + it_behaves_like 'a where_composite scope', :by_project_id_and_iid do + let(:all_results) { [issue_a, issue_b, issue_c, issue_d] } + let(:first_result) { issue_a } + + let(:composite_ids) do + all_results.map { |issue| { project_id: issue.project_id, iid: issue.iid } } + end + end + end + it_behaves_like 'throttled touch' do subject { create(:issue, updated_at: 1.hour.ago) } end diff --git a/spec/support/matchers/exceed_query_limit.rb b/spec/support/matchers/exceed_query_limit.rb index f38ae44a577..b9630b00038 100644 --- a/spec/support/matchers/exceed_query_limit.rb +++ b/spec/support/matchers/exceed_query_limit.rb @@ -73,6 +73,42 @@ module ExceedQueryLimitHelpers end end +RSpec::Matchers.define :issue_same_number_of_queries_as do + supports_block_expectations + + include ExceedQueryLimitHelpers + + def control + block_arg + end + + def control_recorder + @control_recorder ||= ActiveRecord::QueryRecorder.new(&control) + end + + def expected_count + @expected_count ||= control_recorder.count + end + + def verify_count(&block) + @subject_block = block + + (expected_count - actual_count).abs <= threshold + end + + match do |block| + verify_count(&block) + end + + failure_message_when_negated do |actual| + failure_message + end + + def skip_cached + false + end +end + RSpec::Matchers.define :exceed_all_query_limit do |expected| supports_block_expectations diff --git a/spec/support/shared_examples/models/concerns/composite_id_shared_examples.rb b/spec/support/shared_examples/models/concerns/composite_id_shared_examples.rb new file mode 100644 index 00000000000..cd925659311 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/composite_id_shared_examples.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'a where_composite scope' do |scope_name| + let(:result) { described_class.public_send(scope_name, ids) } + + context 'we pass an empty array' do + let(:ids) { [] } + + it 'returns a null relation' do + expect(result).to be_empty + end + end + + context 'we pass nil' do + let(:ids) { nil } + + it 'returns a null relation' do + expect(result).to be_empty + end + end + + context 'we pass a singleton composite id' do + let(:ids) { composite_ids.first } + + it 'finds the first result' do + expect(result).to contain_exactly(first_result) + end + end + + context 'we pass group of ids' do + let(:ids) { composite_ids } + + it 'finds all the results' do + expect(result).to contain_exactly(*all_results) + end + end + + describe 'performance' do + it 'is not O(N)' do + all_ids = composite_ids + one_id = composite_ids.first + + expect { described_class.public_send(scope_name, all_ids) } + .to issue_same_number_of_queries_as { described_class.public_send(scope_name, one_id) } + end + end +end |