Rails 7 upgrade #3250

murny · 2023-09-27T18:38:44Z

This PR is to upgrade Jupiter from Rails v6.1.X to v7.0.X

List of major changes:

Remove require_dependency as this has been dropped (replace with require_relative for now, Rails 7.1 allows autoloading from "lib" directory and this will all go away)
Fix "TimeWithZone#to_s(:db) is deprecated. Please use TimeWithZone#to_fs(:db) instead." warnings
~~fix "include Pundit is deprecated. Please use include Pundit::Authorization instead."~~ PR fix "include Pundit is deprecated. Please use include Pundit::Authorization instead." warnings #3318
Update files from Rails generator
Fix bad fixtures using non-existent associations
Fix issue with redirect_to for an external URL
Explicitly opt into mini_magick for ActiveStorage
~~Fix issue with singleton_class usage in Rails 7 for ActsAsRDFable gem, point gem to branch with hot fix for this~~ PR Bump acts_as_rdfable from 0.4.0 to v0.5.0 #3291

github-actions · 2023-10-07T04:39:53Z

	1 Warning
⚠️	This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

murny · 2023-10-10T18:14:32Z

Tests are failing due to ActsAsRDF gem. We are seeing errors like this:

/Users/murny/.asdf/installs/ruby/3.1.4/lib/ruby/gems/3.1.0/gems/activerecord-7.0.8/lib/active_record/model_schema.rb:633:in `compute_table_name': undefined method `table_name' for nil:NilClass (NoMethodError)

            base_class.table_name
                      ^^^^^^^^^^^
	from /Users/murny/.asdf/installs/ruby/3.1.4/lib/ruby/gems/3.1.0/gems/activerecord-7.0.8/lib/active_record/model_schema.rb:284:in `reset_table_name'
	from /Users/murny/.asdf/installs/ruby/3.1.4/lib/ruby/gems/3.1.0/gems/activerecord-7.0.8/lib/active_record/model_schema.rb:248:in `table_name'
	from /Users/murny/jupiter/app/services/rdf_graph_creation_service.rb:5:in `get_prefixed_predicates'

I believe the issue from troubleshooting is due to the way we were relying on singleton_class in this gem to get the "class" of the model. It appears something broke in a newer version in Rails where the singleton_class no longer returns the correct data back.

I believe this is related to this issue which was closed with no action:
rails/rails#38293.
Which is where the singleton_class no longer responds to .inspect with a similar error.

A simple solution is just use self.class instead of self.singleton_class for getting the models class. Will have to investigate if theirs any side effects from making this change.

Also feel like this gem could be improved. We have multiple ways to "register" the gem into models, not sure why we have many ways? (Activerecord base include and add_annotation_bindings! etc)

UPDATE: PR created for this in ActsAsRDFable to fix this issue: ualbertalib/acts_as_rdfable#14

murny · 2023-10-14T20:30:27Z

config/application.rb

@@ -59,5 +59,9 @@ class Application < Rails::Application
      link_attributes: { rel: 'noopener noreferrer', target: '_blank' }
    }

+    # We should be using VIPS which is now the default, but this requires work to migrate?
+    # Delete this line if we ever migrate to VIPS
+    config.active_storage.variant_processor = :mini_magick


To move to VIPS we probably need some code changes and Production/Staging needs the correct binaries install?

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#active-storage-default-variant-processor-changed-to-vips

I'll throw an issue in the backlog for this. But this isn't a big deal. mini_magick isn't being deprecated. Just that VIPS is a superior and more performant library.

murny · 2023-10-14T20:34:00Z

test/fixtures/items.yml

@@ -16,19 +16,19 @@ item_fancy:

 item_practical:
  visibility: <%= JupiterCore::VISIBILITY_PUBLIC %>
-  owner: regular
+  owner: user_regular


This and the member_of_paths were references non existant associations? Looks like in Rails 7 this will start throwing errors, but not sure how this ever worked before?

We use this fixture in SearchTest 🤔

Good catch!

murny · 2023-10-14T20:35:56Z

app/controllers/admin/google_sessions_controller.rb

@@ -5,7 +5,7 @@ def new

    if params['code'].nil?
      auth_uri = auth_client.authorization_uri.to_s
-      redirect_to auth_uri
+      redirect_to(auth_uri, allow_other_host: true)


Rails 7 protects against trying to redirect_to to an external URL: https://api.rubyonrails.org/v7.0.8/classes/ActionController/Redirecting.html#method-i-redirect_to

This is safe because we are setting this auth_uri ourselves and it's not coming from a user-provided param/link

murny · 2023-12-24T00:28:43Z

config/application.rb

@@ -22,11 +22,11 @@
 module Jupiter
  class Application < Rails::Application

-    require_dependency 'lib/jupiter'
-    require_dependency 'lib/jupiter/version'
+    require_relative '../lib/jupiter'


These require relatives to lib directory can go away after Rails 7.1 which introduces autoloading lib automatically:

https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#config-autoload-lib-ignore

murny · 2023-12-24T00:29:29Z

config/environments/development.rb

@@ -16,6 +16,9 @@
  # Show full error reports.
  config.consider_all_requests_local = true

+  # Enable server timing
+  config.server_timing = true


These environments changes came from the generator. So just updating them to match.

murny · 2023-12-24T16:57:46Z

Gemfile

-  gem 'pry-byebug'
-  gem 'pry-rails'
+  # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
+  gem 'debug', platforms: [:mri, :mingw, :x64_mingw]


Ruby ships with a debugger out of the box now and it's actually really good (and getting better all the time). Will remove pry/byebug unless anyone has concerns about this.

Can use binding.irb now to create a debugger in your code instead of byebug or binding.pry

Looks good, thanks for the link! It looks like debugger is another way to create a debugger in your code.

pgwillia

Nice work! Thanks for your explanatory comments.

I just have some minor questions. Overall looks good 👍

pgwillia · 2024-01-12T16:30:51Z

Gemfile

@@ -102,9 +101,8 @@ end
 group :development do
  gem 'better_errors', '>= 2.3.0'
  gem 'binding_of_caller'
+  # gem "rack-mini-profiler"


Just curious why the comment here?

It's brought in by the generator, I can remove it for now

(although think it is a nice thing to bring in. Rack mini profiler gives a tool tip that shows performance metrics like how fast a page was rendered and where that time was all spent. But we can do this after this gets merged)

Removed this comment 👍

pgwillia · 2024-01-12T16:33:44Z

Gemfile

-  gem 'pry-byebug'
-  gem 'pry-rails'
+  # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
+  gem 'debug', platforms: [:mri, :mingw, :x64_mingw]


Looks good, thanks for the link! It looks like debugger is another way to create a debugger in your code.

pgwillia · 2024-01-12T16:46:08Z

config/application.rb

+    require_relative '../lib/jupiter'
+    require_relative '../lib/jupiter/version'


Shouldn't this work now? https://devdocs.io/rails~7.0/activesupport/dependencies/requiredependency#method-i-require_dependency

Suggested change

require_relative '../lib/jupiter'

require_relative '../lib/jupiter/version'

require 'jupiter'

require 'jupiter/version'

require needs an absolute URL unless its on your $LOAD_PATH. So jupiter shouldn't be on the $LOAD_PATH so this wouldnt work.

But require '/code/jupiter/lib/jupiter/version' would work.

require_relative is just an alternative to allow you to use relative version of require which is what I did here.

Either way in Rails 7.1 this should go away as we will autoload the lib directory.

That's not what I'm observing.

Idiomatic Rails applications only issue require calls to load stuff from their lib directory, the Ruby standard library, Ruby gems, etc. That is, anything that does not belong to their autoload paths, explained below.
https://guides.rubyonrails.org/v7.0/autoloading_and_reloading_constants.html#introduction

Ah cool, thanks for the link. Will change this 👍

Seems like they put lib directory in the $LOAD_PATH by default.

Another nice quote from the above article:

For instance, lib is an idiomatic choice. It does not belong to the autoload paths by default, but it does
belong to $LOAD_PATH. Just perform a regular require to load it.

Updated code to use the simple require version for these

pgwillia · 2024-01-12T16:53:37Z

config/environments/production.rb

@@ -65,16 +65,10 @@

  # Enable locale fallbacks for I18n (makes lookups for any locale fall back to
  # the I18n.default_locale when a translation cannot be found).
-  config.i18n.fallbacks = [:en]
+  config.i18n.fallbacks = true


We changed this deliberately, but it's been a while: 1b8aee7 Was there another change in behaviour in the interim?

This came from the generator but it should be equivalent?

config.i18n.fallbacks = true will fall back to the I18n.default_locale value, which defaults to :en if it isn't defined.

config.i18n.fallbacks = [:en] well, just sets it to :en

So should be the same.

More info: https://guides.rubyonrails.org/v7.0.0/configuring.html#configuring-i18n

Reading that issue and the PR leaves me more confused 🤔 But even on a brand new rails application it would be set as true with the following comment:

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to # the I18n.default_locale when a translation cannot be found). config.i18n.fallbacks = true

Wonder if Rails does something different here?

I think I'm out to lunch. It looks like I missed the instruction about Rails < 5.2.2. We were on 5.2.3 at the time so this never applied to us. All good here!

pgwillia · 2024-01-12T17:01:01Z

config/routes.rb

+require_relative '../lib/admin_constraint'
+require_relative '../lib/entity_constraint'


Same as above.

Suggested change

require_relative '../lib/admin_constraint'

require_relative '../lib/entity_constraint'

require 'admin_constraint'

require 'entity_constraint'

Done! Thanks for calling this out 🙏

pgwillia · 2024-01-12T17:25:49Z

test/fixtures/items.yml

@@ -16,19 +16,19 @@ item_fancy:

 item_practical:
  visibility: <%= JupiterCore::VISIBILITY_PUBLIC %>
-  owner: regular
+  owner: user_regular


Good catch!

pgwillia · 2024-01-12T17:34:20Z

app/models/application_record.rb

@@ -1,5 +1,5 @@
 class ApplicationRecord < ActiveRecord::Base

-  self.abstract_class = true
+  primary_abstract_class


I hadn't seen this discussed anywhere, but maybe I missed it. Is this a new convention? It means the same thing, right?

Came from the generator but yeah, it does the same thing. Just with a bit of checks/guards around it.

More info and the source code of this method here:
https://edgeapi.rubyonrails.org/classes/ActiveRecord/Inheritance/ClassMethods.html#method-i-primary_abstract_class

ConnorSheremeta · 2024-01-12T18:02:09Z

config/application.rb


    # Initialize configuration defaults for originally generated Rails version.
-    config.load_defaults 6.1
+    config.load_defaults 7.0


Found this tid-bit in the Rails upgrade docs regarding staged deployment, I am curious if this is relevant or has an impact:

... Rails 6.1 applications are not able to read this new serialization format, so to ensure a seamless upgrade you must first deploy your Rails 7.0 upgrade with config.active_support.cache_format_version = 6.1, and then only once all Rails processes have been updated you can set config.active_support.cache_format_version = 7.0.

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format

Good call out 👍

But I think we okay here, as "Rails 7.0 is able to read both formats, so the cache won't be invalidated during the upgrade."

We also only use cache in a single place in our application (for Read Only mode), so if the cache does get invalidated, it's not a big issue.

ConnorSheremeta

Looks good from my end! 👍

…_fs(:db) instead." warnings

…gainst external redirects

…ils 7

pgwillia

murny force-pushed the Rails-7-upgrade branch from fbcc621 to 0968d35 Compare October 7, 2023 04:38

murny force-pushed the Rails-7-upgrade branch from de3616b to 2904245 Compare October 14, 2023 19:25

murny commented Oct 14, 2023

View reviewed changes

murny mentioned this pull request Oct 14, 2023

Use .class instead of .singleton_class due to issue with Rails 7 ualbertalib/acts_as_rdfable#14

Merged

murny marked this pull request as ready for review October 14, 2023 23:20

murny requested review from ConnorSheremeta, jefferya, lagoan and pgwillia as code owners October 14, 2023 23:20

murny force-pushed the Rails-7-upgrade branch from 170e0d8 to e2f2839 Compare November 8, 2023 17:00

murny force-pushed the Rails-7-upgrade branch from e2f2839 to 351923e Compare December 14, 2023 05:07

murny commented Dec 24, 2023

View reviewed changes

pgwillia reviewed Jan 12, 2024

View reviewed changes

ConnorSheremeta reviewed Jan 12, 2024

View reviewed changes

ConnorSheremeta reviewed Jan 15, 2024

View reviewed changes

murny added 9 commits January 15, 2024 22:28

Bump Rails version to 7.0

40cbedc

require_dependency no longer exists, update these

f97d97f

Fix rubocop for formatted string

088dc83

Fix "TimeWithZone#to_s(:db) is deprecated. Please use TimeWithZone#to…

a10e7bb

…_fs(:db) instead." warnings

Update files from generator

6c82945

Fix bad fixture, associations do not exist causes issues

86bd2e8

Fix new changes in Rails 7, which uses VIPS by default and protects a…

f63ea33

…gainst external redirects

Update branch to use hotfix in ActsAsRDF gem

bcfc8e1

Fix rubocop violations

ea871a9

murny added 3 commits January 15, 2024 22:29

Add changelog entry

492ab09

Use new ActsAsRDFable release v0.5.0, which contains a bug fix for Ra…

e5b3aa8

…ils 7

Make changes from review feedback

1ddbf8d

murny force-pushed the Rails-7-upgrade branch from 3f72982 to 1ddbf8d Compare January 16, 2024 05:31

pgwillia approved these changes Jan 16, 2024

View reviewed changes

Merge branch 'master' into Rails-7-upgrade

bef06c7

murny merged commit 57bee3a into master Jan 17, 2024
4 checks passed

murny deleted the Rails-7-upgrade branch January 17, 2024 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 7 upgrade #3250

Rails 7 upgrade #3250

murny commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Oct 7, 2023 •

edited

Loading

murny commented Oct 10, 2023 •

edited

Loading

murny Oct 14, 2023 •

edited

Loading

murny Oct 14, 2023

pgwillia Jan 12, 2024

murny Oct 14, 2023

murny Dec 24, 2023

murny Dec 24, 2023 •

edited

Loading

murny Dec 24, 2023

pgwillia Jan 12, 2024

pgwillia left a comment

pgwillia Jan 12, 2024

murny Jan 13, 2024

murny Jan 16, 2024

pgwillia Jan 12, 2024

pgwillia Jan 12, 2024

murny Jan 13, 2024

pgwillia Jan 15, 2024

murny Jan 16, 2024 •

edited

Loading

murny Jan 16, 2024

pgwillia Jan 12, 2024

murny Jan 13, 2024

murny Jan 13, 2024

pgwillia Jan 15, 2024

pgwillia Jan 12, 2024

murny Jan 16, 2024

pgwillia Jan 12, 2024

pgwillia Jan 12, 2024

murny Jan 13, 2024

ConnorSheremeta Jan 12, 2024

murny Jan 13, 2024

ConnorSheremeta left a comment

pgwillia left a comment

		require_relative '../lib/jupiter'
		require_relative '../lib/jupiter/version'

		require_relative '../lib/admin_constraint'
		require_relative '../lib/entity_constraint'

Rails 7 upgrade #3250

Rails 7 upgrade #3250

Conversation

murny commented Sep 27, 2023 • edited Loading

github-actions bot commented Oct 7, 2023 • edited Loading

murny commented Oct 10, 2023 • edited Loading

murny Oct 14, 2023 • 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

Choose a reason for hiding this comment

murny Dec 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgwillia left a 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

Choose a reason for hiding this comment

murny Jan 16, 2024 • 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

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

Choose a reason for hiding this comment

ConnorSheremeta left a comment

Choose a reason for hiding this comment

pgwillia left a comment

Choose a reason for hiding this comment

murny commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Oct 7, 2023 •

edited

Loading

murny commented Oct 10, 2023 •

edited

Loading

murny Oct 14, 2023 •

edited

Loading

murny Dec 24, 2023 •

edited

Loading

murny Jan 16, 2024 •

edited

Loading