refactoring file is directory ownership

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

Author: tstannard

code_ownership.gemspec

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

lib/code_ownership/private.rb

@@ -13,6 +13,7 @@
 require 'code_ownership/private/validations/files_have_unique_owners'
 require 'code_ownership/private/ownership_mappers/file_annotations'
 require 'code_ownership/private/ownership_mappers/team_globs'
+require 'code_ownership/private/ownership_mappers/file_owner'
 require 'code_ownership/private/ownership_mappers/directory_ownership'
 require 'code_ownership/private/ownership_mappers/package_ownership'
 require 'code_ownership/private/ownership_mappers/js_package_ownership'

lib/code_ownership/private/ownership_mappers/directory_ownership.rb

@@ -10,23 +10,21 @@ class DirectoryOwnership
         include Mapper
 
         CODEOWNERS_DIRECTORY_FILE_NAME = '.codeowner'
-        RELATIVE_ROOT = Pathname('.').freeze
-        ABSOLUTE_ROOT = Pathname('/').freeze
 
         @@directory_cache = T.let({}, T::Hash[String, T.nilable(CodeTeams::Team)]) # rubocop:disable Style/ClassVars
 
         sig do
-          override.params(file: String).
-            returns(T.nilable(::CodeTeams::Team))
+          override.params(file: String)
+                  .returns(T.nilable(::CodeTeams::Team))
         end
         def map_file_to_owner(file)
           map_file_to_relevant_owner(file)
         end
 
         sig do
-          override.params(cache: GlobsToOwningTeamMap, files: T::Array[String]).returns(GlobsToOwningTeamMap)
+          override.params(_cache: GlobsToOwningTeamMap, files: T::Array[String]).returns(GlobsToOwningTeamMap)
         end
-        def update_cache(cache, files)
+        def update_cache(_cache, files)
           globs_to_owner(files)
         end
 
@@ -39,17 +37,17 @@ def update_cache(cache, files)
         # subset of files, but rather we want code ownership for all files.
         #
         sig do
-          override.params(files: T::Array[String]).
-            returns(T::Hash[String, ::CodeTeams::Team])
+          override.params(_files: T::Array[String])
+                  .returns(T::Hash[String, ::CodeTeams::Team])
         end
-        def globs_to_owner(files)
+        def globs_to_owner(_files)
           # The T.unsafe is because the upstream RBI is wrong for Pathname.glob
           T
             .unsafe(Pathname)
             .glob(File.join('**/', CODEOWNERS_DIRECTORY_FILE_NAME))
             .map(&:cleanpath)
             .each_with_object({}) do |pathname, res|
-            owner = owner_for_codeowners_file(pathname)
+            owner = FileOwner.owner_for_codeowners_file(pathname)
             res[pathname.dirname.cleanpath.join('**/**').to_s] = owner
           end
         end
@@ -66,56 +64,13 @@ def bust_caches!
 
         private
 
-        sig { params(codeowners_file: Pathname).returns(CodeTeams::Team) }
-        def owner_for_codeowners_file(codeowners_file)
-          raw_owner_value = File.foreach(codeowners_file).first.strip
-
-          Private.find_team!(
-            raw_owner_value,
-            codeowners_file.to_s
-          )
-        end
-
-        # Takes a file and finds the relevant `.codeowner` file by walking up the directory
+        # takes a file and finds the relevant `.codeowner` file by walking up the directory
         # structure. Example, given `a/b/c.rb`, this looks for `a/b/.codeowner`, `a/.codeowner`,
         # and `.codeowner` in that order, stopping at the first file to actually exist.
-        # If the parovided file is a directory, it will look for `.codeowner` in that directory and then upwards.
-        # We do additional caching so that we don't have to check for file existence every time.
+        # We do additional caching so that we don't have to check for file existence every time
         sig { params(file: String).returns(T.nilable(CodeTeams::Team)) }
         def map_file_to_relevant_owner(file)
-          file_path = Pathname.new(file)
-          team = T.let(nil, T.nilable(CodeTeams::Team))
-
-          if File.directory?(file)
-            team = get_team_from_codeowners_file_within_directory(file_path)
-          end
-
-          while team.nil? && file_path != RELATIVE_ROOT && file_path != ABSOLUTE_ROOT
-            file_path = file_path.parent
-            team = get_team_from_codeowners_file_within_directory(file_path)
-          end
-
-          team
-        end
-
-        sig { params(directory: Pathname).returns(T.nilable(CodeTeams::Team)) }
-        def get_team_from_codeowners_file_within_directory(directory)
-          potential_codeowners_file = directory.join(CODEOWNERS_DIRECTORY_FILE_NAME)
-
-          potential_codeowners_file_name = potential_codeowners_file.to_s
-
-          team = nil
-          if @@directory_cache.key?(potential_codeowners_file_name)
-            team = @@directory_cache[potential_codeowners_file_name]
-          elsif potential_codeowners_file.exist?
-            team = owner_for_codeowners_file(potential_codeowners_file)
-
-            @@directory_cache[potential_codeowners_file_name] = team
-          else
-            @@directory_cache[potential_codeowners_file_name] = nil
-          end
-
-          return team
+          FileOwner.for_file(file, @@directory_cache)
         end
       end
     end

lib/code_ownership/private/ownership_mappers/file_owner.rb

