Add sqlcommenter formatting support on query logs

Co-authored-by: Iheanyi Ekechukwu <[email protected]>

Author: modulitos

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Add configurable formatter on query log tags to support sqlcommenter. See #45139
+
+    It is now possible to opt into sqlcommenter-formatted query log tags with `config.active_record.query_log_tags_format = :sqlcommenter`.
+
+    *Modulitos and Iheanyi*
+
 *   Allow any ERB in the database.yml when creating rake tasks.
 
     Any ERB can be used in `database.yml` even if it accesses environment

activerecord/lib/active_record/query_logs.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_support/core_ext/module/attribute_accessors_per_thread"
+require "active_record/query_logs_formatter"
 
 module ActiveRecord
   # = Active Record Query Logs
@@ -73,6 +74,7 @@ module QueryLogs
     mattr_accessor :tags, instance_accessor: false, default: [ :application ]
     mattr_accessor :prepend_comment, instance_accessor: false, default: false
     mattr_accessor :cache_query_log_tags, instance_accessor: false, default: false
+    mattr_accessor :tags_formatter, instance_accessor: false
     thread_mattr_accessor :cached_comment, instance_accessor: false
 
     class << self
@@ -88,6 +90,11 @@ def clear_cache # :nodoc:
         self.cached_comment = nil
       end
 
+      # Updates the formatter to be what the passed in format is.
+      def update_formatter(format = :legacy)
+        self.tags_formatter = QueryLogs::FormatterFactory.from_symbol(format)
+      end
+
       ActiveSupport::ExecutionContext.after_change { ActiveRecord::QueryLogs.clear_cache }
 
       private
@@ -101,6 +108,10 @@ def comment
           end
         end
 
+        def formatter
+          self.tags_formatter ||= QueryLogs::FormatterFactory.from_symbol(:legacy)
+        end
+
         def uncached_comment
           content = tag_content
           if content.present?
@@ -130,7 +141,7 @@ def tag_content
             else
               handler
             end
-            "#{key}:#{val}" unless val.nil?
+            "#{key}#{self.formatter.key_value_separator}#{self.formatter.format_value(val)}" unless val.nil?
           end.join(",")
         end
     end

activerecord/lib/active_record/query_logs_formatter.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  module QueryLogs
+    class Formatter # :nodoc:
+      attr_reader :key_value_separator
+
+      # @param [String] key_value_separator: indicates the string used for
+      # separating keys and values.
+      #
+      # @param [Symbol] quote_values: indicates how values will be formatted (eg:
+      # in single quotes, not quoted at all, etc)
+      def initialize(key_value_separator:)
+        @key_value_separator = key_value_separator
+      end
+
+      # @param [String-coercible] value
+      # @return [String] The formatted value that will be used in our key-value
+      # pairs.
+      def format_value(value)
+        value
+      end
+    end
+
+    class QuotingFormatter < Formatter # :nodoc:
+      def format_value(value)
+        "'#{value.to_s.gsub("'", "\\\\'")}'"
+      end
+    end
+
+    class FormatterFactory # :nodoc:
+      # @param [Symbol] formatter: the kind of formatter we're building.
+      # @return [Formatter]
+      def self.from_symbol(formatter)
+        case formatter
+        when :legacy
+          Formatter.new(key_value_separator: ":")
+        when :sqlcommenter
+          QuotingFormatter.new(key_value_separator: "=")
+        else
+          raise ArgumentError, "Formatter is unsupported: #{formatter}"
+        end
+      end
+    end
+  end
+end

activerecord/lib/active_record/railtie.rb

@@ -34,6 +34,7 @@ class Railtie < Rails::Railtie # :nodoc:
     config.active_record.sqlite3_production_warning = true
     config.active_record.query_log_tags_enabled = false
     config.active_record.query_log_tags = [ :application ]
+    config.active_record.query_log_tags_format = :legacy
     config.active_record.cache_query_log_tags = false
 
     config.active_record.queues = ActiveSupport::InheritableOptions.new
@@ -244,6 +245,7 @@ class Railtie < Rails::Railtie # :nodoc:
           :shard_resolver,
           :query_log_tags_enabled,
           :query_log_tags,
+          :query_log_tags_format,
           :cache_query_log_tags,
           :sqlite3_production_warning,
           :sqlite3_adapter_strict_strings_by_default,
