Allow packwerk to scan for packages inside of engines

AndrewSwerlick · AndrewSwerlick · commit fa46a24c25b2 · 2022-08-10T22:31:00.000-04:00
This commit modifies packwerk so that it can scan for packages defined
in any engines loaded in a given rails application. This allows
support for more complex repository structures where the rails app is
not at the root of the repository, and where that app depends on
engines defined in sibiling directories.

To support this, I modified `PackageSet` so it also looks at
`Rails.application.railties` to pull a list of engines, and their root
directories, and include those in the paths scanned for packages. I
also dropped the restrictions on load paths for constant resolution in
`ApplicationLoadPaths`. The result is that packwerk can now find
packages defined in these engines, and resolve constants in these
engines.

With this setup, packwerk still only scans files in the actual app and
it's subdirectories for violations, but now it can resolve constants
in the engines, and attach them to packages in the engine.

For testing, I created a new fixture which models this sort of
directory structure, and mocks an app with a rails engine.
diff --git a/lib/packwerk/application_load_paths.rb b/lib/packwerk/application_load_paths.rb
@@ -31,11 +31,9 @@ def extract_application_autoload_paths
       end
       def filter_relevant_paths(all_paths, bundle_path: Bundler.bundle_path, rails_root: Rails.root)
         bundle_path_match = bundle_path.join("**")
-        rails_root_match = rails_root.join("**")
 
         all_paths
           .transform_keys { |path| Pathname.new(path).expand_path }
-          .select { |path| path.fnmatch(rails_root_match.to_s) } # path needs to be in application directory
           .reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems
       end
 
diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb
@@ -284,6 +284,7 @@ def package_manifests(glob_pattern = package_glob)
         @configuration.root_path,
         glob_pattern,
         @configuration.exclude,
+        @configuration.packages_outside_of_app_dir_enabled
       )
       package_paths.all_paths.map { |f| File.realpath(f) }
     end
diff --git a/lib/packwerk/configuration.rb b/lib/packwerk/configuration.rb
@@ -41,6 +41,7 @@ def from_packwerk_config(path)
       :custom_associations,
       :config_path,
       :cache_directory,
+      :packages_outside_of_app_dir_enabled,
     )
 
     def initialize(configs = {}, config_path: nil)
@@ -54,6 +55,7 @@ def initialize(configs = {}, config_path: nil)
       @cache_enabled = configs.key?("cache") ? configs["cache"] : false
       @cache_directory = Pathname.new(configs["cache_directory"] || "tmp/cache/packwerk")
       @config_path = config_path
+      @packages_outside_of_app_dir_enabled = configs["packages_outside_of_app_dir_enabled"] || false
 
       if configs["load_paths"]
         warning = <<~WARNING
diff --git a/lib/packwerk/package_paths.rb b/lib/packwerk/package_paths.rb
@@ -17,20 +17,26 @@ class PackagePaths
         root_path: String,
         package_pathspec: T.nilable(PathSpec),
         exclude_pathspec: T.nilable(PathSpec),
+        scan_for_packages_outside_of_app_dir: T.nilable(T::Boolean)
       ).void
     end
-    def initialize(root_path, package_pathspec, exclude_pathspec = nil)
+    def initialize(root_path, package_pathspec, exclude_pathspec = nil, scan_for_packages_outside_of_app_dir = false)
       @root_path = root_path
       @package_pathspec = package_pathspec
       @exclude_pathspec = exclude_pathspec
+      @scan_for_packages_outside_of_app_dir = scan_for_packages_outside_of_app_dir
     end
 
     def all_paths
       exclude_pathspec = Array(@exclude_pathspec).dup
         .push(Bundler.bundle_path.join("**").to_s)
         .map { |glob| File.expand_path(glob) }
 
-      paths_to_scan = [@root_path]
+      paths_to_scan = if @scan_for_packages_outside_of_app_dir
+        engine_paths_to_scan.push(@root_path)
+      else
+        [@root_path]
+      end
 
       glob_patterns = paths_to_scan.product(Array(@package_pathspec)).map do |path, pathspec|
         File.join(path, pathspec, PACKAGE_CONFIG_FILENAME)
