AO3-7231 Upgrade gems and configs to Rails 8.0.x #5515
Conversation
Looked at all uses of to_time: * used for duration (difference, so zone vs offset doesn't matter) * stored in model/DB (rails converts to UTC) * read from model and sent to akismet (rails converts to UTC) * or just forced to utc strict_freshness: I don't see any issues arising from it Regexp.timeout: in my mind, 1s should be sufficient
better safe than sorry?
There was a problem hiding this comment.
Upgrade script wanted puma.rb, but I ignored it per this comment.
It also wanted to set silence_healthcheck_path in production.rb but it doesn't look like we're using the system anyways. (Noting this just in case we want something silenced, but I doubt it.)
New framework defaults
###
# Specifies whether `to_time` methods preserve the UTC offset of their receivers or preserves the timezone.
# If set to `:zone`, `to_time` methods will use the timezone of their receivers.
# If set to `:offset`, `to_time` methods will use the UTC offset.
# If `false`, `to_time` methods will convert to the local system UTC offset instead.
#++
Rails.application.config.active_support.to_time_preserves_timezone = :zone
I looked at all the uses of to_time and none stood out as being timezone (vs offset) dependent. (database is all normalized to UTC, duration calculations shouldn't care about exact zone)
###
# When both `If-Modified-Since` and `If-None-Match` are provided by the client
# only consider `If-None-Match` as specified by RFC 7232 Section 6.
# If set to `false` both conditions need to be satisfied.
#++
Rails.application.config.action_dispatch.strict_freshness = true
I don't see anything wrong with this.
###
# Set `Regexp.timeout` to `1`s by default to improve security over Regexp Denial-of-Service attacks.
#++
Regexp.timeout = 1
Do we have any complex regexes? I think 1s should be plenty of time.
|
|
||
| # Library for helping run pt-online-schema-change commands: | ||
| gem "departure", "~> 6.8" | ||
| gem "departure", "~> 8.0" |
There was a problem hiding this comment.
Didn't see anything of note in the changelog. Note that 8.0.1 seems to have not been published to rubygems (if it does get published I would like to pull it in).
|
|
||
| group :test do | ||
| gem "rspec-rails", "~> 6.0" | ||
| gem "rspec-rails", "~> 8.0" |
There was a problem hiding this comment.
Rails 8.0 now saves the parent model (tag set) before the child model (owned tag set), so we can't always touch it here (after_save). In that case the child model (owned tag set) is going to be saved after anyways, so it's fine to just skip touching.
This change looks to be a side effect of rails/rails#49847, but I think this ordering makes more sense so we were probably just depending on buggy behaviour.
Example, if that explanation doesn't make sense: https://gist.github.com/marcus8448/9459bcc0536f2baf0704cbea91fc9d0b
| config.active_job.queue_adapter = :resque | ||
|
|
||
| # TODO: Remove with Rails 8.0 where this option will be deprecated | ||
| config.active_job.enqueue_after_transaction_commit = :always |
There was a problem hiding this comment.
I've set enqueue_after_transaction_commit = true on our job classes since the option is deprecated.
An alternative would be to set ActiveJob::Base.enqueue_after_transaction_commit = true in an initializer to cover all job types. That's effectively what this option did (but manually set instead). Although the option is deprecated, the delayed queuing might be enabled by default in the future, so this might be a better fix.
| # Raise exceptions for disallowed deprecations. | ||
| config.active_support.disallowed_deprecation = :raise | ||
|
|
||
| # Tell Active Support which deprecation messages to disallow. | ||
| config.active_support.disallowed_deprecation_warnings = [] | ||
|
|
There was a problem hiding this comment.
Removed from example config, we're not using it anyways.
| # Append comments with runtime information tags to SQL queries in logs. | ||
| config.active_record.query_log_tags_enabled = true | ||
|
|
There was a problem hiding this comment.
Example (the /**/ comment):
Tag Load (0.2ms) SELECT `tags`.* FROM `tags` WHERE `tags`.`name` = 'Uncategorized Fandoms' LIMIT 1 /*action='index',application='Otwarchive',controller='home'*/| { namespace: "ao3-v2-dev", compress: true, pool: { size: 10 } } | ||
| config.public_file_server.headers = { | ||
| "Cache-Control" => "public, max-age=#{2.days.to_i}" | ||
| "cache-control" => "public, max-age=#{2.days.to_i}" |
There was a problem hiding this comment.
Rack 3 requires all response headers to be downcased (per this Rails commit). Although we haven't updated yet, I don't see any harm from doing that now.
| :content, :passw, :terms_of_service_non_production, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn | ||
| :passw, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc, | ||
| :content, :terms_of_service_non_production |
There was a problem hiding this comment.
I put our additions onto a new line to make it easier to differentiate from the Rails defaults.
There was a problem hiding this comment.
Not sure if this file is needed since the app should be able to respond itself.
| gem "rails", "~> 7.2" | ||
| gem "rails-i18n" | ||
| gem "rails", "~> 8.0.0" | ||
| gem "rails-i18n", "~> 8.0", git: "https://github.com/marcus8448/rails-i18n", ref: "1d120cc3b2c02793e7cab4ff5951d3a9e0b316a4" |
There was a problem hiding this comment.
This was some fun debugging. Pending PR.
1 participant
Issue
https://otwarchive.atlassian.net/browse/AO3-7231
Purpose
Upgrade to Rails 8.0.
While this PR is ready for review, it probably shouldn't be merged until rails-i18n is updated.
The upgrade was surprisingly (suspiciously?) straightforward, considering it's a major version bump. I'll add additional notes/comments in a review.
Testing Instructions
TBD, see Jira.
Credit
marcus8448 (he/him)