From 5bbedf19410c185d7578c9f7e88ba5f79afff6e4 Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Sun, 16 Oct 2022 12:26:03 -0400 Subject: [PATCH] Extract privacy validations from ApplicationValidator --- lib/packwerk.rb | 8 + lib/packwerk/application_validator.rb | 173 +++--------------- .../check_package_manifests_for_privacy.rb | 134 ++++++++++++++ lib/packwerk/application_validator/helpers.rb | 55 ++++++ lib/packwerk/application_validator/result.rb | 18 ++ test/unit/application_validator_test.rb | 78 +------- ...heck_package_manifests_for_privacy_test.rb | 119 ++++++++++++ 7 files changed, 362 insertions(+), 223 deletions(-) create mode 100644 lib/packwerk/application_validator/check_package_manifests_for_privacy.rb create mode 100644 lib/packwerk/application_validator/helpers.rb create mode 100644 lib/packwerk/application_validator/result.rb create mode 100644 test/unit/validators/check_package_manifests_for_privacy_test.rb diff --git a/lib/packwerk.rb b/lib/packwerk.rb index 9da66f7d8..788e3ef39 100644 --- a/lib/packwerk.rb +++ b/lib/packwerk.rb @@ -85,4 +85,12 @@ module Checkers autoload :PrivacyChecker end end + + class ApplicationValidator + extend ActiveSupport::Autoload + + autoload :Result + autoload :CheckPackageManifestsForPrivacy + autoload :Helpers + end end diff --git a/lib/packwerk/application_validator.rb b/lib/packwerk/application_validator.rb index 8baf5f9d1..6351bb129 100644 --- a/lib/packwerk/application_validator.rb +++ b/lib/packwerk/application_validator.rb @@ -11,6 +11,13 @@ module Packwerk class ApplicationValidator extend T::Sig + # This is a temporary API as we migrate validators to their own files. + # Later, we can expose an API to get package sets to pass into validators when testing + # This API would likely just be `PackageSet.load_all_from(configruation)`, but we might want to clean + # up that API a bit (it looks like there are some unnecessary input variables). + sig { returns(PackageSet) } + attr_reader :package_set + sig do params( config_file_path: String, @@ -22,26 +29,18 @@ def initialize(config_file_path:, configuration:, environment:) @config_file_path = config_file_path @configuration = configuration @environment = environment - @package_set = T.let(PackageSet.load_all_from(@configuration.root_path, package_pathspec: package_glob), - PackageSet) - end - - class Result < T::Struct - extend T::Sig - - const :ok, T::Boolean - const :error_value, T.nilable(String) + package_set = PackageSet.load_all_from( + @configuration.root_path, + package_pathspec: Helpers.package_glob(configuration) + ) - sig { returns(T::Boolean) } - def ok? - ok - end + @package_set = T.let(package_set, PackageSet) end sig { returns(Result) } def check_all results = [ - check_package_manifests_for_privacy, + CheckPackageManifestsForPrivacy.call(@package_set, @configuration), check_package_manifest_syntax, check_application_structure, check_acyclic_graph, @@ -50,50 +49,23 @@ def check_all check_root_package_exists, ] - merge_results(results) - end - - sig { returns(Result) } - def check_package_manifests_for_privacy - privacy_settings = package_manifests_settings_for("enforce_privacy") - - resolver = ConstantResolver.new( - root_path: @configuration.root_path, - load_paths: @configuration.load_paths - ) - - results = T.let([], T::Array[Result]) - - privacy_settings.each do |config_file_path, setting| - next unless setting.is_a?(Array) - - constants = setting - - results += assert_constants_can_be_loaded(constants, config_file_path) - - constant_locations = constants.map { |c| [c, resolver.resolve(c)&.location] } - - constant_locations.each do |name, location| - results << if location - check_private_constant_location(name, location, config_file_path) - else - private_constant_unresolvable(name, config_file_path) - end - end - end - - merge_results(results, separator: "\n---\n") + Helpers.merge_results(results) end sig { returns(Result) } def check_package_manifest_syntax errors = [] - package_manifests.each do |f| + Helpers.package_manifests(@configuration).each do |f| hash = YAML.load_file(f) next unless hash - known_keys = ["enforce_privacy", "enforce_dependencies", "public_path", "dependencies", "metadata"] + known_keys = [ + *CheckPackageManifestsForPrivacy.permitted_keys, + "enforce_dependencies", + "dependencies", + "metadata", + ] unknown_keys = hash.keys - known_keys unless unknown_keys.empty? @@ -102,24 +74,12 @@ def check_package_manifest_syntax "open an issue in https://github.com/Shopify/packwerk" end - if hash.key?("enforce_privacy") - unless [TrueClass, FalseClass, Array].include?(hash["enforce_privacy"].class) - errors << "Invalid 'enforce_privacy' option in #{f.inspect}: #{hash["enforce_privacy"].inspect}" - end - end - if hash.key?("enforce_dependencies") unless [TrueClass, FalseClass].include?(hash["enforce_dependencies"].class) errors << "Invalid 'enforce_dependencies' option in #{f.inspect}: #{hash["enforce_dependencies"].inspect}" end end - if hash.key?("public_path") - unless hash["public_path"].is_a?(String) - errors << "'public_path' option must be a string in #{f.inspect}: #{hash["public_path"].inspect}" - end - end - next unless hash.key?("dependencies") next if hash["dependencies"].is_a?(Array) @@ -173,8 +133,8 @@ def check_acyclic_graph sig { returns(Result) } def check_package_manifest_paths - all_package_manifests = package_manifests("**/") - package_paths_package_manifests = package_manifests(package_glob) + all_package_manifests = Helpers.package_manifests(@configuration, "**/") + package_paths_package_manifests = Helpers.package_manifests(@configuration, Helpers.package_glob(@configuration)) difference = all_package_manifests - package_paths_package_manifests @@ -194,7 +154,7 @@ def check_package_manifest_paths sig { returns(Result) } def check_valid_package_dependencies - packages_dependencies = package_manifests_settings_for("dependencies") + packages_dependencies = Helpers.package_manifests_settings_for(@configuration, "dependencies") .delete_if { |_, deps| deps.nil? } packages_with_invalid_dependencies = @@ -231,7 +191,7 @@ def check_valid_package_dependencies sig { returns(Result) } def check_root_package_exists root_package_path = File.join(@configuration.root_path, "package.yml") - all_packages_manifests = package_manifests(package_glob) + all_packages_manifests = Helpers.package_manifests(@configuration, Helpers.package_glob(@configuration)) if all_packages_manifests.include?(root_package_path) Result.new(ok: true) @@ -263,35 +223,14 @@ def build_cycle_strings(cycles) end end - sig { params(setting: T.untyped).returns(T.untyped) } - def package_manifests_settings_for(setting) - package_manifests.map { |f| [f, (YAML.load_file(File.join(f)) || {})[setting]] } - end - sig { params(list: T.untyped).returns(T.untyped) } def format_yaml_strings(list) list.sort.map { |p| "- \"#{p}\"" }.join("\n") end - sig { returns(T.any(T::Array[String], String)) } - def package_glob - @configuration.package_paths || "**" - end - - sig { params(glob_pattern: T.any(T::Array[String], String)).returns(T::Array[String]) } - def package_manifests(glob_pattern = package_glob) - PackageSet.package_paths(@configuration.root_path, glob_pattern, @configuration.exclude) - .map { |f| File.realpath(f) } - end - sig { params(paths: T::Array[String]).returns(T::Array[Pathname]) } def relative_paths(paths) - paths.map { |path| relative_path(path) } - end - - sig { params(path: String).returns(Pathname) } - def relative_path(path) - Pathname.new(path).relative_path_from(@configuration.root_path) + paths.map { |path| Helpers.relative_path(@configuration, path) } end sig { params(path: T.untyped).returns(T::Boolean) } @@ -302,65 +241,5 @@ def invalid_package_path?(path) package_path = File.join(@configuration.root_path, path, PackageSet::PACKAGE_CONFIG_FILENAME) !File.file?(package_path) end - - sig { params(constants: T.untyped, config_file_path: String).returns(T::Array[Result]) } - def assert_constants_can_be_loaded(constants, config_file_path) - constants.map do |constant| - if !constant.start_with?("::") - Result.new( - ok: false, - error_value: "'#{constant}', listed in the 'enforce_privacy' option in #{config_file_path}, is invalid.\n"\ - "Private constants need to be prefixed with the top-level namespace operator `::`." - ) - else - constant.try(&:constantize) && Result.new(ok: true) - end - end - end - - sig { params(name: T.untyped, config_file_path: T.untyped).returns(Result) } - def private_constant_unresolvable(name, config_file_path) - explicit_filepath = (name.start_with?("::") ? name[2..-1] : name).underscore + ".rb" - - Result.new( - ok: false, - error_value: "'#{name}', listed in #{config_file_path}, could not be resolved.\n"\ - "This is probably because it is an autovivified namespace - a namespace module that doesn't have a\n"\ - "file explicitly defining it. Packwerk currently doesn't support declaring autovivified namespaces as\n"\ - "private. Add a #{explicit_filepath} file to explicitly define the constant." - ) - end - - sig { params(name: T.untyped, location: T.untyped, config_file_path: T.untyped).returns(Result) } - def check_private_constant_location(name, location, config_file_path) - declared_package = @package_set.package_from_path(relative_path(config_file_path)) - constant_package = @package_set.package_from_path(location) - - if constant_package == declared_package - Result.new(ok: true) - else - Result.new( - ok: false, - error_value: "'#{name}' is declared as private in the '#{declared_package}' package but appears to be "\ - "defined\nin the '#{constant_package}' package. Packwerk resolved it to #{location}." - ) - end - end - - sig do - params(results: T::Array[Result], separator: String, errors_headline: String).returns(Result) - end - def merge_results(results, separator: "\n===\n", errors_headline: "") - results.reject!(&:ok?) - - if results.empty? - Result.new(ok: true) - else - Result.new( - ok: false, - error_value: errors_headline + results.map(&:error_value).join(separator) - ) - end - end end end diff --git a/lib/packwerk/application_validator/check_package_manifests_for_privacy.rb b/lib/packwerk/application_validator/check_package_manifests_for_privacy.rb new file mode 100644 index 000000000..58b581f90 --- /dev/null +++ b/lib/packwerk/application_validator/check_package_manifests_for_privacy.rb @@ -0,0 +1,134 @@ +# typed: strict +# frozen_string_literal: true + +module Packwerk + class ApplicationValidator + module CheckPackageManifestsForPrivacy + class << self + extend T::Sig + + sig { params(package_set: PackageSet, configuration: Configuration).returns(Result) } + def call(package_set, configuration) + privacy_settings = Helpers.package_manifests_settings_for(configuration, "enforce_privacy") + + resolver = ConstantResolver.new( + root_path: configuration.root_path, + load_paths: configuration.load_paths + ) + + results = T.let([], T::Array[Result]) + + privacy_settings.each do |config_file_path, setting| + results << check_enforce_privacy_setting(config_file_path, setting) + next unless setting.is_a?(Array) + + constants = setting + + results += assert_constants_can_be_loaded(constants, config_file_path) + + constant_locations = constants.map { |c| [c, resolver.resolve(c)&.location] } + + constant_locations.each do |name, location| + results << if location + check_private_constant_location(configuration, package_set, name, location, config_file_path) + else + private_constant_unresolvable(name, config_file_path) + end + end + end + + public_path_settings = Helpers.package_manifests_settings_for(configuration, "public_path") + public_path_settings.each do |config_file_path, setting| + results << check_public_path(config_file_path, setting) + end + + Helpers.merge_results(results, separator: "\n---\n") + end + + sig { returns(T::Array[String]) } + def permitted_keys + ["public_path", "enforce_privacy"] + end + + private + + sig do + params(config_file_path: String, setting: T.untyped).returns(Result) + end + def check_public_path(config_file_path, setting) + if setting.is_a?(String) || setting.nil? + Result.new(ok: true) + else + Result.new( + ok: false, + error_value: "'public_path' option must be a string in #{config_file_path.inspect}: #{setting.inspect}" + ) + end + end + + sig do + params(config_file_path: String, setting: T.untyped).returns(Result) + end + def check_enforce_privacy_setting(config_file_path, setting) + if [TrueClass, FalseClass, Array, NilClass].include?(setting.class) + Result.new(ok: true) + else + Result.new( + ok: false, + error_value: "Invalid 'enforce_privacy' option in #{config_file_path.inspect}: #{setting.inspect}" + ) + end + end + + sig do + params(configuration: Configuration, package_set: PackageSet, name: T.untyped, location: T.untyped, + config_file_path: T.untyped).returns(Result) + end + def check_private_constant_location(configuration, package_set, name, location, config_file_path) + declared_package = package_set.package_from_path(Helpers.relative_path(configuration, config_file_path)) + constant_package = package_set.package_from_path(location) + + if constant_package == declared_package + Result.new(ok: true) + else + Result.new( + ok: false, + error_value: "'#{name}' is declared as private in the '#{declared_package}' package but appears to be "\ + "defined\nin the '#{constant_package}' package. Packwerk resolved it to #{location}." + ) + end + end + + sig { params(constants: T.untyped, config_file_path: String).returns(T::Array[Result]) } + def assert_constants_can_be_loaded(constants, config_file_path) + constants.map do |constant| + if !constant.start_with?("::") + error_value = "'#{constant}', listed in the 'enforce_privacy' option" \ + " in #{config_file_path}, is invalid.\nPrivate constants need to be" \ + " prefixed with the top-level namespace operator `::`." + Result.new( + ok: false, + error_value: error_value + ) + else + constant.try(&:constantize) && Result.new(ok: true) + end + end + end + + sig { params(name: T.untyped, config_file_path: T.untyped).returns(Result) } + def private_constant_unresolvable(name, config_file_path) + explicit_filepath = (name.start_with?("::") ? name[2..-1] : name).underscore + ".rb" + + Result.new( + ok: false, + error_value: "'#{name}', listed in #{config_file_path}, could not be resolved.\n"\ + "This is probably because it is an autovivified namespace - a namespace module that doesn't have a\n"\ + "file explicitly defining it. Packwerk currently doesn't support declaring autovivified namespaces as\n"\ + "private. Add a #{explicit_filepath} file to explicitly define the constant." + ) + end + end + end + end +end diff --git a/lib/packwerk/application_validator/helpers.rb b/lib/packwerk/application_validator/helpers.rb new file mode 100644 index 000000000..00d0bc2d0 --- /dev/null +++ b/lib/packwerk/application_validator/helpers.rb @@ -0,0 +1,55 @@ +# typed: strict +# frozen_string_literal: true + +module Packwerk + class ApplicationValidator + module Helpers + class << self + extend T::Sig + + sig { params(configuration: Configuration, setting: T.untyped).returns(T.untyped) } + def package_manifests_settings_for(configuration, setting) + package_manifests(configuration).map { |f| [f, (YAML.load_file(File.join(f)) || {})[setting]] } + end + + sig do + params(configuration: Configuration, + glob_pattern: T.nilable(T.any(T::Array[String], String))).returns(T::Array[String]) + end + def package_manifests(configuration, glob_pattern = nil) + glob_pattern ||= package_glob(configuration) + PackageSet.package_paths(configuration.root_path, glob_pattern, configuration.exclude) + .map { |f| File.realpath(f) } + end + + sig { params(configuration: Configuration).returns(T.any(T::Array[String], String)) } + def package_glob(configuration) + configuration.package_paths || "**" + end + + sig do + params(results: T::Array[Result], separator: String, errors_headline: String).returns(Result) + end + def merge_results(results, separator: "\n===\n", errors_headline: "") + results.reject!(&:ok?) + + if results.empty? + Result.new(ok: true) + else + Result.new( + ok: false, + error_value: errors_headline + results.map(&:error_value).join(separator) + ) + end + end + + sig { params(configuration: Configuration, path: String).returns(Pathname) } + def relative_path(configuration, path) + Pathname.new(path).relative_path_from(configuration.root_path) + end + end + end + + private_constant :Helpers + end +end diff --git a/lib/packwerk/application_validator/result.rb b/lib/packwerk/application_validator/result.rb new file mode 100644 index 000000000..d75cb0cb0 --- /dev/null +++ b/lib/packwerk/application_validator/result.rb @@ -0,0 +1,18 @@ +# typed: strict +# frozen_string_literal: true + +module Packwerk + class ApplicationValidator + class Result < T::Struct + extend T::Sig + + const :ok, T::Boolean + const :error_value, T.nilable(String) + + sig { returns(T::Boolean) } + def ok? + ok + end + end + end +end diff --git a/test/unit/application_validator_test.rb b/test/unit/application_validator_test.rb index ccb00e6da..60bdfe845 100644 --- a/test/unit/application_validator_test.rb +++ b/test/unit/application_validator_test.rb @@ -3,11 +3,9 @@ require "test_helper" -# make sure PrivateThing.constantize succeeds to pass the privacy validity check -require "fixtures/skeleton/components/timeline/app/models/private_thing.rb" - module Packwerk class ApplicationValidatorTest < Minitest::Test + extend T::Sig include RailsApplicationFixtureHelper setup do @@ -36,16 +34,6 @@ class ApplicationValidatorTest < Minitest::Test assert_match(/Unknown keys/, result.error_value) end - test "check_package_manifest_syntax returns an error for invalid enforce_privacy value" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_privacy" => "yes, please." }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/Invalid 'enforce_privacy' option/, result.error_value) - end - test "check_package_manifest_syntax returns an error for invalid enforce_dependencies value" do use_template(:minimal) merge_into_app_yaml_file("package.yml", { "enforce_dependencies" => "components/sales" }) @@ -56,16 +44,6 @@ class ApplicationValidatorTest < Minitest::Test assert_match(/Invalid 'enforce_dependencies' option/, result.error_value) end - test "check_package_manifest_syntax returns an error for invalid public_path value" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "public_path" => [] }) - - result = validator.check_package_manifest_syntax - - refute result.ok? - assert_match(/'public_path' option must be a string/, result.error_value) - end - test "check_package_manifest_syntax returns error for invalid dependencies value" do use_template(:minimal) merge_into_app_yaml_file("components/timeline/package.yml", {}) @@ -77,59 +55,6 @@ class ApplicationValidatorTest < Minitest::Test assert_match(/Invalid 'dependencies' option/, result.error_value) end - test "check_package_manifests_for_privacy returns an error for unresolvable privatized constants" do - use_template(:skeleton) - ConstantResolver.expects(:new).returns(stub("resolver", resolve: nil)) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - /'::PrivateThing', listed in #{to_app_path('components\/timeline\/package.yml')}, could not be resolved/, - result.error_value - ) - assert_match( - /Add a private_thing.rb file/, - result.error_value - ) - end - - test "check_package_manifests_for_privacy returns an error for privatized constants in other packages" do - use_template(:skeleton) - context = ConstantResolver::ConstantContext.new("::PrivateThing", "private_thing.rb") - - ConstantResolver.expects(:new).returns(stub("resolver", resolve: context)) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - %r{'::PrivateThing' is declared as private in the 'components/timeline' package}, - result.error_value - ) - assert_match( - /but appears to be defined\sin the '.' package/, - result.error_value - ) - end - - test "check_package_manifests_for_privacy returns an error for constants without `::` prefix" do - use_template(:minimal) - merge_into_app_yaml_file("package.yml", { "enforce_privacy" => ["::PrivateThing", "OtherThing"] }) - - result = validator.check_package_manifests_for_privacy - - refute result.ok?, result.error_value - assert_match( - /'OtherThing', listed in the 'enforce_privacy' option in .*package.yml, is invalid./, - result.error_value - ) - assert_match( - /Private constants need to be prefixed with the top-level namespace operator `::`/, - result.error_value - ) - end - test "check_acyclic_graph returns error when package set contains circular dependencies" do use_template(:minimal) merge_into_app_yaml_file("components/sales/package.yml", { "dependencies" => ["components/timeline"] }) @@ -190,6 +115,7 @@ class ApplicationValidatorTest < Minitest::Test assert_match(/A root package does not exist./, result.error_value) end + sig { returns(Packwerk::ApplicationValidator) } def validator @application_validator ||= Packwerk::ApplicationValidator.new( config_file_path: config.config_path, diff --git a/test/unit/validators/check_package_manifests_for_privacy_test.rb b/test/unit/validators/check_package_manifests_for_privacy_test.rb new file mode 100644 index 000000000..b641e9156 --- /dev/null +++ b/test/unit/validators/check_package_manifests_for_privacy_test.rb @@ -0,0 +1,119 @@ +# typed: true +# frozen_string_literal: true + +require "test_helper" + +# make sure PrivateThing.constantize succeeds to pass the privacy validity check +require "fixtures/skeleton/components/timeline/app/models/private_thing" + +module Packwerk + class CheckPackageManifestsForPrivacyTest < Minitest::Test + extend T::Sig + include RailsApplicationFixtureHelper + + setup do + setup_application_fixture + end + + teardown do + teardown_application_fixture + end + + test "check_all returns an error for invalid enforce_privacy value" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_privacy" => "yes, please." }) + + result = Packwerk::ApplicationValidator::CheckPackageManifestsForPrivacy.call( + validator.package_set, + config + ) + + refute result.ok? + assert_match(/Invalid 'enforce_privacy' option/, result.error_value) + end + + test "check_all returns an error for invalid public_path value" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "public_path" => [] }) + + result = Packwerk::ApplicationValidator::CheckPackageManifestsForPrivacy.call( + validator.package_set, + config + ) + + refute result.ok? + assert_match(/'public_path' option must be a string/, result.error_value) + end + + test "check_package_manifests_for_privacy returns an error for unresolvable privatized constants" do + use_template(:skeleton) + ConstantResolver.expects(:new).returns(stub("resolver", resolve: nil)) + + result = Packwerk::ApplicationValidator::CheckPackageManifestsForPrivacy.call( + validator.package_set, + config + ) + + refute result.ok?, result.error_value + assert_match( + /'::PrivateThing', listed in #{to_app_path('components\/timeline\/package.yml')}, could not be resolved/, + result.error_value + ) + assert_match( + /Add a private_thing.rb file/, + result.error_value + ) + end + + test "check_package_manifests_for_privacy returns an error for privatized constants in other packages" do + use_template(:skeleton) + context = ConstantResolver::ConstantContext.new("::PrivateThing", "private_thing.rb") + + ConstantResolver.expects(:new).returns(stub("resolver", resolve: context)) + + result = Packwerk::ApplicationValidator::CheckPackageManifestsForPrivacy.call( + validator.package_set, + config + ) + + refute result.ok?, result.error_value + assert_match( + %r{'::PrivateThing' is declared as private in the 'components/timeline' package}, + result.error_value + ) + assert_match( + /but appears to be defined\sin the '.' package/, + result.error_value + ) + end + + test "check_package_manifests_for_privacy returns an error for constants without `::` prefix" do + use_template(:minimal) + merge_into_app_yaml_file("package.yml", { "enforce_privacy" => ["::PrivateThing", "OtherThing"] }) + + result = Packwerk::ApplicationValidator::CheckPackageManifestsForPrivacy.call( + validator.package_set, + config + ) + + refute result.ok?, result.error_value + assert_match( + /'OtherThing', listed in the 'enforce_privacy' option in .*package.yml, is invalid./, + result.error_value + ) + assert_match( + /Private constants need to be prefixed with the top-level namespace operator `::`/, + result.error_value + ) + end + + sig { returns(Packwerk::ApplicationValidator) } + def validator + @application_validator ||= Packwerk::ApplicationValidator.new( + config_file_path: config.config_path, + configuration: config, + environment: "test" + ) + end + end +end