Support rails 7.2 #239

alpaca-tc · 2024-05-30T10:36:07Z

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.

alpaca-tc · 2024-08-14T00:54:19Z

lib/activerecord-multi-tenant/habtm.rb

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


In rails 7.2, the keys of _reflections was changed from String to Symbol.
I used _reflect_on_association to work without type conversion.

alpaca-tc · 2024-08-14T00:56:49Z

lib/activerecord-multi-tenant/query_rewriter.rb

-  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)


In Rails 7.2, connection was added as the first argument.

alpaca-tc · 2024-08-14T00:57:28Z

lib/activerecord-multi-tenant/relation_extension.rb

+             elsif ActiveRecord::VERSION::MAJOR >= 7 && ActiveRecord::VERSION::MINOR >= 2
+               build_arel(klass.connection)


In Rails 7.2, connection was added as the first argument.

alpaca-tc · 2024-08-14T01:00:45Z

spec/activerecord-multi-tenant/model_extensions_spec.rb

+      # 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


In Rails 7.2, #inspect doesn't print all attributes. This around hook will output all attributes for compatibility.

related: rails/rails#49765

alpaca-tc · 2024-08-14T01:07:40Z

lib/activerecord-multi-tenant/model_extensions.rb

+                               elsif table_name &&
+                                     connection.schema_cache.columns_hash(table_name).include?(DEFAULT_ID_FIELD)
+                                 DEFAULT_ID_FIELD
+                               end
          end


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.

alpaca-tc · 2024-08-14T01:11:51Z

lib/activerecord-multi-tenant/model_extensions.rb

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


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.

alpaca-tc · 2024-08-14T01:16:50Z

@serprex Rails 7.2 has been released and PR is now open. Please review 😄 🙏

serprex

lgtm, note however that it's almost been a year now since I left MS for PeerDB

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.

related: rails/rails#51725

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

gurkanindibay · 2024-08-22T15:55:40Z

@alpaca-tc static-checks are failing

serprex · 2024-08-22T16:18:35Z

lib/activerecord-multi-tenant/query_rewriter.rb

+      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)


Suggested change

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)

alpaca-tc · 2024-08-23T01:12:11Z

@gurkanindibay fixed 💛

gurkanindibay · 2024-08-23T13:03:47Z

@alpaca-tc thanks for your contribution

yonda · 2024-09-02T04:21:02Z

@gurkanindibay @alpaca-tc

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 find_by method.

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!

follow up citusdata#239

alpaca-tc force-pushed the support-rails-7-2 branch 3 times, most recently from c593a5b to c0707d1 Compare August 14, 2024 00:50

alpaca-tc commented Aug 14, 2024

View reviewed changes

alpaca-tc marked this pull request as ready for review August 14, 2024 01:13

serprex requested a review from gurkanindibay August 14, 2024 01:33

serprex approved these changes Aug 14, 2024

View reviewed changes

gurkanindibay approved these changes Aug 14, 2024

View reviewed changes

alpaca-tc added 6 commits August 21, 2024 22:36

Add rails 7.2 to Appraisals

2f967a3

The key of _reflections is a symbol since Rails 7.2.

7e9538a

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.

cached_find_by_statement requires two positional arguments in Rails 7.2.

01808bf

related: rails/rails#51725

build_arel requires connection as arguments in Rails 7.2.

11b8af8

related: rails/rails#51725

Introduced #attributes_for_inspect in Rails 7.2

b81b63b

By default, only `id` is output, so set `:all` to output all attributes for testing. related: rails/rails#49765

alpaca-tc force-pushed the support-rails-7-2 branch from c0707d1 to ff19aab Compare August 21, 2024 13:36

serprex reviewed Aug 22, 2024

View reviewed changes

fix rubocop

756e7d3

related: citusdata#239 (comment)

gurkanindibay merged commit 0ac43aa into citusdata:master Aug 23, 2024
123 checks passed

alpaca-tc deleted the support-rails-7-2 branch August 28, 2024 01:56

kimihito added a commit to kimihito/activerecord-multi-tenant that referenced this pull request Sep 2, 2024

Update README Rails version details

3530fdf

follow up citusdata#239

kimihito mentioned this pull request Sep 2, 2024

Update README Rails version details #258

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rails 7.2 #239

Support rails 7.2 #239

alpaca-tc commented May 30, 2024 •

edited

Loading

alpaca-tc Aug 14, 2024

alpaca-tc Aug 14, 2024

alpaca-tc Aug 14, 2024

alpaca-tc Aug 14, 2024 •

edited

Loading

alpaca-tc Aug 14, 2024

alpaca-tc Aug 14, 2024

alpaca-tc commented Aug 14, 2024

serprex left a comment

gurkanindibay commented Aug 22, 2024

serprex Aug 22, 2024

alpaca-tc commented Aug 23, 2024

gurkanindibay commented Aug 23, 2024

yonda commented Sep 2, 2024

		elsif ActiveRecord::VERSION::MAJOR >= 7 && ActiveRecord::VERSION::MINOR >= 2
		build_arel(klass.connection)

Support rails 7.2 #239

Support rails 7.2 #239

Conversation

alpaca-tc commented May 30, 2024 • edited Loading

alpaca-tc Aug 14, 2024

Choose a reason for hiding this comment

alpaca-tc Aug 14, 2024

Choose a reason for hiding this comment

alpaca-tc Aug 14, 2024

Choose a reason for hiding this comment

alpaca-tc Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

alpaca-tc Aug 14, 2024

Choose a reason for hiding this comment

alpaca-tc Aug 14, 2024

Choose a reason for hiding this comment

alpaca-tc commented Aug 14, 2024

serprex left a comment

Choose a reason for hiding this comment

gurkanindibay commented Aug 22, 2024

serprex Aug 22, 2024

Choose a reason for hiding this comment

alpaca-tc commented Aug 23, 2024

gurkanindibay commented Aug 23, 2024

yonda commented Sep 2, 2024

alpaca-tc commented May 30, 2024 •

edited

Loading

alpaca-tc Aug 14, 2024 •

edited

Loading