Support rails 7.2 #239
Support rails 7.2 #239
Conversation
alpaca-tc
force-pushed
the
support-rails-7-2
branch
3 times, most recently
from
c593a5b
to
c0707d1
Compare
| @@ -12,7 +12,7 @@ def has_and_belongs_to_many_with_tenant(name, scope = nil, **options, &extension | |||
| # rubocop:enable Naming/PredicateName | |||
| has_and_belongs_to_many_without_tenant(name, scope, **options, &extension) | |||
|
|
|||
| middle_reflection = _reflections[name.to_s].through_reflection | |||
| middle_reflection = _reflect_on_association(name.to_s).through_reflection | |||
There was a problem hiding this comment.
In rails 7.2, the keys of _reflections was changed from String to Symbol.
I used _reflect_on_association to work without type conversion.
| def cached_find_by_statement(key, &block) | ||
| return super unless respond_to?(:scoped_by_tenant?) && scoped_by_tenant? | ||
| if ActiveRecord::VERSION::MAJOR >= 7 && ActiveRecord::VERSION::MINOR >= 2 | ||
| def cached_find_by_statement(connection, key, &block) |
There was a problem hiding this comment.
In Rails 7.2, connection was added as the first argument.
| elsif ActiveRecord::VERSION::MAJOR >= 7 && ActiveRecord::VERSION::MINOR >= 2 | ||
| build_arel(klass.connection) |
There was a problem hiding this comment.
In Rails 7.2, connection was added as the first argument.
| # related: https://github.com/rails/rails/pull/49765 | ||
| around do |example| | ||
| prev = Account.attributes_for_inspect | ||
| Account.attributes_for_inspect = :all | ||
| example.run | ||
| ensure | ||
| Account.attributes_for_inspect = prev | ||
| end |
There was a problem hiding this comment.
In Rails 7.2, #inspect doesn't print all attributes. This around hook will output all attributes for compatibility.
related: rails/rails#49765
| elsif table_name && | ||
| connection.schema_cache.columns_hash(table_name).include?(DEFAULT_ID_FIELD) | ||
| DEFAULT_ID_FIELD | ||
| end | ||
| end |
There was a problem hiding this comment.
To maintain compatibility with existing behavior, pk returns a single value rather than an array. However, the default Rails 7.2 is working on composite primary key support, which will return an array.
I think it is worth considering to what extent this gem will be left to the behavior of Rails itself in the future, and to what extent the gem will be extended.
| @@ -42,20 +42,15 @@ def partition_key | |||
| .try(:instance_variable_get, :@partition_key) | |||
| end | |||
|
|
|||
| # Avoid primary_key errors when using composite primary keys (e.g. id, tenant_id) | |||
There was a problem hiding this comment.
In supporting multiple versions, the variable states and values have become more complex.
I tried to override the #reset_primary_key private method that is called when initializing @primary_key so that it can use the same logic for each version.
Please review as it may have side effects compared to other changes.
|
@serprex Rails 7.2 has been released and PR is now open. Please review 😄 🙏 |
related: rails/rails#51726 `_reflect_on_association` is also a private API and will may be broken, but it works for both String and Symbol, absorbing type differences, so use this one.
By default, only `id` is output, so set `:all` to output all attributes for testing. related: rails/rails#49765
ar-multitenant expects that #primary_key returns single column instead of composite primary_keys for backward compatibilities. Previously overwriting #primary_key to return single pk, but since the timing for writing @primary_key has changed since Rails 7.2, this way is no longer available. Instead, overwriting `#reset_primary_key`, which handles calculating `@primary_key`.
alpaca-tc
force-pushed
the
support-rails-7-2
branch
from
c0707d1
to
ff19aab
Compare
|
@alpaca-tc static-checks are failing |
| key = Array.wrap(key) + [MultiTenant.current_tenant_id.to_s] | ||
| super(connection, key, &block) | ||
| end | ||
| else | ||
| def cached_find_by_statement(key, &block) | ||
| return super unless respond_to?(:scoped_by_tenant?) && scoped_by_tenant? | ||
|
|
||
| key = Array.wrap(key) + [MultiTenant.current_tenant_id.to_s] | ||
| super(key, &block) |
There was a problem hiding this comment.
| key = Array.wrap(key) + [MultiTenant.current_tenant_id.to_s] | |
| super(connection, key, &block) | |
| end | |
| else | |
| def cached_find_by_statement(key, &block) | |
| return super unless respond_to?(:scoped_by_tenant?) && scoped_by_tenant? | |
| key = Array.wrap(key) + [MultiTenant.current_tenant_id.to_s] | |
| super(key, &block) | |
| super(connection, Array.wrap(key) + [MultiTenant.current_tenant_id.to_s], &block) | |
| end | |
| else | |
| def cached_find_by_statement(key, &block) | |
| return super unless respond_to?(:scoped_by_tenant?) && scoped_by_tenant? | |
| super(Array.wrap(key) + [MultiTenant.current_tenant_id.to_s], &block) |
not sure if super with implicit args picks up on key being rebound, maybe inlining expression gets past static check
related: citusdata#239 (comment)
|
@gurkanindibay fixed 💛 |
|
@alpaca-tc thanks for your contribution |
|
Thank you for addressing this issue. This fix is crucial for users who have upgraded to Rails 7.2, as the current version throws an ArgumentError with the Is there an estimated timeline for when we might see a new release that includes this change? Many of us are eagerly waiting for this fix to be available in a stable release. Thanks again for your work on this valuable gem! |
The pull request is currently in draft status.Once the beta release of Rails is replaced by a stable release, I will open the pull request.
Rails 7.2.0 has been released, I have added support for activerecord-multitenant.
The changes were commented on in the review.