From 45ccadeeeb87a939a1744c38bbba444bcb2db234 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 21 Apr 2021 12:36:32 +0300 Subject: [PATCH] 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 | 14 ++-- Gemfile.lock | 76 +++++++++---------- app/controllers/admin/backups_controller.rb | 2 +- app/controllers/metadata_controller.rb | 2 +- app/controllers/static_controller.rb | 10 +-- app/helpers/application_helper.rb | 2 + app/models/email_token.rb | 2 +- app/models/invite_redeemer.rb | 4 +- app/models/published_page.rb | 6 +- app/services/staff_action_logger.rb | 10 ++- app/services/user_notification_renderer.rb | 3 +- lib/backup_restore.rb | 2 +- lib/bookmark_reminder_notification_handler.rb | 19 +++-- lib/db_helper.rb | 4 +- lib/freedom_patches/fast_pluck.rb | 4 +- lib/i18n/backend/discourse_i18n.rb | 2 + lib/migration/base_dropper.rb | 2 +- lib/turbo_tests/runner.rb | 4 +- lib/validators/unique_among_validator.rb | 23 +++++- spec/models/topic_spec.rb | 9 +++ .../models/user_notification_schedule_spec.rb | 2 +- .../requests/admin/backups_controller_spec.rb | 1 + .../admin/embeddable_hosts_controller_spec.rb | 2 +- .../admin/web_hooks_controller_spec.rb | 2 +- spec/requests/application_controller_spec.rb | 4 +- 25 files changed, 116 insertions(+), 95 deletions(-) diff --git a/Gemfile b/Gemfile index e4963029f7..b009cc8f79 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 6dd7ee8e35..7507db8571 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index c49b6aeeb8..f8b3300f99 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/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 diff --git a/app/controllers/metadata_controller.rb b/app/controllers/metadata_controller.rb index 8c13f2e286..47213031d9 100644 --- a/app/controllers/metadata_controller.rb +++ b/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 diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 5ef7704baf..63064c7e70 100644 --- a/app/controllers/static_controller.rb +++ b/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 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 0e53e2788b..2df54fc8fd 100644 --- a/app/helpers/application_helper.rb +++ b/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 diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 00e91a4f95..8472881b8b 100644 --- a/app/models/email_token.rb +++ b/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 diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index f8c607afe1..a6353d1843 100644 --- a/app/models/invite_redeemer.rb +++ b/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 diff --git a/app/models/published_page.rb b/app/models/published_page.rb index 5322efaea7..8345ae05bb 100644 --- a/app/models/published_page.rb +++ b/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) diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 772de24bb6..e859f42cbd 100644 --- a/app/services/staff_action_logger.rb +++ b/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 diff --git a/app/services/user_notification_renderer.rb b/app/services/user_notification_renderer.rb index 64bb497dcf..e66b7d9dec 100644 --- a/app/services/user_notification_renderer.rb +++ b/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 diff --git a/lib/backup_restore.rb b/lib/backup_restore.rb index b8a19efe4a..60b5d25b27 100644 --- a/lib/backup_restore.rb +++ b/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 diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index e0c105c575..55c8247811 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/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 diff --git a/lib/db_helper.rb b/lib/db_helper.rb index 4abe66f353..6510d49d97 100644 --- a/lib/db_helper.rb +++ b/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 diff --git a/lib/freedom_patches/fast_pluck.rb b/lib/freedom_patches/fast_pluck.rb index bc79177e73..a9faa87b32 100644 --- a/lib/freedom_patches/fast_pluck.rb +++ b/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 diff --git a/lib/i18n/backend/discourse_i18n.rb b/lib/i18n/backend/discourse_i18n.rb index fa6533165d..52b387e69a 100644 --- a/lib/i18n/backend/discourse_i18n.rb +++ b/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 diff --git a/lib/migration/base_dropper.rb b/lib/migration/base_dropper.rb index b88a5ba357..9aaf77e138 100644 --- a/lib/migration/base_dropper.rb +++ b/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 diff --git a/lib/turbo_tests/runner.rb b/lib/turbo_tests/runner.rb index aa4d8c35e1..1bf1b6e3d9 100644 --- a/lib/turbo_tests/runner.rb +++ b/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'] diff --git a/lib/validators/unique_among_validator.rb b/lib/validators/unique_among_validator.rb index ed3d142014..d9c63516eb 100644 --- a/lib/validators/unique_among_validator.rb +++ b/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 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 81e8c198a2..07f4de9684 100644 --- a/spec/models/topic_spec.rb +++ b/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 diff --git a/spec/models/user_notification_schedule_spec.rb b/spec/models/user_notification_schedule_spec.rb index 63eecb149d..47ee14f812 100644 --- a/spec/models/user_notification_schedule_spec.rb +++ b/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, diff --git a/spec/requests/admin/backups_controller_spec.rb b/spec/requests/admin/backups_controller_spec.rb index 03b9245744..e750b6dc51 100644 --- a/spec/requests/admin/backups_controller_spec.rb +++ b/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 diff --git a/spec/requests/admin/embeddable_hosts_controller_spec.rb b/spec/requests/admin/embeddable_hosts_controller_spec.rb index ea246d78e2..2dcc5081f1 100644 --- a/spec/requests/admin/embeddable_hosts_controller_spec.rb +++ b/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) diff --git a/spec/requests/admin/web_hooks_controller_spec.rb b/spec/requests/admin/web_hooks_controller_spec.rb index 125032351a..61cd3db6a0 100644 --- a/spec/requests/admin/web_hooks_controller_spec.rb +++ b/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 diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 67d22a6f95..0fde425c27 100644 --- a/spec/requests/application_controller_spec.rb +++ b/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