Unexpected interaction with I18n

[geoffharcourt]

I'm trying to generate a small test case app to demonstrate this issue, but we experienced something odd when we tried to upgrade our Rails 6.0.0 app from rack-attack 6.1.0 to 6.2.0. The PR with the update, which does nothing beyond bump the one line in Gemfile.lock for rack-attack, results in all of our explicitly defined time.formats in our I18n locale files being replaced with the out-of-the-box formats distributed by Rails. We use YAML files in config/locales/*.yml to set our formats, and when I inspect them with I18n.backend.translations[:en][:time], I can see that with rack-attack 6.2.0 they are no longer what they normally are in our app.

I'm completely unclear how this could happen, as I can't find references to I18n or formats in the gem, but flipping the gem version back and forth in our development and test environments causes the formats in our config/locales/en.yml file to be wiped out.

[fatkodima]

Can't reproduce an issue. So would you provide a test app?

[geoffharcourt]

I can't make it happen in a toy app, but I did bisect the changes between 6.1.0 and 6.2.0, and the change that's causing our I18n to be modified is in #431. I'm looking now to see if I can tell why, it's not apparent at all why this would happen.

[geoffharcourt]

If I comment out this entire block, the mutation doesn't happen, but it has to be everything in this block including the initializer itself:

      initializer 'rack.attack.middleware', after: :load_config_initializers, before: :build_middleware_stack do |app|
        if Gem::Version.new(::Rails::VERSION::STRING) >= Gem::Version.new("5.1")
          middlewares = app.config.middleware
          operations = middlewares.send(:operations) + middlewares.send(:delete_operations)

          use_middleware = operations.none? do |operation|
            middleware = operation[1]
            middleware.include?(Rack::Attack)
          end

          middlewares.use(Rack::Attack) if use_middleware
        end
      end

[geoffharcourt]

Is there something about this initializer technique that might be mutating the middleware stack or nullifying other changes made to the stack? The mere use of Rack::Attack on its own (manually declaring it as we did before this commit) doesn't cause our translation format customizations to be cancelled out.

[eliotsykes]

I've seen something like this with the react-rails gem but I've not had the opportunity to debug it fully: reactjs/react-rails#1033

It looks like Rails itself encountered a related issue:

Avoid to define an initializer after the load_config_initializers
This make the config/initializers run before the railties are loaded
what can break some configurations.
rails/rails@0a120a8

I'm guessing a similar issue is happening here. It might be worth trying variations on when the initializer runs:

-initializer 'rack.attack.middleware', after: :load_config_initializers, before: :build_middleware_stack do |app|

+# No after, before options
+initializer 'rack.attack.middleware' do |app|

+# Try omitting and changing after and/or before options, values
+# listed at https://guides.rubyonrails.org/configuring.html#initializers
+initializer 'rack.attack.middleware', before: :build_middleware_stack do |app|

[grzuy]

I can't make it happen in a toy app, but I did bisect the changes between 6.1.0 and 6.2.0, and the change that's causing our I18n to be modified is in #431. I'm looking now to see if I can tell why, it's not apparent at all why this would happen.

Interesting.

Have you checked what's the output of $ rake middleware with 6.1.0 and 6.2.0?

[geoffharcourt]

@grzuy it's the exact same order and contents for both. I suspect causing initializers to fire early is somehow to blame, but I haven't managed to put enough from our large application into a toy to replicate the same behavior.

[geoffharcourt]

We were able to get around this issue in our application by removing a reference to I18n.load_path += in an initializer. I'm worried this might have other very subtle impacts on folks application loading given the comment in rails/rails@0a120a8.

Given that using rack-attack already requires configuration beyond just enabling the middleware, I'm not sure the convenience benefit of automatic inclusion offsets the risk of altering the order in which code is loaded in applications and the use of a private method to get the middleware loaded.

I apologize for not yet being able yet to produce a small public repo to reproduce the problem.

[grzuy]

We were able to get around this issue in our application by removing a reference to I18n.load_path += in an initializer. I'm worried this might have other very subtle impacts on folks application loading given the comment in rails/rails@0a120a8.

Given that using rack-attack already requires configuration beyond just enabling the middleware, I'm not sure the convenience benefit of automatic inclusion offsets the risk of altering the order in which code is loaded in applications and the use of a private method to get the middleware loaded.

I apologize for not yet being able yet to produce a small public repo to reproduce the problem.

Weird...
Thanks for sharing!

[grzuy]

@geoffharcourt Also trying to troubleshoot this on my end.

In the meantime, do you mind trying on your app if moving gem rack-attack to either the top or the bottom of the Gemfile has some effect?

Thank you!

[geoffharcourt]

Will do.

One thing that might also be possible to try would be to do require: false and then load it only when desired.

I still think this practice of using the before middleware initializer to autoload the middleware conditionally might be dangerous and cause unintended behaviors beyond the one I encountered.

[grzuy]

FWIW I was able to, at least, reproduce how gem 'rack-attack' placement in the Gemfile affects other gems loading order (e.g. sass-rails) in respect to config/initializers/* loading, which is of course unwanted and possibly problematic.

See grzuy/test_app_rack_attack_452@c49b739.

[grzuy]

I still think this practice of using the before middleware initializer to autoload the middleware conditionally might be dangerous and cause unintended behaviors beyond the one I encountered.

Agree.
Changed in #457.

[grzuy]

Fixed in v6.2.1.