search

cookpad / arproxy

Arproxy is a proxy between ActiveRecord and database adapter
https://github.com/cookpad/arproxy
MIT License
327 stars 26 forks source link

Support ActiveRecord 7.1.x #28

Closed hirocaster closed 2 months ago

hirocaster commented 5 months ago

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

--

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

ref https://github.com/rails/rails/commit/63c0d6b31bcd0fc33745ec6fd278b2d1aab9be54#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.

mirakui commented 2 months ago

@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 commented 2 months ago

@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.