From 12cc24a733190693c9e703c3edbd024a445d52db Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 25 Aug 2021 14:22:40 +0200 Subject: [PATCH] Refactor ActiveRecord::QueryLogs hook point Hooking into `execute` and `exec_query` is problematic because some adapters like MySQL2 have `exec_query` call `execute` which forces QueryLogs to first check wether the comment was already applied. Using a prepended module is also a bit problematic because it means it has to be prepended to the "final" adapter classes but if the user application has a custom adapter that inherits from a built-in one, the built-in one no longer have QueryLogs working. So instead this PR introduce `ActiveRecord.query_transformers`, and the adpters are responsible for applying to transformers only once. --- activerecord/lib/active_record.rb | 3 +++ .../connection_adapters/abstract_adapter.rb | 7 ++++++ .../mysql/database_statements.rb | 17 ++++++++++++-- .../postgresql/database_statements.rb | 1 + .../connection_adapters/postgresql_adapter.rb | 1 + .../sqlite3/database_statements.rb | 3 +++ activerecord/lib/active_record/query_logs.rb | 23 ++++++------------- activerecord/lib/active_record/railtie.rb | 7 +----- activerecord/test/cases/query_logs_test.rb | 23 ++++++------------- railties/test/application/query_logs_test.rb | 8 +++---- 10 files changed, 49 insertions(+), 44 deletions(-) diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 9f2ecb8a65..f9aded49fe 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -323,6 +323,9 @@ module ActiveRecord singleton_class.attr_accessor :verify_foreign_keys_for_fixtures self.verify_foreign_keys_for_fixtures = false + singleton_class.attr_accessor :query_transformers + self.query_transformers = [] + def self.eager_load! super ActiveRecord::Locking.eager_load! diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 4b672717cf..afc89c2b59 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -741,6 +741,13 @@ module ActiveRecord end end + def transform_query(sql) + ActiveRecord.query_transformers.each do |transformer| + sql = transformer.call(sql) + end + sql + end + def translate_exception(exception, message:, sql:, binds:) # override in derived class case exception diff --git a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb index 610c4efedb..38a4a997dd 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb @@ -37,15 +37,26 @@ module ActiveRecord MySQL::ExplainPrettyPrinter.new.pp(result, elapsed) end + def execute(sql, name = nil, async: false) + # make sure we carry over any changes to ActiveRecord.default_timezone that have been + # made since we established the connection + @connection.query_options[:database_timezone] = ActiveRecord.default_timezone + + super + end + alias_method :raw_execute, :execute + private :raw_execute + # Executes the SQL statement in the context of this connection. def execute(sql, name = nil, async: false) + sql = transform_query(sql) check_if_write_query(sql) # make sure we carry over any changes to ActiveRecord.default_timezone that have been # made since we established the connection @connection.query_options[:database_timezone] = ActiveRecord.default_timezone - super + raw_execute(sql, name, async: async) end def exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: @@ -81,8 +92,9 @@ module ActiveRecord private def execute_batch(statements, name = nil) + statements = statements.map { |sql| transform_query(sql) } combine_multi_statements(statements).each do |statement| - execute(statement, name) + raw_execute(statement, name) end @connection.abandon_results! end @@ -147,6 +159,7 @@ module ActiveRecord end def exec_stmt_and_free(sql, name, binds, cache_stmt: false, async: false) + sql = transform_query(sql) check_if_write_query(sql) materialize_transactions diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb index 33010b0b92..54c5549058 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb @@ -35,6 +35,7 @@ module ActiveRecord # Note: the PG::Result object is manually memory managed; if you don't # need it specifically, you may want consider the exec_query wrapper. def execute(sql, name = nil) + sql = transform_query(sql) check_if_write_query(sql) materialize_transactions diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 2de09eae69..c806d9277a 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -696,6 +696,7 @@ module ActiveRecord FEATURE_NOT_SUPPORTED = "0A000" # :nodoc: def execute_and_clear(sql, name, binds, prepare: false, async: false) + sql = transform_query(sql) check_if_write_query(sql) if !prepare || without_prepared_statement?(binds) diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb index b61ee405bd..1883a33e43 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb @@ -19,6 +19,7 @@ module ActiveRecord end def execute(sql, name = nil) # :nodoc: + sql = transform_query(sql) check_if_write_query(sql) materialize_transactions @@ -32,6 +33,7 @@ module ActiveRecord end def exec_query(sql, name = nil, binds = [], prepare: false, async: false) # :nodoc: + sql = transform_query(sql) check_if_write_query(sql) materialize_transactions @@ -104,6 +106,7 @@ module ActiveRecord end def execute_batch(statements, name = nil) + statements = statements.map { |sql| transform_query(sql) } sql = combine_multi_statements(statements) check_if_write_query(sql) diff --git a/activerecord/lib/active_record/query_logs.rb b/activerecord/lib/active_record/query_logs.rb index 3189291f0b..0542267d39 100644 --- a/activerecord/lib/active_record/query_logs.rb +++ b/activerecord/lib/active_record/query_logs.rb @@ -118,13 +118,14 @@ module ActiveRecord inline_tags.pop end - def add_query_log_tags_to_sql(sql) # :nodoc: - comments.each do |comment| - unless sql.include?(comment) - sql = prepend_comment ? "#{comment} #{sql}" : "#{sql} #{comment}" - end + def call(sql) # :nodoc: + parts = self.comments + if prepend_comment + parts << sql + else + parts.unshift(sql) end - sql + parts.join(" ") end private @@ -198,15 +199,5 @@ module ActiveRecord inline_tags.join end end - - module ExecutionMethods - def execute(sql, *args, **kwargs) - super(ActiveRecord::QueryLogs.add_query_log_tags_to_sql(sql), *args, **kwargs) - end - - def exec_query(sql, *args, **kwargs) - super(ActiveRecord::QueryLogs.add_query_log_tags_to_sql(sql), *args, **kwargs) - end - end end end diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index cca0912b85..938e66c349 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -360,6 +360,7 @@ To keep using the current cache store, you can turn off cache versioning entirel initializer "active_record.query_log_tags_config" do |app| config.after_initialize do if app.config.active_record.query_log_tags_enabled + ActiveRecord.query_transformers << ActiveRecord::QueryLogs ActiveRecord::QueryLogs.taggings.merge!( application: Rails.application.class.name.split("::").first, pid: -> { Process.pid }, @@ -375,12 +376,6 @@ To keep using the current cache store, you can turn off cache versioning entirel if app.config.active_record.cache_query_log_tags ActiveRecord::QueryLogs.cache_query_log_tags = true end - - ActiveSupport.on_load(:active_record) do - ConnectionAdapters::AbstractAdapter.descendants.each do |klass| - klass.prepend(QueryLogs::ExecutionMethods) if klass.descendants.empty? - end - end end end end diff --git a/activerecord/test/cases/query_logs_test.rb b/activerecord/test/cases/query_logs_test.rb index 10765355dd..a90cbb4950 100644 --- a/activerecord/test/cases/query_logs_test.rb +++ b/activerecord/test/cases/query_logs_test.rb @@ -12,16 +12,16 @@ class QueryLogsTest < ActiveRecord::TestCase def setup # Enable the query tags logging - ActiveRecord::Base.connection.class_eval do - prepend(ActiveRecord::QueryLogs::ExecutionMethods) - end + @original_transformers = ActiveRecord.query_transformers @original_prepend = ActiveRecord::QueryLogs.prepend_comment + ActiveRecord.query_transformers += [ActiveRecord::QueryLogs] ActiveRecord::QueryLogs.prepend_comment = false ActiveRecord::QueryLogs.cache_query_log_tags = false ActiveRecord::QueryLogs.cached_comment = nil end def teardown + ActiveRecord.query_transformers = @original_transformers ActiveRecord::QueryLogs.prepend_comment = @original_prepend ActiveRecord::QueryLogs.tags = [] end @@ -100,10 +100,10 @@ class QueryLogsTest < ActiveRecord::TestCase ActiveRecord::QueryLogs.cache_query_log_tags = true ActiveRecord::QueryLogs.tags = [ :application ] - assert_equal " /*application:active_record*/", ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + assert_equal " /*application:active_record*/", ActiveRecord::QueryLogs.call("") ActiveRecord::QueryLogs.stub(:cached_comment, "/*cached_comment*/") do - assert_equal " /*cached_comment*/", ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + assert_equal " /*cached_comment*/", ActiveRecord::QueryLogs.call("") end ensure ActiveRecord::QueryLogs.cached_comment = nil @@ -115,12 +115,12 @@ class QueryLogsTest < ActiveRecord::TestCase ActiveRecord::QueryLogs.update_context(temporary: "value") ActiveRecord::QueryLogs.tags = [ temporary_tag: ->(context) { context[:temporary] } ] - assert_equal " /*temporary_tag:value*/", ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + assert_equal " /*temporary_tag:value*/", ActiveRecord::QueryLogs.call("") ActiveRecord::QueryLogs.update_context(temporary: "new_value") assert_nil ActiveRecord::QueryLogs.cached_comment - assert_equal " /*temporary_tag:new_value*/", ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + assert_equal " /*temporary_tag:new_value*/", ActiveRecord::QueryLogs.call("") ensure ActiveRecord::QueryLogs.cached_comment = nil ActiveRecord::QueryLogs.cache_query_log_tags = false @@ -201,15 +201,6 @@ class QueryLogsTest < ActiveRecord::TestCase end end - def test_inline_tags_are_deduped - ActiveRecord::QueryLogs.tags = [ :application ] - assert_sql(%r{select id from posts /\*foo\*/ /\*application:active_record\*/$}) do - ActiveRecord::QueryLogs.with_tag("foo") do - ActiveRecord::Base.connection.execute "select id from posts /*foo*/" - end - end - end - def test_empty_comments_are_not_added original_tags = ActiveRecord::QueryLogs.tags ActiveRecord::QueryLogs.tags = [ empty: -> { nil } ] diff --git a/railties/test/application/query_logs_test.rb b/railties/test/application/query_logs_test.rb index 25d80c758a..b16c11c582 100644 --- a/railties/test/application/query_logs_test.rb +++ b/railties/test/application/query_logs_test.rb @@ -18,7 +18,7 @@ module ApplicationTests app_file "app/controllers/users_controller.rb", <<-RUBY class UsersController < ApplicationController def index - render inline: ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + render inline: ActiveRecord::QueryLogs.call("") end def dynamic_content @@ -30,7 +30,7 @@ module ApplicationTests app_file "app/jobs/user_job.rb", <<-RUBY class UserJob < ActiveJob::Base def perform - ActiveRecord::QueryLogs.add_query_log_tags_to_sql("") + ActiveRecord::QueryLogs.call("") end def dynamic_content @@ -57,7 +57,7 @@ module ApplicationTests test "does not modify the query execution path by default" do boot_app - assert_equal ActiveRecord::Base.connection.method(:execute).owner, ActiveRecord::ConnectionAdapters::SQLite3::DatabaseStatements + assert_not_includes ActiveRecord.query_transformers, ActiveRecord::QueryLogs end test "prepends the query execution path when enabled" do @@ -65,7 +65,7 @@ module ApplicationTests boot_app - assert_equal ActiveRecord::Base.connection.method(:execute).owner, ActiveRecord::QueryLogs::ExecutionMethods + assert_includes ActiveRecord.query_transformers, ActiveRecord::QueryLogs end test "controller and job tags are defined by default" do