Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract privacy validations from ApplicationValidator #232

Merged
merged 1 commit into from
Oct 31, 2022
+362 −223
Merged

Extract privacy validations from ApplicationValidator #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/packwerk.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,12 @@ module Checkers
autoload :PrivacyChecker
end
end

class ApplicationValidator
extend ActiveSupport::Autoload

autoload :Result
autoload :CheckPackageManifestsForPrivacy
autoload :Helpers
end
end
173 changes: 26 additions & 147 deletions lib/packwerk/application_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# This API would likely just be `PackageSet.load_all_from(configruation)`, but we might want to clean
# This API would likely just be `PackageSet.load_all_from(configuration)`, 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,
Expand All @@ -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,
Expand All @@ -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"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't want the main validator knowing anything about enforce_privacy, so this can become a plugin concern.

I was thinking that a plugin can implement one or both interfaces – a checker interface or a validator interface (or just a single Plugin interface which is both of these). The validator would load those and allow top-level keys that plugins can specify.

known_keys = [
*CheckPackageManifestsForPrivacy.permitted_keys,
"enforce_dependencies",
"dependencies",
"metadata",
]
unknown_keys = hash.keys - known_keys

unless unknown_keys.empty?
Expand All @@ -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)

Expand Down Expand Up @@ -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

Expand All @@ -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 =
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) }
Expand All @@ -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
Loading