AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039

Bilka2 · 2025-01-29T10:48:09Z

Issue

https://otwarchive.atlassian.net/browse/AO3-6893

Purpose

Upgrade to Rails 7.1. Should go in after Ruby 3.2 and AO3-6892.

Testing Instructions

Refer to Jira.

Credit

Bilka

Bilka2 · 2025-01-29T10:57:56Z

app/controllers/blocked/users_controller.rb

+        .includes(blocked: [:pseuds, { default_pseud: { icon_attachment: { blob: {
+                    variant_records: { image_attachment: :blob },
+                    preview_image_attachment: { blob: { variant_records: { image_attachment: :blob } } }
+                  } } } }])


This change and the same one in muted/users_controller.rb is based on the same change in Active Storage, because this is a copy of the with_attached_* scope: https://github.com/rails/rails/blob/v7.0.8.3/activestorage/lib/active_storage/attached/model.rb -> https://github.com/rails/rails/blob/v7.1.5.1/activestorage/lib/active_storage/attached/model.rb (track_variants is true for us)

It's 7.1 so we should be able to re-enable these lines:

otwarchive/spec/requests/blocked_users_n_plus_one_spec.rb

Lines 15 to 17 in 603cb16

# Rails doesn't seem to want to include variants, so this won't work right now.

# We can revisit when https://github.com/rails/rails/pull/49231 is released OR we upgrade to Rails 7.1

# blocked.default_pseud.icon.attach(io: File.open(Rails.root.join("features/fixtures/icon.gif")), filename: "icon.gif", content_type: "image/gif")

and

otwarchive/spec/requests/muted_users_n_plus_one_spec.rb

Lines 15 to 17 in 603cb16

# Rails doesn't seem to want to include variants, so this won't work right now.

# We can revisit when https://github.com/rails/rails/pull/49231 is released OR we upgrade to Rails 7.1

# muted.default_pseud.icon.attach(io: File.open(Rails.root.join("features/fixtures/icon.gif")), filename: "icon.gif", content_type: "image/gif")

However, with these lines restored, I didn't get any test failures on the current main branch / Rails 7.0. Not sure why yet.

Maybe something related to these comments got lost in the whole active storage PR, it was quite large and there were changes to the related scope after these comments were added. I'll just uncomment the tests and we can be happy that they pass now, I guess?

Good? news:

The test changes behaviour based on whether we show the images via the proxy url or with processed.url, it fails only when using processed.url. This matches the documentation. So AO3-6898 temporarily broke it, then AO3-6900 fixed it again.

I reverted AO3-6900 locally to try to make the test work, but sadly it doesn't seem to want to behave, no matter what includes I use. According to bullet, the includes are effective when viewing the blocked user index in a browser, but the test still fails.

I recall that we also had problems with this in the initial Active Storage PR. So I'll go ahead and disable the test again and note that using processed.url may result in N+1 queries. Hopefully we can just stick with the proxy url.

Bilka2 · 2025-01-29T10:59:50Z

app/controllers/works_controller.rb

@@ -899,7 +899,7 @@ def build_options(params)
      external_coauthor_name: params[:external_coauthor_name],
      external_coauthor_email: params[:external_coauthor_email],
      language_id: params[:language_id]
-    }
+    }.compact_blank!


Due to the nokogiri update, when encoding defaulted to empty string here, that empty string was getting used as the encoding instead of the actual encoding of the downloaded story. Getting rid of the empty string via compact_blank! makes it fall back to the encoding of the downloaded story as expected.

config/environments/production.rb

Bilka2 · 2025-01-29T11:02:17Z

app/models/search/bookmark_indexer.rb

