Code generation should only append "Service" when necessary.

[darronschall]

Protobuf services should end in Service. This is a buf lint rule, recommended via the protobuf style guide as linked to via Twirp Best Practices)

Therefore, we only append "Service" to fixup services that are not well named. This avoids generating e.g. "MessagesServiceService".

This change updates the generator library itself, and will require a new release. Since the release changes the name of the generated services in some cases, it should be flagged as a breaking change.

Testing this was a bit of a challenge for me as I've never written go before. Here are the steps I took:

First, build and install the updated generator code.

# Working directory is my forked repository, with my service-suffix-handling branch checked out
cd ./protoc-gen-twirp_ruby 
go build   
go install

Then, I updated the example service definition in commit 074692d to follow Potobuf style guides.

Finally, I ran my built code-gen:

cd ../example    
protoc --proto_path=. ./hello_world/service.proto --ruby_out=. --twirp_ruby_out=.

For consistency, I manually updated the Ruby specs in commit e20b974 such that the fake services follow the "service ends in Service" naming convention.

[wmatveyenko]

Hi, thanks for your contribution. We are in favour of updating the tests and examples to have better naming conventions, but we don't want to update how the code is generated. The buf lint rule applies to the protobuf definition (see buf lint overview), not to the generated code. Feel free to modify this PR or create a new one with only the test and example changes.

[darronschall]

@wmatveyenko Thanks for the response!

Unfortunately, for a well-named service, the existing code generates an erroneous extra "Service" in the class name for the ruby output.

For example, this definition:

service HelloWorldService {
     rpc Hello(HelloRequest) returns (HelloResponse);
 }

Currently generates this:

class HelloWorldServiceService < Twirp::Service
  package 'example'
  service 'HelloWorldService'
  rpc :Hello, HelloRequest, HelloResponse, :ruby_method => :hello
end

The only way to change this is to change the code generator itself, which I did in https://github.com/twitchtv/twirp-ruby/pull/91/files#diff-ba514abe40e6d5f1452e22798a8bb10949cf2937727d8f6efb34f46e1de030f9R112-R115

[wmatveyenko]

Thanks for the clarification. We will look at your request more closely. One quick comment is that the generated client name should probably not change, as it is easier to update the service code than it is the client code.

[darronschall]

Thanks for the feedback @wmatveyenko! I've pushed another small commit in ~~0361f28~~ 01ce2e8 such that the generated client names do not have Service in them.

[darronschall]

I looked at the failing test runs, but I can't reproduce. It doesn't seem to be related to this PR:

NameError: uninitialized constant Rack::Request
Did you mean?  Rack::MockRequest

Running locally everything passes:

bundle exec rake test
Run options: --seed 19879

# Running:

.........................................................................................

Finished in 0.009981s, 8916.9421 runs/s, 44985.4720 assertions/s.

89 runs, 449 assertions, 0 failures, 0 errors, 0 skips

[danielmorrison]

@darronschall Rack 3.0.0 came out and that's likely what's causing the test errors.

[danielmorrison]

Adding require 'rack' (or just require 'rack/request') to the top of lib/twirp/service.rb fixes the test errors in Rack 3.

Outside the scope of this PR, but rack is listed is a development dependency, but it is really a runtime one.

[darronschall]

Thanks for the tip @danielmorrison. I added e589253 to green up the build.

[darronschall]

I saw rack was added/merged via #94 to fix the build errors. I'll rebase this and remove my rack fixup commit since it's not necessary anymore.

[darronschall]

@wmatveyenko Just curious if there's anything else I need to do here for this PR, or if you're still deciding internally if you want to include this change?

[swiftyspiffy]

@darronschall Hi Darron. Twitch is out for holiday this week. More to the point though, this package is in the process of transferring ownership to Github. Twitch no longer controls it, and I believe Arthur (the current owner) is working to move it under Github.

[arthurnn]

👋 hi everyone:

I am in favor of this change. 1. It updates the tests to follow convention and 2. it help folks using the generator to also push them towards better convention

