diff --git a/Gemfile.lock b/Gemfile.lock index e45c8b9..54c72eb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - package_protections (1.0.0) + package_protections (1.1.0) activesupport parse_packwerk rubocop @@ -23,9 +23,9 @@ GEM i18n (1.10.0) concurrent-ruby (~> 1.0) method_source (1.0.0) - minitest (5.15.0) + minitest (5.16.1) parallel (1.21.0) - parse_packwerk (0.10.0) + parse_packwerk (0.10.1) sorbet-runtime parser (3.1.0.0) ast (~> 2.4.1) @@ -67,7 +67,7 @@ GEM parser (>= 3.0.1.1) rubocop-rspec (2.8.0) rubocop (~> 1.19) - rubocop-sorbet (0.6.8) + rubocop-sorbet (0.6.10) rubocop (>= 0.90.0) ruby-progressbar (1.11.0) sorbet (0.5.9588) diff --git a/lib/package_protections/private.rb b/lib/package_protections/private.rb index 56dc660..cc0b93a 100644 --- a/lib/package_protections/private.rb +++ b/lib/package_protections/private.rb @@ -3,11 +3,9 @@ require 'package_protections/private/colorized_string' require 'package_protections/private/output' -require 'package_protections/private/typed_api_protection' require 'package_protections/private/incoming_privacy_protection' require 'package_protections/private/outgoing_dependency_protection' require 'package_protections/private/metadata_modifiers' -require 'package_protections/private/multiple_namespaces_protection' require 'package_protections/private/visibility_protection' require 'package_protections/private/configuration' diff --git a/lib/package_protections/private/configuration.rb b/lib/package_protections/private/configuration.rb index 818ab6c..7c9a2bb 100644 --- a/lib/package_protections/private/configuration.rb +++ b/lib/package_protections/private/configuration.rb @@ -28,8 +28,8 @@ def default_protections [ Private::OutgoingDependencyProtection.new, Private::IncomingPrivacyProtection.new, - Private::TypedApiProtection.new, - Private::MultipleNamespacesProtection.new, + RuboCop::Cop::PackageProtections::TypedPublicApi.new, + RuboCop::Cop::PackageProtections::NamespacedUnderPackageName.new, Private::VisibilityProtection.new ] end diff --git a/lib/package_protections/private/multiple_namespaces_protection.rb b/lib/package_protections/private/multiple_namespaces_protection.rb deleted file mode 100644 index d8eb6ec..0000000 --- a/lib/package_protections/private/multiple_namespaces_protection.rb +++ /dev/null @@ -1,128 +0,0 @@ -# frozen_string_literal: true - -# typed: strict - -module PackageProtections - module Private - class MultipleNamespacesProtection - extend T::Sig - - include ProtectionInterface - include RubocopProtectionInterface - - IDENTIFIER = 'prevent_this_package_from_creating_other_namespaces' - COP_NAME = 'PackageProtections/NamespacedUnderPackageName' - - sig { override.returns(String) } - def identifier - IDENTIFIER - end - - sig { override.params(behavior: ViolationBehavior, package: ParsePackwerk::Package).returns(T.nilable(String)) } - def unmet_preconditions_for_behavior(behavior, package) - if !behavior.enabled? && !package.metadata['global_namespaces'].nil? - "Invalid configuration for package `#{package.name}`. `#{identifier}` must be turned on to use `global_namespaces` configuration." - else - # We don't need to validate if the behavior is currentely fail_never - return if behavior.fail_never? - - # The reason for this is precondition is the `MultipleNamespacesProtection` assumes this to work properly. - # To remove this precondition, we need to modify `MultipleNamespacesProtection` to be more generalized! - if EXPECTED_PACK_DIRECTORIES.include?(Pathname.new(package.name).dirname.to_s) || package.name == ParsePackwerk::ROOT_PACKAGE_NAME - nil - else - "Package #{package.name} must be located in one of #{EXPECTED_PACK_DIRECTORIES.join(', ')} (or be the root) to use this protection" - end - end - end - - sig do - override - .params(packages: T::Array[ProtectedPackage]) - .returns(T::Array[CopConfig]) - end - def cop_configs(packages) - include_paths = T.let([], T::Array[String]) - packages.each do |p| - next if p.name == ParsePackwerk::ROOT_PACKAGE_NAME - - if p.violation_behavior_for(identifier).enabled? - include_paths << p.original_package.directory.join('app', '**', '*').to_s - include_paths << p.original_package.directory.join('lib', '**', '*').to_s - end - end - - [ - CopConfig.new( - name: COP_NAME, - enabled: include_paths.any?, - include_paths: include_paths - ) - ] - end - - sig do - params(package: ProtectedPackage).returns(T::Hash[T.untyped, T.untyped]) - end - def custom_cop_config(package) - { - 'GlobalNamespaces' => package.metadata['global_namespaces'] - } - end - - sig do - override.params( - protected_packages: T::Array[ProtectedPackage] - ).returns(T::Array[Offense]) - end - def get_offenses_for_existing_violations(protected_packages) - exclude_list = exclude_for_rule(COP_NAME) - offenses = [] - - protected_packages.each do |package| - violation_behavior = package.violation_behavior_for(identifier) - - case violation_behavior - when ViolationBehavior::FailNever, ViolationBehavior::FailOnNew - next - when ViolationBehavior::FailOnAny - # Continue - else - T.absurd(violation_behavior) - end - - package.original_package.directory.glob('**/**/*.*').each do |relative_path_to_file| - next unless exclude_list.include?(relative_path_to_file.to_s) - - file = relative_path_to_file.to_s - offenses << Offense.new( - file: file, - message: "`#{file}` should be namespaced under the package namespace", - violation_type: identifier, - package: package.original_package - ) - end - end - - offenses - end - - sig { override.returns(String) } - def humanized_protection_name - 'Multiple Namespaces Violations' - end - - sig { override.returns(String) } - def humanized_protection_description - <<~MESSAGE - These files cannot have ANY modules/classes that are not submodules of the package's allowed namespaces. - This is failing because these files are in `.rubocop_todo.yml` under `#{COP_NAME}`. - If you want to be able to ignore these files, you'll need to open the file's package's `package.yml` file and - change `#{IDENTIFIER}` to `#{ViolationBehavior::FailOnNew.serialize}` - - See https://go/packwerk_cheatsheet_namespaces for more info. - MESSAGE - end - end - end -end diff --git a/lib/package_protections/private/typed_api_protection.rb b/lib/package_protections/private/typed_api_protection.rb deleted file mode 100644 index b1c17eb..0000000 --- a/lib/package_protections/private/typed_api_protection.rb +++ /dev/null @@ -1,106 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module PackageProtections - module Private - class TypedApiProtection - extend T::Sig - - include ProtectionInterface - include RubocopProtectionInterface - - IDENTIFIER = 'prevent_this_package_from_exposing_an_untyped_api' - COP_NAME = 'PackageProtections/TypedPublicApi' - - sig { override.returns(String) } - def identifier - IDENTIFIER - end - - sig { override.params(behavior: ViolationBehavior, package: ParsePackwerk::Package).returns(T.nilable(String)) } - def unmet_preconditions_for_behavior(behavior, package) - # We might decide that we should check that `package.enforces_privacy?` is true here too, since that signifies the app has decided they want - # a public api in `app/public`. For now, we say there are no preconditions, because the user can still make `app/public` even if they are not yet - # ready to enforce privacy, and they might want to enforce a typed API. - nil - end - - sig do - override - .params(packages: T::Array[ProtectedPackage]) - .returns(T::Array[CopConfig]) - end - def cop_configs(packages) - include_paths = T.let([], T::Array[String]) - packages.each do |p| - if p.violation_behavior_for(identifier).enabled? - directory = p.original_package.directory - include_paths << directory.join('app', 'public', '**', '*').to_s - end - end - - [ - CopConfig.new( - name: COP_NAME, - enabled: include_paths.any?, - include_paths: include_paths - ) - ] - end - - sig do - override.params( - protected_packages: T::Array[ProtectedPackage] - ).returns(T::Array[Offense]) - end - def get_offenses_for_existing_violations(protected_packages) - exclude_list = exclude_for_rule(COP_NAME) - - offenses = T.let([], T::Array[Offense]) - protected_packages.flat_map do |protected_package| - violation_behavior = protected_package.violation_behavior_for(identifier) - - case violation_behavior - when ViolationBehavior::FailNever, ViolationBehavior::FailOnNew - next - when ViolationBehavior::FailOnAny - # Continue - else - T.absurd(violation_behavior) - end - - protected_package.original_package.directory.glob('app/public/**/**/*.*').each do |relative_path_to_file| - next unless exclude_list.include?(relative_path_to_file.to_s) - - file = relative_path_to_file.to_s - offenses << Offense.new( - file: file, - message: "#{file} should be `typed: strict`", - violation_type: identifier, - package: protected_package.original_package - ) - end - end - - offenses - end - - sig { override.returns(String) } - def humanized_protection_name - 'Typed API Violations' - end - - sig { override.returns(String) } - def humanized_protection_description - <<~MESSAGE - These files cannot have ANY Ruby files in the public API that are not typed strict or higher. - This is failing because these files are in `.rubocop_todo.yml` under `#{COP_NAME}`. - If you want to be able to ignore these files, you'll need to open the file's package's `package.yml` file and - change `#{IDENTIFIER}` to `#{ViolationBehavior::FailOnNew.serialize}` - - See https://go/packwerk_cheatsheet_typed_api for more info. - MESSAGE - end - end - end -end diff --git a/lib/package_protections/rubocop_protection_interface.rb b/lib/package_protections/rubocop_protection_interface.rb index 5e3f2ab..6085991 100644 --- a/lib/package_protections/rubocop_protection_interface.rb +++ b/lib/package_protections/rubocop_protection_interface.rb @@ -3,26 +3,6 @@ # typed: strict module PackageProtections module RubocopProtectionInterface - include ProtectionInterface - extend T::Sig - extend T::Helpers - - abstract! - - sig do - abstract - .params(packages: T::Array[ProtectedPackage]) - .returns(T::Array[CopConfig]) - end - def cop_configs(packages); end - - sig do - params(package: ProtectedPackage).returns(T::Hash[T.untyped, T.untyped]) - end - def custom_cop_config(package) - {} - end - class CopConfig < T::Struct extend T::Sig const :name, String @@ -46,6 +26,40 @@ def to_rubocop_yml_compatible_format end end + include ProtectionInterface + extend T::Sig + extend T::Helpers + + abstract! + + ########################################################################### + # Abstract Methods: These are methods that the client needs to implement + ############################################################################ + sig { abstract.returns(String) } + def cop_name; end + + sig do + abstract.params(file: String).returns(String) + end + def message_for_fail_on_any(file); end + + sig { abstract.returns(T::Array[String]) } + def included_globs_for_pack; end + + ########################################################################### + # Overriddable Methods: These are methods that the client can override, + # but a default is provided. + ############################################################################ + sig do + params(package: ProtectedPackage).returns(T::Hash[T.untyped, T.untyped]) + end + def custom_cop_config(package) + {} + end + + sig { override.params(behavior: ViolationBehavior, package: ParsePackwerk::Package).returns(T.nilable(String)) } + def unmet_preconditions_for_behavior(behavior, package); end + sig do override.params( new_violations: T::Array[PerFileViolation] @@ -73,6 +87,66 @@ def self.rubocop_todo_yml end end + sig do + override.params( + protected_packages: T::Array[ProtectedPackage] + ).returns(T::Array[Offense]) + end + def get_offenses_for_existing_violations(protected_packages) + exclude_list = exclude_for_rule(cop_name) + offenses = [] + + protected_packages.each do |package| + violation_behavior = package.violation_behavior_for(identifier) + + case violation_behavior + when ViolationBehavior::FailNever, ViolationBehavior::FailOnNew + next + when ViolationBehavior::FailOnAny + # Continue + else + T.absurd(violation_behavior) + end + + package.original_package.directory.glob('**/**/*.*').each do |relative_path_to_file| + next unless exclude_list.include?(relative_path_to_file.to_s) + + file = relative_path_to_file.to_s + offenses << Offense.new( + file: file, + message: message_for_fail_on_any(file), + violation_type: identifier, + package: package.original_package + ) + end + end + + offenses + end + + sig do + params(packages: T::Array[ProtectedPackage]) + .returns(T::Array[CopConfig]) + end + def cop_configs(packages) + include_paths = T.let([], T::Array[String]) + packages.each do |p| + next unless p.violation_behavior_for(identifier).enabled? + + included_globs_for_pack.each do |glob| + include_paths << p.original_package.directory.join(glob).to_s + end + end + + [ + CopConfig.new( + name: cop_name, + enabled: include_paths.any?, + include_paths: include_paths + ) + ] + end + private sig { params(rule: String).returns(T::Set[String]) } diff --git a/lib/rubocop/cop/package_protections/namespaced_under_package_name.rb b/lib/rubocop/cop/package_protections/namespaced_under_package_name.rb index 04aea38..1867d2c 100644 --- a/lib/rubocop/cop/package_protections/namespaced_under_package_name.rb +++ b/lib/rubocop/cop/package_protections/namespaced_under_package_name.rb @@ -7,7 +7,10 @@ module RuboCop module Cop module PackageProtections class NamespacedUnderPackageName < Base + extend T::Sig + include RangeHelp + include ::PackageProtections::RubocopProtectionInterface def on_new_investigation absolute_filepath = Pathname.new(processed_source.file_path) @@ -85,6 +88,77 @@ def on_new_investigation end end + IDENTIFIER = 'prevent_this_package_from_creating_other_namespaces'.freeze + + sig { override.returns(String) } + def identifier + IDENTIFIER + end + + sig { override.params(behavior: ::PackageProtections::ViolationBehavior, package: ParsePackwerk::Package).returns(T.nilable(String)) } + def unmet_preconditions_for_behavior(behavior, package) + if !behavior.enabled? && !package.metadata['global_namespaces'].nil? + "Invalid configuration for package `#{package.name}`. `#{identifier}` must be turned on to use `global_namespaces` configuration." + else + # We don't need to validate if the behavior is currentely fail_never + return if behavior.fail_never? + + # The reason for this is precondition is the `MultipleNamespacesProtection` assumes this to work properly. + # To remove this precondition, we need to modify `MultipleNamespacesProtection` to be more generalized! + if ::PackageProtections::EXPECTED_PACK_DIRECTORIES.include?(Pathname.new(package.name).dirname.to_s) || package.name == ParsePackwerk::ROOT_PACKAGE_NAME + nil + else + "Package #{package.name} must be located in one of #{::PackageProtections::EXPECTED_PACK_DIRECTORIES.join(', ')} (or be the root) to use this protection" + end + end + end + + sig { override.returns(T::Array[String]) } + def included_globs_for_pack + [ + 'app/**/*', + 'lib/**/*' + ] + end + + sig do + params(package: ::PackageProtections::ProtectedPackage).returns(T::Hash[T.untyped, T.untyped]) + end + def custom_cop_config(package) + { + 'GlobalNamespaces' => package.metadata['global_namespaces'] + } + end + + sig do + override.params(file: String).returns(String) + end + def message_for_fail_on_any(file) + "`#{file}` should be namespaced under the package namespace" + end + + sig { override.returns(String) } + def cop_name + 'PackageProtections/NamespacedUnderPackageName' + end + + sig { override.returns(String) } + def humanized_protection_name + 'Multiple Namespaces Violations' + end + + sig { override.returns(String) } + def humanized_protection_description + <<~MESSAGE + These files cannot have ANY modules/classes that are not submodules of the package's allowed namespaces. + This is failing because these files are in `.rubocop_todo.yml` under `#{cop_name}`. + If you want to be able to ignore these files, you'll need to open the file's package's `package.yml` file and + change `#{IDENTIFIER}` to `#{::PackageProtections::ViolationBehavior::FailOnNew.serialize}` + + See https://go/packwerk_cheatsheet_namespaces for more info. + MESSAGE + end + private def get_allowed_namespaces(package_name) diff --git a/lib/rubocop/cop/package_protections/typed_public_api.rb b/lib/rubocop/cop/package_protections/typed_public_api.rb index b521c03..ee05b85 100644 --- a/lib/rubocop/cop/package_protections/typed_public_api.rb +++ b/lib/rubocop/cop/package_protections/typed_public_api.rb @@ -16,6 +16,61 @@ module PackageProtections # We can apply this same pattern if we want to use other cops in the context of package protections and prevent clashing. # class TypedPublicApi < Sorbet::StrictSigil + extend T::Sig + + include ::PackageProtections::ProtectionInterface + include ::PackageProtections::RubocopProtectionInterface + + IDENTIFIER = T.let('prevent_this_package_from_exposing_an_untyped_api'.freeze, String) + + sig { override.returns(String) } + def identifier + IDENTIFIER + end + + sig { override.params(behavior: ::PackageProtections::ViolationBehavior, package: ParsePackwerk::Package).returns(T.nilable(String)) } + def unmet_preconditions_for_behavior(behavior, package) + # We might decide that we should check that `package.enforces_privacy?` is true here too, since that signifies the app has decided they want + # a public api in `app/public`. For now, we say there are no preconditions, because the user can still make `app/public` even if they are not yet + # ready to enforce privacy, and they might want to enforce a typed API. + nil + end + + sig { override.returns(String) } + def cop_name + 'PackageProtections/TypedPublicApi' + end + + sig { override.returns(T::Array[String]) } + def included_globs_for_pack + [ + 'app/public/**/*' + ] + end + + sig do + override.params(file: String).returns(String) + end + def message_for_fail_on_any(file) + "#{file} should be `typed: strict`" + end + + sig { override.returns(String) } + def humanized_protection_name + 'Typed API Violations' + end + + sig { override.returns(String) } + def humanized_protection_description + <<~MESSAGE + These files cannot have ANY Ruby files in the public API that are not typed strict or higher. + This is failing because these files are in `.rubocop_todo.yml` under `#{cop_name}`. + If you want to be able to ignore these files, you'll need to open the file's package's `package.yml` file and + change `#{IDENTIFIER}` to `#{::PackageProtections::ViolationBehavior::FailOnNew.serialize}` + + See https://go/packwerk_cheatsheet_typed_api for more info. + MESSAGE + end end end end diff --git a/package_protections.gemspec b/package_protections.gemspec index ab19b3f..78b3eaa 100644 --- a/package_protections.gemspec +++ b/package_protections.gemspec @@ -1,6 +1,6 @@ Gem::Specification.new do |spec| spec.name = 'package_protections' - spec.version = '1.0.0' + spec.version = '1.1.0' spec.authors = ['Gusto Engineers'] spec.email = ['stephan.hagemann@gusto.com'] spec.summary = 'Package protections for Rails apps' diff --git a/spec/package_protections_spec.rb b/spec/package_protections_spec.rb index e4160a5..2b9292c 100644 --- a/spec/package_protections_spec.rb +++ b/spec/package_protections_spec.rb @@ -665,7 +665,9 @@ def offense( expect(offenses).to contain_exactly(0).offenses end - it 'succeeds even if there are files from private implementation in the rubocop TODO list' do + # Note -- to simplify implementation, we look at *any* files in the exclude list. + # There shouldn't be much of a reason to add to this list with files that the cop doesn't look at in the first place + it 'fails when there are files from private implementation in the rubocop TODO list' do apples_package_yml_with_typed_api_protection_set_to_fail_on_any write_file('packs/apples/app/services/tool.rb', '') write_file('.rubocop_todo.yml', <<~YML.strip) @@ -675,7 +677,11 @@ def offense( YML offenses = PackageProtections.get_offenses(packages: get_packages, new_violations: []) - expect(offenses).to contain_exactly(0).offenses + expect(offenses).to contain_exactly(1).offense + expect(offenses).to include_offense offense( + 'packs/apples', + 'packs/apples/app/services/tool.rb should be `typed: strict`', 'packs/apples/app/services/tool.rb', 'prevent_this_package_from_exposing_an_untyped_api' + ) end it 'fails if there is a public API file in the rubocop TODO list' do