From 5743a6ec1e5dfb9ef2415b09dc6cdd412edba8b9 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 29 Mar 2022 13:38:42 +0800 Subject: [PATCH] DEV: Remove Zeitwerk inflection monkey patch. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There isn't a good reason we need to patch the inflector. Co-authored-by: Loïc Guitaut --- app/jobs/onceoff/onceoff.rb | 2 - config/application.rb | 7 +- config/initializers/000-zeitwerk.rb | 31 +++ config/initializers/100-sidekiq.rb | 197 +++++++++--------- ...quire_dependency_backward_compatibility.rb | 22 ++ lib/zeitwerk_config.rb | 22 -- spec/jobs/create_linked_topic_spec.rb | 2 - spec/jobs/feature_topic_users_spec.rb | 2 - spec/jobs/process_post_spec.rb | 2 - spec/jobs/pull_hotlinked_images_spec.rb | 2 - spec/jobs/send_system_message_spec.rb | 2 - 11 files changed, 150 insertions(+), 141 deletions(-) create mode 100644 config/initializers/000-zeitwerk.rb create mode 100644 lib/require_dependency_backward_compatibility.rb delete mode 100644 lib/zeitwerk_config.rb diff --git a/app/jobs/onceoff/onceoff.rb b/app/jobs/onceoff/onceoff.rb index 847779b817b..d10b1f41a66 100644 --- a/app/jobs/onceoff/onceoff.rb +++ b/app/jobs/onceoff/onceoff.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require_relative '../base.rb' - class Jobs::Onceoff < ::Jobs::Base sidekiq_options retry: false diff --git a/config/application.rb b/config/application.rb index 1d9600d8173..1bd7ba921ec 100644 --- a/config/application.rb +++ b/config/application.rb @@ -55,8 +55,6 @@ require 'pry-rails' if Rails.env.development? require 'discourse_fonts' -require_relative '../lib/zeitwerk_config.rb' - if defined?(Bundler) bundler_groups = [:default] @@ -69,6 +67,8 @@ if defined?(Bundler) Bundler.require(*bundler_groups) end +require_relative '../lib/require_dependency_backward_compatibility' + module Discourse class Application < Rails::Application @@ -110,9 +110,6 @@ module Discourse config.autoloader = :zeitwerk # Custom directories with classes and modules you want to be autoloadable. - config.autoload_paths += Dir["#{config.root}/app"] - config.autoload_paths += Dir["#{config.root}/app/jobs"] - config.autoload_paths += Dir["#{config.root}/app/serializers"] config.autoload_paths += Dir["#{config.root}/lib"] config.autoload_paths += Dir["#{config.root}/lib/common_passwords"] config.autoload_paths += Dir["#{config.root}/lib/highlight_js"] diff --git a/config/initializers/000-zeitwerk.rb b/config/initializers/000-zeitwerk.rb new file mode 100644 index 00000000000..f0b8feaef18 --- /dev/null +++ b/config/initializers/000-zeitwerk.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +# This custom inflector is needed because of our jobs directory structure. +# Ideally, we should not prefix our jobs with a `Jobs` namespace but instead +# have a `Job` suffix to follow the Rails conventions on naming. +class DiscourseInflector < Zeitwerk::Inflector + def camelize(basename, abspath) + return basename.camelize if abspath.ends_with?("onceoff.rb") + return 'Jobs' if abspath.ends_with?("jobs/base.rb") + super + end +end + +Rails.autoloaders.each do |autoloader| + autoloader.inflector = DiscourseInflector.new + + # We have filenames that do not follow Zeitwerk's camelization convention. Maintain an inflections for these files + # for now until we decide to fix them one day. + autoloader.inflector.inflect( + 'canonical_url' => 'CanonicalURL', + 'clean_up_unmatched_ips' => 'CleanUpUnmatchedIPs', + 'homepage_constraint' => 'HomePageConstraint', + 'ip_addr' => 'IPAddr', + 'onpdiff' => 'ONPDiff', + 'pop3_polling_enabled_setting_validator' => 'POP3PollingEnabledSettingValidator', + 'version' => 'Discourse', + 'onceoff' => 'Jobs', + 'regular' => 'Jobs', + 'scheduled' => 'Jobs', + ) +end diff --git a/config/initializers/100-sidekiq.rb b/config/initializers/100-sidekiq.rb index 4f7a77b69a5..211f667a3a6 100644 --- a/config/initializers/100-sidekiq.rb +++ b/config/initializers/100-sidekiq.rb @@ -1,126 +1,119 @@ # frozen_string_literal: true -# Ensure that scheduled jobs are loaded before mini_scheduler is configured. -if Rails.env == "development" - require "jobs/base" +Rails.application.reloader.to_prepare do + require "sidekiq/pausable" - Dir.glob("#{Rails.root}/app/jobs/scheduled/*.rb") do |f| - require(f) - end -end - -require "sidekiq/pausable" - -Sidekiq.configure_client do |config| - config.redis = Discourse.sidekiq_redis_config -end - -Sidekiq.configure_server do |config| - config.redis = Discourse.sidekiq_redis_config - - config.server_middleware do |chain| - chain.add Sidekiq::Pausable - end -end - -MiniScheduler.configure do |config| - - config.redis = Discourse.redis - - config.job_exception_handler do |ex, context| - Discourse.handle_job_exception(ex, context) + Sidekiq.configure_client do |config| + config.redis = Discourse.sidekiq_redis_config end - config.job_ran do |stat| - DiscourseEvent.trigger(:scheduled_job_ran, stat) + Sidekiq.configure_server do |config| + config.redis = Discourse.sidekiq_redis_config + + config.server_middleware do |chain| + chain.add Sidekiq::Pausable + end end - config.skip_schedule { Sidekiq.paused? } + MiniScheduler.configure do |config| + + config.redis = Discourse.redis + + config.job_exception_handler do |ex, context| + Discourse.handle_job_exception(ex, context) + end + + config.job_ran do |stat| + DiscourseEvent.trigger(:scheduled_job_ran, stat) + end + + config.skip_schedule { Sidekiq.paused? } + + config.before_sidekiq_web_request do + RailsMultisite::ConnectionManagement.establish_connection( + db: RailsMultisite::ConnectionManagement::DEFAULT + ) + end - config.before_sidekiq_web_request do - RailsMultisite::ConnectionManagement.establish_connection( - db: RailsMultisite::ConnectionManagement::DEFAULT - ) end -end + if Sidekiq.server? -if Sidekiq.server? + module Sidekiq + class CLI + private - module Sidekiq - class CLI - private + def print_banner + # banner takes up too much space + end + end + end - def print_banner - # banner takes up too much space + # defer queue should simply run in sidekiq + Scheduler::Defer.async = false + + Rails.application.config.after_initialize do + # warm up AR + RailsMultisite::ConnectionManagement.safe_each_connection do + (ActiveRecord::Base.connection.tables - %w[schema_migrations versions]).each do |table| + table.classify.constantize.first rescue nil + end + end + + scheduler_hostname = ENV["UNICORN_SCHEDULER_HOSTNAME"] + + if !scheduler_hostname || scheduler_hostname.split(',').include?(Discourse.os_hostname) + begin + MiniScheduler.start(workers: GlobalSetting.mini_scheduler_workers) + rescue MiniScheduler::DistributedMutex::Timeout + sleep 5 + retry + end end end end - # defer queue should simply run in sidekiq - Scheduler::Defer.async = false + # Sidekiq#logger= applies patches to whichever logger we pass it. + # Therefore something like Sidekiq.logger = Rails.logger will break + # all logging in the application. + # + # Instead, this patch adds a dedicated logger instance and patches + # the #add method to forward messages to Rails.logger. + Sidekiq.logger = Logger.new(nil) + Sidekiq.logger.define_singleton_method(:add) do |severity, message = nil, progname = nil, &blk| + Rails.logger.add(severity, message, progname, &blk) + end - Rails.application.config.after_initialize do - # warm up AR - RailsMultisite::ConnectionManagement.safe_each_connection do - (ActiveRecord::Base.connection.tables - %w[schema_migrations versions]).each do |table| - table.classify.constantize.first rescue nil + class SidekiqLogsterReporter < Sidekiq::ExceptionHandler::Logger + def call(ex, context = {}) + + return if Jobs::HandledExceptionWrapper === ex + Discourse.reset_active_record_cache_if_needed(ex) + + # Pass context to Logster + fake_env = {} + context.each do |key, value| + Logster.add_to_env(fake_env, key, value) end - end - scheduler_hostname = ENV["UNICORN_SCHEDULER_HOSTNAME"] - - if !scheduler_hostname || scheduler_hostname.split(',').include?(Discourse.os_hostname) - begin - MiniScheduler.start(workers: GlobalSetting.mini_scheduler_workers) - rescue MiniScheduler::DistributedMutex::Timeout - sleep 5 - retry + text = "Job exception: #{ex}\n" + if ex.backtrace + Logster.add_to_env(fake_env, :backtrace, ex.backtrace) end + + Logster.add_to_env(fake_env, :current_hostname, Discourse.current_hostname) + + Thread.current[Logster::Logger::LOGSTER_ENV] = fake_env + Logster.logger.error(text) + rescue => e + Logster.logger.fatal("Failed to log exception #{ex} #{hash}\nReason: #{e.class} #{e}\n#{e.backtrace.join("\n")}") + ensure + Thread.current[Logster::Logger::LOGSTER_ENV] = nil end end + + Sidekiq.error_handlers.clear + Sidekiq.error_handlers << SidekiqLogsterReporter.new + + Sidekiq.strict_args! end - -# Sidekiq#logger= applies patches to whichever logger we pass it. -# Therefore something like Sidekiq.logger = Rails.logger will break -# all logging in the application. -# -# Instead, this patch adds a dedicated logger instance and patches -# the #add method to forward messages to Rails.logger. -Sidekiq.logger = Logger.new(nil) -Sidekiq.logger.define_singleton_method(:add) do |severity, message = nil, progname = nil, &blk| - Rails.logger.add(severity, message, progname, &blk) -end - -class SidekiqLogsterReporter < Sidekiq::ExceptionHandler::Logger - def call(ex, context = {}) - - return if Jobs::HandledExceptionWrapper === ex - Discourse.reset_active_record_cache_if_needed(ex) - - # Pass context to Logster - fake_env = {} - context.each do |key, value| - Logster.add_to_env(fake_env, key, value) - end - - text = "Job exception: #{ex}\n" - if ex.backtrace - Logster.add_to_env(fake_env, :backtrace, ex.backtrace) - end - - Logster.add_to_env(fake_env, :current_hostname, Discourse.current_hostname) - - Thread.current[Logster::Logger::LOGSTER_ENV] = fake_env - Logster.logger.error(text) - rescue => e - Logster.logger.fatal("Failed to log exception #{ex} #{hash}\nReason: #{e.class} #{e}\n#{e.backtrace.join("\n")}") - ensure - Thread.current[Logster::Logger::LOGSTER_ENV] = nil - end -end - -Sidekiq.error_handlers.clear -Sidekiq.error_handlers << SidekiqLogsterReporter.new - -Sidekiq.strict_args! diff --git a/lib/require_dependency_backward_compatibility.rb b/lib/require_dependency_backward_compatibility.rb new file mode 100644 index 00000000000..ced0bbbeeb4 --- /dev/null +++ b/lib/require_dependency_backward_compatibility.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# Patch `require_dependency` to maintain backward compatibility with some +# plugins. Calls to `require_dependency` are deprecated and we should remove +# them whenever possible. +# +# Here we do nothing if `jobs/base` is required since all our jobs are +# autoloaded through Zeitwerk. Requiring explicitly `jobs/base` actually breaks +# the app with the “new” autoloader. +# `lib` should not appear in a path that is required but we had probably a bug +# at some point regarding this matter so we need to maintain compatibility with +# some plugins that rely on this. +module RequireDependencyBackwardCompatibility + def require_dependency(filename) + name = filename.to_s + return if name == 'jobs/base' + return super(name.sub(/^lib\//, '')) if name.start_with?('lib/') + super + end + + ActiveSupport::Dependencies::ZeitwerkIntegration::RequireDependency.prepend(self) +end diff --git a/lib/zeitwerk_config.rb b/lib/zeitwerk_config.rb deleted file mode 100644 index 0c5882f8cab..00000000000 --- a/lib/zeitwerk_config.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -module ActiveSupport::Dependencies::ZeitwerkIntegration::Inflector - CUSTOM_PATHS = { - 'canonical_url' => 'CanonicalURL', - 'clean_up_unmatched_ips' => 'CleanUpUnmatchedIPs', - 'homepage_constraint' => 'HomePageConstraint', - 'ip_addr' => 'IPAddr', - 'onpdiff' => 'ONPDiff', - 'onceoff' => 'Jobs', - 'pop3_polling_enabled_setting_validator' => 'POP3PollingEnabledSettingValidator', - 'regular' => 'Jobs', - 'scheduled' => 'Jobs', - 'version' => 'Discourse', - } - - def self.camelize(basename, abspath) - return basename.camelize if abspath.ends_with?("onceoff.rb") - return 'Jobs' if abspath.ends_with?("jobs/base.rb") - CUSTOM_PATHS.fetch(basename, basename.camelize) - end -end diff --git a/spec/jobs/create_linked_topic_spec.rb b/spec/jobs/create_linked_topic_spec.rb index 3765a7d45f5..ccc0a16fa15 100644 --- a/spec/jobs/create_linked_topic_spec.rb +++ b/spec/jobs/create_linked_topic_spec.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'jobs/regular/create_linked_topic' - describe Jobs::CreateLinkedTopic do it "returns when the post cannot be found" do diff --git a/spec/jobs/feature_topic_users_spec.rb b/spec/jobs/feature_topic_users_spec.rb index 56a95602d08..84d375da8c9 100644 --- a/spec/jobs/feature_topic_users_spec.rb +++ b/spec/jobs/feature_topic_users_spec.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'jobs/regular/process_post' - describe Jobs::FeatureTopicUsers do it "raises an error without a topic_id" do diff --git a/spec/jobs/process_post_spec.rb b/spec/jobs/process_post_spec.rb index 84fa44fe9cf..6d5d65bb791 100644 --- a/spec/jobs/process_post_spec.rb +++ b/spec/jobs/process_post_spec.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'jobs/regular/process_post' - describe Jobs::ProcessPost do it "returns when the post cannot be found" do diff --git a/spec/jobs/pull_hotlinked_images_spec.rb b/spec/jobs/pull_hotlinked_images_spec.rb index d41b9d31d89..1d478d1d163 100644 --- a/spec/jobs/pull_hotlinked_images_spec.rb +++ b/spec/jobs/pull_hotlinked_images_spec.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'jobs/regular/pull_hotlinked_images' - describe Jobs::PullHotlinkedImages do let(:image_url) { "http://wiki.mozilla.org/images/2/2e/Longcat1.png" } diff --git a/spec/jobs/send_system_message_spec.rb b/spec/jobs/send_system_message_spec.rb index 0dd4f1d2c36..db6540eee5e 100644 --- a/spec/jobs/send_system_message_spec.rb +++ b/spec/jobs/send_system_message_spec.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'jobs/regular/send_system_message' - describe Jobs::SendSystemMessage do it "raises an error without a user_id" do