Merge branch...

Merge branch '21033-controller-groups-groupmemberscontroller-index-executes-more-than-100-sql-queries-p80-108-4' into 'master'

Ignore managing_group n+1 false positive on GroupMemberPresenter

See merge request gitlab-org/gitlab!58927

config/bullet.yml

+---
+
+exclusions:
+  # See https://github.com/flyerhzm/bullet#configuration for exclusion formats
+  # Example usage:
+  #
+  # paths with method name(recommended use):
+  #
+  # example_path_with_method_exclusion_name:
+  #   merge_request: 'some merge request link for context'
+  #   exact_file_name: true
+  #   exclude:
+  #     - 'some_ruby_file_name.rb'
+  #     - 'method_name_inside_the_file_above'
+  #
+  # path or pattern only to file(fuzzy, not recommended):
+  #
+  # example_path_with_exact_file_name:
+  #   merge_request: 'some merge request link for context'
+  #   exact_file_name: true
+  #   exclude:
+  #     - 'some_ruby_file_name.rb'
+  #
+  # example_path_with_pattern:
+  #   merge_request: 'some merge request link for context'
+  #   exact_file_name: false
+  #   exclude:
+  #     - 'file_pattern'
+  #
+  # path with line numbers(extremely fragile, not recommended):
+  #
+  # example_path_with_line_range:
+  #   merge_request: 'some merge request link for context'
+  #   exact_file_name: true
+  #   exclude:
+  #     - 'some_ruby_file_name.rb'
+  #     - 5..10
+  #
+  group_member_presenter_managing_group:
+    merge_request: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58927'
+    path_with_method: true
+    exclude:
+      - 'ee/app/presenters/ee/group_member_presenter.rb'
+      - 'group_managed_account?'

config/initializers/bullet.rb

 # frozen_string_literal: true
 
-def bullet_enabled?
-  Gitlab::Utils.to_boolean(ENV['ENABLE_BULLET'].to_s)
-end
-
-if defined?(Bullet) && (bullet_enabled? || Rails.env.development?)
+if Gitlab::Bullet.configure_bullet?
   Rails.application.configure do
     config.after_initialize do
       Bullet.enable = true
 
-      Bullet.bullet_logger = bullet_enabled?
-      Bullet.console = bullet_enabled?
+      if Gitlab::Bullet.extra_logging_enabled?
+        Bullet.bullet_logger = true
+        Bullet.console = true
+      end
 
       Bullet.raise = Rails.env.test?
+
+      Bullet.stacktrace_excludes = Gitlab::Bullet::Exclusions.new.execute
     end
   end
 end

ee/spec/lib/gitlab/bullet/exclusions_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::Bullet::Exclusions do
+  describe '#validate_paths!' do
+    it 'validates paths for existence' do
+      described_class.new.validate_paths!
+    end
+  end
+end

lib/gitlab/bullet.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Bullet
+    extend self
+
+    def enabled?
+      Gitlab::Utils.to_boolean(ENV['ENABLE_BULLET'], default: false)
+    end
+    alias_method :extra_logging_enabled?, :enabled?
+
+    def configure_bullet?
+      defined?(::Bullet) && (enabled? || Rails.env.development?)
+    end
+  end
+end

lib/gitlab/bullet/exclusions.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Bullet
+    class Exclusions
+      def initialize(config_file = Gitlab.root.join('config/bullet.yml'))
+        @config_file = config_file
+      end
+
+      def execute
+        exclusions.map { |v| v['exclude'] }
+      end
+
+      def validate_paths!
+        exclusions.each do |properties|
+          next unless properties['path_with_method']
+
+          file = properties['exclude'].first
+
+          raise "Bullet: File used by #{config_file} doesn't exist, validate the #{file} exclusion!" unless File.exist?(file)
+        end
+      end
+
+      private
+
+      attr_reader :config_file
+
+      def exclusions
+        @exclusions ||= if File.exist?(config_file)
+                          YAML.load_file(config_file)['exclusions']&.values || []
+                        else
+                          []
+                        end
+      end
+    end
+  end
+end

