DEV: Upgrade Rails to 6.1.3.1 (#12688)

Rails 6.1.3.1 deprecates a few API and has some internal changes that break our tests suite, so this commit fixes all the deprecations and errors and now Discourse should be fully compatible with Rails 6.1.3.1. We also have a new release of the rails_failover gem that's compatible with Rails 6.1.3.1.

Gemfile

@@ -18,13 +18,13 @@ else
   # this allows us to include the bits of rails we use without pieces we do not.
   #
   # To issue a rails update bump the version number here
-  gem 'actionmailer', '6.0.3.5'
-  gem 'actionpack', '6.0.3.5'
-  gem 'actionview', '6.0.3.5'
-  gem 'activemodel', '6.0.3.5'
-  gem 'activerecord', '6.0.3.5'
-  gem 'activesupport', '6.0.3.5'
-  gem 'railties', '6.0.3.5'
+  gem 'actionmailer', '6.1.3.1'
+  gem 'actionpack', '6.1.3.1'
+  gem 'actionview', '6.1.3.1'
+  gem 'activemodel', '6.1.3.1'
+  gem 'activerecord', '6.1.3.1'
+  gem 'activesupport', '6.1.3.1'
+  gem 'railties', '6.1.3.1'
   gem 'sprockets-rails'
 end
 

Gemfile.lock

@@ -8,21 +8,22 @@ GIT
 GEM
   remote: https://rubygems.org/
   specs:
-    actionmailer (6.0.3.5)
-      actionpack (= 6.0.3.5)
-      actionview (= 6.0.3.5)
-      activejob (= 6.0.3.5)
+    actionmailer (6.1.3.1)
+      actionpack (= 6.1.3.1)
+      actionview (= 6.1.3.1)
+      activejob (= 6.1.3.1)
+      activesupport (= 6.1.3.1)
       mail (~> 2.5, >= 2.5.4)
       rails-dom-testing (~> 2.0)
-    actionpack (6.0.3.5)
-      actionview (= 6.0.3.5)
-      activesupport (= 6.0.3.5)
-      rack (~> 2.0, >= 2.0.8)
+    actionpack (6.1.3.1)
+      actionview (= 6.1.3.1)
+      activesupport (= 6.1.3.1)
+      rack (~> 2.0, >= 2.0.9)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.2.0)
-    actionview (6.0.3.5)
-      activesupport (= 6.0.3.5)
+    actionview (6.1.3.1)
+      activesupport (= 6.1.3.1)
       builder (~> 3.1)
       erubi (~> 1.4)
       rails-dom-testing (~> 2.0)
@@ -31,20 +32,20 @@ GEM
       actionview (>= 6.0.a)
     active_model_serializers (0.8.4)
       activemodel (>= 3.0)
-    activejob (6.0.3.5)
-      activesupport (= 6.0.3.5)
+    activejob (6.1.3.1)
+      activesupport (= 6.1.3.1)
       globalid (>= 0.3.6)
-    activemodel (6.0.3.5)
-      activesupport (= 6.0.3.5)
-    activerecord (6.0.3.5)
-      activemodel (= 6.0.3.5)
-      activesupport (= 6.0.3.5)
-    activesupport (6.0.3.5)
+    activemodel (6.1.3.1)
+      activesupport (= 6.1.3.1)
+    activerecord (6.1.3.1)
+      activemodel (= 6.1.3.1)
+      activesupport (= 6.1.3.1)
+    activesupport (6.1.3.1)
       concurrent-ruby (~> 1.0, >= 1.0.2)
-      i18n (>= 0.7, < 2)
-      minitest (~> 5.1)
-      tzinfo (~> 1.1)
-      zeitwerk (~> 2.2, >= 2.2.2)
+      i18n (>= 1.6, < 2)
+      minitest (>= 5.1)
+      tzinfo (~> 2.0)
+      zeitwerk (~> 2.3)
     addressable (2.7.0)
       public_suffix (>= 2.0.2, < 5.0)
     annotate (3.1.1)
@@ -315,19 +316,19 @@ GEM
       nokogiri (>= 1.6)
     rails-html-sanitizer (1.3.0)
       loofah (~> 2.3)
-    rails_failover (0.6.5)
+    rails_failover (0.7.3)
       activerecord (~> 6.0)
       concurrent-ruby
       railties (~> 6.0)
-    rails_multisite (2.5.0)
+    rails_multisite (3.0.0)
       activerecord (> 5.0, < 7)
       railties (> 5.0, < 7)
-    railties (6.0.3.5)
-      actionpack (= 6.0.3.5)
-      activesupport (= 6.0.3.5)
+    railties (6.1.3.1)
+      actionpack (= 6.1.3.1)
+      activesupport (= 6.1.3.1)
       method_source
       rake (>= 0.8.7)
