Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Injection issue in rails 7 & ruby 3.1 #53

Open
itx-rb opened this issue Feb 16, 2022 · 13 comments
Open

Injection issue in rails 7 & ruby 3.1 #53

itx-rb opened this issue Feb 16, 2022 · 13 comments
Labels
feature help wanted Extra attention is needed

Comments

@itx-rb
Copy link

itx-rb commented Feb 16, 2022

We have more than 20 rails app using rails 6 & below and ruby 2.7 & below, there are implemented service pattern using dry injection, it is broken on rails 7 & ruby 3.1

I'm getting an error looks like

Started POST "/api/v1/posts/search.json" for ::1 at 2022-02-16 00:26:43 +0000
  
ArgumentError (wrong number of arguments (given 1, expected 0)):
  
actionpack (7.0.2.2) lib/action_controller/metal.rb:150:in `initialize'
actionpack (7.0.2.2) lib/action_dispatch/routing/url_for.rb:108:in `initialize'
dry-auto_inject (0.9.0) lib/dry/auto_inject/strategies/kwargs.rb:71:in `block (2 levels) in define_initialize_with_splat'
dry-auto_inject (0.9.0) lib/dry/auto_inject/strategies/kwargs.rb:22:in `new'
dry-auto_inject (0.9.0) lib/dry/auto_inject/strategies/kwargs.rb:22:in `block (2 levels) in define_new'
actionpack (7.0.2.2) lib/action_controller/metal.rb:251:in `dispatch'
actionpack (7.0.2.2) lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
actionpack (7.0.2.2) lib/action_dispatch/routing/route_set.rb:32:in `serve'
actionpack (7.0.2.2) lib/action_dispatch/journey/router.rb:50:in `block in serve'
actionpack (7.0.2.2) lib/action_dispatch/journey/router.rb:32:in `each'
actionpack (7.0.2.2) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (7.0.2.2) lib/action_dispatch/routing/route_set.rb:850:in `call'
rack (2.2.3) lib/rack/etag.rb:27:in `call'
rack (2.2.3) lib/rack/conditional_get.rb:40:in `call'
rack (2.2.3) lib/rack/head.rb:12:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
activesupport (7.0.2.2) lib/active_support/callbacks.rb:99:in `run_callbacks'
actionpack (7.0.2.2) lib/action_dispatch/middleware/callbacks.rb:26:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/actionable_exceptions.rb:17:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/debug_exceptions.rb:28:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/show_exceptions.rb:26:in `call'
railties (7.0.2.2) lib/rails/rack/logger.rb:36:in `call_app'
railties (7.0.2.2) lib/rails/rack/logger.rb:25:in `block in call'
activesupport (7.0.2.2) lib/active_support/tagged_logging.rb:99:in `block in tagged'
activesupport (7.0.2.2) lib/active_support/tagged_logging.rb:37:in `tagged'
activesupport (7.0.2.2) lib/active_support/tagged_logging.rb:99:in `tagged'
railties (7.0.2.2) lib/rails/rack/logger.rb:25:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/remote_ip.rb:93:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/request_id.rb:26:in `call'
rack (2.2.3) lib/rack/runtime.rb:22:in `call'
activesupport (7.0.2.2) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/server_timing.rb:20:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/executor.rb:14:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/static.rb:23:in `call'
rack (2.2.3) lib/rack/sendfile.rb:110:in `call'
actionpack (7.0.2.2) lib/action_dispatch/middleware/host_authorization.rb:137:in `call'
railties (7.0.2.2) lib/rails/engine.rb:530:in `call'
puma (5.6.2) lib/puma/configuration.rb:252:in `call'
puma (5.6.2) lib/puma/request.rb:77:in `block in handle_request'
puma (5.6.2) lib/puma/thread_pool.rb:340:in `with_force_shutdown'
puma (5.6.2) lib/puma/request.rb:76:in `handle_request'
puma (5.6.2) lib/puma/server.rb:441:in `process_client'
puma (5.6.2) lib/puma/thread_pool.rb:147:in `block in spawn_thread'

To Reproduce

  1. create a new app using rails 7 with api mode
  2. create a service with a single method so we have this structure under app directory
app/
  controllers/
    api/
      v1/
  ...
  ...
  services/
    v1/
module V1
    class PostService
        def search(keyword)
             "OK"
        end
    end
end
  1. create a container class named di_container.rb on lib/marka/ directory and register and initialize the PostService class
require 'dry-container'
require 'dry-auto_inject'

module Marka
    class DiContainer
        extend Dry::Container::Mixin
        
        register :v1_post_service do
            V1::PostService.new
        end
    end
    INJECT = Dry::AutoInject(Marka::DiContainer)
end
  1. create a new controller called posts_controller under app/controllers/api/v1/ directory, and try to inject the method which registered on Marka module
require 'marka/di_container'
module Api
  module V1
    class PostsController < ApplicationController
      include Marka::INJECT[:v1_post_service]

      def search
         render json: { status: "OK" }
      end
    end
  end

Expected behavior

Upgrade our apps to rails 7 using the same pattern.

My environment

  • Affects my production application: YES due to development issue
  • Ruby version: 3.1
  • Rails 7.0.2.2
  • OS: MacOS Mojave 10.14.6
  • dry-auto_inject (0.9.0)
  • dry-container (0.9.0)

stakeoverflow question :
https://stackoverflow.com/q/71131422/2858044

@itx-rb itx-rb added bug Something isn't working help wanted Extra attention is needed labels Feb 16, 2022
@solnic
Copy link
Member

solnic commented Feb 16, 2022

This is a change in Rails that broke your code, by using auto-inject you define a constructor and it looks like it's no longer compatible with Rails 7. dry-auto_inject shouldn't be used in 3rd-party libraries exactly because of this. We can't do anything about this here so I'm going to move this issue to dry-rails and we can see how it could be done there.

@solnic solnic removed the bug Something isn't working label Feb 16, 2022
@solnic solnic transferred this issue from dry-rb/dry-auto_inject Feb 16, 2022
@solnic
Copy link
Member

solnic commented Feb 16, 2022

This was moved from dry-auto_inject because we could come up with a nice integration with the controller API. Marking it as help-wanted as I don't have time (for now at least) to work on this.

@solnic solnic added the feature label Feb 16, 2022
@itx-rb
Copy link
Author

itx-rb commented Feb 20, 2022

I thank you @solnic for your response and help to moved this issue.

I thought dry-auto-_inject is supported the ruby ecosystem, including rails. I have use that (starting rails 4 & ruby 2.3) which is since 5 years ago and stable for production until now with more than 20 rails apps using rails 6 and below, and still thinking upgrade them early to rails 7.

I need to find another way to make it work without big changes, for the moment I try to use dry-rails, and I'm not sure about this strategy

Add a path app/services to container

# config/initializers/marka.rb
Dry::Rails.container do
    config.component_dirs.add "app/services"
end

and in controller I can call looks like

MyApp::Container['v1.post_service'].search(params[:keyword])

What do you think about this?

@solnic
Copy link
Member

solnic commented Feb 21, 2022

I thought dry-auto-_inject is supported the ruby ecosystem, including rails.

I should clarify - dry-auto_inject works with Rails but you assume you can use it with any class, which is not the case because the purpose of dry-auto_inject is to define a constructor method that will receive dependencies that are automatically resolved from the configured container. Because of this, it is not advices to include injection modules in classes that you don't own because it may break them.

We should definitely explain this in the docs 🙂

MyApp::Container['v1.post_service'].search(params[:keyword])
What do you think about this?

Actually, dry-rails gives you a resolve helper in controllers, so this can become resolve('v1.post_service').search(params[:keyword]). You could also provide a temporary solution while you're in the process of migrating to dry-rails by simply defining method_missing in your application controller, something like this should work:

def method_missing(name, *args)
  if container.key?("v1.#{name}")
    resolve("v1.#{name}")
  else
    super
  end
end

This is obviously a hack but it should help with the transition 🙂

@elja
Copy link

elja commented May 16, 2022

On our project we updated dry-rb to the latest version and faced the same issue, so we decided to build our own injection strategy for controllers:

# frozen_string_literal: true

require "dry/auto_inject/dependency_map"

module Dry
  module AutoInject
    class Strategies
      class Resolve < Module
        InstanceMethods = Class.new(Module)

        attr_reader :container
        attr_reader :dependency_map
        attr_reader :instance_mod

        def initialize(container, *dependency_names)
          super()
          @container = container
          @dependency_map = DependencyMap.new(*dependency_names)
          @instance_mod = InstanceMethods.new
        end

        # @api private
        def included(klass)
          define_resolvers
          klass.send(:include, instance_mod)
          super
        end

        private

        def define_resolvers
          instance_mod.class_exec(container, dependency_map) do |container, dependency_map|
            dependency_map.to_h.each do |name, key|
              define_method name do
                container[key]
              end
            end
          end
        end
      end

      register :resolve, Resolve
    end
  end
end

and then in controllers we do:

include YourApp::Import.resolve[
  'your.operations.name',
]

@k0va1
Copy link

k0va1 commented Dec 25, 2022

Hi @solnic ! I'm also struggling with this issue after upgrading from dry-rails v0.2 to the latest. I don't think that it's a Rails problem because I'm still at Rails v6.1.6.1. I'm also tried to reproduce the bug in test for dry-rails and it's failed. Like this:

class ApiUsersController < ActionController::API
  include Dummy::Deps[
    mailer: "mailer"
  ]
  ...
end
 ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # ./spec/requests/api_users_spec.rb:16:in `block (4 levels) in <top (required)>'

