Introduce Arproxy::Proxy: incompatible but keep functionalities over v0.x

README.md

@@ -9,11 +9,11 @@ Arproxy is a library that can intercept SQL queries executed by ActiveRecord to
 Create your custom proxy and add its configuration in your Rails' `config/initializers/` directory:
 
 ```ruby
-class QueryTracer < Arproxy::Base
-  def execute(sql, name=nil)
+class QueryTracer < Arproxy::Proxy
+  def execute(sql, context)
     Rails.logger.debug sql
     Rails.logger.debug caller(1).join("\n")
-    super(sql, name)
+    super(sql, context)
   end
 end
 
@@ -31,12 +31,40 @@ Then you can see the backtrace of SQLs in the Rails' log.
 MyTable.where(id: id).limit(1) # => The SQL and the backtrace appear in the log
 ```
 
-## What the `name' argument is
+## What the `context` argument is
+
+`context` is an instance of `Arproxy::QueryContext` and contains values that are passed from Arproxy to the Database Adapter.
+`context` is a set of values used when calling Database Adapter methods, and you don't need to use the `context` values directly.
+However, you must always pass `context` to `super` like `super(sql, context)`.
+
+For example, let's look at the Mysql2Adapter implementation. When executing a query in Mysql2Adapter, the `Mysql2Adapter#internal_exec_query` method is called internally.
+
+```
+# https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb#L21
+def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc:
+  # ...
+end
+```
+
+In Arproxy, this method is called at the end of the `Arproxy::Proxy#execute` method chain, and at this time `context` contains the arguments to be passed to `#internal_exec_query`:
+
+| member           | example value                      |
+|------------------|------------------------------------|
+| `context.name`   | `"SQL"`                            |
+| `context.binds`  | `[]`                               |
+| `context.kwargs` | `{ prepare: false, async: false }` |
+
+You can modify the values of `context` in the proxy, but do so after understanding the implementation of the Database Adapter.
+
+### `context.name`
+
 In the Rails' log you may see queries like this:
+
 ```
 User Load (22.6ms)  SELECT `users`.* FROM `users` WHERE `users`.`name` = 'Issei Naruta'
 ```
-Then `"User Load"` is the `name`.
+
+Then `"User Load"` is the `context.name`.
 
 # Architecture
 Without Arproxy:
@@ -94,128 +122,82 @@ We have tested with the following versions of Ruby, ActiveRecord, and databases:
 # Examples
 
 ## Adding Comments to SQLs
+
 ```ruby
-class CommentAdder < Arproxy::Base
-  def execute(sql, name=nil)
+class CommentAdder < Arproxy::Proxy
+  def execute(sql, context)
     sql += ' /*this_is_comment*/'
-    super(sql, name)
+    super(sql, context)
   end
 end
 ```
 
-# Use plug-in
+## Slow Query Logger
 
 ```ruby
-# any_gem/lib/arproxy/plugin/my_plugin
-module Arproxy::Plugin
-  class MyPlugin < Arproxy::Base
-    Arproxy::Plugin.register(:my_plugin, self)
+class SlowQueryLogger < Arproxy::Proxy
+  def initialize(slow_ms)
+    @slow_ms = slow_ms
+  end
 
-    def execute(sql, name=nil)
-      # Any processing
+  def execute(sql, context)
+    result = nil
+    ms = Benchmark.ms { result = super(sql, context) }
+    if ms >= @slow_ms
+      Rails.logger.info "Slow(#{ms.to_i}ms): #{sql}"
     end
+    result
   end
 end
-```
 
-```ruby
 Arproxy.configure do |config|
-  config.plugin :my_plugin
+  config.use SlowQueryLogger, 1000
 end
 ```
 
-# Upgrading guide from v0.x to v1
-
-There are several incompatible changes from Arproxy v0.x to v1.
-In most cases, existing configurations can be used as-is in v1, but there are some exceptions.
-The specification of custom proxies (classes inheriting from Arproxy::Base) has changed as follows:
-
-## 1. Removal of keyword arguments (kwargs)
+## Readonly Access
 