-      thor (>= 0.20.3, < 2.0)
+      thor (~> 1.0)
     rainbow (3.0.0)
     raindrops (0.19.1)
     rake (13.0.3)
@@ -444,10 +445,9 @@ GEM
     stackprof (0.2.16)
     test-prof (1.0.2)
     thor (1.1.0)
-    thread_safe (0.3.6)
     tilt (2.0.10)
-    tzinfo (1.2.9)
-      thread_safe (~> 0.1)
+    tzinfo (2.0.4)
+      concurrent-ruby (~> 1.0)
     uglifier (4.2.0)
       execjs (>= 0.3.0, < 3)
     unf (0.1.4)
@@ -479,14 +479,14 @@ PLATFORMS
   x86_64-linux
 
 DEPENDENCIES
-  actionmailer (= 6.0.3.5)
-  actionpack (= 6.0.3.5)
-  actionview (= 6.0.3.5)
+  actionmailer (= 6.1.3.1)
+  actionpack (= 6.1.3.1)
+  actionview (= 6.1.3.1)
   actionview_precompiler
   active_model_serializers (~> 0.8.3)
-  activemodel (= 6.0.3.5)
-  activerecord (= 6.0.3.5)
-  activesupport (= 6.0.3.5)
+  activemodel (= 6.1.3.1)
+  activerecord (= 6.1.3.1)
+  activesupport (= 6.1.3.1)
   addressable
   annotate
   aws-sdk-s3
@@ -566,7 +566,7 @@ DEPENDENCIES
   rack-protection
   rails_failover
   rails_multisite
-  railties (= 6.0.3.5)
+  railties (= 6.1.3.1)
   rake
   rb-fsevent
   rbtrace

app/controllers/admin/backups_controller.rb

@@ -72,7 +72,7 @@ class Admin::BackupsController < Admin::AdminController
   def show
     if !EmailBackupToken.compare(current_user.id, params.fetch(:token))
       @error = I18n.t('download_backup_mailer.no_token')
-      return render template: 'admin/backups/show.html.erb', layout: 'no_ember', status: 422
+      return render layout: 'no_ember', status: 422, formats: [:html]
     end
 
     store = BackupRestore::BackupStore.create

app/controllers/metadata_controller.rb

@@ -11,7 +11,7 @@ class MetadataController < ApplicationController
 
   def opensearch
     expires_in 1.minutes
-    render template: "metadata/opensearch.xml"
+    render template: "metadata/opensearch", formats: [:xml]
   end
 
   def app_association_android

app/controllers/static_controller.rb

@@ -29,7 +29,7 @@ class StaticController < ApplicationController
     if map.has_key?(@page)
       site_setting_key = map[@page][:redirect]
       url = SiteSetting.get(site_setting_key)
-      return redirect_to(url) unless url.blank?
+      return redirect_to(url) if url.present?
     end
 
     # The /guidelines route ALWAYS shows our FAQ, ignoring the faq_url site setting.
@@ -70,12 +70,8 @@ class StaticController < ApplicationController
       cookies[:email] = { value: params[:email], expires: 1.day.from_now }
     end
 
-    file = "static/#{@page}.#{I18n.locale}"
-    file = "static/#{@page}.en" if lookup_context.find_all("#{file}.html").empty?
-    file = "static/#{@page}"    if lookup_context.find_all("#{file}.html").empty?
-
-    if lookup_context.find_all("#{file}.html").any?
-      render file, layout: !request.xhr?, formats: [:html]
+    if lookup_context.find_all("static/#{@page}").any?
+      render "static/#{@page}", layout: !request.xhr?, formats: [:html]
       return
     end
 

app/helpers/application_helper.rb

@@ -369,6 +369,8 @@ module ApplicationHelper
 
   def loading_admin?
     return false unless defined?(controller)
+    return false if controller.class.name.blank?
+
     controller.class.name.split("::").first == "Admin"
   end
 

app/models/email_token.rb

@@ -77,7 +77,7 @@ class EmailToken < ActiveRecord::Base
 
       if user
         if Invite.redeem_from_email(user.email).present?
-          return user.reload
+          user.reload
         end
         user
       end

app/models/invite_redeemer.rb

@@ -6,11 +6,9 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
     Invite.transaction do
       if invite_was_redeemed?
         process_invitation
-        return invited_user
+        invited_user
       end
     end
-
-    nil
   end
 
   # extracted from User cause it is very specific to invites

app/models/published_page.rb