One question though:

This change updates the generator library itself, and will require a new release.

yes I agree here

Since the release changes the name of the generated services in some cases, it should be flagged as a breaking change.

I agree we might want a MINOR bump on the version. Not sure I see we would need a MAJOR one, nor this would break current users of the library, as this only alteres the generator. Am I missing something?

thanks

[darronschall]

Thanks, @arthurnn. I'm easily convinced that the breaking changes are more minor than major... I mainly just wanted to point out that this PR does include a breaking change worth documenting.

The breaking change only happens when the .proto file is still under development and code-gen needs to re-run for a project already using the library. Post code-gen, library users need to do a find/replace in their projects to ensure they're using the generated service class names correctly (including class references and string names in routes).

[arthurnn]

thanks for your contribution

[arthurnn]

I am trying this in a few projects and I am realizing there is no easy-migration path to it. Note: my project don't have the Service suffix.

• After using the latest version, and doing a change into my .proto file, the code generation will now append the Service to all my Twirp::Service classes.
• If I deploy those to my clients, they will fail to talk to the server, as my server don't have the Service suffix yet.
• If I deploy the server, adding the Server suffix first, it will break the clients.

---

I guess I misunderstood the core change here

Therefore, we only append "Service" to fixup services that are not well named. This avoids generating e.g. "MessagesServiceService".

I thought this was only to avoid the ServiceService bit, I missed the part that this appends Service if not yet there.

Anyways, this is a Major breaking change. I don't think we should go with it. As much as it is nice to please the linter, we should not try to be too clever and break the path to migration to newer gem versions.

Please correct me if I am wrong, if not I will revert this change. Thanks

[darronschall]

@arthurnn The generator always appends Service to the service class names extending Twirp::Service, which is why services named like MessagesService would generate class MessagesServiceService < Twirp::Service.

The major breaking change this PR introduces is a result of fixing up the service name itself, which results in a different service name being set on the service DSL. You can see this reflected as a result of running code-gen at https://github.com/github/twirp-ruby/pull/91/files#diff-f0266f794109e757f0331ea6b8d819dd0048cc725c64b6f9c31e391134716985R9

To say it differently, previously the generator always appends Service to the class name, regardless of what the service name was. Sticking with service MessagesService in the proto file, that would generate similar to:

class MessagesServiceService < Twirp::Service
  service 'MessagesService'
end

Whereas, after this change, it generates:

class MessagesService < Twirp::Service
  service 'MessagesService'
end

However, for poorly named services, such as service HelloWorld, the generator would generate:

class HelloWorldService < Twirp::Service
  service 'HelloWorld'
end

And after this change the generator generates:

class HelloWorldService < Twirp::Service
  service 'HelloWorldService' # Here is where the major breaking change happens
end

I still think this change is important (my services are named well, and ServiceService is clearly incorrect).

What do you think the best path forward is? I can take another pass at this PR to try to preserve backwards compatibility by not changing the string passed to the service DSL. Would that work for you?

[arthurnn]

service 'HelloWorldService' # Here is where the major breaking change happens

That is the problem. As everybody using this library is probably stuck at the following case, it is almost impossible for them to migrate:

class HelloWorldService < Twirp::Service
  service 'HelloWorld'
end

(my services are named well, and ServiceService is clearly incorrect).

I agree here.

What do you think the best path forward is? I can take another pass at this PR to try to preserve backwards compatibility by not changing the string passed to the service DSL. Would that work for you?

Thanks a lot for bearing with me here 🙏 .

I think the path forward would be to not double add the Service to the class, ONLY, and allow old services to still define it as service 'HelloWorld'. WDYT, I think that would preserve backwards compatibility as well as incentivizing folks into the better-named-service path.

We could also add a extra piece of documentation, saying the linter wants services named with the Service suffix.

[darronschall]

That works for me; I'll submit a new PR that doesn't cut quite as deep.

Thanks for the back-and-forth!