@@ -84,7 +84,7 @@ def document(object)
    json_object = object.as_json(
      root: false,
      only: [
-        :id, :created_at, :bookmarkable_type, :bookmarkable_id, :user_id,
+        :id, :created_at, :bookmarkable_type, :bookmarkable_id,


This user_id was duplicate with the one in the merge call later (as_json uses string keys, the merge uses symbols, so it wasn't getting overwritten by the merge). Something changed in the json serialization for Elasticsearch so it wasn't getting rid of this duplicate "user_id": nil anymore, so deleting it here is the fix.

Bilka2 · 2025-01-29T11:03:37Z

config/application.rb

+
+    # Use secret from archive config
+    config.secret_key_base = ArchiveConfig.SESSION_SECRET


According to the Rails documentation, it's preferred to do this in credentials.yml.enc instead: https://guides.rubyonrails.org/v7.1/configuring.html#config-secret-key-base / https://api.rubyonrails.org/v7.1.5.1/classes/Rails/Application.html#method-i-secret_key_base

Bilka2 · 2025-01-29T11:14:56Z

config/application.rb

@@ -99,7 +103,6 @@ class Application < Rails::Application
      "X-Frame-Options" => "SAMEORIGIN",
      "X-XSS-Protection" => "1; mode=block",
      "X-Content-Type-Options" => "nosniff",
-      "X-Download-Options" => "noopen",


This is suggested by the update script. It worsens Internet Explorer support.

brianjaustin

Still some tests failing so not my final review, but here are my initial thoughts. (And if you have time, it might be nice to convert the failing tests to RSpec instead of minitest, but that's out of scope so not a big deal either way)

db/migrate/20250129101815_add_service_name_to_active_storage_blobs.active_storage.rb

config/environments/production.rb

Bilka2 · 2025-01-29T20:55:12Z

The tests seem to be flaky, they didn't fail on the commit with the active storage changes, just the one with the robocop changes. I also had them fail on master on my fork, and then pass after a re-run: https://github.com/Bilka2/otwarchive/actions/runs/13028412099/attempts/1 I won't have time to investigate why they're flaky before Sunday, if someone else wants to have a look.

And I figured I know better than rubocop for the rest of the reviewdog complaints/didn't want to change the generated migrations.

spec/requests/blocked_users_n_plus_one_spec.rb

redsummernight · 2025-02-06T07:35:16Z

config/config.yml

@@ -739,7 +739,7 @@ PERCONA_ARGS: >
  --max-load Threads_running=15
  --critical-load Threads_running=100
  --set-vars innodb_lock_wait_timeout=2
-  --alter-foreign-keys-method=auto
+  --alter-foreign-keys-method=drop_swap


I'd like @zz9pzza or @brianjaustin to confirm that this option is fine.

From the pt-online-schema-change manpage:

drop_swap Disable foreign key checks (FOREIGN_KEY_CHECKS=0), then drop the original table before renaming the new table into its place. This is different from the normal method of swapping the old and new table, which uses an atomic "RENAME" that is undetectable to client applications. This method is faster and does not block, but it is riskier for two reasons. First, for a short time between dropping the original table and renaming the temporary table, the table to be altered simply does not exist, and queries against it will result in an error. Secondly, if there is an error and the new table cannot be renamed into the place of the old one, then it is too late to abort, because the old table is gone permanently. This method forces "--no-swap-tables" and "--no-drop-old-table".

The last line seems a bit contradictory to the initial paragraph, but we'll want to be aware that this might leave us with tables to clean up (although we haven't always done that in the past so...) Also noting that this setting is overridden by Ansible, so we'll need to change that as well.

Overall, I think this is OK. If we're particularly worried about a table going wonky, we have backups and can also disable replication for a bit to have a "good" copy to fall back to.

We ended up using rebuild_constraints instead (#5066).

Bilka2 · 2025-02-06T14:41:21Z

config/application.rb

-    # The default locale is :en and all translations from config/locales/**/*.rb,yml are auto loaded.
-    config.i18n.load_path += Dir[Rails.root.join("config/locales/**/*.{rb,yml}")]
+    # The default locale is :en.


The load_path configuration defaults to this since Rails 7.0: https://guides.rubyonrails.org/v7.1/configuring.html#config-i18n-load-path

Bilka2 added 3 commits January 29, 2025 11:20

AO3-6893 Initial Rails upgrade

b6a7d30

AO3-6893 Flipped config options and fixed deprecations

c2d1c7b

Add Active Storage 7.1 changes to manual with_attached_* scopes

fce2bf4

github-actions bot added Has Migrations Contains migrations and therefore needs special attention when deploying Gem Updates Awaiting Review labels Jan 29, 2025

AO3-6893 Rubocop

6c5a8bc

Bilka2 commented Jan 29, 2025

View reviewed changes

brianjaustin reviewed Jan 29, 2025

View reviewed changes

db/migrate/20250129101815_add_service_name_to_active_storage_blobs.active_storage.rb Show resolved Hide resolved

config/environments/production.rb Show resolved Hide resolved

Bilka2 added 5 commits February 2, 2025 20:50

AO3-6893 Enable active storage related n+1 tests

e5fa7ee

AO3-6893 Use departure for active_storage_blobs migration

a28ec79

Merge branch 'master' into AO3-6893-rails-7-1-x

0bf2a13

AO3-6893 Fix tests

d646bf3

Doggy

3df6088

redsummernight approved these changes Feb 6, 2025

View reviewed changes

AO3-6893 Minor fixes

a989f8d

Bilka2 commented Feb 6, 2025

View reviewed changes

brianjaustin added Reviewed: Ready to Merge Has Production Config Changes Modifies the config file and needs special attention when deploying and removed Awaiting Review labels Feb 6, 2025

Bilka2 and others added 2 commits February 13, 2025 10:56

Merge branch 'master' into AO3-6893-rails-7-1-x

a354360

Merge upstream

641e80a

brianjaustin merged commit e44c571 into otwcode:master Feb 15, 2025
29 checks passed

Bilka2 deleted the AO3-6893-rails-7-1-x branch February 15, 2025 18:30

This was referenced Feb 15, 2025

AO3-6893 Use rebuild_constraints to modify foreign keys #5066

Merged

AO3-6893 Remove patch of departure.rb #5072

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039

AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039

Bilka2 commented Jan 29, 2025

Bilka2 Jan 29, 2025

redsummernight Feb 1, 2025

Bilka2 Feb 2, 2025

Bilka2 Feb 4, 2025

Bilka2 Jan 29, 2025

Bilka2 Jan 29, 2025

Bilka2 Jan 29, 2025

Bilka2 Jan 29, 2025

brianjaustin left a comment

Bilka2 commented Jan 29, 2025 •

edited

Loading

redsummernight Feb 6, 2025

brianjaustin Feb 6, 2025

redsummernight Feb 18, 2025

Bilka2 Feb 6, 2025

	# Rails doesn't seem to want to include variants, so this won't work right now.
	# We can revisit when https://github.com/rails/rails/pull/49231 is released OR we upgrade to Rails 7.1
	# blocked.default_pseud.icon.attach(io: File.open(Rails.root.join("features/fixtures/icon.gif")), filename: "icon.gif", content_type: "image/gif")

	# Rails doesn't seem to want to include variants, so this won't work right now.
	# We can revisit when https://github.com/rails/rails/pull/49231 is released OR we upgrade to Rails 7.1
	# muted.default_pseud.icon.attach(io: File.open(Rails.root.join("features/fixtures/icon.gif")), filename: "icon.gif", content_type: "image/gif")


		# Use secret from archive config
		config.secret_key_base = ArchiveConfig.SESSION_SECRET

AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039

AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039

Conversation

Bilka2 commented Jan 29, 2025

Issue

Purpose

Testing Instructions

Credit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjaustin left a comment

Choose a reason for hiding this comment

Bilka2 commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 commented Jan 29, 2025 •

edited

Loading