Merge branch '215191-load-codeowner-entries-from-sectional-codeowners' into 'master'

Enable  #entry_for_path for sectional CODEOWNERS behind feature flag

See merge request gitlab-org/gitlab!31044

ee/lib/gitlab/code_owners/file.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module CodeOwners
     class File
+      include ::Gitlab::Utils::StrongMemoize
+
       SECTION_HEADER_REGEX = /\[(.*?)\]/.freeze
 
       def initialize(blob, project = nil)
@@ -21,11 +23,11 @@ module Gitlab
       def entry_for_path(path)
         path = "/#{path}" unless path.start_with?('/')
 
-        matching_pattern = parsed_data.keys.reverse.detect do |pattern|
-          path_matches?(pattern, path)
+        if sectional_codeowners?
+          sectional_entry_for_path(path)
+        else
+          non_sectional_entry_for_path(path)
         end
-
-        parsed_data[matching_pattern].dup if matching_pattern
       end
 
       def path
@@ -34,6 +36,28 @@ module Gitlab
 
       private
 
+      def non_sectional_entry_for_path(path)
+        matching_pattern = parsed_data.keys.reverse.detect do |pattern|
+          path_matches?(pattern, path)
+        end
+
+        matching_pattern ? [parsed_data[matching_pattern].dup] : []
+      end
+
+      def sectional_entry_for_path(path)
+        matches = []
+
+        parsed_data.each do |_, section_entries|
+          matching_pattern = section_entries.keys.reverse.detect do |pattern|
+            path_matches?(pattern, path)
+          end
+
+          matches << section_entries[matching_pattern].dup if matching_pattern
+        end
+
+        matches
+      end
+
       def data
         if @blob && !@blob.binary?
           @blob.data
@@ -43,10 +67,14 @@ module Gitlab
       end
 
       def get_parsed_data
-        if Feature.enabled?(:sectional_codeowners, @project, default_enabled: false)
-          return get_parsed_sectional_data
+        if sectional_codeowners?
+          get_parsed_sectional_data
+        else
+          get_parsed_non_sectional_data
         end
+      end
 
+      def get_parsed_non_sectional_data
         parsed = {}
 
         data.lines.each do |line|
@@ -140,6 +168,12 @@ module Gitlab
 
         ::File.fnmatch?(pattern, path, flags)
       end
+
+      def sectional_codeowners?
+        strong_memoize(:sectional_codeowners_check) do
+          Feature.enabled?(:sectional_codeowners, @project, default_enabled: false)
+        end
+      end
     end
   end
 end

ee/lib/gitlab/code_owners/loader.rb

@@ -48,7 +48,7 @@ module Gitlab
           code_owners_file.entry_for_path(path)
         end
 
-        entries.compact.uniq
+        entries.flatten.compact.uniq
       end
 
       def code_owners_file

ee/spec/fixtures/mixed_case_sectional_codeowners_example

