-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement v1 #32
base: main
Are you sure you want to change the base?
Implement v1 #32
Conversation
mirakui
commented
Oct 13, 2024
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing these changes!
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_execute
of 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docker-compose.yml
Outdated
@@ -0,0 +1,46 @@ | |||
services: | |||
mysql: | |||
image: mysql:8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO: MySQL 9.0 has been released on 2024-07-01. How about using the latest version of MySQL?
(Please note that the mysql_native_password plugin has been removed at MySQL 9.0. We would be able to choose not to set the password, though.)
README.md
Outdated
- SQLServer | ||
- `activerecord-sqlserver-adapter` | ||
|
||
We have tested with the following versions of Ruby and ActiveRecord: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: How about adding the versions of databases which we run the integration tests?