@@ -49,5 +55,16 @@ def exclude_path?(globs, path)
         path.realpath.fnmatch(glob, File::FNM_EXTGLOB)
       end
     end
+
+    sig { returns(T::Array[String]) }
+    def engine_paths_to_scan
+      bundle_path_match = Bundler.bundle_path.join("**")
+
+      Rails.application.railties
+        .select { |r| r.is_a?(Rails::Engine) }
+        .map { |r| Pathname.new(r.root).expand_path }
+        .reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems
+        .map(&:to_s)
+    end
   end
 end
diff --git a/lib/packwerk/package_set.rb b/lib/packwerk/package_set.rb
@@ -19,10 +19,11 @@ class << self
       extend T::Sig
 
       sig do
-        params(root_path: String, package_pathspec: T.nilable(PackagePaths::PathSpec)).returns(PackageSet)
+        params(root_path: String, package_pathspec: T.nilable(PackagePaths::PathSpec),
+          scan_for_packages_outside_of_app_dir: T.nilable(T::Boolean)).returns(PackageSet)
       end
-      def load_all_from(root_path, package_pathspec: nil)
-        package_paths = PackagePaths.new(root_path, package_pathspec || "**", nil)
+      def load_all_from(root_path, package_pathspec: nil, scan_for_packages_outside_of_app_dir: false)
+        package_paths = PackagePaths.new(root_path, package_pathspec || "**", nil, scan_for_packages_outside_of_app_dir)
 
         packages = package_paths.all_paths.map do |path|
           root_relative = path.dirname.relative_path_from(root_path)
diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb
@@ -26,6 +26,7 @@ def from_configuration(configuration)
           root_path: configuration.root_path,
           load_paths: configuration.load_paths,
           package_paths: configuration.package_paths,
+          packages_outside_of_app_dir_enabled: configuration.packages_outside_of_app_dir_enabled,
           inflector: inflector,
           custom_associations: configuration.custom_associations,
           cache_enabled: configuration.cache_enabled?,
@@ -46,6 +47,7 @@ def from_configuration(configuration)
         custom_associations: AssociationInspector::CustomAssociations,
         checkers: T::Array[ReferenceChecking::Checkers::Checker],
         cache_enabled: T::Boolean,
