Support ActiveRecord 7.1.x

[hirocaster]

**I recommend waiting for the release of arproxy v1. ref: #30 **

--

Since ActiveRecord 7.1, using #execute no longer works as intended.

ref
rails/rails@63c0d6b#diff-0b861bb8bc857373d0f0280779eb119e2937ee0edd1cc 3497292c7d14e909591
#26

Instead, I have made a change to use #raw_execute.

Since #raw_execute is a private method, I'm not too keen on it, but if you have a better implementation, please give me feedback.

Currently, this Patch is not in production quality, so please use it at your own risk. Also, feedback is welcome.

[sorah]

minor comments

[hirocaster]

Thanks for the patch & review. 👍

[nekketsuuu]

Doesn't this break codes that directly call execute, which might call raw_execute internally, by invoking proxy_chain twice?

Perhaps the specs also need changes, since we only have DummyAdapter, which has independent execute and raw_execute.

[hirocaster]

Are you talking about the case where both execute and raw_execute are defined?
I am assuming that only one of them is defined based on the version of ActiveRecord you are using.

Do you have ideas for a more approach in changing use?

[nekketsuuu]

Both of the two methods should be defined in connection adapters. For example, Mysql2Adapter has both of them:

% bin/rails c
Loading development environment (Rails 7.1.4)
irb(main):001> ActiveRecord::ConnectionAdapters::Mysql2Adapter.instance_methods.include?(:execute)
=> true
irb(main):002> ActiveRecord::ConnectionAdapters::Mysql2Adapter.private_instance_methods.include?(:raw_execute)
=> true

Also ActiveRecord::ConnectionAdapters::DatabaseStatements defines both two: https://github.com/rails/rails/blob/v7.1.4/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

[nekketsuuu]

We may be able to assume that raw_execute must be invoked by execute. So how about make an alias of execute if raw_execute is not defined?

It'd be great if we don't rely on private methods, but I'm unsure how to implement this without overriding raw_execute.

@mirakui Do you have any ideas?

[hirocaster]

Thank you for more information.
I got to make sense of what you pointed out. 😄
I will try to change the code.

[nekketsuuu]

I forgot to send pending comments...

[nekketsuuu]

We may be able to assume that raw_execute must be invoked by execute. So how about make an alias of execute if raw_execute is not defined?

It'd be great if we don't rely on private methods, but I'm unsure how to implement this without overriding raw_execute.

@mirakui Do you have any ideas?

[mirakui]

@hirocaster
Thank you for the pull request. Also, sorry for the wait for your reply.
I have tested the behavior of this pull request using ActiveRecord since 7.1. Indeed, it has been confirmed that overriding ActiveRecord::Base.connection.execute like Arproxy does not work with AR 7.1 and later.

However, I think there are two major problems with the way this pull request is implemented, i.e., hooking AbstractAdapter#raw_execute.

First, it is not backward compatible with the proxies that Arproxy users have developed so far, and as the author of Arproxy, it is not stable enough to use without Arproxy users being sensitive to changes in the internal implementation of ActiveRecord, especially private methods such as #raw_execute. Arproxy is a “dangerous” library that rewrites query execution by ActiveRecord, so trust is important.

Second, this pull request works for the mysql2 adapter, but does not work for Connection Adapters such as the Postgres Adapter by hooking #raw_execute. Since ActiveRecord 7.1, there is a difference between Database Adapters in what method SQL is executed when executing a query. So, even if we change the interface of Proxy from public #execute hook to private #raw_execute hook, as you proposed in the pull request, it is still not enough to support multiple Adapters. Adapter.

For these reasons, unfortunately I cannot merge this pull request. Instead, as I suggested in #30, I have started development of a new major version, v1, for ActiveRecord 7.1 and later. Arproxy v1 will be designed to support multiple Database Adapters while maintaining backward compatibility with the previous Proxy.

I hope you will be patient and look forward to the v1 release.
Finally, a big thank you for using Arproxy and suggesting this pull-request. Thank you very much.

[hirocaster]

@mirakui
Thank you for your message.
I am happy to have been present at the occasion of the big decision of arproxy v1.

As you pointed out, I don't consider this PR to be in an ideal situation.
If there is anything I can do, I will continue to support you.

for someone
If someone wants to use the AR7.1+mysql/trilogy environment right now at this time, I assume you are aware of the risks and will use it, but I will leave this code for your reference.