@@ -384,7 +386,7 @@ class Railtie < Rails::Railtie # :nodoc:
           ActiveRecord.query_transformers << ActiveRecord::QueryLogs
           ActiveRecord::QueryLogs.taggings.merge!(
             application:  Rails.application.class.name.split("::").first,
-            pid:          -> { Process.pid },
+            pid:          -> { Process.pid.to_s },
             socket:       -> { ActiveRecord::Base.connection_db_config.socket },
             db_host:      -> { ActiveRecord::Base.connection_db_config.host },
             database:     -> { ActiveRecord::Base.connection_db_config.database }
@@ -394,6 +396,10 @@ class Railtie < Rails::Railtie # :nodoc:
             ActiveRecord::QueryLogs.tags = app.config.active_record.query_log_tags
           end
 
+          if app.config.active_record.query_log_tags_format.present?
+            ActiveRecord::QueryLogs.update_formatter(app.config.active_record.query_log_tags_format)
+          end
+
           if app.config.active_record.cache_query_log_tags
             ActiveRecord::QueryLogs.cache_query_log_tags = true
           end

activerecord/test/cases/query_logs_formatter_test.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+
+class QueryLogsFormatter < ActiveRecord::TestCase
+  def test_factory_invalid_formatter
+    assert_raises(ArgumentError) do
+      ActiveRecord::QueryLogs::FormatterFactory.from_symbol(:non_existing_formatter)
+    end
+    end
+
+  def test_sqlcommenter_key_value_separator
+    formatter = ActiveRecord::QueryLogs::FormatterFactory.from_symbol(:sqlcommenter)
+    assert_equal("=", formatter.key_value_separator)
+  end
+
+  def test_sqlcommenter_format_value
+    formatter = ActiveRecord::QueryLogs::FormatterFactory.from_symbol(:sqlcommenter)
+    assert_equal("'Joe\\'s Crab Shack'", formatter.format_value("Joe's Crab Shack"))
+  end
+
+  def test_sqlcommenter_format_value_string_coercible
+    formatter = ActiveRecord::QueryLogs::FormatterFactory.from_symbol(:sqlcommenter)
+    assert_equal("'1234'", formatter.format_value(1234))
+  end
+end

activerecord/test/cases/query_logs_test.rb

@@ -32,6 +32,8 @@ def teardown
     ActiveRecord::QueryLogs.prepend_comment = false
     ActiveRecord::QueryLogs.cache_query_log_tags = false
     ActiveRecord::QueryLogs.cached_comment = nil
+    ActiveRecord::QueryLogs.update_formatter
+
     # ActiveSupport::ExecutionContext context is automatically reset in Rails app via an executor hooks set in railtie
     # But not in Active Record's own test suite.
     ActiveSupport::ExecutionContext.clear
@@ -41,6 +43,11 @@ def test_escaping_good_comment
     assert_equal "app:foo", ActiveRecord::QueryLogs.send(:escape_sql_comment, "app:foo")
   end
 
+  def test_escaping_good_comment_with_custom_separator
+    ActiveRecord::QueryLogs.update_formatter(:sqlcommenter)
+    assert_equal "app='foo'", ActiveRecord::QueryLogs.send(:escape_sql_comment, "app='foo'")
+  end
+
   def test_escaping_bad_comments
     assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "*/; DROP TABLE USERS;/*")
     assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "**//; DROP TABLE USERS;/*")
@@ -148,6 +155,13 @@ def test_empty_comments_are_not_added
     end
   end
 
+  def test_sql_commenter_format
+    ActiveRecord::QueryLogs.update_formatter(:sqlcommenter)
+    assert_sql(%r{/\*application='active_record'\*/}) do
+      Dashboard.first
+    end
+  end
+
   def test_custom_basic_tags
     ActiveRecord::QueryLogs.tags = [ :application, { custom_string: "test content" } ]
 

guides/source/configuring.md

@@ -1125,6 +1125,10 @@ Define an `Array` specifying the key/value tags to be inserted in an SQL
 comment. Defaults to `[ :application ]`, a predefined tag returning the
 application name.
 
+#### `config.active_record.query_log_tags_format`
+
+A `Symbol` specifying the formatter to use for tags. Valid values are `:sqlcommenter` and `:legacy`.  Defaults to `:legacy`.
+
 #### `config.active_record.cache_query_log_tags`
 
 Specifies whether or not to enable caching of query log tags. For applications

railties/test/application/query_logs_test.rb

@@ -22,7 +22,7 @@ def index
           end
 
           def dynamic_content
-            Time.now.to_f
+            Time.now.to_f.to_s
           end
         end
       RUBY
@@ -34,7 +34,7 @@ def perform
           end
 
           def dynamic_content
-            Time.now.to_f
+            Time.now.to_f.to_s
           end
         end
       RUBY
@@ -87,6 +87,21 @@ def app
       assert_includes comment, "controller:users"
     end
 
+    test "sqlcommenter formatting works when specified" do
+      add_to_config "config.active_record.query_log_tags_enabled = true"
+      add_to_config "config.active_record.query_log_tags_format = :sqlcommenter"
+
+      add_to_config "config.active_record.query_log_tags = [ :pid ]"
+
+      boot_app
+
+      get "/"
+      comment = last_response.body.strip
+
+      assert_match(/pid='\d+'/, comment)
+      assert_includes comment, "controller='users'"
+    end
+
     test "controller actions tagging filters can be disabled" do
       add_to_config "config.active_record.query_log_tags_enabled = true"
       add_to_config "config.action_controller.log_query_tags_around_actions = false"