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)

If I revert this change, everything works fine for me, also db:create.

I don't know why is failing for you but OK. I think wrapping the ActiveSupport.on_load(:active_record) block under to_prepare can lead it to be executed multiple times, once after every change in the code, when in development.

However I don't think its a big issue because:

After every change in the code, a new callback will be added to be executed when :active_record is loaded, but I'm not sure ActiveRecord is loaded after every code change as well, I'd say it doesn't, so in the worst case you would end up with a stack of callbacks in memory which will never be called.

Even if the callbacks are called, I assume ActiveRecord::Base.send(:include, ThreeScale::HasMoney) is idempotent, ActiveRecord::Base will end up having ThreeScale::HasMoney just once in its ancestors.

So I would ask you to revert the changes like I did, to verify it fails again for you, since apparently it works for me. That would be ideal because we could avoid having to_prepare and on_load nested calls. If it keeps failing for you, I'd say what you have now is good enough.

Retesting is fine. But the thing is that ThreeScale::HasMoney should be reloaded on code reload. So it may disappear from ActiveRecord::Base and not readded. That's why it looks correct to have it wrapped in to_prepare.

If ThreeScale::HasMoney changes, Zeitwerk will reload it the first time it's accessed again, but I'm not sure how true is that for modules included from libraries. Anyway, good to leave it there.

app/lib/three_scale/has_money.rb, it is not in lib/

I'm fine if it works either way but I though any code relying on app/ in initializers has to be within to_prepare. It's a tricky question.

By "libraries" I meant ActiveRecord::Base

If I revert this change, everything works fine for me, also db:create.

I don't know why is failing for you but OK.
So I would ask you to revert the changes like I did, to verify it fails again for you, since apparently it works for me. That would be ideal because we could avoid having to_prepare and on_load nested calls. If it keeps failing for you, I'd say what you have now is good enough.

That's very strange that it works for you. I already retested it twice I think, always with the same result. And also, in CircleCI it fails with the same error. So, maybe you have something magical in your local setup 🤔

I think wrapping the ActiveSupport.on_load(:active_record) block under to_prepare can lead it to be executed multiple times, once after every change in the code, when in development.

However I don't think its a big issue because:

After every change in the code, a new callback will be added to be executed when :active_record is loaded, but I'm not sure ActiveRecord is loaded after every code change as well, I'd say it doesn't, so in the worst case you would end up with a stack of callbacks in memory which will never be called.

Even if the callbacks are called, I assume ActiveRecord::Base.send(:include, ThreeScale::HasMoney) is idempotent, ActiveRecord::Base will end up having ThreeScale::HasMoney just once in its ancestors.

Actually, rechecking this doc I understand that ThreeScale::HasMoney should not be reloadable, so I'll actually move it from app/lib to lib, and this way it works without being in to_prepare.

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.

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

config/initializers/new_framework_defaults_7_0.rb

this file can be removed

Yep, this was left accidentally. Thanks for catching!

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

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