-ee/ @gl-admin
+# This is an example code owners file, lines starting with a `#` will
+# be ignored.
+
+# app/ @commented-rule
+
+# We can specify a default match using wildcards:
+* @default-codeowner
+
+# Rules defined later in the file take precedence over the rules
+# defined before.
+# This will match all files for which the file name ends in `.rb`
+*.rb @ruby-owner
+
+# Files with a `#` can still be accesssed by escaping the pound sign
+\#file_with_pound.rb @owner-file-with-pound
+
+# Multiple codeowners can be specified, separated by whitespace
+CODEOWNERS @multiple @owners	@tab-separated
+
+# Both usernames or email addresses can be used to match
+# users. Everything else will be ignored. For example this will
+# specify `@legal` and a user with email `janedoe@gitlab.com` as the
+# owner for the LICENSE file
+LICENSE @legal this does not match janedoe@gitlab.com
+
+# Ending a path in a `/` will specify the code owners for every file
+# nested in that directory, on any level
+/docs/ @all-docs
+
+# Ending a path in `/*` will specify code owners for every file in
+# that directory, but not nested deeper. This will match
+# `docs/index.md` but not `docs/projects/index.md`
+/docs/* @root-docs
+
+# This will make a `lib` directory nested anywhere in the repository
+# match
+lib/ @lib-owner
+
+# This will only match a `config` directory in the root of the
+# repository
+/config/ @config-owner
+
+# If the path contains spaces, these need to be escaped like this:
+path\ with\ spaces/ @space-owner
 
 [Documentation]
 ee/docs    @gl-docs

ee/spec/fixtures/sectional_codeowners_example

-ee/ @gl-admin
+# This is an example code owners file, lines starting with a `#` will
+# be ignored.
+
+# app/ @commented-rule
+
+# We can specify a default match using wildcards:
+* @default-codeowner
+
+# Rules defined later in the file take precedence over the rules
+# defined before.
+# This will match all files for which the file name ends in `.rb`
+*.rb @ruby-owner
+
+# Files with a `#` can still be accesssed by escaping the pound sign
+\#file_with_pound.rb @owner-file-with-pound
+
+# Multiple codeowners can be specified, separated by whitespace
+CODEOWNERS @multiple @owners	@tab-separated
+
+# Both usernames or email addresses can be used to match
+# users. Everything else will be ignored. For example this will
+# specify `@legal` and a user with email `janedoe@gitlab.com` as the
+# owner for the LICENSE file
+LICENSE @legal this does not match janedoe@gitlab.com
+
+# Ending a path in a `/` will specify the code owners for every file
+# nested in that directory, on any level
+/docs/ @all-docs
+
+# Ending a path in `/*` will specify code owners for every file in
+# that directory, but not nested deeper. This will match
+# `docs/index.md` but not `docs/projects/index.md`
+/docs/* @root-docs
+
+# This will make a `lib` directory nested anywhere in the repository
+# match
+lib/ @lib-owner
+
+# This will only match a `config` directory in the root of the
+# repository
+/config/ @config-owner
+
+# If the path contains spaces, these need to be escaped like this:
+path\ with\ spaces/ @space-owner
 
 [Documentation]
 ee/docs    @gl-docs

ee/spec/lib/gitlab/code_owners/file_spec.rb

@@ -52,8 +52,21 @@ describe Gitlab::CodeOwners::File do
           expect(data.keys).to contain_exactly("codeowners", "Documentation", "Database")
         end
 
+        codeowners_section_paths = [
+          "/**/#file_with_pound.rb", "/**/*", "/**/*.rb", "/**/CODEOWNERS",
+          "/**/LICENSE", "/**/lib/**/*", "/**/path with spaces/**/*",
+          "/config/**/*", "/docs/*", "/docs/**/*"
+        ]
+
+        codeowners_section_owners = [
+          "@all-docs", "@config-owner", "@default-codeowner",
+          "@legal this does not match janedoe@gitlab.com", "@lib-owner",
+          "@multiple @owners\t@tab-separated", "@owner-file-with-pound",
+          "@root-docs", "@ruby-owner", "@space-owner"
+        ]
+
         where(:section, :patterns, :owners) do
-          "codeowners"    | ["/**/ee/**/*"] | ["@gl-admin"]
+          "codeowners"    | codeowners_section_paths | codeowners_section_owners
           "Documentation" | ["/**/README.md", "/**/ee/docs", "/**/docs"] | ["@gl-docs"]
           "Database"      | ["/**/README.md", "/**/model/db"] | ["@gl-database"]
         end
@@ -143,137 +156,176 @@ describe Gitlab::CodeOwners::File do
   end
 
   describe '#entry_for_path' do
-    context 'for a path without matches' do
-      let(:file_content) do
-        <<~CONTENT
-        # Simulating a CODOWNERS without entries
-        CONTENT
+    shared_examples_for "returns expected matches" do
+      context 'for a path without matches' do
+        let(:file_content) do
+          <<~CONTENT
+          # Simulating a CODOWNERS without entries
+          CONTENT
+        end
+
+        it 'returns an empty array for an unmatched path' do
+          entry = file.entry_for_path('no_matches')
+
+          expect(entry).to be_a Array
+          expect(entry).to be_empty
+        end
       end
 
-      it 'returns an nil for an unmatched path' do
-        entry = file.entry_for_path('no_matches')
+      it 'matches random files to a pattern' do
+        entry = file.entry_for_path('app/assets/something.vue').first
 
-        expect(entry).to be_nil
+        expect(entry.pattern).to eq('*')
+        expect(entry.owner_line).to include('default-codeowner')
       end
-    end
 
-    it 'matches random files to a pattern' do
-      entry = file.entry_for_path('app/assets/something.vue')
+      it 'uses the last pattern if multiple patterns match' do
+        entry = file.entry_for_path('hello.rb').first
 
-      expect(entry.pattern).to eq('*')
-      expect(entry.owner_line).to include('default-codeowner')
-    end
+        expect(entry.pattern).to eq('*.rb')
+        expect(entry.owner_line).to eq('@ruby-owner')
+      end
 
