THREESCALE-11448: Upgrade to Rails 7.0 #3945

mayorova · 2024-11-15T23:17:05Z

What this PR does / why we need it:

Upgrading to Rails 7.0 to keep our stack up-to-date.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-11448

Verification steps

Everything should work.

Special notes for your reviewer:

References:

check for deprecation messages in current production logs
upgrade Rails and dependent gems
go through upgrade guides:
go through notable changes in release notes
remove workarounds and monkey patches in code pointed to by rg -i "rails \d"
regenerate database schema as Rails 7.0 updated format
make sure oracle-enhanced works
Set load_defaults to 7.0 at application.rb
Note about Ruby/Rails compat matrix: https://www.fastruby.io/blog/ruby/rails/versions/compatibility-table.html

Additional references:
https://dev.to/hassanahmed/enabling-rails-70s-new-framework-defaults-hkb

.circleci/config.yml

jlledom · 2024-11-27T09:54:31Z

Ready to deploy!

test/unit/models_test.rb

test/unit/liquid/forms_test.rb

test/unit/services/signup_service_test.rb

akostadinov · 2024-12-11T13:24:34Z

lib/developer_portal/lib/liquid/template/fallback_resolver_no_prefix.rb

-
-        query(path, details, details[:formats], locals, cache: !!key)
+        # force just liquid format and set an empty prefix
+        super(name, '', partial, details.merge(handlers: [:liquid]), key, locals)


Could you explain this? In particular it is not clear why do we force an empty prefix while previously the method used the prefix passed.

It's not very clear to me either 😅

Basically, it has to do with the structure of the template files... We have some "shared" template files that are loaded from the file system, if they are not present in the provider's CMS, and as they are shared, they are not prefixed with the prefix, corresponding to the controller.

I actually tried removing this FallbackResolverNoPrefix class, but had some templates not being loaded.

For example, there is a template lib/developer_portal/app/views/developer_portal/_submenu.html.liquid, which is kind of "shared", and can be included as a partial in different templates. And in this case, we need to remove the prefix that the controller adds (e.g. prefix: "login" for the LoginController) to get the template resolved.

Makes sense. Thanks I couldn't figure out how id it work in the past so that's why I asked. Could it have been related to the default path value? Anyway, I'm fine with it.

Yeah, I guess they are related in the sense that the path is constructed as: "path" + "prefix" + "name", so all of the values should be correct for the right template to be found.

akostadinov · 2024-12-11T13:27:22Z

lib/developer_portal/lib/liquid/template/fallback_resolver.rb

@@ -8,19 +8,8 @@ def initialize(path = DeveloperPortal::VIEW_PATH)
      end


can we also remove this constructor? Or the default value is important? Why is default value not important with FallbackResolverNoPrefix?

I think provided path is needed in the constructor. In FallbackResolverNoPrefix not that it's not important, but it's just that it inherits from FallbackResolver.

The FileSystemResolver has

def initialize(path)

So the path argument exists. The difference is that we add a default value. So the question is whether we need this default value here as well in FallbackResolverNoPrefix.

Yes, what I mean is that as FallbackResolverNoPrefix is a subclass of FallbackResolver, it inherits that constructor, and it already has that default value for path.

lib/arel/visitors/oracle12_hack.rb

akostadinov · 2024-12-11T13:33:27Z

config/initializers/wrap_parameters.rb

-# Enable parameter wrapping for JSON. You can disable this by setting :format to an empty array.
-ActiveSupport.on_load(:action_controller) do
-  wrap_parameters format: []
-end


I guess not needed anymore but just to confirm.

akostadinov · 2024-12-11T13:36:53Z

config/initializers/money.rb

+
+  ActiveSupport.on_load(:active_record) do
+    ActiveRecord::Base.include ThreeScale::HasMoney
+  end


😮 this might have been an overlook previously causing ActiveRecord to be loaded prematurely 🤔

Why? The code that references ActiveRecord was not being executed until ActiveSupport was loaded

$ RAILS_ENV=test bundle exec rake ci:db:ready db:create Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead. Connected to mysql2://[email protected]/systemdb0 rake aborted! NameError: uninitialized constant ThreeScale::HasMoney ActiveRecord::Base.include ThreeScale::HasMoney ^^^^^^^^^^ Did you mean? Hashery /home/dmayorov/Projects/3scale/porta/config/initializers/money.rb:4:in `block in <main>' /home/dmayorov/Projects/3scale/porta/config/initializers/money.rb:3:in `<main>' /home/dmayorov/Projects/3scale/porta/config/environment.rb:8:in `<main>' /home/dmayorov/.asdf/installs/ruby/3.1.5/bin/bundle:25:in `load' /home/dmayorov/.asdf/installs/ruby/3.1.5/bin/bundle:25:in `<main>' Tasks: TOP => db:create => db:load_config => environment (See full trace by running task with --trace)

