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

Service class implementation incompatible with Rails' Zeitwerk autoloader #429

Open
liveh2o opened this issue Aug 31, 2022 · 6 comments
Open

Comments

@liveh2o
Copy link
Contributor

liveh2o commented Aug 31, 2022

Rails 6.0 has a new autoloading mode which delegates to the Zeitwerk gem:

The classic autoloader has been extremely useful, but had a number of issues that made autoloading a bit tricky and confusing at times. Zeitwerk was developed to address this, among other motivations.

When upgrading to Rails 6.x, it is highly encouraged to switch to zeitwerk mode because it is a better autoloader, classic mode is deprecated.

Rails 7 ends the transition period and does not include classic mode.

Our RPC service class implementation depends on a stub being generated from the protobuf definition, then being reopened in your application (i.e., monkey patching) to implement the service endpoints:

# app/services/foo/user_service.rb
require 'lib/foo/user.pb'

# Reopen the service class and provide the implementation for each defined RPC method.
module Foo
  class UserService

    # request -> Foo::UserRequest
    # response -> Foo::UserResponse
    def find
      # request.email will be the unpacked string that was sent by the client request
      users = users_by_email.map(&:to_proto)
      respond_with(:users => users)
    end

    private

    def users_by_email
      User.find_by_email(request.email)
    end

  end
end

This pattern is incompatible with the Zeitwerk autoloader because files are loaded as classes/namespaces are defined by the generated Ruby protobuf definition. When Zeitwerk encounters the namespace/class while loading app/services (a sensible place to put service classes in a Rails app), the files are skipped:

[email protected]: the namespace Foo already exists, descending into .../app/services/foo
[email protected]: file .../app/services/foo/user_service.rb is ignored because Foo::UserService is already defined

To fix this, we need to reconsider how services are expected to be implemented.

One option would be to treat the generated service as a router that dispatches RPC requests to a separate application service class similar to the Rails router. The first cut would simply create the glue that allows this routing to take place based on the generated services (i.e., endpoint names and class methods would still need to be the same; no custom routing would be allowed). A future cut could make this more flexible, allowing requests to be routed as needed (i.e., conditionally based on a version scheme, etc.), but that may not be needed.

I would also anticipate this change being part of a major release in the event that backwards incompatible changes are required.

@film42
Copy link
Member

film42 commented Sep 1, 2022

I found this comment discussion money-patching in Zeitwerk: fxn/zeitwerk#141 (comment)

Are we ok to keep things the same if eager loading is used all the time? Just trying to figure out how broken this will be with Rails 7.

@liveh2o
Copy link
Contributor Author

liveh2o commented Sep 13, 2022

This issue isn't fixed by eager loading because it's more of an autoloading issue. Because these constants (modules, classes) are already defined by our Protobuf definition gem, they are skipped by Zeitwerk.

Otherwise, this horrible hack is needed in config/application.rb:

services = "#{Rails.root}/app/services"
Rails.autoloaders.main.ignore(services)

config.to_prepare do
  Dir.glob("#{atlas_services}/atlas/bifrost/*_service.rb").each do |service|
    load service
  end
end

With the middleware pattern introduced a while back, adding a routing layer shouldn't be terribly difficult (famous last words, I know).

@film42
Copy link
Member

film42 commented Sep 15, 2022

Is there a way for us to write a simple routing (or loading) layer without breaking the interface?

@liveh2o
Copy link
Contributor Author

liveh2o commented Sep 19, 2022

I'm pretty sure there is since the "interface" is simply reopening the class to monkeypatch it. The final middleware in the stack is the service dispatcher. It's responsible for calling the endpoint, and capturing the response.

Seems like adding a router there with some sensible defaults/fallbacks is the simplest approach.

The workaround, for now, is to simply use the classic autoloader. That works with Rails 6.x. I planned on tackling this once we have our internal apps all running Rails 6.1, which is getting closer.

@qd3v
Copy link

qd3v commented Feb 14, 2024

Adding few cents: actually because of strange decision to suffix all files with .pb.rb besides making filenames non-standard, the generated code is not compatible with ZW at all:

loader = Zeitwerk::Loader.new
loader.push_dir(Pathname(__dir__).join('pb'))
loader.setup
loader.eager_load
Zeitwerk::NameError: wrong constant name Common.pb inferred by Zeitwerk::Inflector from file (Zeitwerk::NameError)

This affects any other way to autoload, but native ruby, where you must set path for each const. It's worth to say that native Google's compiler makes similar thing, by adding _pb suffix, but at least the result is ready to be autoloaded.
Am I miss something?
Thanks!


Offtopic: is ruby_package option respected somehow? I can't see it really does what is it for. As mentioned in #238 if definitions are used in many langs this top namespacing is a must, to avoid noisy definitions and improve portability. For example there won't be a need in any suffixes above, if we can just put everything generated in top level module of choice.

@liveh2o
Copy link
Contributor Author

liveh2o commented Feb 21, 2024

Ah, I hadn't even considered that the suffix pattern would be a problem for Zeitwerk as well. Seems like the _pb pattern might work better.

As for the question on ruby_package, @qd3v, can you open an issue so we can discuss?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants