Fix bug with "files have unique owners" validation (#31)

* Add failing test

* Fix failing test

* Bump version

* Fix non-determinism in output so CI passes

Author: Alex Evanczuk

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    code_ownership (1.29.3)
+    code_ownership (1.30.0)
       code_teams (~> 1.0)
       packs
       sorbet-runtime

code_ownership.gemspec

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

lib/code_ownership/private.rb

@@ -10,6 +10,7 @@
 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'
+require 'code_ownership/private/validations/no_overlapping_globs'
 require 'code_ownership/private/ownership_mappers/interface'
 require 'code_ownership/private/ownership_mappers/file_annotations'
 require 'code_ownership/private/ownership_mappers/team_globs'
@@ -39,6 +40,7 @@ def self.validate!(files:, autocorrect: true, stage_changes: true)
         Validations::FilesHaveOwners.new,
         Validations::FilesHaveUniqueOwners.new,
         Validations::GithubCodeownersUpToDate.new,
+        Validations::NoOverlappingGlobs.new,
       ]
 
       errors = validators.flat_map do |validator|

lib/code_ownership/private/ownership_mappers/team_globs.rb

@@ -31,6 +31,58 @@ def map_files_to_owners(files) # rubocop:disable Lint/UnusedMethodArgument
           end
         end
 
+        class MappingContext < T::Struct
+          const :glob, String
+          const :team, CodeTeams::Team
+        end
+
+        class GlobOverlap < T::Struct
+          extend T::Sig
+
+          const :mapping_contexts, T::Array[MappingContext]
+
+          sig { returns(String) }
+          def description
+            # These are sorted only to prevent non-determinism in output between local and CI environments.
+            sorted_contexts = mapping_contexts.sort_by{|context| context.team.config_yml.to_s }
+            description_args = sorted_contexts.map do |context|
+              "`#{context.glob}` (from `#{context.team.config_yml}`)"
+            end
+
+            description_args.join(', ')
+          end
+        end
+
+        sig do
+          returns(T::Array[GlobOverlap])
+        end
+        def find_overlapping_globs
+          mapped_files = T.let({}, T::Hash[String, T::Array[MappingContext]])
+          CodeTeams.all.each_with_object({}) do |team, map| # rubocop:disable Style/ClassVars
+            TeamPlugins::Ownership.for(team).owned_globs.each do |glob|
+              Dir.glob(glob).each do |filename|
+                mapped_files[filename] ||= []
+                T.must(mapped_files[filename]) << MappingContext.new(glob: glob, team: team)
+              end
+            end
+          end
+
+          overlaps = T.let([], T::Array[GlobOverlap])
+          mapped_files.each do |filename, mapping_contexts|
+            if mapping_contexts.count > 1
+              overlaps << GlobOverlap.new(mapping_contexts: mapping_contexts)
+            end
+          end
+
+          deduplicated_overlaps = overlaps.uniq do |glob_overlap|
+            glob_overlap.mapping_contexts.map do |context|
+              [context.glob, context.team.name]
+            end
+          end
+
+          deduplicated_overlaps
+        end
+
         sig do
           override.params(file: String).
             returns(T.nilable(::CodeTeams::Team))

lib/code_ownership/private/validations/no_overlapping_globs.rb

@@ -0,0 +1,30 @@
+# typed: strict
+
+module CodeOwnership
+  module Private
+    module Validations
+      class NoOverlappingGlobs
+        extend T::Sig
+        extend T::Helpers
+        include Interface
+
+        sig { override.params(files: T::Array[String], autocorrect: T::Boolean, stage_changes: T::Boolean).returns(T::Array[String]) }
+        def validation_errors(files:, autocorrect: true, stage_changes: true)
+          overlapping_globs = OwnershipMappers::TeamGlobs.new.find_overlapping_globs
+
+          errors = T.let([], T::Array[String])
+
+          if overlapping_globs.any?
+            errors << <<~MSG
+              `owned_globs` cannot overlap between teams. The following globs overlap:
+
+              #{overlapping_globs.map { |overlap| "- #{overlap.description}"}.join("\n")}
+            MSG
+          end
+
+          errors
+        end
+      end
+    end
+  end
+end

spec/lib/code_ownership_spec.rb

@@ -662,6 +662,51 @@
         end
       end
     end
+
+    describe 'no overlapping globs validation' do
+      context 'two teams own the same exact glob' do
+        before do
+          write_file('config/code_ownership.yml', <<~YML)
+            owned_globs:
+              - '{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx}'
+          YML
+
+          write_file('packs/my_pack/owned_file.rb')
+          write_file('frontend/javascripts/blah/my_file.rb')
+          write_file('frontend/javascripts/blah/subdir/my_file.rb')
+
+          write_file('config/teams/bar.yml', <<~CONTENTS)
+            name: Bar
+            owned_globs:
+              - packs/**/**
+              - frontend/javascripts/blah/subdir/my_file.rb
+          CONTENTS
+
+          write_file('config/teams/foo.yml', <<~CONTENTS)
+            name: Foo
+            owned_globs:
+              - packs/**/**
+              - frontend/javascripts/blah/**/**
+          CONTENTS
+        end
+
+        it 'lets the user know that `owned_globs` can not overlap' do
+          expect { CodeOwnership.validate! }.to raise_error do |e|
+            expect(e).to be_a CodeOwnership::InvalidCodeOwnershipConfigurationError
+            puts e.message
+            expect(e.message).to eq <<~EXPECTED.chomp
+              `owned_globs` cannot overlap between teams. The following globs overlap:
+
+              - `packs/**/**` (from `config/teams/bar.yml`), `packs/**/**` (from `config/teams/foo.yml`)
+              - `frontend/javascripts/blah/subdir/my_file.rb` (from `config/teams/bar.yml`), `frontend/javascripts/blah/**/**` (from `config/teams/foo.yml`)
+
+              See https://github.com/rubyatscale/code_ownership#README.md for more details
+            EXPECTED
+          end
+        end
+      end
+    end
+
   end
 
   describe '.for_file' do
@@ -983,5 +1028,4 @@
     end
     end
   end
-
 end