+        packages_outside_of_app_dir_enabled: T.nilable(T::Boolean),
       ).void
     end
     def initialize(
@@ -57,7 +59,8 @@ def initialize(
       package_paths: nil,
       custom_associations: [],
       checkers: DEFAULT_CHECKERS,
-      cache_enabled: false
+      cache_enabled: false,
+      packages_outside_of_app_dir_enabled: false
     )
       @root_path = root_path
       @load_paths = load_paths
@@ -68,6 +71,7 @@ def initialize(
       @cache_enabled = cache_enabled
       @cache_directory = cache_directory
       @config_path = config_path
+      @packages_outside_of_app_dir_enabled = packages_outside_of_app_dir_enabled
       @file_processor = T.let(nil, T.nilable(FileProcessor))
       @context_provider = T.let(nil, T.nilable(ConstantDiscovery))
       # We need to initialize this before we fork the process, see https://github.com/Shopify/packwerk/issues/182
@@ -127,6 +131,7 @@ def package_set
       ::Packwerk::PackageSet.load_all_from(
         @root_path,
         package_pathspec: @package_paths,
+        scan_for_packages_outside_of_app_dir: @packages_outside_of_app_dir_enabled,
       )
     end
 
diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb b/test/fixtures/external_packages/app/components/timeline/app/models/concerns/has_timeline.rb
@@ -0,0 +1,5 @@
+# typed: ignore
+# frozen_string_literal: true
+
+module HasTimeline
+end
diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/imaginary/.gitkeep b/test/fixtures/external_packages/app/components/timeline/app/models/imaginary/.gitkeep
diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb b/test/fixtures/external_packages/app/components/timeline/app/models/private_thing.rb
@@ -0,0 +1,2 @@
+class PrivateThing
+end
diff --git a/test/fixtures/external_packages/app/components/timeline/app/models/sales/.gitkeep b/test/fixtures/external_packages/app/components/timeline/app/models/sales/.gitkeep
diff --git a/test/fixtures/external_packages/app/components/timeline/nested/package.yml b/test/fixtures/external_packages/app/components/timeline/nested/package.yml
diff --git a/test/fixtures/external_packages/app/components/timeline/package.yml b/test/fixtures/external_packages/app/components/timeline/package.yml
@@ -0,0 +1,2 @@
+enforce_privacy:
+  - "::PrivateThing"
diff --git a/test/fixtures/external_packages/app/config/environment.rb b/test/fixtures/external_packages/app/config/environment.rb
diff --git a/test/fixtures/external_packages/app/package.yml b/test/fixtures/external_packages/app/package.yml
diff --git a/test/fixtures/external_packages/app/packwerk.yml b/test/fixtures/external_packages/app/packwerk.yml
@@ -0,0 +1 @@
+packages_outside_of_app_dir_enabled: true
diff --git a/test/fixtures/external_packages/sales/components/sales/app/models/order.rb b/test/fixtures/external_packages/sales/components/sales/app/models/order.rb
@@ -0,0 +1,5 @@
+# typed: ignore
+# frozen_string_literal: true
+
+class Order
+end
diff --git a/test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb b/test/fixtures/external_packages/sales/components/sales/app/models/sales/order.rb
@@ -0,0 +1,7 @@
+# typed: ignore
+# frozen_string_literal: true
+
+module Sales
+  class Order
+  end
+end
diff --git a/test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb b/test/fixtures/external_packages/sales/components/sales/app/public/sales/record_new_order.rb
@@ -0,0 +1,7 @@
+# typed: ignore
+# frozen_string_literal: true
+
+module Sales
+  module RecordNewOrder
+  end
+end
diff --git a/test/fixtures/external_packages/sales/components/sales/package.yml b/test/fixtures/external_packages/sales/components/sales/package.yml
@@ -0,0 +1,8 @@
+---
+enforce_privacy: true
+
+metadata:
+  stewards:
+  - "@Shopify/sales"
+  slack_channels:
+  - "#sales"
diff --git a/test/integration/custom_executable_test.rb b/test/integration/custom_executable_test.rb
@@ -7,21 +7,22 @@
 module Packwerk
   module Integration
     class CustomExecutableTest < Minitest::Test
-      include ApplicationFixtureHelper
+      include RailsApplicationFixtureHelper
 
       TIMELINE_PATH = Pathname.new("components/timeline")
 
       setup do
         reset_output
         setup_application_fixture
-        use_template(:skeleton)
       end
 
       teardown do
         teardown_application_fixture
       end
 
       test "'packwerk check' with no violations succeeds in all variants" do
+        use_template(:skeleton)
+
         assert_successful_run("check")
         assert_match(/No offenses detected/, captured_output)
 
@@ -35,6 +36,8 @@ class CustomExecutableTest < Minitest::Test
       end
 
       test "'packwerk check' with violations only in nested packages has different outcomes per variant" do
+        use_template(:skeleton)
+
         open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file|
           file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end")
           file.flush
@@ -55,6 +58,8 @@ class CustomExecutableTest < Minitest::Test
       end
 
       test "'packwerk check' with failures in different parts of the app has different outcomes per variant" do
+        use_template(:skeleton)
+
         open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file|
           file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end")
           file.flush
@@ -74,6 +79,8 @@ class CustomExecutableTest < Minitest::Test
       end
 
       test "'packwerk update-deprecations' with no violations succeeds and updates no files" do
+        use_template(:skeleton)
+
         deprecated_reference_content = read_deprecated_references
 
         assert_successful_run("update-deprecations")
@@ -94,6 +101,8 @@ class CustomExecutableTest < Minitest::Test
       end
 
       test "'packwerk update-deprecations' with violations succeeds and updates relevant deprecated_references" do
+        use_template(:skeleton)
+
         deprecated_reference_content = read_deprecated_references
         timeline_deprecated_reference_path = to_app_path(File.join(TIMELINE_PATH, "deprecated_references.yml"))
 
@@ -129,6 +138,19 @@ class CustomExecutableTest < Minitest::Test
         end
       end
 
+      test "'packwerk check' for an app with external packages it detects a violation referencing a constant in the external package" do
+        use_template(:external_packages)
+
+        open_app_file(TIMELINE_PATH.join("nested", "timeline_comment.rb")) do |file|
+          file.write("class TimelineComment; belongs_to :order, class_name: '::Order'; end")
+          file.flush
+        end
+
+        refute_successful_run("check")
+        assert_match(/Privacy violation: '::Order'/, captured_output)
+        assert_match(/1 offense detected/, captured_output)
+      end
+
       private
 
       def assert_successful_run(command)
diff --git a/test/support/application_fixture_helper.rb b/test/support/application_fixture_helper.rb
@@ -22,6 +22,10 @@ def use_template(template)
     raise "use_template may only be called once per test" if using_template?
 
     copy_dir("test/fixtures/#{template}")
+    if template == :external_packages
+      @app_dir = File.join(app_dir, "./app")
+    end
+
     Dir.chdir(app_dir)
   end
 
diff --git a/test/support/rails_application_fixture_helper.rb b/test/support/rails_application_fixture_helper.rb
@@ -1,4 +1,4 @@
-# typed: true
+# typed: false
 # frozen_string_literal: true
 
 require "rails_test_helper"
@@ -32,16 +32,21 @@ def use_template(template)
 
     Rails.stubs(:autoloaders).returns(Autoloaders.new)
 
+    root = Pathname.new(app_dir)
+
     case template
     when :minimal
       set_load_paths_for_minimal_template
     when :skeleton
       set_load_paths_for_skeleton_template
+    when :external_packages
+      set_load_paths_for_external_packages_template
+      create_new_engine_at_path(*to_app_paths("../sales/components/sales/"))
+      Rails.application.stubs(:railties).returns(Rails::Engine::Railties.new)
     else
       raise "Unknown fixture template #{template}"
     end
 
-    root = Pathname.new(app_dir)
     Rails.application.config.stubs(:root).returns(root)
   end
 
@@ -59,4 +64,18 @@ def set_load_paths_for_skeleton_template
     Rails.autoloaders.once.push_dir(*to_app_paths("components/timeline/app/models/concerns"))
     Rails.autoloaders.once.push_dir(*to_app_paths("vendor/cache/gems/example/models"))
   end
+
+  def set_load_paths_for_external_packages_template
+    Rails.autoloaders.main.push_dir(*to_app_paths("/components/timeline/app/models"))
+
+    Rails.autoloaders.once.push_dir(*to_app_paths("../sales/components/sales/app/models"))
+  end
+
+  def create_new_engine_at_path(path)
+    Class.new(Rails::Engine) do
+      define_method(:root) do
+        path
+      end
+    end
+  end
 end
diff --git a/test/unit/application_load_paths_test.rb b/test/unit/application_load_paths_test.rb
@@ -18,19 +18,6 @@ class ApplicationLoadPathsTest < Minitest::Test
       assert_equal({ relative_path => Object }, relative_path_strings)
     end
 
-    test ".filter_relevant_paths excludes paths outside of the application root" do
-      valid_paths = ["/application/app/models"]
-      paths = valid_paths + ["/users/tobi/.gems/something/app/models", "/application/../something/"]
-      paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object }
-      filtered_paths = ApplicationLoadPaths.filter_relevant_paths(
-        paths,
-        bundle_path: Pathname.new("/application/vendor/"),
-        rails_root: Pathname.new("/application/")
-      )
-
-      assert_equal({ "/application/app/models" => Object }, filtered_paths.transform_keys(&:to_s))
-    end
-
     test ".filter_relevant_paths excludes paths from vendored gems" do
       valid_paths = ["/application/app/models"]
       paths = valid_paths + ["/application/vendor/something/app/models"]
diff --git a/test/unit/package_set_test.rb b/test/unit/package_set_test.rb

Original file line number	Diff line number	Diff line change
`@@ -284,6 +284,7 @@ def package_manifests(glob_pattern = package_glob)`
`284`	`284`	`@configuration.root_path,`
`285`	`285`	`glob_pattern,`
`286`	`286`	`@configuration.exclude,`
	`287`	`+ @configuration.packages_outside_of_app_dir_enabled`
`287`	`288`	`)`
`288`	`289`	`package_paths.all_paths.map { \|f\| File.realpath(f) }`
`289`	`290`	`end`