app/assets/stylesheets/provider/_forms.scss

akostadinov · 2024-12-11T13:46:34Z

config/environments/test.rb

-  # test suite. You never need to work with it otherwise. Remember that
-  # your test database is "scratch space" for the test suite and is wiped
-  # and recreated between test runs. Don't rely on the data there!
+  # Turn false under Spring and add config.action_view.cache_template_loading = true.
  config.cache_classes = true


Suggested change

config.cache_classes = true

config.cache_classes = !Kernel.const_defined?(:Spring)

Can you please explain?
Basically, I applied the new defaults for the environment files.

And... we don't even use spring, do we?... 🤔

If spring is defined, then we don't cache classes as explained in the comment. Just make it automatic. Yeah, no Spring. I think we had it a few years ago still. Not sure.

Well, if we ever do use it again, I'd jsut change the config, as this comment instructs:

Turn false under Spring and add config.action_view.cache_template_loading = true.

Using spring is a personal decision IMO. But whatever, not important.

akostadinov · 2024-12-11T13:49:18Z

config/environments/test.rb

@@ -94,7 +94,7 @@
    if defined?(Bullet)
      Bullet.enable = true
      Bullet.bullet_logger = true
-      Bullet.raise = true
+      Bullet.raise = false # FIXME: disable


were failures a lot? We need to check this before merging but perhaps better to get things ready and then investigate the N+1 issues

Yes, there were a lot of failures, so I temporarily disabled exceptions just to move forward, and the plan is to come back to it later.

mayorova · 2024-12-19T14:47:42Z

config/initializers/access_token_authentication.rb

-        include ApiAuthentication::ByAccessToken::ConnectionExtension
-      end
+ActiveSupport.on_load(:active_record) do
+  require "api_authentication.rb"


Is the .rb needed?

mayorova · 2024-12-19T14:51:37Z

lib/api_authentication.rb

@@ -0,0 +1,130 @@
+# frozen_string_literal: true
+
+module ApiAuthentication


Maybe call it AccessTokenAuthentication?... So it's more specific.

The issue is that this will require a lot of updates.

Remove irrelevant comment

Add a note for 'raise_on_missing_translations' config Fix unit tests

DEPRECATION WARNING: Rails 7.0 has deprecated Enumerable.sum in favor of Ruby's native implementation available since 2.4. Sum of non-numeric elements requires an initial argument. (called from timeline_data at /home/dmayorov/Projects/3scale/porta/app/controllers/provider/admin/dashboard/widget_controller.rb:31)

…g = true` Message:Couldn't find Cinstance with [WHERE `cinstances`.`type` = ? AND `cinstances`.`user_account_id` = ?] (ActiveRecord::RecordNotFound) /opt/ci/project/vendor/bundle/ruby/3.1.0/gems/activerecord-7.0.8.6/lib/active_record/relation/finder_methods.rb:378:in `raise_record_not_found_exception!' /opt/ci/project/vendor/bundle/ruby/3.1.0/gems/activerecord-7.0.8.6/lib/active_record/relation/finder_methods.rb:153:in `first!' /opt/ci/project/app/models/account/provider_methods.rb:157:in `provider_key' /opt/ci/project/app/lib/backend/model_extensions/service.rb:20:in `update_backend_service' Remove reset in #provider_key and fix tests

FIXME: temporarily disable some Account associations for tenant ID checker supercede e415673 fixing tenant integrity check

akostadinov · 2024-12-24T13:51:33Z

config/initializers/new_framework_defaults_7_0.rb

this file can be removed

mayorova force-pushed the rails70 branch 2 times, most recently from 11a0b23 to 636ab7e Compare November 21, 2024 15:45

mayorova commented Nov 21, 2024

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

mayorova force-pushed the rails70 branch 2 times, most recently from 59ac224 to 86612ed Compare November 22, 2024 11:00

jlledom previously approved these changes Nov 27, 2024

View reviewed changes

mayorova dismissed jlledom’s stale review via 3e04117 November 27, 2024 11:57

mayorova force-pushed the rails70 branch 11 times, most recently from b61b6c4 to ea69c96 Compare December 4, 2024 11:40