@@ -0,0 +1,100 @@
+# frozen_string_literal: true
+
+# typed: strict
+
+module CodeOwnership
+  module Private
+    module OwnershipMappers
+      class FileOwner
+        extend T::Sig
+
+        sig { returns(String) }
+        attr_reader :file_path
+        sig { returns(T::Hash[String, T.nilable(CodeTeams::Team)]) }
+        attr_reader :directory_cache
+
+        private_class_method :new
+
+        sig { params(file_path: String, directory_cache: T::Hash[String, T.nilable(CodeTeams::Team)]).void }
+        def initialize(file_path, directory_cache)
+          @file_path = file_path
+          @directory_cache = directory_cache
+        end
+
+        sig { params(file_path: String, directory_cache: T::Hash[String, T.nilable(CodeTeams::Team)]).returns(T.nilable(CodeTeams::Team)) }
+        def self.for_file(file_path, directory_cache)
+          new(file_path, directory_cache).owner
+        end
+
+        sig { returns(T.nilable(CodeTeams::Team)) }
+        def owner
+          path_components_end_index.downto(0).each do |index|
+            team = team_for_path_components(T.must(path_components[0..index]))
+            return team if team
+          end
+          nil
+        end
+
+        sig { params(codeowners_file: Pathname).returns(CodeTeams::Team) }
+        def self.owner_for_codeowners_file(codeowners_file)
+          raw_owner_value = File.foreach(codeowners_file).first.strip
+
+          Private.find_team!(
+            raw_owner_value,
+            codeowners_file.to_s
+          )
+        end
+
+        private
+
+        sig { params(path_components: T::Array[Pathname]).returns(T.nilable(CodeTeams::Team)) }
+        def team_for_path_components(path_components)
+          potential_relative_path_name = path_components.reduce(Pathname.new('')) do |built_path, path|
+            built_path.join(path)
+          end
+
+          potential_codeowners_file = potential_relative_path_name.join(DirectoryOwnership::CODEOWNERS_DIRECTORY_FILE_NAME)
+          potential_codeowners_file_name = potential_codeowners_file.to_s
+
+          team = nil
+          if directory_cache.key?(potential_codeowners_file_name)
+            team = directory_cache[potential_codeowners_file_name]
+          elsif potential_codeowners_file.exist?
+            team = self.class.owner_for_codeowners_file(potential_codeowners_file)
+            directory_cache[potential_codeowners_file_name] = team
+          else
+            directory_cache[potential_codeowners_file_name] = nil
+          end
+
+          team
+        end
+
+        sig { returns(Pathname) }
+        def file_path_name
+          T.let(Pathname.new(file_path), T.nilable(Pathname))
+          @file_path_name ||= T.let(Pathname.new(file_path), T.nilable(Pathname))
+        end
+
+        sig { returns(T::Boolean) }
+        def file_is_dir?
+          file_path_name.directory?
+        end
+
+        sig { returns(Integer) }
+        def path_components_end_index
+          # include the directory itself if it is a directory, but not if it is a file
+          if file_is_dir?
+            path_components.length
+          else
+            path_components.length - 1
+          end
+        end
+
+        sig { returns(T::Array[Pathname]) }
+        def path_components
+          file_path_name.each_filename.to_a.map { |path| Pathname.new(path) }
+        end
+      end
+    end
+  end
+end

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

@@ -1,5 +1,5 @@
 module CodeOwnership
-  RSpec.describe Private::OwnershipMappers::JsPackageOwnership do
+  RSpec.describe Private::OwnershipMappers::DirectoryOwnership do
     describe 'CodeOwnershp.for_file' do
       before do
         write_configuration
@@ -22,9 +22,13 @@ module CodeOwnership
         expect(CodeOwnership.for_file('a/b/c/c_file.jsx').name).to eq 'Bar'
       end
 
+
+      it 'looks for codeowner file within lower directory' do
+        expect(CodeOwnership.for_file('a/b/c').name).to eq 'Bar'
+      end
+
       it 'looks for codeowner file within directory' do
         expect(CodeOwnership.for_file('a/b').name).to eq 'Bar'
-        expect(CodeOwnership.for_file(Pathname.pwd.join('a/b').to_s).name).to eq 'Bar'
       end
     end
   end

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

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+module CodeOwnership
+  RSpec.describe Private::OwnershipMappers::FileOwner do
+    describe 'CodeOwnershp.for_file' do
+      subject { described_class.for_file(file_path, directory_cache) }
+      let(:directory_cache) { {} }
+      before do
+        write_configuration
+
+        write_file('a/b/.codeowner', <<~CONTENTS)
+          Bar
+        CONTENTS
+        write_file('z/y/x/x_file.jsx')
+        write_file('a/b/c/c_file.jsx')
+        write_file('a/b/b_file.jsx')
+        write_file('a/b/c/d/e/f/g/h_file.jsx')
+        write_file('config/teams/bar.yml', <<~CONTENTS)
+          name: Bar
+        CONTENTS
+      end
+
+      context 'when file provided' do
+        let(:file_path) { 'a/b/b_file.jsx' }
+
+        it 'can find the owner of files in team-owned directory' do
+          expect(subject.name).to eq 'Bar'
+        end
+
+        context 'when un-owned file' do
+          let(:file_path) { 'z/y/x/x_file.jsx' }
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+      end
+
+      context 'when directory provided' do
+        let(:file_path) { 'a/b' }
+
+        it 'can find the owner of files in team-owned directory' do
+          expect(subject.name).to eq 'Bar'
+        end
+
+        context 'when deeply nested directory' do
+          let(:file_path) { 'a/b/c/d/e/f/g' }
+
+          it 'can find the owner of files in team-owned directory' do
+            expect(subject.name).to eq 'Bar'
+          end
+        end
+
+        context 'when un-owned directory' do
+          let(:file_path) { 'z/y' }
+          it 'returns nil' do
+            expect(subject).to be_nil
+          end
+        end
+      end
+    end
+  end
+end