-    it 'uses the last pattern if multiple patterns match' do
-      entry = file.entry_for_path('hello.rb')
+      it 'returns the usernames for a file matching a pattern with a glob' do
+        entry = file.entry_for_path('app/models/repository.rb').first
 
-      expect(entry.pattern).to eq('*.rb')
-      expect(entry.owner_line).to eq('@ruby-owner')
-    end
+        expect(entry.owner_line).to eq('@ruby-owner')
+      end
 
-    it 'returns the usernames for a file matching a pattern with a glob' do
-      entry = file.entry_for_path('app/models/repository.rb')
+      it 'allows specifying multiple users' do
+        entry = file.entry_for_path('CODEOWNERS').first
 
-      expect(entry.owner_line).to eq('@ruby-owner')
-    end
+        expect(entry.owner_line).to include('multiple', 'owners', 'tab-separated')
+      end
 
-    it 'allows specifying multiple users' do
-      entry = file.entry_for_path('CODEOWNERS')
+      it 'returns emails and usernames for a matched pattern' do
+        entry = file.entry_for_path('LICENSE').first
 
-      expect(entry.owner_line).to include('multiple', 'owners', 'tab-separated')
-    end
+        expect(entry.owner_line).to include('legal', 'janedoe@gitlab.com')
+      end
 
-    it 'returns emails and usernames for a matched pattern' do
-      entry = file.entry_for_path('LICENSE')
+      it 'allows escaping the pound sign used for comments' do
+        entry = file.entry_for_path('examples/#file_with_pound.rb').first
 
-      expect(entry.owner_line).to include('legal', 'janedoe@gitlab.com')
-    end
+        expect(entry.owner_line).to include('owner-file-with-pound')
+      end
 
-    it 'allows escaping the pound sign used for comments' do
-      entry = file.entry_for_path('examples/#file_with_pound.rb')
+      it 'returns the usernames for a file nested in a directory' do
+        entry = file.entry_for_path('docs/projects/index.md').first
 
-      expect(entry.owner_line).to include('owner-file-with-pound')
-    end
+        expect(entry.owner_line).to include('all-docs')
+      end
 
-    it 'returns the usernames for a file nested in a directory' do
-      entry = file.entry_for_path('docs/projects/index.md')
+      it 'returns the usernames for a pattern matched with a glob in a folder' do
+        entry = file.entry_for_path('docs/index.md').first
 
-      expect(entry.owner_line).to include('all-docs')
-    end
+        expect(entry.owner_line).to include('root-docs')
+      end
 
-    it 'returns the usernames for a pattern matched with a glob in a folder' do
-      entry = file.entry_for_path('docs/index.md')
+      it 'allows matching files nested anywhere in the repository', :aggregate_failures do
+        lib_entry = file.entry_for_path('lib/gitlab/git/repository.rb').first
+        other_lib_entry = file.entry_for_path('ee/lib/gitlab/git/repository.rb').first
 
-      expect(entry.owner_line).to include('root-docs')
-    end
+        expect(lib_entry.owner_line).to include('lib-owner')
+        expect(other_lib_entry.owner_line).to include('lib-owner')
+      end
 
-    it 'allows matching files nested anywhere in the repository', :aggregate_failures do
-      lib_entry = file.entry_for_path('lib/gitlab/git/repository.rb')
-      other_lib_entry = file.entry_for_path('ee/lib/gitlab/git/repository.rb')
+      it 'allows allows limiting the matching files to the root of the repository', :aggregate_failures do
+        config_entry = file.entry_for_path('config/database.yml').first
+        other_config_entry = file.entry_for_path('other/config/database.yml').first
 
-      expect(lib_entry.owner_line).to include('lib-owner')
-      expect(other_lib_entry.owner_line).to include('lib-owner')
-    end
+        expect(config_entry.owner_line).to include('config-owner')
+        expect(other_config_entry.owner_line).to eq('@default-codeowner')
+      end
 
-    it 'allows allows limiting the matching files to the root of the repository', :aggregate_failures do
-      config_entry = file.entry_for_path('config/database.yml')
-      other_config_entry = file.entry_for_path('other/config/database.yml')
+      it 'correctly matches paths with spaces' do
+        entry = file.entry_for_path('path with spaces/docs.md').first
 