spec/lib/gitlab/bullet/exclusions_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::Bullet::Exclusions do
+  let(:config_file) do
+    file = Tempfile.new('bullet.yml')
+    File.basename(file)
+  end
+
+  let(:exclude) { [] }
+  let(:config) do
+    {
+      exclusions: {
+        abc: {
+          merge_request: '_mr_',
+          path_with_method: true,
+          exclude: exclude
+        }
+      }
+    }
+  end
+
+  before do
+    File.write(config_file, config.deep_stringify_keys.to_yaml)
+  end
+
+  after do
+    FileUtils.rm_f(config_file)
+  end
+
+  describe '#execute' do
+    subject(:executor) { described_class.new(config_file).execute }
+
+    shared_examples_for 'loads exclusion results' do
+      let(:config) { { exclusions: { abc: { exclude: exclude } } } }
+      let(:results) { [exclude] }
+
+      specify do
+        expect(executor).to match(results)
+      end
+    end
+
+    context 'with preferred method of path and method name' do
+      it_behaves_like 'loads exclusion results' do
+        let(:exclude) { %w[_path_ _method_] }
+      end
+    end
+
+    context 'with file pattern' do
+      it_behaves_like 'loads exclusion results' do
+        let(:exclude) { ['_file_pattern_'] }
+      end
+    end
+
+    context 'with file name and line range' do
+      it_behaves_like 'loads exclusion results' do
+        let(:exclude) { ['file_name.rb', 5..10] }
+      end
+    end
+
+    context 'without exclusions' do
+      it_behaves_like 'loads exclusion results' do
+        let(:exclude) { [] }
+      end
+    end
+
+    context 'without exclusions key in config' do
+      it_behaves_like 'loads exclusion results' do
+        let(:config) { {} }
+        let(:results) { [] }
+      end
+    end
+
+    context 'when config file does not exist' do
+      it 'provides an empty array for exclusions' do
+        expect(described_class.new('_some_bogus_file_').execute).to match([])
+      end
+    end
+  end
+
+  describe '#validate_paths!' do
+    context 'when validating scenarios' do
+      let(:source_file) do
+        file = Tempfile.new('bullet_test_source_file.rb')
+        File.basename(file)
+      end
+
+      subject { described_class.new(config_file).validate_paths! }
+
+      before do
+        FileUtils.touch(source_file)
+      end
+
+      after do
+        FileUtils.rm_f(source_file)
+      end
+
+      context 'when using paths with method name' do
+        let(:exclude) { [source_file, '_method_'] }
+
+        context 'when source file for exclusion exists' do
+          specify do
+            expect { subject }.not_to raise_error
+          end
+        end
+
+        context 'when source file for exclusion does not exist' do
+          let(:exclude) { %w[_bogus_file_ _method_] }
+
+          specify do
+            expect { subject }.to raise_error(RuntimeError)
+          end
+        end
+      end
+
+      context 'when using path only' do
+        let(:exclude) { [source_file] }
+
+        context 'when source file for exclusion exists' do
+          specify do
+            expect { subject }.not_to raise_error
+          end
+        end
+
+        context 'when source file for exclusion does not exist' do
+          let(:exclude) { '_bogus_file_' }
+
+          specify do
+            expect { subject }.to raise_error(RuntimeError)
+          end
+        end
+      end
+
+      context 'when path_with_method is false for a file pattern' do
+        let(:exclude) { ['_file_pattern_'] }
+        let(:config) do
+          {
+            exclusions: {
+              abc: {
+                merge_request: '_mr_',
+                path_with_method: false,
+                exclude: exclude
+              }
+            }
+          }
+        end
+
+        specify do
+          expect { subject }.not_to raise_error
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/bullet_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Bullet do
+  describe '#enabled?' do
+    it 'is enabled' do
+      stub_env('ENABLE_BULLET', true)
+
+      expect(described_class.enabled?).to be(true)
+    end
+
+    it 'is not enabled' do
+      stub_env('ENABLE_BULLET', nil)
+
+      expect(described_class.enabled?).to be(false)
+    end
+
+    it 'is correctly aliased for #extra_logging_enabled?' do
+      expect(described_class.method(:extra_logging_enabled?).original_name).to eq(:enabled?)
+    end
+  end
+
+  describe '#configure_bullet?' do
+    context 'with ENABLE_BULLET true' do
+      before do
+        stub_env('ENABLE_BULLET', true)
+      end
+
+      it 'is configurable' do
+        expect(described_class.configure_bullet?).to be(true)
+      end
+    end
+
+    context 'with ENABLE_BULLET falsey' do
+      before do
+        stub_env('ENABLE_BULLET', nil)
+      end
+
+      it 'is not configurable' do
+        expect(described_class.configure_bullet?).to be(false)
+      end
+
+      it 'is configurable in development' do
+        allow(Rails).to receive_message_chain(:env, :development?).and_return(true)
+
+        expect(described_class.configure_bullet?).to be(true)
+      end
+    end
+  end
+end