From 378adbb891cf5b369215c4dd95cb5e9a749cbe64 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Thu, 25 Jul 2019 19:42:07 +0100 Subject: [PATCH] Rails 6.2.0.rc2 compatibility Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins. Worked around by using only Arel joins in the affected query. See https://github.com/rails/rails/issues/36761 Fixes #822 --- Gemfile | 2 +- app/models/concerns/thredded/topic_common.rb | 16 ++++++++-- app/models/thredded/messageboard.rb | 32 ++++++++++++++------ lib/thredded.rb | 7 +++++ lib/thredded/db_tools.rb | 11 ++++--- spec/gemfiles/rails_6_0.gemfile | 2 +- 6 files changed, 51 insertions(+), 19 deletions(-) diff --git a/Gemfile b/Gemfile index f2dc8c65b..066446c58 100644 --- a/Gemfile +++ b/Gemfile @@ -2,7 +2,7 @@ source 'https://rubygems.org' -gem 'rails', '~> 6.0.0.rc1' +gem 'rails', '~> 6.0.0.rc2' gem 'rails-i18n', '~> 6.0.0.beta1' # https://github.com/rails/rails/blob/v6.0.0.rc1/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L12 diff --git a/app/models/concerns/thredded/topic_common.rb b/app/models/concerns/thredded/topic_common.rb index c6a34912d..d0ca44a9c 100644 --- a/app/models/concerns/thredded/topic_common.rb +++ b/app/models/concerns/thredded/topic_common.rb @@ -53,9 +53,19 @@ def unread(user) topics = arel_table reads_class = reflect_on_association(:user_read_states).klass reads = reads_class.arel_table - joins(topics.join(reads, Arel::Nodes::OuterJoin) - .on(topics[:id].eq(reads[:postable_id]).and(reads[:user_id].eq(user.id))).join_sources) - .merge(reads_class.where(reads[:id].eq(nil).or(reads[:unread_posts_count].not_eq(0)))) + + joins_reads = + topics.outer_join(reads) + .on(topics[:id].eq(reads[:postable_id]).and(reads[:user_id].eq(user.id))).join_sources + + unread_scope = reads_class.where(reads[:id].eq(nil).or(reads[:unread_posts_count].not_eq(0))) + + # Work around https://github.com/rails/rails/issues/36761 + if Thredded.rails_gte_600_rc_2? + merge(unread_scope).joins(joins_reads) + else + joins(joins_reads).merge(unread_scope) + end end private diff --git a/app/models/thredded/messageboard.rb b/app/models/thredded/messageboard.rb index d9c307b4f..727c1696d 100644 --- a/app/models/thredded/messageboard.rb +++ b/app/models/thredded/messageboard.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Thredded - class Messageboard < ActiveRecord::Base + class Messageboard < ActiveRecord::Base # rubocop:disable Metrics/ClassLength extend FriendlyId friendly_id :slug_candidates, use: %i[slugged reserved], @@ -128,14 +128,28 @@ def unread_topics_counts(user:, topics_scope: Thredded::Topic.all) messageboards = arel_table read_states = Thredded::UserTopicReadState.arel_table topics = topics_scope.arel_table - joins(:topics).merge(topics_scope).joins( - messageboards.outer_join(read_states).on( - messageboards[:id].eq(read_states[:messageboard_id]) - .and(read_states[:postable_id].eq(topics[:id])) - .and(read_states[:user_id].eq(user.id)) - .and(read_states[:unread_posts_count].eq(0)) - ).join_sources - ).group(messageboards[:id]).pluck( + + read_states_join_cond = + messageboards[:id].eq(read_states[:messageboard_id]) + .and(read_states[:postable_id].eq(topics[:id])) + .and(read_states[:user_id].eq(user.id)) + .and(read_states[:unread_posts_count].eq(0)) + + scope = + # Work around https://github.com/rails/rails/issues/36761 + if Thredded.rails_gte_600_rc_2? + merge(topics_scope).joins( + messageboards.join(topics) + .on(topics[:messageboard_id].eq(messageboards[:id])) + .outer_join(read_states).on(read_states_join_cond).join_sources + ) + else + joins(:topics).merge(topics_scope).joins( + messageboards.outer_join(read_states).on(read_states_join_cond).join_sources + ) + end + + scope.group(messageboards[:id]).pluck( :id, Arel::Nodes::Subtraction.new(topics[:id].count, read_states[:id].count) ).to_h diff --git a/lib/thredded.rb b/lib/thredded.rb index 3ca928724..9387c2247 100644 --- a/lib/thredded.rb +++ b/lib/thredded.rb @@ -251,6 +251,13 @@ def rails_gte_51? @rails_gte_51 end + # @api private + # Mainly to work around https://github.com/rails/rails/issues/36761 + def rails_gte_600_rc_2? + @rails_gte_600_rc_2 = (Rails.gem_version >= Gem::Version.new('6.0.0.rc2')) if @rails_gte_600_rc_2.nil? + @rails_gte_600_rc_2 + end + # @api private def rails_supports_csp_nonce? @rails_supports_csp_nonce = (Rails.gem_version >= Gem::Version.new('5.2.0')) if @rails_supports_csp_nonce.nil? diff --git a/lib/thredded/db_tools.rb b/lib/thredded/db_tools.rb index 74f814cb4..852de0682 100644 --- a/lib/thredded/db_tools.rb +++ b/lib/thredded/db_tools.rb @@ -9,13 +9,14 @@ class << self def migrate(paths:, quiet:, &filter) verbose_was = ActiveRecord::Migration.verbose ActiveRecord::Migration.verbose = !quiet - migrate = -> { - if Rails::VERSION::STRING >= '5.2' - ActiveRecord::MigrationContext.new(paths).migrate(nil, &filter) + migrate = + if Rails.gem_version >= Gem::Version.new('6.0.0.rc2') + -> { ActiveRecord::MigrationContext.new(paths, ActiveRecord::SchemaMigration).migrate(nil, &filter) } + elsif Rails::VERSION::STRING >= '5.2' + -> { ActiveRecord::MigrationContext.new(paths).migrate(nil, &filter) } else - ActiveRecord::Migrator.migrate(paths, &filter) + -> { ActiveRecord::Migrator.migrate(paths, &filter) } end - } if quiet silence_active_record(&migrate) else diff --git a/spec/gemfiles/rails_6_0.gemfile b/spec/gemfiles/rails_6_0.gemfile index 0c2c1369f..5ce064c5f 100644 --- a/spec/gemfiles/rails_6_0.gemfile +++ b/spec/gemfiles/rails_6_0.gemfile @@ -4,7 +4,7 @@ source 'https://rubygems.org' gemspec path: '../../' eval_gemfile '../../shared.gemfile' -gem 'rails', '~> 6.0.0.rc1' +gem 'rails', '~> 6.0.0.rc2' gem 'rails-i18n', '~> 6.0.0.beta1' # https://github.com/rails/rails/blob/v6.0.0.rc1/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L12