-      expect(config_entry.owner_line).to include('config-owner')
-      expect(other_config_entry.owner_line).to eq('@default-codeowner')
-    end
+        expect(entry.owner_line).to eq('@space-owner')
+      end
 
-    it 'correctly matches paths with spaces' do
-      entry = file.entry_for_path('path with spaces/README.md')
+      context 'paths with whitespaces and username lookalikes' do
+        let(:file_content) do
+          'a/weird\ path\ with/\ @username\ /\ and-email@lookalikes.com\ / @user-1 email@gitlab.org @user-2'
+        end
 
-      expect(entry.owner_line).to eq('@space-owner')
-    end
+        it 'parses correctly' do
+          entry = file.entry_for_path('a/weird path with/ @username / and-email@lookalikes.com /test.rb').first
 
-    context 'paths with whitespaces and username lookalikes' do
-      let(:file_content) do
-        'a/weird\ path\ with/\ @username\ /\ and-email@lookalikes.com\ / @user-1 email@gitlab.org @user-2'
+          expect(entry.owner_line).to include('user-1', 'user-2', 'email@gitlab.org')
+          expect(entry.owner_line).not_to include('username', 'and-email@lookalikes.com')
+        end
       end
 
-      it 'parses correctly' do
-        entry = file.entry_for_path('a/weird path with/ @username / and-email@lookalikes.com /test.rb')
+      context 'a glob on the root directory' do
+        let(:file_content) do
+          '/* @user-1 @user-2'
+        end
+
+        it 'matches files in the root directory' do
+          entry = file.entry_for_path('README.md').first
+
+          expect(entry.owner_line).to include('user-1', 'user-2')
+        end
+
+        it 'does not match nested files' do
+          entry = file.entry_for_path('nested/path/README.md').first
+
+          expect(entry).to be_nil
+        end
 
-        expect(entry.owner_line).to include('user-1', 'user-2', 'email@gitlab.org')
-        expect(entry.owner_line).not_to include('username', 'and-email@lookalikes.com')
+        context 'partial matches' do
+          let(:file_content) do
+            'foo/* @user-1 @user-2'
+          end
+
+          it 'does not match a file in a folder that looks the same' do
+            entry = file.entry_for_path('fufoo/bar').first
+
+            expect(entry).to be_nil
+          end
+
+          it 'matches the file in any folder' do
+            expect(file.entry_for_path('baz/foo/bar').first.owner_line).to include('user-1', 'user-2')
+            expect(file.entry_for_path('/foo/bar').first.owner_line).to include('user-1', 'user-2')
+          end
+        end
       end
     end
 
-    context 'a glob on the root directory' do
-      let(:file_content) do
-        '/* @user-1 @user-2'
+    context "when feature flag `:sectional_codeowners` is enabled" do
+      before do
+        stub_feature_flags(sectional_codeowners: true)
       end
 
-      it 'matches files in the root directory' do
-        entry = file.entry_for_path('README.md')
-
-        expect(entry.owner_line).to include('user-1', 'user-2')
+      context "when CODEOWNERS file contains no sections" do
+        it_behaves_like "returns expected matches"
       end
 
-      it 'does not match nested files' do
-        entry = file.entry_for_path('nested/path/README.md')
+      context "when CODEOWNERS file contains multiple sections" do
+        let(:file_content) do
+          File.read(Rails.root.join("ee", "spec", "fixtures", "sectional_codeowners_example"))
+        end
 
-        expect(entry).to be_nil
+        it_behaves_like "returns expected matches"
       end
     end
 
-    context 'partial matches' do
-      let(:file_content) do
-        'foo/* @user-1 @user-2'
+    context "when feature flag `:sectional_codeowners` is disabled" do
+      before do
+        stub_feature_flags(sectional_codeowners: false)
       end
 
-      it 'does not match a file in a folder that looks the same' do
-        entry = file.entry_for_path('fufoo/bar')
-
-        expect(entry).to be_nil
+      context "when CODEOWNERS file contains no sections" do
+        it_behaves_like "returns expected matches"
       end
 
-      it 'matches the file in any folder' do
-        expect(file.entry_for_path('baz/foo/bar').owner_line).to include('user-1', 'user-2')
-        expect(file.entry_for_path('/foo/bar').owner_line).to include('user-1', 'user-2')
+      context "when CODEOWNERS file contains multiple sections" do
+        let(:file_content) do
+          File.read(Rails.root.join("ee", "spec", "fixtures", "sectional_codeowners_example"))
+        end
+
+        it_behaves_like "returns expected matches"
       end
     end
   end