@@ -26,18 +26,18 @@ class PublishedPage < ActiveRecord::Base
   def self.publish!(publisher, topic, slug, options = {})
     pp = nil
 
-    transaction do
+    results = transaction do
       pp = find_or_initialize_by(topic: topic)
       pp.slug = slug.strip
       pp.public = options[:public] || false
 
       if pp.save
         StaffActionLogger.new(publisher).log_published_page(topic.id, slug)
-        return [true, pp]
+        [true, pp]
       end
     end
 
-    [false, pp]
+    results || [false, pp]
   end
 
   def self.unpublish!(publisher, topic)

app/services/staff_action_logger.rb

@@ -811,10 +811,12 @@ class StaffActionLogger
     changes.delete("updated_at")
     old_values = []
     new_values = []
-    changes.each do |k, v|
-      old_values << "#{k}: #{v[0]}"
-      new_values << "#{k}: #{v[1]}"
-    end
+    changes
+      .sort_by { |k, _| k.to_s }
+      .each do |k, v|
+        old_values << "#{k}: #{v[0]}"
+        new_values << "#{k}: #{v[1]}"
+      end
 
     [old_values, new_values]
   end

app/services/user_notification_renderer.rb

@@ -9,11 +9,10 @@ class UserNotificationRenderer < ActionView::Base
 
   def self.render(*args)
     LOCK.synchronize do
-      @instance ||= UserNotificationRenderer.with_view_paths(
+      @instance ||= UserNotificationRenderer.with_empty_template_cache.with_view_paths(
         Rails.configuration.paths["app/views"]
       )
       @instance.render(*args)
     end
   end
-
 end

lib/backup_restore.rb

@@ -110,7 +110,7 @@ module BackupRestore
   DatabaseConfiguration = Struct.new(:host, :port, :username, :password, :database)
 
   def self.database_configuration
-    config = ActiveRecord::Base.connection_pool.spec.config
+    config = ActiveRecord::Base.connection_pool.db_config.configuration_hash
     config = config.with_indifferent_access
 
     # credentials for PostgreSQL in CI environment

lib/bookmark_reminder_notification_handler.rb

@@ -5,17 +5,16 @@ class BookmarkReminderNotificationHandler
     return if bookmark.blank?
     Bookmark.transaction do
       if bookmark.post.blank? || bookmark.post.deleted_at.present?
-        return clear_reminder(bookmark)
+        clear_reminder(bookmark)
+      elsif bookmark.topic
+        create_notification(bookmark)
+
+        if bookmark.auto_delete_when_reminder_sent?
+          BookmarkManager.new(bookmark.user).destroy(bookmark.id)
+        end
+
+        clear_reminder(bookmark)
       end
-      return unless bookmark.topic
-
-      create_notification(bookmark)
-
-      if bookmark.auto_delete_when_reminder_sent?
-        BookmarkManager.new(bookmark.user).destroy(bookmark.id)
-      end
-
-      clear_reminder(bookmark)
     end
   end
 

lib/db_helper.rb

@@ -5,7 +5,7 @@ require_dependency "migration/base_dropper"
 class DbHelper
 
   REMAP_SQL ||= <<~SQL
-    SELECT table_name, column_name, character_maximum_length
+    SELECT table_name::text, column_name::text, character_maximum_length
       FROM information_schema.columns
      WHERE table_schema = 'public'
        AND is_updatable = 'YES'
@@ -14,7 +14,7 @@ class DbHelper
   SQL
 
   TRIGGERS_SQL ||= <<~SQL
-    SELECT trigger_name
+    SELECT trigger_name::text
       FROM information_schema.triggers
      WHERE trigger_name LIKE '%_readonly'
   SQL

lib/freedom_patches/fast_pluck.rb

@@ -54,9 +54,7 @@ class ActiveRecord::Relation
       enforce_raw_sql_whitelist(column_names)
       relation = spawn
 
-      relation.select_values = column_names.map { |cn|
-        @klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
-      }
+      relation.select_values = column_names
 
       klass.connection.select_raw(relation.arel) do |result, _|
         result.type_map = DB.type_map

lib/i18n/backend/discourse_i18n.rb

@@ -14,6 +14,8 @@ module I18n
 
       def reload!
         @pluralizers = {}
+        # this calls `reload!` in our patch lib/freedom_patches/translate_accelerator.rb
+        I18n.reload!
         super
       end
 

lib/migration/base_dropper.rb

@@ -61,7 +61,7 @@ module Migration
 
     def self.existing_discourse_function_names
       DB.query_single(<<~SQL)
-        SELECT routine_name
+        SELECT routine_name::text
         FROM information_schema.routines
         WHERE routine_type = 'FUNCTION' AND specific_schema = '#{FUNCTION_SCHEMA_NAME}'
       SQL

lib/turbo_tests/runner.rb

@@ -82,7 +82,9 @@ module TurboTests
     def check_for_migrations
       config =
         ActiveRecord::Base
-          .configurations["test"]
+          .configurations
+          .find_db_config("test")
+          .configuration_hash
           .merge("database" => "discourse_test_1")
 
       ActiveRecord::Tasks::DatabaseTasks.migrations_paths = ['db/migrate', 'db/post_migrate']

lib/validators/unique_among_validator.rb

@@ -2,21 +2,36 @@
 
 class UniqueAmongValidator < ActiveRecord::Validations::UniquenessValidator
   def validate_each(record, attribute, value)
-    old_errors = record.errors[attribute].size
+    old_errors = []
+    record.errors.each do |error|
+      old_errors << error if error.attribute == attribute
+    end
 
     # look for any duplicates at all
     super
 
-    new_errors = record.errors[attribute].size - old_errors
+    new_errors = []
+    record.errors.each do |error|
+      new_errors << error if error.attribute == attribute
+    end
 
     # do nothing further unless there were some duplicates.
-    unless new_errors == 0
+    if new_errors.size - old_errors.size != 0
       # now look only in the collection we care about.
       dupes = options[:collection].call(record).where("lower(#{attribute}) = ?", value.downcase)
       dupes = dupes.where("id != ?", record.id) if record.persisted?
 
       # pop off the error, if it was a false positive
-      record.errors[attribute].pop(new_errors) unless dupes.exists?
+      if !dupes.exists?
+        record.errors.delete(attribute)
+        old_errors.each do |error|
+          record.errors.add(
+            error.attribute,
+            error.type,
+            **error.options
+          )
+        end
+      end
     end
   end
 

spec/models/topic_spec.rb

@@ -324,6 +324,15 @@ describe Topic do
       it "won't allow another topic to be created with the same name in same category" do
         expect(new_topic).not_to be_valid
       end
+
+      it "other errors will not be cleared" do
+        SiteSetting.min_topic_title_length = 5
+        topic.update!(title: "more than 5 characters but less than 134")
+        SiteSetting.min_topic_title_length = 134
+        new_topic_different_cat.title = "more than 5 characters but less than 134"
+        expect(new_topic_different_cat).not_to be_valid
+        expect(new_topic_different_cat.errors[:title]).to include(I18n.t("errors.messages.too_short", count: 134))
+      end
     end
 
   end

spec/models/user_notification_schedule_spec.rb

@@ -11,7 +11,7 @@ describe UserNotificationSchedule do
         user: user,
         enabled: true
       })