-In v0.2.9, `**kwargs` was added to the arguments of the `#execute` method ([#21](https://github.com/cookpad/arproxy/pull/21)), but this argument has been removed in v1.
-
-These `kwargs` were removed in v1 because their specifications differed depending on the Connection Adapter of each database.
+If you don't call `super` in the proxy, you can block the query execution.
 
 ```ruby
-# ~> v0.2.9
-class MyProxy < Arproxy::Base
-  def execute(sql, name=nil, **kwargs)
-    super
-  end
-end
-
-# >= v1.0.0
-class MyProxy < Arproxy::Base
-  def execute(sql, name=nil)
-    super
+class Readonly < Arproxy::Proxy
+  def execute(sql, context)
+    if sql =~ /^(SELECT|SET|SHOW|DESCRIBE)\b/
+      super(sql, context)
+    else
+      Rails.logger.warn "#{context.name} (BLOCKED) #{sql}"
+      nil
+    end
   end
 end
 ```
 
-## 2. `Arproxy::Base#execute` (`super`) no longer executes queries
-
-In v0.x, the `Arproxy::Base#execute` method was a method to execute a query on the Database Adapter.
-That is, when `super` is called in the `#execute` method of a custom proxy, a query is executed on the Database Adapter at the end of the proxy chain.
-
-In v1, the `Arproxy::Base#execute` method does not execute a query. The query is executed outside the `#execute` method after the proxy chain of `#execute` is complete.
-
-This change was necessary to support various Database Adapters while maintaining backward compatibility with custom proxies as much as possible.
-
-However, this change has the following incompatibilities:
-
-- The return value of `super` cannot be used.
-- The query execution time cannot be measured.
-
-### 2.1. The return value of `super` cannot be used
-
-In v0.x, the return value of `super` was the result of actually executing a query on the Database Adapter.
-For example, if you are using the `mysql2` Adapter, the return value of `super` was a `Mysql2::Result` object.
-
-In v1, the return value of `super` is a value used internally by Arproxy's proxy chain instead of the result of actually executing a query on the Database Adapter.
-You still need to return the return value of `super` in the `#execute` method of your custom proxy.
-However, the `Arproxy::Base` in v1 does not expect to use this return value in the custom proxy.
-
-If your custom proxy expects the return value of `super` to be an object representing the query result, you need to be careful because it is not available in v1.
+# Use plug-in
 
 ```ruby
-class MyProxy < Arproxy::Base
-  def execute(sql, name=nil)
-    result = super(sql, name)
-    # ...
-    # In v0.x, the return value of the Database Adapter such as Mysql2::Result was stored,
-    # but in v1, the value used internally by Arproxy's proxy chain is stored.
-    result
+# any_gem/lib/arproxy/plugin/my_plugin
+module Arproxy::Plugin
+  class MyPlugin < Arproxy::Proxy
+    Arproxy::Plugin.register(:my_plugin, self)
+
+    def execute(sql, context)
+      # Any processing
+      # ...
+      super(sql, context)
+    end
   end
 end
 ```
 
-### 2.2. The query execution time cannot be measured
-
-For example, even if you write code to measure the execution time of `super`, it no longer means the query execution time.
-
 ```ruby
-class MyProxy < Arproxy::Base
-  def execute(sql, name=nil)
-    t = Time.now
-    result = super(sql, name)
-    # This code no longer means the query execution time
-    Rails.logger.info "Slow(#{Time.now - t}ms): #{sql}"
-    result
-  end
+Arproxy.configure do |config|
+  config.plugin :my_plugin
 end
 ```
 
-# Discussion
-
-The specification changes in v1 have allowed more Database Adapters to be supported and made Arproxy more resistant to changes in ActiveRecord's internal structure.
-However, as described in the previous section, there are cases where the custom proxies written in v0.x will no longer work.
-
-We do not know the use cases of Arproxy users other than ourselves very well, so we are soliciting opinions on the changes in this time.
-If there are many requests, we will prepare a new base class for custom proxies with a different interface from `Arproxy::Base`, so that custom proxy writing similar to that in v0.x can be done.
-
-For this issue, we are collecting opinions on the following discussion:
+# Upgrading guide from v0.x to v1
 
-[Calling for opinions: incompatibility between v0.x and v1 · cookpad/arproxy · Discussion #33](https://github.com/cookpad/arproxy/discussions/33)
+See [UPGRADING.md](https://github.com/cookpad/arproxy/main/)
 
 # Development
 

UPGRADING.md

@@ -0,0 +1,23 @@
+# Upgrading guide from v0.x to v1
+
+The proxy specification has changed from v0.x to v1 and is not backward compatible.
+The base class for proxies has changed from `Arproxy::Base` to `Arproxy::Proxy`.
+Also, the arguments to `#execute` have changed from `sql, name=nil, **kwargs` to `sql, context`.
+
+```ruby
+# ~> v0.2.9
+class MyProxy < Arproxy::Base
+  def execute(sql, name=nil, **kwargs)
+    super
+  end
+end
+
+# >= v1.0.0
+class MyProxy < Arproxy::Proxy
+  def execute(sql, context)
+    super
+  end
+end
+```
+
+There are no other backward incompatible changes besides the above changes in proxy base class and arguments.

lib/arproxy.rb

@@ -1,6 +1,7 @@
 require 'logger'
 require 'arproxy/base'
 require 'arproxy/config'
+require 'arproxy/connection_adapter_patch'
 require 'arproxy/proxy_chain'
 require 'arproxy/error'
 require 'arproxy/plugin'

lib/arproxy/base.rb

@@ -1,9 +1,5 @@
 module Arproxy
+  # This class is no longer used since Arproxy v1.
   class Base
-    attr_accessor :proxy_chain, :next_proxy
-
-    def execute(sql, name=nil)
-      next_proxy.execute sql, name
-    end
   end
 end

lib/arproxy/config.rb

@@ -1,5 +1,7 @@
 require 'active_record'
 require 'active_record/base'
+require 'arproxy/base'
+require 'arproxy/error'
 
 module Arproxy
   class Config
@@ -14,12 +16,21 @@ def initialize
     end
 
     def use(proxy_class, *options)
+      if proxy_class.is_a?(Class) && proxy_class.ancestors.include?(Arproxy::Base)
+        raise Arproxy::Error, "Error on loading a proxy `#{proxy_class.inspect}`: the superclass `Arproxy::Base` is no longer supported since Arproxy v1. Use `Arproxy::Proxy` instead. See: https://github.com/cookpad/arproxy/blob/main/UPGRADING.md"
+      end
+
       ::Arproxy.logger.debug("Arproxy: Mounting #{proxy_class.inspect} (#{options.inspect})")
       @proxies << [proxy_class, options]
     end
 
     def plugin(name, *options)
       plugin_class = Plugin.get(name)
+
+      if plugin_class.is_a?(Class) && plugin_class.ancestors.include?(Arproxy::Base)
+        raise Arproxy::Error, "Error on loading a plugin `#{plugin_class.inspect}`: the superclass `Arproxy::Base` is no longer supported since Arproxy v1. Use `Arproxy::Proxy` instead. See: https://github.com/cookpad/arproxy/blob/main/UPGRADING.md"
+      end
+
       use(plugin_class, *options)
     end
 

lib/arproxy/connection_adapter_patch.rb

@@ -73,38 +73,44 @@ def disable!
       def apply_patch(target_method)
         return if @applied_patches.include?(target_method)
         adapter_class.class_eval do
-          break if instance_methods.include?(:"#{target_method}_with_arproxy")
-          define_method("#{target_method}_with_arproxy") do |sql, name=nil, **kwargs|
-            ::Arproxy.proxy_chain.connection = self
-            proxy_chain_result = ::Arproxy.proxy_chain.head.execute(sql, name)
-            if proxy_chain_result && proxy_chain_result.is_a?(Array)
-              _sql, _name = proxy_chain_result
-              self.send("#{target_method}_without_arproxy", _sql, _name, **kwargs)
-            else
-              nil
-            end
+          raw_execute_method_name = :"#{target_method}_without_arproxy"
+          patched_execute_method_name = :"#{target_method}_with_arproxy"
+          break if instance_methods.include?(patched_execute_method_name)
+          define_method(patched_execute_method_name) do |sql, name=nil, **kwargs|
+            context = QueryContext.new(
+              raw_connection: self,
+              execute_method_name: raw_execute_method_name,
+              with_binds: false,
+              name: name,
+              kwargs: kwargs,
+            )
+            ::Arproxy.proxy_chain.head.execute(sql, context)
           end
-          alias_method :"#{target_method}_without_arproxy", target_method
-          alias_method target_method, :"#{target_method}_with_arproxy"
+          alias_method raw_execute_method_name, target_method
+          alias_method target_method, patched_execute_method_name
         end
         @applied_patches << target_method
       end
 
       def apply_patch_binds(target_method)
         return if @applied_patches.include?(target_method)
         adapter_class.class_eval do
-          define_method("#{target_method}_with_arproxy") do |sql, name=nil, binds=[], **kwargs|
-            ::Arproxy.proxy_chain.connection = self
-            proxy_chain_result = ::Arproxy.proxy_chain.head.execute(sql, name)
-            if proxy_chain_result && proxy_chain_result.is_a?(Array)
-              _sql, _name = proxy_chain_result
-              self.send("#{target_method}_without_arproxy", _sql, _name, binds, **kwargs)
-            else
-              nil
-            end
+          raw_execute_method_name = :"#{target_method}_without_arproxy"
+          patched_execute_method_name = :"#{target_method}_with_arproxy"
+          break if instance_methods.include?(patched_execute_method_name)
+          define_method(patched_execute_method_name) do |sql, name=nil, binds=[], **kwargs|
+            context = QueryContext.new(
+              raw_connection: self,
+              execute_method_name: raw_execute_method_name,
+              with_binds: true,
+              name: name,
+              binds: binds,
+              kwargs: kwargs,
+            )
+            ::Arproxy.proxy_chain.head.execute(sql, context)
           end
-          alias_method :"#{target_method}_without_arproxy", target_method
-          alias_method target_method, :"#{target_method}_with_arproxy"
+          alias_method raw_execute_method_name, target_method
+          alias_method target_method, patched_execute_method_name
         end
         @applied_patches << target_method
       end

lib/arproxy/proxy.rb

@@ -0,0 +1,15 @@
+require 'arproxy/query_context'
+
+module Arproxy
+  class Proxy
+    attr_accessor :context, :next_proxy
+
+    def execute(sql, context)
+      unless context.instance_of?(QueryContext)
+        raise Arproxy::Error, "`context` is expected a `Arproxy::QueryContext` but got `#{context.class}`"
+      end
+
+      next_proxy.execute(sql, context)
+    end
+  end
+end