diff options
18 files changed, 295 insertions, 121 deletions
diff --git a/app/experiments/application_experiment.rb b/app/experiments/application_experiment.rb index 2dabf33405d..ec6762096ad 100644 --- a/app/experiments/application_experiment.rb +++ b/app/experiments/application_experiment.rb @@ -1,14 +1,6 @@ # frozen_string_literal: true class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass - def enabled? - return false if Feature::Definition.get(feature_flag_name).nil? # there has to be a feature flag yaml file - return false unless Gitlab.dev_env_or_com? # we have to be in an environment that allows experiments - - # the feature flag has to be rolled out - Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet - end - def publish(_result = nil) super @@ -72,12 +64,4 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp actor = context.try(:actor) actor.respond_to?(:id) ? actor : context.try(:user) end - - def feature_flag_name - name.tr('/', '_') - end - - def experiment_group? - Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml) - end end diff --git a/app/models/ci/namespace_mirror.rb b/app/models/ci/namespace_mirror.rb index 2b98aa7aa18..d5cbbb96134 100644 --- a/app/models/ci/namespace_mirror.rb +++ b/app/models/ci/namespace_mirror.rb @@ -15,9 +15,6 @@ module Ci end scope :by_namespace_id, -> (namespace_id) { where(namespace_id: namespace_id) } - scope :namespace_id_from_traversal_ids, -> do - select('ci_namespace_mirrors.traversal_ids[array_length(ci_namespace_mirrors.traversal_ids, 1)] AS namespace_id') - end class << self def sync!(event) diff --git a/app/models/preloaders/users_max_access_level_in_projects_preloader.rb b/app/models/preloaders/users_max_access_level_in_projects_preloader.rb new file mode 100644 index 00000000000..b4ce61a869c --- /dev/null +++ b/app/models/preloaders/users_max_access_level_in_projects_preloader.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Preloaders + # This class preloads the max access level (role) for the users within the given projects and + # stores the values in requests store via the ProjectTeam class. + class UsersMaxAccessLevelInProjectsPreloader + def initialize(projects:, users:) + @projects = projects + @users = users + end + + def execute + return unless @projects.present? && @users.present? + + access_levels.each do |(project_id, user_id), access_level| + project = projects_by_id[project_id] + + project.team.write_member_access_for_user_id(user_id, access_level) + end + end + + private + + def access_levels + ProjectAuthorization + .where(project_id: project_ids, user_id: user_ids) + .group(:project_id, :user_id) + .maximum(:access_level) + end + + # Use reselect to override the existing select to prevent + # the error `subquery has too many columns` + # NotificationsController passes in an Array so we need to check the type + def project_ids + @projects.is_a?(ActiveRecord::Relation) ? @projects.reselect(:id) : @projects + end + + def user_ids + @users.is_a?(ActiveRecord::Relation) ? @users.reselect(:id) : @users + end + + def projects_by_id + @projects_by_id ||= @projects.index_by(&:id) + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index f73eb04dc32..313c1726429 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2230,32 +2230,20 @@ class User < ApplicationRecord end def ci_owned_project_runners_from_group_members - cte_project_ids = Gitlab::SQL::CTE.new( - :cte_project_ids, - Ci::ProjectMirror - .select(:project_id) - .joins('JOIN ci_namespace_mirrors ON ci_namespace_mirrors.traversal_ids[array_length(ci_namespace_mirrors.traversal_ids, 1)] = ci_project_mirrors.namespace_id') - .merge(ci_namespace_mirrors_for_group_members(Gitlab::Access::MAINTAINER)) - ) - - Ci::Runner + Ci::RunnerProject .select('ci_runners.*') - .joins(:runner_projects) - .where('ci_runner_projects.project_id IN (SELECT project_id FROM cte_project_ids)') - .with(cte_project_ids.to_arel) + .joins(:runner) + .joins('JOIN ci_project_mirrors ON ci_project_mirrors.project_id = ci_runner_projects.project_id') + .joins('JOIN ci_namespace_mirrors ON ci_namespace_mirrors.namespace_id = ci_project_mirrors.namespace_id') + .merge(ci_namespace_mirrors_for_group_members(Gitlab::Access::MAINTAINER)) end def ci_owned_group_runners - cte_namespace_ids = Gitlab::SQL::CTE.new( - :cte_namespace_ids, - ci_namespace_mirrors_for_group_members(Gitlab::Access::OWNER).namespace_id_from_traversal_ids - ) - - Ci::Runner + Ci::RunnerNamespace .select('ci_runners.*') - .joins(:runner_namespaces) - .where('ci_runner_namespaces.namespace_id IN (SELECT namespace_id FROM cte_namespace_ids)') - .with(cte_namespace_ids.to_arel) + .joins(:runner) + .joins('JOIN ci_namespace_mirrors ON ci_namespace_mirrors.namespace_id = ci_runner_namespaces.namespace_id') + .merge(ci_namespace_mirrors_for_group_members(Gitlab::Access::OWNER)) end def ci_namespace_mirrors_for_group_members(level) diff --git a/config/initializers/gitlab_experiment.rb b/config/initializers/gitlab_experiment.rb index 6b027011d10..3d4077c858d 100644 --- a/config/initializers/gitlab_experiment.rb +++ b/config/initializers/gitlab_experiment.rb @@ -23,9 +23,7 @@ Gitlab::Experiment.configure do |config| # Customize the logic of our default rollout, which shouldn't include # assigning the control yet -- we specifically set it to false for now. # - config.default_rollout = Gitlab::Experiment::Rollout::Percent.new( - include_control: false - ) + config.default_rollout = Gitlab::Experiment::Rollout::Feature.new # Mount the engine and middleware at a gitlab friendly style path. # diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 28b7a67ad34..3bbaef72fea 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -9675,6 +9675,7 @@ Represents a DAST profile schedule. | <a id="dastprofileschedulecadence"></a>`cadence` | [`DastProfileCadence`](#dastprofilecadence) | Cadence of the DAST profile schedule. | | <a id="dastprofilescheduleid"></a>`id` | [`DastProfileScheduleID!`](#dastprofilescheduleid) | ID of the DAST profile schedule. | | <a id="dastprofileschedulenextrunat"></a>`nextRunAt` | [`Time`](#time) | Next run time of the DAST profile schedule in the given timezone. | +| <a id="dastprofilescheduleownervalid"></a>`ownerValid` | [`Boolean`](#boolean) | Status of the current owner of the DAST profile schedule. | | <a id="dastprofileschedulestartsat"></a>`startsAt` | [`Time`](#time) | Start time of the DAST profile schedule in the given timezone. | | <a id="dastprofilescheduletimezone"></a>`timezone` | [`String`](#string) | Time zone of the start time of the DAST profile schedule. | diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 3b9bdda50a3..8cee567ae93 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -159,6 +159,8 @@ Imported users can be mapped by their public email addresses on self-managed ins for mapping to work correctly. - For contributions to be mapped correctly, users must be an existing member of the namespace, or they can be added as a member of the project. Otherwise, a supplementary comment is left to mention that the original author and the MRs, notes, or issues that are owned by the importer. +- Imported users are set as [direct members](../members/index.md) + in the imported project. For project migration imports performed over GitLab.com groups, preserving author information is possible through a [professional services engagement](https://about.gitlab.com/services/migration/). diff --git a/lib/gitlab/experiment/rollout/feature.rb b/lib/gitlab/experiment/rollout/feature.rb new file mode 100644 index 00000000000..5a14e3c272e --- /dev/null +++ b/lib/gitlab/experiment/rollout/feature.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Gitlab + class Experiment + module Rollout + class Feature < Percent + # For this rollout strategy to consider an experiment as enabled, we + # must: + # + # - have a feature flag yaml file that declares it. + # - be in an environment that permits it. + # - not have rolled out the feature flag at all (no percent of actors, + # no inclusions, etc.) + def enabled? + return false if ::Feature::Definition.get(feature_flag_name).nil? + return false unless Gitlab.dev_env_or_com? + + ::Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet + end + + # For assignment we first check to see if our feature flag is enabled + # for "self". This is done by calling `#flipper_id` (used behind the + # scenes by `Feature`). By default this is our `experiment.id` (or more + # specifically, the context key, which is an anonymous SHA generated + # using the details of an experiment. + # + # If the `Feature.enabled?` check is false, we return nil implicitly, + # which will assign the control. Otherwise we call super, which will + # assign a variant evenly, or based on our provided distribution rules. + def execute_assigment + super if ::Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml) + end + + # NOTE: There's a typo in the name of this method that we'll fix up. + alias_method :execute_assignment, :execute_assigment + + # This is what's provided to the `Feature.enabled?` call that will be + # used to determine experiment inclusion. An experiment may provide an + # override for this method to make the experiment work on user, group, + # or projects. + # + # For example, when running an experiment on a project, you could make + # the experiment assignable by project (using chatops) by implementing + # a `flipper_id` method in the experiment: + # + # def flipper_id + # context.project.flipper_id + # end + # + # Or even cleaner, simply delegate it: + # + # delegate :flipper_id, to: -> { context.project } + def flipper_id + return experiment.flipper_id if experiment.respond_to?(:flipper_id) + + "Experiment;#{id}" + end + + private + + def feature_flag_name + experiment.name.tr('/', '_') + end + end + end + end +end diff --git a/lib/gitlab/graphql/project/dast_profile_connection_extension.rb b/lib/gitlab/graphql/project/dast_profile_connection_extension.rb new file mode 100644 index 00000000000..a3c3f2f2b7e --- /dev/null +++ b/lib/gitlab/graphql/project/dast_profile_connection_extension.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true +module Gitlab + module Graphql + module Project + class DastProfileConnectionExtension < GraphQL::Schema::Field::ConnectionExtension + def after_resolve(value:, object:, context:, **rest) + preload_authorizations(context[:project_dast_profiles]) + context[:project_dast_profiles] = nil + value + end + + def preload_authorizations(dast_profiles) + return unless dast_profiles + + projects = dast_profiles.map(&:project) + users = dast_profiles.filter_map { |dast_profile| dast_profile.dast_profile_schedule&.owner } + Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute + end + end + end + end +end diff --git a/lib/gitlab/usage/metrics/names_suggestions/generator.rb b/lib/gitlab/usage/metrics/names_suggestions/generator.rb index d1a7bb65cc3..626bd3d4ad4 100644 --- a/lib/gitlab/usage/metrics/names_suggestions/generator.rb +++ b/lib/gitlab/usage/metrics/names_suggestions/generator.rb @@ -7,7 +7,7 @@ module Gitlab class Generator < ::Gitlab::UsageData class << self def generate(key_path) - uncached_data.deep_stringify_keys.dig(*key_path.split('.')) + data.deep_stringify_keys.dig(*key_path.split('.')) end def add_metric(metric, time_frame: 'none', options: {}) diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 5af35b1f206..e66a565246b 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -42,7 +42,11 @@ module Gitlab include Gitlab::Usage::TimeFrame def data - uncached_data + clear_memoized + + with_finished_at(:recording_ce_finished_at) do + usage_data_metrics + end end def license_usage_data @@ -683,14 +687,6 @@ module Gitlab private - def uncached_data - clear_memoized - - with_finished_at(:recording_ce_finished_at) do - usage_data_metrics - end - end - def stage_manage_events(time_period) if time_period.empty? Gitlab::Utils::UsageData::FALLBACK diff --git a/lib/gitlab/usage_data_counters/known_events/common.yml b/lib/gitlab/usage_data_counters/known_events/common.yml index fc610f1e2d6..ff3e8f90e23 100644 --- a/lib/gitlab/usage_data_counters/known_events/common.yml +++ b/lib/gitlab/usage_data_counters/known_events/common.yml @@ -383,3 +383,8 @@ redis_slot: geo aggregation: daily feature_flag: track_geo_proxy_events +# Growth +- name: users_clicking_registration_features_offer + category: growth + redis_slot: users + aggregation: weekly diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb index 8e06c06a5d1..896cb1e5d55 100644 --- a/spec/experiments/application_experiment_spec.rb +++ b/spec/experiments/application_experiment_spec.rb @@ -24,38 +24,6 @@ RSpec.describe ApplicationExperiment, :experiment do expect { experiment('namespaced/stub') { } }.not_to raise_error end - describe "#enabled?" do - before do - allow(application_experiment).to receive(:enabled?).and_call_original - - allow(Feature::Definition).to receive(:get).and_return('_instance_') - allow(Gitlab).to receive(:dev_env_or_com?).and_return(true) - allow(Feature).to receive(:get).and_return(double(state: :on)) - end - - it "is enabled when all criteria are met" do - expect(application_experiment).to be_enabled - end - - it "isn't enabled if the feature definition doesn't exist" do - expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil) - - expect(application_experiment).not_to be_enabled - end - - it "isn't enabled if we're not in dev or dotcom environments" do - expect(Gitlab).to receive(:dev_env_or_com?).and_return(false) - - expect(application_experiment).not_to be_enabled - end - - it "isn't enabled if the feature flag state is :off" do - expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off)) - - expect(application_experiment).not_to be_enabled - end - end - describe "#publish" do let(:should_track) { true } @@ -214,26 +182,6 @@ RSpec.describe ApplicationExperiment, :experiment do ) end - it "tracks the event correctly even when using the base class" do - subject = Gitlab::Experiment.new(:unnamed) - subject.track(:action, context: [fake_context]) - - expect_snowplow_event( - category: 'unnamed', - action: 'action', - context: [ - { - schema: 'iglu:com.gitlab/fake/jsonschema/0-0-0', - data: { data: '_data_' } - }, - { - schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0', - data: { experiment: 'unnamed', key: subject.context.key, variant: 'control' } - } - ] - ) - end - context "when using known context resources" do let(:user) { build(:user, id: non_existing_record_id) } let(:project) { build(:project, id: non_existing_record_id) } @@ -347,23 +295,15 @@ RSpec.describe ApplicationExperiment, :experiment do end context "when resolving variants" do - it "uses the default value as specified in the yaml" do - expect(Feature).to receive(:enabled?).with('namespaced_stub', application_experiment, type: :experiment, default_enabled: :yaml) - - expect(application_experiment.variant.name).to eq('control') + before do + stub_feature_flags(namespaced_stub: true) end - context "when rolled out to 100%" do - before do - stub_feature_flags(namespaced_stub: true) - end + it "returns an assigned name" do + application_experiment.variant(:variant1) {} + application_experiment.variant(:variant2) {} - it "returns an assigned name" do - application_experiment.variant(:variant1) {} - application_experiment.variant(:variant2) {} - - expect(application_experiment.variant.name).to eq('variant2') - end + expect(application_experiment.assigned.name).to eq('variant2') end end diff --git a/spec/lib/gitlab/experiment/rollout/feature_spec.rb b/spec/lib/gitlab/experiment/rollout/feature_spec.rb new file mode 100644 index 00000000000..d73757be79b --- /dev/null +++ b/spec/lib/gitlab/experiment/rollout/feature_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Experiment::Rollout::Feature, :experiment do + subject { described_class.new.for(subject_experiment) } + + let(:subject_experiment) { experiment('namespaced/stub') } + + describe "#enabled?" do + before do + allow(Feature::Definition).to receive(:get).and_return('_instance_') + allow(Gitlab).to receive(:dev_env_or_com?).and_return(true) + allow(Feature).to receive(:get).and_return(double(state: :on)) + end + + it "is enabled when all criteria are met" do + expect(subject).to be_enabled + end + + it "isn't enabled if the feature definition doesn't exist" do + expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil) + + expect(subject).not_to be_enabled + end + + it "isn't enabled if we're not in dev or dotcom environments" do + expect(Gitlab).to receive(:dev_env_or_com?).and_return(false) + + expect(subject).not_to be_enabled + end + + it "isn't enabled if the feature flag state is :off" do + expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off)) + + expect(subject).not_to be_enabled + end + end + + describe "#execute_assignment" do + before do + allow(Feature).to receive(:enabled?).with('namespaced_stub', any_args).and_return(true) + end + + it "uses the default value as specified in the yaml" do + expect(Feature).to receive(:enabled?).with( + 'namespaced_stub', + subject, + type: :experiment, + default_enabled: :yaml + ).and_return(false) + + expect(subject.execute_assignment).to be_nil + end + + it "returns an assigned name" do + allow(subject).to receive(:behavior_names).and_return([:variant1, :variant2]) + + expect(subject.execute_assignment).to eq(:variant2) + end + end + + describe "#flipper_id" do + it "returns the expected flipper id if the experiment doesn't provide one" do + subject.instance_variable_set(:@experiment, double(id: '__id__')) + expect(subject.flipper_id).to eq('Experiment;__id__') + end + + it "lets the experiment provide a flipper id so it can override the default" do + allow(subject_experiment).to receive(:flipper_id).and_return('_my_overridden_id_') + + expect(subject.flipper_id).to eq('_my_overridden_id_') + end + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index f4a112d35aa..cf8c26e46ff 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -529,6 +529,7 @@ project: - vulnerability_feedback - vulnerability_identifiers - vulnerability_scanners +- dast_profiles - dast_site_profiles - dast_scanner_profiles - dast_sites diff --git a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb index f7ff68af8a2..5e74ea3293c 100644 --- a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb +++ b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb @@ -49,7 +49,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s 'secure', 'importer', 'network_policies', - 'geo' + 'geo', + 'growth' ) end end diff --git a/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb b/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb new file mode 100644 index 00000000000..7ecb6bb9861 --- /dev/null +++ b/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +RSpec.describe Preloaders::UsersMaxAccessLevelInProjectsPreloader do + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } + + let_it_be(:project_1) { create(:project) } + let_it_be(:project_2) { create(:project) } + let_it_be(:project_3) { create(:project) } + + let(:projects) { [project_1, project_2, project_3] } + let(:users) { [user1, user2] } + + before do + project_1.add_developer(user1) + project_1.add_developer(user2) + + project_2.add_developer(user1) + project_2.add_developer(user2) + + project_3.add_developer(user1) + project_3.add_developer(user2) + end + + context 'preload maximum access level to avoid querying project_authorizations', :request_store do + it 'avoids N+1 queries', :request_store do + Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute + + expect(count_queries).to eq(0) + end + + it 'runs N queries without preloading' do + query_count_without_preload = count_queries + + Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute + count_queries_with_preload = count_queries + + expect(count_queries_with_preload).to be < query_count_without_preload + end + end + + def count_queries + ActiveRecord::QueryRecorder.new do + projects.each do |project| + user1.can?(:read_project, project) + user2.can?(:read_project, project) + end + end.count + end +end diff --git a/spec/support/import_export/import_export.yml b/spec/support/import_export/import_export.yml index 116bc8d0b9c..fa10e1335c5 100644 --- a/spec/support/import_export/import_export.yml +++ b/spec/support/import_export/import_export.yml @@ -28,4 +28,4 @@ excluded_attributes: - :iid project: - :id - - :created_at
\ No newline at end of file + - :created_at |