-      expect(schedule.errors.keys).to eq([
+      expect(schedule.errors.attribute_names).to eq([
         :day_0_start_time,
         :day_0_end_time,
         :day_1_start_time,

spec/requests/admin/backups_controller_spec.rb

@@ -116,6 +116,7 @@ RSpec.describe Admin::BackupsController do
 
         expect(response.status).to eq(422)
         expect(response.headers['Content-Disposition']).not_to match(/attachment; filename/)
+        expect(response.body).to include(I18n.t("download_backup_mailer.no_token"))
       end
     end
 

spec/requests/admin/embeddable_hosts_controller_spec.rb

@@ -40,7 +40,7 @@ describe Admin::EmbeddableHostsController do
         history_exists = UserHistory.where(
             acting_user_id: admin.id,
             action: UserHistory.actions[:embeddable_host_update],
-            new_value: "host: test.com, class_name: test-class, category_id: #{category.id}").exists?
+            new_value: "category_id: #{category.id}, class_name: test-class, host: test.com").exists?
 
         expect(history_exists).to eq(true)
 

spec/requests/admin/web_hooks_controller_spec.rb

@@ -69,7 +69,7 @@ describe Admin::WebHooksController do
         expect(response.status).to eq(200)
         expect(UserHistory.where(acting_user_id: admin.id,
                                  action: UserHistory.actions[:web_hook_update],
-                                 new_value: "payload_url: https://test.com, active: false").exists?).to eq(true)
+                                 new_value: "active: false, payload_url: https://test.com").exists?).to eq(true)
       end
     end
 

spec/requests/application_controller_spec.rb

@@ -282,9 +282,7 @@ RSpec.describe ApplicationController do
       get "/search/query.json", params: { trem: "misspelled term" }
 
       expect(response.status).to eq(400)
-      expect(response.parsed_body).to eq(
-        "errors" => ["param is missing or the value is empty: term"]
-      )
+      expect(response.parsed_body["errors"].first).to include("param is missing or the value is empty: term")
     end
   end