Implement v1 #32
Implement v1 #32
Conversation
mirakui
commented
Oct 13, 2024
mirakui
commented
- Added support for ActiveRecord 7.1.
- Redesigned the proxy chain to accommodate internal structure changes in ActiveRecord 7.1.
- Introduced integration tests using real databases, allowing for more robust testing of functionality with MySQL, PostgreSQL, SQLite, and SQLServer.
- close Developing arproxy v1: integration tests, redesign aiming to fully support for AR7.1+ #30
| @@ -2,8 +2,8 @@ module Arproxy | |||
| class Base | |||
| attr_accessor :proxy_chain, :next_proxy | |||
|
|
|||
| def execute(sql, name=nil, **kwargs) | |||
There was a problem hiding this comment.
Q. This removal of kwargs may break some proxies which use keyword arguments.
- At Rails 7.1 and 7.2, the interface of ActiveRecord::ConnectionAdapters::DatabaseStatements#execute has a keyword argument: https://github.com/rails/rails/blob/f2b4bfb626c52d9cbebd576f615f32ac650fe021/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L130
- Also some other adapters have keyword arguments on the interface of patched methods. For example,
raw_executeof activerecord-sqlserver-adapter has: https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/7dcff7381cc4c2f5f964fbfe95f067327c702be3/lib/active_record/connection_adapters/sqlserver/database_statements.rb#L45
Have you assumed any proxies, which inherits Arprxy::Base, should not accept keyword arguments?
There was a problem hiding this comment.
@nekketsuuu Good question.
Indeed, in the current Arproxy, Arproxy::Base#execute actually replaced the call to ConnectionAdapter’s #execute, so kwargs was necessary as an argument.
However, in Arproxy v1, a new class named Arproxy::ConnectionAdapterPatch takes on this role. Arproxy::Base#execute is called with two arguments of (sql, name) from the patch, so kwargs is no longer needed in this context.
There was a problem hiding this comment.
I agree that this won't break ConnectionAdapter's execute, but I mean that this may break the behavior of execute of the existing proxies (classes that inherit Arproxy::Base). If these proxies use the given keyword arguments, all of them will be changed to nil in v1. I heard that you'd like to maintain backward compatibility for proxies (#30 ), so I wonder if this is your intention.
There was a problem hiding this comment.
I'm still struggling with this change in specification, but in order to keep up with ActiveRecord version upgrades, I'd like to release it with this specification first.
I've also written about this non-compatible change in the README.
https://github.com/cookpad/arproxy/tree/v1?tab=readme-ov-file#upgrading-guide-from-v0x-to-v1
|
LGTM for the main application code |
