diff options
25 files changed, 377 insertions, 32 deletions
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 6d9efb433c2..dfa67f042e2 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -26,12 +26,12 @@ class RegistrationsController < Devise::RegistrationsController end def create - track_experiment_event(:terms_opt_in, 'end') accept_pending_invitations super do |new_user| persist_accepted_terms_if_required(new_user) set_role_required(new_user) + track_terms_experiment(new_user) yield new_user if block_given? end @@ -201,6 +201,13 @@ class RegistrationsController < Devise::RegistrationsController true end + def track_terms_experiment(new_user) + return unless new_user.persisted? + + track_experiment_event(:terms_opt_in, 'end') + record_experiment_user(:terms_opt_in) + end + def load_recaptcha Gitlab::Recaptcha.load_configurations! end diff --git a/app/finders/packages/group_packages_finder.rb b/app/finders/packages/group_packages_finder.rb index ffc8c35fbcc..8b948bb056d 100644 --- a/app/finders/packages/group_packages_finder.rb +++ b/app/finders/packages/group_packages_finder.rb @@ -22,6 +22,9 @@ module Packages def packages_for_group_projects packages = ::Packages::Package + .including_build_info + .including_project_route + .including_tags .for_projects(group_projects_visible_to_current_user) .processed .has_version diff --git a/app/finders/packages/package_finder.rb b/app/finders/packages/package_finder.rb index 0e911491da2..f1874b77845 100644 --- a/app/finders/packages/package_finder.rb +++ b/app/finders/packages/package_finder.rb @@ -9,6 +9,9 @@ module Packages def execute @project .packages + .including_build_info + .including_project_route + .including_tags .processed .find(@package_id) end diff --git a/app/finders/packages/packages_finder.rb b/app/finders/packages/packages_finder.rb index c533cb266a2..519e8bf9c34 100644 --- a/app/finders/packages/packages_finder.rb +++ b/app/finders/packages/packages_finder.rb @@ -13,7 +13,12 @@ module Packages end def execute - packages = project.packages.processed.has_version + packages = project.packages + .including_build_info + .including_project_route + .including_tags + .processed + .has_version packages = filter_by_package_type(packages) packages = filter_by_package_name(packages) packages = order_packages(packages) diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 0ab6b337ac0..159c43fbb47 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -51,6 +51,9 @@ class Packages::Package < ApplicationRecord scope :with_version, ->(version) { where(version: version) } scope :without_version_like, -> (version) { where.not(arel_table[:version].matches(version)) } scope :with_package_type, ->(package_type) { where(package_type: package_type) } + scope :including_build_info, -> { includes(build_info: { pipeline: :user }) } + scope :including_project_route, -> { includes(project: { namespace: :route }) } + scope :including_tags, -> { includes(:tags) } scope :with_conan_channel, ->(package_channel) do joins(:conan_metadatum).where(packages_conan_metadata: { package_channel: package_channel }) @@ -143,6 +146,8 @@ class Packages::Package < ApplicationRecord def versions project.packages + .including_build_info + .including_tags .with_name(name) .where.not(version: version) .with_package_type(package_type) diff --git a/changelogs/unreleased/219171-package-n2-bug.yml b/changelogs/unreleased/219171-package-n2-bug.yml new file mode 100644 index 00000000000..0444f2b0249 --- /dev/null +++ b/changelogs/unreleased/219171-package-n2-bug.yml @@ -0,0 +1,5 @@ +--- +title: Fix package API query performance when pipelines and multiple versions are present +merge_request: 40770 +author: +type: performance diff --git a/doc/development/experiment_guide/index.md b/doc/development/experiment_guide/index.md index b1a15c7749f..07b803603a5 100644 --- a/doc/development/experiment_guide/index.md +++ b/doc/development/experiment_guide/index.md @@ -16,7 +16,7 @@ In either case, an outcome of the experiment should be posted to the issue with ## Code reviews -Since the code of experiments will not be part of the codebase for a long time and we want to iterate fast to retrieve data,the code quality of experiments might sometimes not fulfill our standards but should not negatively impact the availability of GitLab whether the experiment is running or not. +Since the code of experiments will not be part of the codebase for a long time and we want to iterate fast to retrieve data, the code quality of experiments might sometimes not fulfill our standards but should not negatively impact the availability of GitLab whether the experiment is running or not. Initially experiments will only be deployed to a fraction of users but we still want a flawless experience for those users. Therefore, experiments still require tests. For reviewers and maintainers: if you find code that would usually not make it through the review, but is temporarily acceptable, please mention your concerns but note that it's not necessary to change. diff --git a/doc/development/secure_coding_guidelines.md b/doc/development/secure_coding_guidelines.md index 65953620ce6..1961d1dcc34 100644 --- a/doc/development/secure_coding_guidelines.md +++ b/doc/development/secure_coding_guidelines.md @@ -84,7 +84,7 @@ This Ruby Regex specialty can have security impact, as often regular expressions GitLab specific examples can be found [here](https://gitlab.com/gitlab-org/gitlab/-/issues/36029#note_251262187) and [there](https://gitlab.com/gitlab-org/gitlab/-/issues/33569). -Another example would be this fictional Ruby On Rails controller: +Another example would be this fictional Ruby on Rails controller: ```ruby class PingController < ApplicationController @@ -127,9 +127,9 @@ class Email < ApplicationRecord DOMAIN_MATCH = Regexp.new('([a-zA-Z0-9]+)+\.com') validates :domain_matches - + private - + def domain_matches errors.add(:email, 'does not match') if email =~ DOMAIN_MATCH end @@ -184,7 +184,7 @@ have been reported to GitLab include: - Reading internal services, including cloud service metadata. - The latter can be a serious problem, because an attacker can obtain keys that allow control of the victim's cloud infrastructure. (This is also a good reason to give only necessary privileges to the token.). [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/51490). -- When combined with CRLF vulnerability, remote code execution. [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41293) +- When combined with CRLF vulnerability, remote code execution. [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41293). ### When to Consider @@ -213,7 +213,7 @@ the mitigations for a new feature. #### Feature-specific Mitigations -For situtions in which an allowlist or GitLab:HTTP cannot be used, it will be necessary to implement mitigations directly in the feature. It is best to validate the destination IP addresses themselves, not just domain names, as DNS can be controlled by the attacker. Below are a list of mitigations that should be implemented. +For situations in which an allowlist or GitLab:HTTP cannot be used, it will be necessary to implement mitigations directly in the feature. It is best to validate the destination IP addresses themselves, not just domain names, as DNS can be controlled by the attacker. Below are a list of mitigations that should be implemented. **Important Note:** There are many tricks to bypass common SSRF validations. If feature-specific mitigations are necessary, they should be reviewed by the AppSec team, or a developer who has worked on SSRF mitigations previously. diff --git a/doc/user/application_security/dast/index.md b/doc/user/application_security/dast/index.md index 56a373a8856..a15bc2be10b 100644 --- a/doc/user/application_security/dast/index.md +++ b/doc/user/application_security/dast/index.md @@ -455,7 +455,7 @@ DAST can be [configured](#customizing-the-dast-settings) using environment varia | `DAST_USERNAME_FIELD` | string | The name of username field at the sign-in HTML form. | | `DAST_PASSWORD_FIELD` | string | The name of password field at the sign-in HTML form. | | `DAST_MASK_HTTP_HEADERS` | string | Comma-separated list of request and response headers to be masked (GitLab 13.1). Must contain **all** headers to be masked. Refer to [list of headers that are masked by default](#hide-sensitive-information). | -| `DAST_AUTH_EXCLUDE_URLS` | URLs | The URLs to skip during the authenticated scan; comma-separated, no spaces in between. Not supported for API scans. | +| `DAST_AUTH_EXCLUDE_URLS` | URLs | The URLs to skip during the authenticated scan; comma-separated. Regular expression syntax can be used to match multiple URLs. For example, `.*` matches an arbitrary character sequence. Not supported for API scans. | | `DAST_FULL_SCAN_ENABLED` | boolean | Set to `true` to run a [ZAP Full Scan](https://github.com/zaproxy/zaproxy/wiki/ZAP-Full-Scan) instead of a [ZAP Baseline Scan](https://github.com/zaproxy/zaproxy/wiki/ZAP-Baseline-Scan). Default: `false` | | `DAST_FULL_SCAN_DOMAIN_VALIDATION_REQUIRED` | boolean | Set to `true` to require [domain validation](#domain-validation) when running DAST full scans. Not supported for API scans. Default: `false` | | `DAST_AUTO_UPDATE_ADDONS` | boolean | ZAP add-ons are pinned to specific versions in the DAST Docker image. Set to `true` to download the latest versions when the scan starts. Default: `false` | diff --git a/doc/user/project/repository/gpg_signed_commits/index.md b/doc/user/project/repository/gpg_signed_commits/index.md index 5526828c969..646d708d896 100644 --- a/doc/user/project/repository/gpg_signed_commits/index.md +++ b/doc/user/project/repository/gpg_signed_commits/index.md @@ -44,7 +44,7 @@ If you don't already have a GPG key, the following steps will help you get started: 1. [Install GPG](https://www.gnupg.org/download/index.html) for your operating system. - If your Operating System has `gpg2` installed, replace `gpg` with `gpg2` in + If your operating system has `gpg2` installed, replace `gpg` with `gpg2` in the following commands. 1. Generate the private/public key pair with the following command, which will spawn a series of questions: diff --git a/lib/api/entities/package.rb b/lib/api/entities/package.rb index 670965b225c..d903f50befa 100644 --- a/lib/api/entities/package.rb +++ b/lib/api/entities/package.rb @@ -28,7 +28,7 @@ module API expose :pipeline, if: ->(package) { package.build_info }, using: Package::Pipeline - expose :versions, using: ::API::Entities::PackageVersion + expose :versions, using: ::API::Entities::PackageVersion, unless: ->(_, opts) { opts[:collection] } private diff --git a/lib/gitlab/database/background_migration_job.rb b/lib/gitlab/database/background_migration_job.rb index 445735b232a..1b9d7cbc9a1 100644 --- a/lib/gitlab/database/background_migration_job.rb +++ b/lib/gitlab/database/background_migration_job.rb @@ -3,6 +3,8 @@ module Gitlab module Database class BackgroundMigrationJob < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord + include EachBatch + self.table_name = :background_migration_jobs scope :for_migration_class, -> (class_name) { where(class_name: normalize_class_name(class_name)) } diff --git a/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb index e6d8ec55319..04c63d27f9e 100644 --- a/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb +++ b/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb @@ -6,6 +6,7 @@ module Gitlab module TableManagementHelpers include ::Gitlab::Database::SchemaHelpers include ::Gitlab::Database::DynamicModelHelpers + include ::Gitlab::Database::MigrationHelpers include ::Gitlab::Database::Migrations::BackgroundMigrationHelpers ALLOWED_TABLES = %w[audit_events].freeze @@ -15,6 +16,12 @@ module Gitlab BATCH_INTERVAL = 2.minutes.freeze BATCH_SIZE = 50_000 + JobArguments = Struct.new(:start_id, :stop_id, :source_table_name, :partitioned_table_name, :source_column) do + def self.from_array(arguments) + self.new(*arguments) + end + end + # Creates a partitioned copy of an existing table, using a RANGE partitioning strategy on a timestamp column. # One partition is created per month between the given `min_date` and `max_date`. Also installs a trigger on # the original table to copy writes into the partitioned table. To copy over historic data from before creation @@ -132,6 +139,42 @@ module Gitlab end end + # Executes cleanup tasks from a previous BackgroundMigration to backfill a partitioned table by finishing + # pending jobs and performing a final data synchronization. + # This performs two steps: + # 1. Wait to finish any pending BackgroundMigration jobs that have not succeeded + # 2. Inline copy any missed rows from the original table to the partitioned table + # + # **NOTE** Migrations using this method cannot be scheduled in the same release as the migration that + # schedules the background migration using the `enqueue_background_migration` helper, or else the + # background migration jobs will be force-executed. + # + # Example: + # + # finalize_backfilling_partitioned_table :audit_events + # + def finalize_backfilling_partitioned_table(table_name) + assert_table_is_allowed(table_name) + assert_not_in_transaction_block(scope: ERROR_SCOPE) + + partitioned_table_name = make_partitioned_table_name(table_name) + unless table_exists?(partitioned_table_name) + raise "could not find partitioned table for #{table_name}, " \ + "this could indicate the previous partitioning migration has been rolled back." + end + + Gitlab::BackgroundMigration.steal(MIGRATION_CLASS_NAME) do |raw_arguments| + JobArguments.from_array(raw_arguments).source_table_name == table_name.to_s + end + + primary_key = connection.primary_key(table_name) + copy_missed_records(table_name, partitioned_table_name, primary_key) + + disable_statement_timeout do + execute("VACUUM FREEZE ANALYZE #{partitioned_table_name}") + end + end + private def assert_table_is_allowed(table_name) @@ -284,7 +327,7 @@ module Gitlab create_trigger(table_name, trigger_name, function_name, fires: 'AFTER INSERT OR UPDATE OR DELETE') end - def enqueue_background_migration(source_table_name, partitioned_table_name, source_key) + def enqueue_background_migration(source_table_name, partitioned_table_name, source_column) source_model = define_batchable_model(source_table_name) queue_background_migration_jobs_by_range_at_intervals( @@ -292,13 +335,35 @@ module Gitlab MIGRATION_CLASS_NAME, BATCH_INTERVAL, batch_size: BATCH_SIZE, - other_job_arguments: [source_table_name.to_s, partitioned_table_name, source_key], + other_job_arguments: [source_table_name.to_s, partitioned_table_name, source_column], track_jobs: true) end def cleanup_migration_jobs(table_name) ::Gitlab::Database::BackgroundMigrationJob.for_partitioning_migration(MIGRATION_CLASS_NAME, table_name).delete_all end + + def copy_missed_records(source_table_name, partitioned_table_name, source_column) + backfill_table = BackfillPartitionedTable.new + relation = ::Gitlab::Database::BackgroundMigrationJob.pending + .for_partitioning_migration(MIGRATION_CLASS_NAME, source_table_name) + + relation.each_batch do |batch| + batch.each do |pending_migration_job| + job_arguments = JobArguments.from_array(pending_migration_job.arguments) + start_id = job_arguments.start_id + stop_id = job_arguments.stop_id + + say("Backfilling data into partitioned table for ids from #{start_id} to #{stop_id}") + job_updated_count = backfill_table.perform(start_id, stop_id, source_table_name, + partitioned_table_name, source_column) + + unless job_updated_count > 0 + raise "failed to update tracking record for ids from #{start_id} to #{stop_id}" + end + end + end + end end end end diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 2c766035d87..309f8d38f74 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -316,6 +316,12 @@ RSpec.describe RegistrationsController do stub_experiment(signup_flow: true, terms_opt_in: true) end + it 'records user for the terms_opt_in experiment' do + expect(controller).to receive(:record_experiment_user).with(:terms_opt_in) + + subject + end + context 'when user is not part of the experiment' do before do stub_experiment_for_user(signup_flow: true, terms_opt_in: false) diff --git a/spec/fixtures/api/schemas/public_api/v4/packages/collection_package.json b/spec/fixtures/api/schemas/public_api/v4/packages/collection_package.json new file mode 100644 index 00000000000..109b0e59635 --- /dev/null +++ b/spec/fixtures/api/schemas/public_api/v4/packages/collection_package.json @@ -0,0 +1,34 @@ +{ + "type": "object", + "required": [ + "name", + "version", + "package_type", + "_links" + ], + "properties": { + "name": { + "type": "string" + }, + "version": { + "type": "string" + }, + "package_type": { + "type": "string" + }, + "_links": { + "type": "object", + "required": [ + "web_path" + ], + "properties": { + "details": { + "type": "string" + } + } + }, + "created_at": { + "type": "string" + } + } +}
\ No newline at end of file diff --git a/spec/fixtures/api/schemas/public_api/v4/packages/packages.json b/spec/fixtures/api/schemas/public_api/v4/packages/packages.json index 66364da4fdb..a03a49db4f9 100644 --- a/spec/fixtures/api/schemas/public_api/v4/packages/packages.json +++ b/spec/fixtures/api/schemas/public_api/v4/packages/packages.json @@ -1,4 +1,4 @@ { "type": "array", - "items": { "$ref": "./package.json" } + "items": { "$ref": "./collection_package.json" } } diff --git a/spec/lib/gitlab/database/background_migration_job_spec.rb b/spec/lib/gitlab/database/background_migration_job_spec.rb index 40f47325be3..dd5bf8b512f 100644 --- a/spec/lib/gitlab/database/background_migration_job_spec.rb +++ b/spec/lib/gitlab/database/background_migration_job_spec.rb @@ -71,6 +71,15 @@ RSpec.describe Gitlab::Database::BackgroundMigrationJob do expect(job4.reload).to be_pending end + it 'returns the number of jobs updated' do + expect(described_class.succeeded.count).to eq(0) + + jobs_updated = described_class.mark_all_as_succeeded('::TestJob', [1, 100]) + + expect(jobs_updated).to eq(2) + expect(described_class.succeeded.count).to eq(2) + end + context 'when previous matching jobs have already succeeded' do let(:initial_time) { Time.now.round } let!(:job1) { create(:background_migration_job, :succeeded, created_at: initial_time, updated_at: initial_time) } diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb index 49f3f87fe61..ec3d0a6dbcb 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/backfill_partitioned_table_spec.rb @@ -107,6 +107,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::BackfillPartition end.to change { ::Gitlab::Database::BackgroundMigrationJob.succeeded.count }.from(0).to(1) end + it 'returns the number of job records marked as succeeded' do + create(:background_migration_job, class_name: "::#{described_class.name}", + arguments: [source1.id, source3.id, source_table, destination_table, unique_key]) + + jobs_updated = subject.perform(source1.id, source3.id, source_table, destination_table, unique_key) + + expect(jobs_updated).to eq(1) + end + context 'when the feature flag is disabled' do let(:mock_connection) { double('connection') } diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb index 86f79b213ae..44ef0b307fe 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb @@ -480,6 +480,153 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end end + describe '#finalize_backfilling_partitioned_table' do + let(:source_table) { 'todos' } + let(:source_column) { 'id' } + + context 'when the table is not allowed' do + let(:source_table) { :this_table_is_not_allowed } + + it 'raises an error' do + expect(migration).to receive(:assert_table_is_allowed).with(source_table).and_call_original + + expect do + migration.finalize_backfilling_partitioned_table source_table + end.to raise_error(/#{source_table} is not allowed for use/) + end + end + + context 'when the partitioned table does not exist' do + it 'raises an error' do + expect(migration).to receive(:table_exists?).with(partitioned_table).and_return(false) + + expect do + migration.finalize_backfilling_partitioned_table source_table + end.to raise_error(/could not find partitioned table for #{source_table}/) + end + end + + context 'finishing pending background migration jobs' do + let(:source_table_double) { double('table name') } + let(:raw_arguments) { [1, 50_000, source_table_double, partitioned_table, source_column] } + + before do + allow(migration).to receive(:table_exists?).with(partitioned_table).and_return(true) + allow(migration).to receive(:copy_missed_records) + allow(migration).to receive(:execute).with(/VACUUM/) + end + + it 'finishes remaining jobs for the correct table' do + expect_next_instance_of(described_class::JobArguments) do |job_arguments| + expect(job_arguments).to receive(:source_table_name).and_call_original + end + + expect(Gitlab::BackgroundMigration).to receive(:steal) + .with(described_class::MIGRATION_CLASS_NAME) + .and_yield(raw_arguments) + + expect(source_table_double).to receive(:==).with(source_table.to_s) + + migration.finalize_backfilling_partitioned_table source_table + end + end + + context 'when there is missed data' do + let(:partitioned_model) { Class.new(ActiveRecord::Base) } + let(:timestamp) { Time.utc(2019, 12, 1, 12).round } + let!(:todo1) { create(:todo, created_at: timestamp, updated_at: timestamp) } + let!(:todo2) { create(:todo, created_at: timestamp, updated_at: timestamp) } + let!(:todo3) { create(:todo, created_at: timestamp, updated_at: timestamp) } + let!(:todo4) { create(:todo, created_at: timestamp, updated_at: timestamp) } + + let!(:pending_job1) do + create(:background_migration_job, + class_name: described_class::MIGRATION_CLASS_NAME, + arguments: [todo1.id, todo2.id, source_table, partitioned_table, source_column]) + end + + let!(:pending_job2) do + create(:background_migration_job, + class_name: described_class::MIGRATION_CLASS_NAME, + arguments: [todo3.id, todo3.id, source_table, partitioned_table, source_column]) + end + + let!(:succeeded_job) do + create(:background_migration_job, :succeeded, + class_name: described_class::MIGRATION_CLASS_NAME, + arguments: [todo4.id, todo4.id, source_table, partitioned_table, source_column]) + end + + before do + partitioned_model.primary_key = :id + partitioned_model.table_name = partitioned_table + + allow(migration).to receive(:queue_background_migration_jobs_by_range_at_intervals) + + migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date + + allow(Gitlab::BackgroundMigration).to receive(:steal) + allow(migration).to receive(:execute).with(/VACUUM/) + end + + it 'idempotently cleans up after failed background migrations' do + expect(partitioned_model.count).to eq(0) + + partitioned_model.insert!(todo2.attributes) + + expect_next_instance_of(Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable) do |backfill| + allow(backfill).to receive(:transaction_open?).and_return(false) + + expect(backfill).to receive(:perform) + .with(todo1.id, todo2.id, source_table, partitioned_table, source_column) + .and_call_original + + expect(backfill).to receive(:perform) + .with(todo3.id, todo3.id, source_table, partitioned_table, source_column) + .and_call_original + end + + migration.finalize_backfilling_partitioned_table source_table + + expect(partitioned_model.count).to eq(3) + + [todo1, todo2, todo3].each do |original| + copy = partitioned_model.find(original.id) + expect(copy.attributes).to eq(original.attributes) + end + + expect(partitioned_model.find_by_id(todo4.id)).to be_nil + + [pending_job1, pending_job2].each do |job| + expect(job.reload).to be_succeeded + end + end + + it 'raises an error if no job tracking records are marked as succeeded' do + expect_next_instance_of(Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable) do |backfill| + allow(backfill).to receive(:transaction_open?).and_return(false) + + expect(backfill).to receive(:perform).and_return(0) + end + + expect do + migration.finalize_backfilling_partitioned_table source_table + end.to raise_error(/failed to update tracking record/) + end + + it 'vacuums the table after loading is complete' do + expect_next_instance_of(Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable) do |backfill| + allow(backfill).to receive(:perform).and_return(1) + end + + expect(migration).to receive(:disable_statement_timeout).and_call_original + expect(migration).to receive(:execute).with("VACUUM FREEZE ANALYZE #{partitioned_table}") + + migration.finalize_backfilling_partitioned_table source_table + end + end + end + def filter_columns_by_name(columns, names) columns.reject { |c| names.include?(c.name) } end diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb index 6a235d3aa17..960db31d488 100644 --- a/spec/models/resource_label_event_spec.rb +++ b/spec/models/resource_label_event_spec.rb @@ -3,10 +3,12 @@ require 'spec_helper' RSpec.describe ResourceLabelEvent, type: :model do - subject { build(:resource_label_event, issue: issue) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:label) { create(:label, project: project) } - let(:issue) { create(:issue) } - let(:merge_request) { create(:merge_request) } + subject { build(:resource_label_event, issue: issue, label: label) } it_behaves_like 'having unique enum values' diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index bdd13a77d7f..f50c8bd908e 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Service do let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } describe "Associations" do it { is_expected.to belong_to :project } @@ -15,8 +16,6 @@ RSpec.describe Service do describe 'validations' do using RSpec::Parameterized::TableSyntax - let(:project) { create(:project) } - it { is_expected.to validate_presence_of(:type) } where(:project_id, :group_id, :template, :instance, :valid) do @@ -145,11 +144,11 @@ RSpec.describe Service do end describe "Test Button" do + let(:service) { build(:service, project: project) } + describe '#can_test?' do subject { service.can_test? } - let(:service) { build(:service, project: project) } - context 'when repository is not empty' do let(:project) { build(:project, :repository) } @@ -185,7 +184,6 @@ RSpec.describe Service do describe '#test' do let(:data) { 'test' } - let(:service) { build(:service, project: project) } context 'when repository is not empty' do let(:project) { build(:project, :repository) } @@ -264,8 +262,6 @@ RSpec.describe Service do end describe 'template' do - let(:project) { create(:project) } - shared_examples 'retrieves service templates' do it 'returns the available service templates' do expect(Service.find_or_create_templates.pluck(:type)).to match_array(Service.available_services_types) @@ -453,7 +449,7 @@ RSpec.describe Service do describe "{property}_changed?" do let(:service) do BambooService.create( - project: create(:project), + project: project, properties: { bamboo_url: 'http://gitlab.com', username: 'mic', @@ -493,7 +489,7 @@ RSpec.describe Service do describe "{property}_touched?" do let(:service) do BambooService.create( - project: create(:project), + project: project, properties: { bamboo_url: 'http://gitlab.com', username: 'mic', @@ -533,7 +529,7 @@ RSpec.describe Service do describe "{property}_was" do let(:service) do BambooService.create( - project: create(:project), + project: project, properties: { bamboo_url: 'http://gitlab.com', username: 'mic', @@ -573,7 +569,7 @@ RSpec.describe Service do describe 'initialize service with no properties' do let(:service) do BugzillaService.create( - project: create(:project), + project: project, project_url: 'http://gitlab.example.com' ) end @@ -588,7 +584,6 @@ RSpec.describe Service do end describe "callbacks" do - let(:project) { create(:project) } let!(:service) do RedmineService.new( project: project, @@ -655,7 +650,6 @@ RSpec.describe Service do end context 'logging' do - let(:project) { build(:project) } let(:service) { build(:service, project: project) } let(:test_message) { "test message" } let(:arguments) do diff --git a/spec/requests/api/group_packages_spec.rb b/spec/requests/api/group_packages_spec.rb index e02f6099637..f67cafbd8f5 100644 --- a/spec/requests/api/group_packages_spec.rb +++ b/spec/requests/api/group_packages_spec.rb @@ -141,5 +141,7 @@ RSpec.describe API::GroupPackages do it_behaves_like 'returning response status', :bad_request end + + it_behaves_like 'does not cause n^2 queries' end end diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb index 0ece3bff8f9..2f0d0fc87ec 100644 --- a/spec/requests/api/project_packages_spec.rb +++ b/spec/requests/api/project_packages_spec.rb @@ -104,6 +104,8 @@ RSpec.describe API::ProjectPackages do expect(json_response.first['name']).to include(package2.name) end end + + it_behaves_like 'does not cause n^2 queries' end end @@ -127,6 +129,22 @@ RSpec.describe API::ProjectPackages do end context 'without the need for a license' do + context 'with build info' do + it 'does not result in additional queries' do + control = ActiveRecord::QueryRecorder.new do + get api(package_url, user) + end + + pipeline = create(:ci_pipeline, user: user) + create(:ci_build, user: user, pipeline: pipeline) + create(:package_build_info, package: package1, pipeline: pipeline) + + expect do + get api(package_url, user) + end.not_to exceed_query_limit(control) + end + end + context 'project is public' do it 'returns 200 and the package information' do subject diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index f087f72ca46..475f036ffc6 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -41,7 +41,7 @@ RSpec.describe Notes::CreateService do end it 'TodoService#new_note is called' do - note = build(:note, project: project) + note = build(:note, project: project, noteable: issue) allow(Note).to receive(:new).with(opts) { note } expect_any_instance_of(TodoService).to receive(:new_note).with(note, user) @@ -50,7 +50,7 @@ RSpec.describe Notes::CreateService do end it 'enqueues NewNoteWorker' do - note = build(:note, id: non_existing_record_id, project: project) + note = build(:note, id: non_existing_record_id, project: project, noteable: issue) allow(Note).to receive(:new).with(opts) { note } expect(NewNoteWorker).to receive(:perform_async).with(note.id) diff --git a/spec/support/shared_examples/requests/api/packages_shared_examples.rb b/spec/support/shared_examples/requests/api/packages_shared_examples.rb index 6f4a0236b66..4bcff505008 100644 --- a/spec/support/shared_examples/requests/api/packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/packages_shared_examples.rb @@ -41,3 +41,32 @@ RSpec.shared_examples 'deploy token for package uploads' do end end end + +RSpec.shared_examples 'does not cause n^2 queries' do + it 'avoids N^2 database queries' do + # we create a package to set the baseline for expected queries from 1 package + create( + :npm_package, + name: "@#{project.root_namespace.path}/my-package", + project: project, + version: "0.0.1" + ) + + control = ActiveRecord::QueryRecorder.new do + get api(url) + end + + 5.times do |n| + create( + :npm_package, + name: "@#{project.root_namespace.path}/my-package", + project: project, + version: "#{n}.0.0" + ) + end + + expect do + get api(url) + end.not_to exceed_query_limit(control) + end +end |