Fix regex chars in dir name (#91)

* add file to expanded cache if it exists

* extracting OwnerAssigner for testing

* fixing flake

* commenting on the need for adding a file without a glob match

Co-authored-by: Joey Schoblaska <[email protected]>

* bumping version

Co-authored-by: Perry Hertler <[email protected]>

* ignoring Gemfile.lock

* ignoring Gemfile.lock

---------

Co-authored-by: Joey Schoblaska <[email protected]>

Author: perryqh

.gitignore

@@ -7,5 +7,7 @@
 /spec/reports/
 /tmp/
 
+Gemfile.lock
+
 # rspec failure tracking
 .rspec_status

Gemfile.lock

@@ -1,6 +1,6 @@
 Gem::Specification.new do |spec|
   spec.name          = 'code_ownership'
-  spec.version       = '1.36.1'
+  spec.version       = '1.36.2'
   spec.authors       = ['Gusto Engineers']
   spec.email         = ['[email protected]']
   spec.summary       = 'A gem to help engineering teams declare ownership of code'

code_ownership.gemspec

@@ -8,6 +8,7 @@
 require 'code_ownership/private/codeowners_file'
 require 'code_ownership/private/parse_js_packages'
 require 'code_ownership/private/glob_cache'
+require 'code_ownership/private/owner_assigner'
 require 'code_ownership/private/validations/files_have_owners'
 require 'code_ownership/private/validations/github_codeowners_up_to_date'
 require 'code_ownership/private/validations/files_have_unique_owners'
@@ -98,7 +99,6 @@ def self.file_tracked?(file)
       in_unowned_globs = configuration.unowned_globs.any? do |unowned_glob|
         File.fnmatch?(unowned_glob, file, File::FNM_PATHNAME | File::FNM_EXTGLOB)
       end
-
       in_owned_globs && !in_unowned_globs && File.exist?(file)
     end
 

lib/code_ownership/private.rb

@@ -53,14 +53,8 @@ def expanded_cache
         @expanded_cache ||= begin
           expanded_cache = {}
           @raw_cache_contents.each do |mapper_description, globs_by_owner|
-            expanded_cache[mapper_description] = {}
-            globs_by_owner.each do |glob, owner|
-              Dir.glob(glob).each do |file, owner|
-                expanded_cache[mapper_description][file] = owner
-              end
-            end
+            expanded_cache[mapper_description] = OwnerAssigner.assign_owners(globs_by_owner)
           end
-          
           expanded_cache
         end
       end

lib/code_ownership/private/glob_cache.rb

@@ -0,0 +1,22 @@
+# typed: strict
+# frozen_string_literal: true
+
+module CodeOwnership
+  module Private
+    class OwnerAssigner
+      extend T::Sig
+
+      sig { params(globs_to_owning_team_map: GlobsToOwningTeamMap).returns(GlobsToOwningTeamMap) }
+      def self.assign_owners(globs_to_owning_team_map)
+        globs_to_owning_team_map.each_with_object({}) do |(glob, owner), mapping|
+          # addresses the case where a directory name includes regex characters
+          # such as `app/services/[test]/some_other_file.ts`
+          mapping[glob] = owner if File.exist?(glob)
+          Dir.glob(glob).each do |file|
+            mapping[file] ||= owner
+          end
+        end
+      end
+    end
+  end
+end

lib/code_ownership/private/owner_assigner.rb

@@ -0,0 +1,96 @@
+module CodeOwnership
+  RSpec.describe Private::OwnerAssigner do
+    describe '.assign_owners' do
+      subject(:assign_owners) { described_class.assign_owners(globs_to_owning_team_map) }
+
+      let(:team_1) { instance_double(CodeTeams::Team) }
+      let(:team_2) { instance_double(CodeTeams::Team) }
+
+      let(:globs_to_owning_team_map) do
+        {
+          'app/services/[test]/some_other_file.ts' => team_1,
+          'app/services/withoutbracket/file.ts' => team_2,
+          'app/models/*.rb' => team_2
+        }
+      end
+
+      before do
+        write_file('app/services/[test]/some_other_file.ts', <<~YML)
+        // @team Bar
+        YML
+
+        write_file('app/services/withoutbracket/file.ts', <<~YML)
+          // @team Bar
+        YML
+      end
+
+      it 'returns a hash with the same keys and the values that are files' do
+        expect(assign_owners).to eq(
+          'app/services/[test]/some_other_file.ts' => team_1,
+          'app/services/withoutbracket/file.ts' => team_2
+        )
+      end
+
+      context 'when file name includes square brackets' do
+        let(:globs_to_owning_team_map) do
+          {
+            'app/services/[test]/some_other_[test]_file.ts' => team_1,
+          }
+        end
+
+        before do
+          write_file('app/services/[test]/some_other_[test]_file.ts', <<~YML)
+          // @team Bar
+          YML
+
+          write_file('app/services/t/some_other_e_file.ts', <<~YML)
+          // @team Bar
+          YML
+        end
+
+        it 'matches the glob pattern' do
+          expect(assign_owners).to eq(
+            'app/services/[test]/some_other_[test]_file.ts' => team_1,
+            'app/services/t/some_other_e_file.ts' => team_1
+          )
+        end
+      end
+
+      context 'when glob pattern also exists' do
+        before do
+          write_file('app/services/t/some_other_file.ts', <<~YML)
+          // @team Bar
+          YML
+        end
+
+        it 'also matches the glob pattern' do
+          expect(assign_owners).to eq(
+            'app/services/[test]/some_other_file.ts' => team_1,
+            'app/services/t/some_other_file.ts' => team_1,
+            'app/services/withoutbracket/file.ts' => team_2
+          )
+        end
+      end
+
+      context 'when * is used in glob pattern' do
+        before do
+          write_file('app/models/some_file.rb', <<~YML)
+          // @team Bar
+          YML
+
+          write_file('app/models/nested/some_file.rb', <<~YML)
+          // @team Bar
+          YML
+        end
+
+        it 'also matches the glob pattern' do
+          expect(assign_owners).to eq(
+            'app/services/[test]/some_other_file.ts' => team_1,
+            'app/services/withoutbracket/file.ts' => team_2,
+            'app/models/some_file.rb' => team_2
+          )
+        end
+      end
+    end
+  end
+end

spec/lib/code_ownership/private/owner_assigner_spec.rb

@@ -9,6 +9,7 @@ module CodeOwnership
         CONTENTS
         write_file('a/b/c/c_file.jsx')
         write_file('a/b/b_file.jsx')
+        write_file('a/b/[test]/b_file.jsx')
         write_file('config/teams/bar.yml', <<~CONTENTS)
           name: Bar
         CONTENTS
@@ -24,6 +25,10 @@ module CodeOwnership
         expect(subject.map_file_to_owner('a/b/b_file.jsx').name).to eq 'Bar'
       end
 
+      it 'can find the owner of files containing [] dirs' do
+        expect(subject.map_file_to_owner('a/b/[test]/b_file.jsx').name).to eq 'Bar'
+      end
+
       it 'can find the owner of files in a sub-directory of a team-owned directory' do
         expect(subject.map_file_to_owner('a/b/c/c_file.jsx').name).to eq 'Bar'
       end

spec/lib/code_ownership/private/ownership_mappers/directory_ownership_spec.rb

@@ -38,13 +38,20 @@ module CodeOwnership
           }
         CONTENTS
         write_file('frontend/javascripts/packages/my_other_package/my_file.jsx')
+        write_file('frontend/javascripts/packages/different_package/test/my_file.ts', <<~CONTENTS)
+          // @team Bar
+        CONTENTS
+        write_file('frontend/javascripts/packages/different_package/[test]/my_file.ts', <<~CONTENTS)
+          // @team Bar
+        CONTENTS
         write_file('config/teams/bar.yml', <<~CONTENTS)
           name: Bar
         CONTENTS
       end
 
       it 'can find the owner of files in team-owned javascript packages' do
         expect(CodeOwnership.for_file('frontend/javascripts/packages/my_other_package/my_file.jsx').name).to eq 'Bar'
+        expect(CodeOwnership.for_file('frontend/javascripts/packages/different_package/[test]/my_file.ts').name).to eq 'Bar'
       end
     end
   end