Skip to content
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

Support Rails 6.1 #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

AnD00
Copy link
Contributor

@AnD00 AnD00 commented Jun 12, 2023

I'd like to add Rails 6.1 support for activerecord-turntable.

In this PR, the turntable itself is judged whether the version of activerecord is 6.1 or later, and the process is branched and executed.

Since 6.1, activerecord has been changed drastically to support multiple DBs, etc., it may be simpler to make turntable itself a major version up and not support 6.0 or lower.

Please let me know if there is anything else I need to do.

@AnD00 AnD00 changed the title Supports rails 6.1 Support Rails 6.1 Jun 12, 2023
@gussan
Copy link
Collaborator

gussan commented Nov 18, 2023

TODO:

  • Upgrade rspec-rails to 4.x or later ( rspec-rails supports Rails 6.1)
  • Support Ruby 3.0.x (Rails 6.1.x supports Ruby >= 2.5.0 and <= 3.0.x)
    • Fix compatibility with Ruby 3.0.x keyword arguments
  • [want] Support ar-import

Issue about Ruby 3.0.x

Ruby 3.0 changes about keyword arguments may break some turntable methods that have options argument.

For example:

  32) ActiveRecord::Turntable::ActiveRecordExt::Persistence When the model is sharded by surrogate key When updating
      Failure/Error:
        shard.connection.transaction(options) do
          yield
        end

      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./lib/active_record/turntable/cluster.rb:52:in `shards_transaction'
      # ./lib/active_record/turntable/connection_proxy.rb:22:in `shards_transaction'
      # ./lib/active_record/turntable/active_record_ext/transactions.rb:19:in `with_transaction_returning_status'
      # ./spec/factories/users.rb:7:in `block (3 levels) in <top (required)>'
      # ./spec/active_record/turntable/active_record_ext/persistence_spec.rb:23:in `block (3 levels) in <top (required)>'
      # ./spec/active_record/turntable/active_record_ext/persistence_spec.rb:31:in `block (4 levels) in <top (required)>'

This should be changed to receive keyword arguments.

Issue about ar-import

On rails 3.1.x, AR-import's SQL may be changed to no FROM clause.

  1) ActiveRecord::Turntable::ActiveRecordExt::ActiverecordImportExt With sequencer enabled model is expected not to raise Exception
     Failure/Error: it { is_expected.not_to raise_error }

       expected no Exception, got #<NoMethodError: undefined method `size' for nil:NilClass> with backtrace:
         # ./lib/active_record/turntable/mixer.rb:129:in `build_select_fader'
         # ./lib/active_record/turntable/mixer.rb:40:in `build_fader'
         # ./lib/active_record/turntable/connection_proxy.rb:93:in `method_missing'
         # ./spec/active_record/turntable/active_record_ext/activerecord_import_ext_spec.rb:6:in `block (4 levels) in <top (required)>'
         # ./spec/active_record/turntable/active_record_ext/activerecord_import_ext_spec.rb:15:in `block (3 levels) in <top (required)>'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants