Hanami 2.2 server fails to start with dry-system 1.2

[wuarmin]

Hey @flash-gordon

Describe the bug

If I start the hanami server I get following backtrace:

! Unable to load application: ArgumentError: wrong number of arguments (given 0, expected 1+)
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/loader.rb:54:in `call'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block (2 levels) in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `each'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:665:in `run_hooks'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:322:in `finalize!'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:330:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice_registrar.rb:64:in `each_value'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice_registrar.rb:64:in `each'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:331:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami.rb:214:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/boot.rb:5:in `<top (required)>'
        from /usr/local/lib/ruby/3.3.0/bundled_gems.rb:75:in `require'
        from /usr/local/lib/ruby/3.3.0/bundled_gems.rb:75:in `block (2 levels) in replace_require'
        from config.ru:3:in `block in <main>'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:116:in `eval'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:116:in `new_from_string'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:105:in `load_file'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:66:in `parse_file'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/configuration.rb:384:in `load_rackup'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/configuration.rb:297:in `app'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/runner.rb:166:in `load_and_bind'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/single.rb:44:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/launcher.rb:196:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/cli.rb:75:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/bin/puma:10:in `<top (required)>'
        from /usr/local/bundle/bin/puma:25:in `load'
        from /usr/local/bundle/bin/puma:25:in `<main>'

To Reproduce

calling bundle update at a Hanami 2.2 project updates dry-system to 1.2 because of
https://github.com/hanami/hanami/blob/2ca35811d4f2c7b0535c5bffdd973db9327f92ca/hanami.gemspec#L39

Expected behavior

The hanami server should start with no errors.

My environment

• Affects my production application: NO
• Ruby version: 3.3.5
• OS: Debian bookworm

[flash-gordon]

@wuarmin thanks for checking it out. Do you know how to reproduce it?

[cllns]

I just tried to reproduce this in my application and I wasn't able to. I first updated dry-system to 1.2 and then also updated dry-initializer to 3.2. All my tests pass and I'm able to start the server manually as well. I then updated all my gems and it's still working for me.

I really want to make sure others don't run into this problem, so any tips on reproducing would be super helpful.

[wuarmin]

Hey @cllns,
it has something to do with auto-registration. In my opinion, the boot behavior has changed in dry-system. Let me explain:

I have custom-changesets like:

# ots/repos/changesets/new_object_changeset.rb

module Ots
  module Repos
    module Changesets
      class NewObjectChangeset < ROM::Changeset::Create
        include Deps["now"]

        map do |tuple|
          current_time = now.call
          tuple.except(:id).merge(created_at: current_time, updated_at: current_time)
        end
      end
    end
  end
end 

Actually, “auto_register: false” would be appropriate here , but I forgot.

Behavior with dry-system 1.1:

services[development]> Ots::Slice.boot
=> Ots::Slice

With dry-system 1.1 no error happens at boot-time. To provoke the error, I have to explicitly access the key:

services[development]> Ots::Slice.boot
=> Ots::Slice
services[development]> Ots::Slice["repos.changesets.new_object_changeset"]
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/loader.rb:54:in `call'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:145:in `[]'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:624:in `[]'
        from (irb):2:in `<main>'
        from <internal:kernel>:187:in `loop'
        from /usr/local/bundle/gems/hanami-cli-2.2.1/lib/hanami/cli/commands/app/console.rb:48:in `call'
        ... 4 levels...

Behavior with dry-system 1.2

services[development]> Ots::Slice.boot
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/loader.rb:57:in `call'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block (2 levels) in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `each'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:665:in `run_hooks'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:322:in `finalize!'

Here the error happens at boot-time.

Conclusion:

With dry-system 1.1 there is no component-instantiation during the boot process, which in my opinion is correct.
With dry-system 1.2 there is a component-instantiation during boot process, which is unnecessary as you will receive a new instantiation when you access it anyway (except the component should be memoized).

What do you think?

Update
I think it is because of the eager_load-ft #276

Here is a quote from you:

Running .finalize! on a container would check to see if constants that correspond to each key that's registered are defined. It shouldn't actually resolve the components, but it should check to see if the components are resolveable (preresolve?)

In my opinion, it meets your requirement exactly. We want to see if the constant is defined and not create it completely. Or are you of the opinion that the extra instancing doesn't matter? I just think that users might be confused if the objects are created right at the start. It could lead to concerns that the object is not always recreated.

best regards

[timriley]

That's some great investigation, thank you @wuarmin!

It looks to me like the behaviour added via #276 has actually exposed an issue in your app setup: that your changeset classes should have been opted out from auto-registration, either via # auto_register: false magic comments or via container config (which you can do through Hanami with the app's config.no_auto_register_paths). Is this right?

So if you made these changes, @wuarmin, would your app boot again?

The issue you've hit here also makes me think we should add a conventional changesets path to our default value for no_auto_register_paths. We don't want to create extra work for users when they start using changesets. (n.b. I would not set it to be repos/changesets as you have used, @wuarmin, I would have it instead be db/changesets, since db/ is the directory we create for users to place their internal ROM components).

Now as for the change in #276, I do have to apologise: part of why I never commented on that PR is because I didn't want to bring this behaviour for Hanami 2.2 without due consideration, especially since that change was a "nice to have" and not essential for the Hanami release. I realise I should have been clear from the start and written that in a comment on the PR. I'm sorry!

This dry-system change relates to a few other areas we still need to investigate:

• The impact of defaulting to memoize: true for components in Hanami apps
• Whether we should set eager_load: true for dry-system's Zeitwerk plugin when hanami is in production mode (right now we have it as always false)
• And measuring the performance impact of all of these

IMO, these are the kinds of things that would require some thorough testing and would eventually become part of a significant new Hanami release, e.g. 2.3 or later.

Given #276 feature intersects with all of this, and given dry-system 1.2.0 has only been out for a day, I'd be inclined to revert #276 for now, and roll forward to a dry-system 1.2.1 release with that feature removed again.

@cllns @flash-gordon — would you be OK if we did this? Do you have any alternative approaches you'd rather we try?

[timriley]

One last thing: looking at the CHANGELOG line for that dry-system feature:

Option to skip eager loading during finalize with eager_load: false

...I wonder if @flash-gordon considered this change to be more minor than it actually was? It does add an eager_load: option, yes, but it also adds the whole eager loading machinery at the same time, which is a high-impact change that probably could have done with some more team discussion before we merged.

This is OK, though! It's just a learning experience for us :) dry-system is one of our more critical gems, and going forward we probably need to put a bit more time into assessing changes there before release.

[flash-gordon]

@timriley That's actually my dyslexia. I somehow read eager_load: true as false; otherwise, I would be a lot more cautious with this change. Maybe we can just set it to false for now and push 1.2.1?

[flash-gordon]

Also, maybe more testing is needed for this

[timriley]

@flash-gordon all good :) And thanks for hopping back into this thread so quickly! ❤️

Since the change is so small, and since it's still an open question about how exactly we address the fundamental issue ("how can we surface name errors in user code earlier?"), I think it's worth just reverting this change and pushing that out as 1.2.1.

This would give us the time and space to consider some different options.

What do you think?

[flash-gordon]

@timriley I don't mind reverting #282
Interestingly, it didn't impact me because I intentionally check all components are loadable in dev/prod environments