I think there is something wrong with dry-auto_inject but I don't know yet what exactly. Maybe you can give some advice?

Thanks!

k0va1 added a commit to k0va1/dry-rails that referenced this issue Dec 25, 2022
@k0va1
Copy link

k0va1 commented Dec 25, 2022

I'm not sure that this is a smarter one but it fixes the problem k0va1@767154a

@itx-rb
Copy link
Author

itx-rb commented Sep 4, 2023

Hi @solnic I just got informed, it's weird the problem is solved when I use action controller base instead of using the api mode. meanwhile the api mode is a lightweight version, which is exclude some modules from base.

@oskargargas
Copy link

My app worked without problems using Rails 7.0 and Ruby 3.3. Problem appeared after upgrading to Rails 7.1.
Solution proposed by @k0va1 helped. At least for now. Can we get it merged?

@elct9620
Copy link

elct9620 commented May 24, 2024

Same problem with @oskargargas after upgrading to Rails 7.1


I think it breaks due to this change:
rails/rails@fc95032

My debug gem backtrace

(rdbg) bt    # backtrace command
=>#0    ActionView::Layouts#initialize at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/actionview-7.1.3.3/lib/action_view/layouts.rb:365
  #1    ActionController::RequestForgeryProtection#initialize at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.1.3.3/lib/action_controller/metal/request_forgery_protection.rb:346
  #2    ActionController::Instrumentation#initialize at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.1.3.3/lib/action_controller/metal/instrumentation.rb:22
  #3    ActiveRecord::Railties::ControllerRuntime#initialize at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.1.3.3/lib/active_record/railties/controller_runtime.rb:20
  #4    block {|args=[], kwargs={:menu_query=>#<MenuQuery:0x000000010c091..., block=nil|} in define_initialize_with_splat (2 levels) at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/dry-auto_inject-1.0.1/lib/dry/auto_inject/strategies/kwargs.rb:68
  #5    [C] Class#new at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/dry-auto_inject-1.0.1/lib/dry/auto_inject/strategies/kwargs.rb:19
  #6    block {|args=[], kwargs={:menu_query=>#<MenuQuery:0x000000010c091..., block=nil|} in define_new (2 levels) at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/dry-auto_inject-1.0.1/lib/dry/auto_inject/strategies/kwargs.rb:19
  #7    #<Class:ActionController::Metal>#dispatch(name="index", req=#<ActionDispatch::Request GET "http://www..., res=#<ActionDispatch::Response:0x000000010d69...) at ~/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.1.3.3/lib/action_controller/metal.rb:309

The class/module after inject is changed to add initialize(...) for performance reason.

@charlieoliver
Copy link

Is there any expectation of movement on this? Is this known to be something that will be tackled?

@timriley
Copy link
Member

@charlieoliver I'd love it if someone could submit a PR to address this.

@charlieoliver
Copy link

@charlieoliver I'd love it if someone could submit a PR to address this.

Not in my skill set but I hope that putting the ask out there may spark something! Thank you for responding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants