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

Feature request: use idiomatic gRPC endpoint URLs #599

Closed
sander opened this issue Apr 15, 2019 · 10 comments
Closed

Feature request: use idiomatic gRPC endpoint URLs #599

sander opened this issue Apr 15, 2019 · 10 comments
Assignees

Comments

@sander
Copy link

sander commented Apr 15, 2019

Currently, when defining:

package com.example.ns {
  @outputPackage("com.example.grpcns")
  object protocol {
    // ...
    @service(Protobuf) {
    trait MyService[F[_]] {
      def myMethod(request: MyRequest): F[MyResponse]
    }
  }
}

the idlGen command produces idiomatic gRPC code:

// ...
package com.example.grpcns;
// ...
service MyService {
  rpc MyMethod (MyRequest) returns (MyResponse);
}

When using protoc to generate client code for this, the client calls com.example.grpcns.MyService/MyMethod as expected:

  • with namespace
  • with UpperCamelCase method name

But the Mu server will only accept MyService/myMethod calls:

  • without a namespace
  • with lowerCamelCase method name

This seems to be non-idiomatic gRPC usage and breaks compatibility with IDL-generated clients.

Suggestions for the served gRPC endpoint URLs:

  1. Use the @outputPackage annotation to define the namespace.
  2. Capitalize the first letter of the method name.

Example project:

@sander sander changed the title Use idiomatic gRPC endpoint URLs Feature request: use idiomatic gRPC endpoint URLs Apr 15, 2019
@Chicker
Copy link
Contributor

Chicker commented Apr 19, 2019

I faced the same problem during implementing grpc health checks (see https://github.com/grpc/grpc/blob/master/doc/health-checking.md) for our service deployed in Kubernetes. According to the doc above we need to specify concrete namespace grpc.health.v1 of our implementation of the health check service. But, I don’t know how to do it. I tried to do something like this:

package grpc.health.v1

import higherkindness.mu.rpc.protocol._

@service(Protobuf)
trait Health[F[_]] {
  def Check(request: ReqHealthCheck): F[RespHealthCheck]
}

But it didn’t help me. I get the same error: grpc-message: Method not found: grpc.health.v1.Health/Check

@sander Did you solve the problem of specifying namespace somehow?

@sander
Copy link
Author

sander commented Apr 20, 2019

@Chicker not yet. @fedefernandez said in Gitter chat that he’d try to prioritize this issue.

Haven’t tried, but maybe this works as a workaround?

trait `grpc.health.v1.Health`[F[_]] {

@fedefernandez fedefernandez self-assigned this Apr 22, 2019
@Chicker
Copy link
Contributor

Chicker commented Apr 22, 2019

Haven’t tried, but maybe this works as a workaround?

trait `grpc.health.v1.Health`[F[_]] {

@sander Thanks for the good suggestion! But it doesn't work, since the dot . is replaced to the interpolation of the u+002e Unicode character. As a result, we get something like this grpc$u002Ehealth$u002Ev1$u002EHealth as our serviceName.

@fedefernandez
Copy link
Contributor

Thanks for the submission @sander. I'm currently working in this, I'll test it with your sample project.

@fedefernandez
Copy link
Contributor

fedefernandez commented Apr 23, 2019

@sander we have added a couple of parameters to the @service annotation for controlling the service prefix and method name case. I've created a PR in your demo project to show how to configure it.

We're planning to add a new setting key to the idl plugin for controlling that when converting from avdl or proto to scala and update the docs. Once these two tasks are done, we'll release a new version.

@sander
Copy link
Author

sander commented Apr 23, 2019

Thanks @fedefernandez! What will the new setting key be for exactly?

@fedefernandez
Copy link
Contributor

I'm planning to add a flag that when active, it'll take the namespace defined in the avdl file and put it in the annotation namespace. It'll also set the method name style to capitalize. Still need a name though, ideas are welcomed 😉

@sander
Copy link
Author

sander commented May 15, 2019

@fedefernandez Is this still blocked on higherkindness/skeuomorph#85 ? Is there a (snapshot) version that can be used already?

@fedefernandez
Copy link
Contributor

@sander sadly is still blocked. Couldn't find any spare time for addressing that.

@fedefernandez
Copy link
Contributor

@sander thanks again for your feedback. This issue has been accomplished in #622 and released with the 0.18.4 version (it'll reach maven central soon)

Now you can set the idlGenIdiomaticEndpoints setting key to true for generating scala service definitions that will append the namespace to the operations and their names will be capitalized.

L-Lavigne added a commit to higherkindness/skeuomorph that referenced this issue Oct 2, 2020
This PR addresses [sbt-mu-srcgen #93](higherkindness/sbt-mu-srcgen#93) along with an upcoming code-change PR for `sbt-mu-srcgen` and a documentation update PR for `mu-scala`.

It does so by splitting the `useIdiomaticEndpoints` source-generation flag, created in [mu-scala #599](higherkindness/mu-scala#599) to control service method capitalization and package prefixes, into 2 distinct flags with different defaults:

1) `useIdiomaticGrpc` which controls the namespace prefix and defaults to `true`;
2) `useIdiomaticScala` which controls the endpoint capitalization and defaults to `false`.

More formally, if `useIdiomaticScala` is true and if _all_ the RPC call definitions in an input IDL are capitalized, the Scala methods in the services generated from this IDL will be _de_-capitalized, while the gRPC endpoints derived from those will be re-capitalized to match the original IDL. For example, a Protobuf IDL `rpc GetBook` definition becomes a Scala `def getBook` method inside a `methodNameStyle = Capitalized` service, which then generates a `GetBook` gRPC endpoint. A `getBook` IDL definition (for example from Avro) would remain unchanged (`methodNameStyle = Unchanged`, `def getBook`, and `getBook` gRPC endpoint).

The reason for the limitation that all RPC calls must be capitalized for the option to apply, is that we do not annotate individual methods, only the top-level `@service` annotation. Thus if we were to de-capitalize a mixed-capitalization group of calls and apply `methodNameStyle = Capitalized` to the service, some of the originally-lowercased methods would be incorrectly re-capitalized which is part of the issue we're addressing here (for example, `getBook` and `GetBook` would both become `def getBook` Scala and `GetBook` endpoints).

Note that the PR also moves some test resources into the `resource` root, and normalizes them a bit between the Avro and Proto cases.

I'm happy to provide some more background on the "idiomatic" flag's history as I understand it, in case it helps in reviewing this change. Feel free to ask about this or anything else unclear.
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