feat(GrpcServlet): ability override method name

[long76]

Refs: #5066

Complex application may have many war-s and do not allow monopolize root context path(/). Add ability to override method name just override getMethod from GrpcServlet

requestURI = contextPath + servletPath + pathInfo

Usage example, for remove context path:

@WebServlet(urlPatterns = "/*", asyncSupported = true)
public class GrpcWebServlet extends GrpcServlet {
...
    @Override
    protected String getMethod(HttpServletRequest req) {
        return req.getRequestURI().substring(req.getContextPath().length() + 1);
    }
}

Links:
https://stackoverflow.com/a/42706412
https://coderanch.com/t/610432/certification/getContextPath-getServletPath-getPathInfo

[long76]

don't understand why crash tests) error in test env?

[long76]

don't understand why crash tests) error in test env?

found error - getInitParameter throw exception on tests

[kannanjgithub]

Left a minor comment.

Need a few more changes.

[kannanjgithub]

Some more changes.

[long76]

@ejona86 ping

[ejona86]

@panchenko, do you have any opinions for this PR?

[panchenko]

@ejona86 The use case feels legit when trying to use gRPC in big legacy web applications.
Only the REMOVE_CONTEXT_PATH parameter is controversial.

Another way to achieve that would be making methodNameResolver configurable in ServletAdapter, e.g. in the following way

diff --git a/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java b/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
index f68ed0835..e2adba4c5 100644
--- a/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
+++ b/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
@@ -39,7 +39,10 @@ public class GrpcServlet extends HttpServlet {
 
   private final ServletAdapter servletAdapter;
 
-  GrpcServlet(ServletAdapter servletAdapter) {
+  /**
+   * Constructor for use by subclasses.
+   */
+  protected GrpcServlet(ServletAdapter servletAdapter) {
     this.servletAdapter = servletAdapter;
   }
 
diff --git a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
index 4bfe89497..cf5eb83ca 100644
--- a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
+++ b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
@@ -45,6 +45,7 @@ import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
 import java.util.logging.Logger;
 import javax.servlet.AsyncContext;
 import javax.servlet.AsyncEvent;
@@ -75,15 +76,18 @@ public final class ServletAdapter {
 
   private final ServerTransportListener transportListener;
   private final List<? extends ServerStreamTracer.Factory> streamTracerFactories;
+  private final Function<HttpServletRequest, String> methodNameResolver;
   private final int maxInboundMessageSize;
   private final Attributes attributes;
 
   ServletAdapter(
       ServerTransportListener transportListener,
       List<? extends ServerStreamTracer.Factory> streamTracerFactories,
+      Function<HttpServletRequest, String> methodNameResolver,
       int maxInboundMessageSize) {
     this.transportListener = transportListener;
     this.streamTracerFactories = streamTracerFactories;
+    this.methodNameResolver = methodNameResolver;
     this.maxInboundMessageSize = maxInboundMessageSize;
     attributes = transportListener.transportReady(Attributes.EMPTY);
   }
@@ -119,7 +123,7 @@ public final class ServletAdapter {
 
     AsyncContext asyncCtx = req.startAsync(req, resp);
 
-    String method = req.getRequestURI().substring(1); // remove the leading "/"
+    String method = methodNameResolver.apply(req);
     Metadata headers = getHeaders(req);
 
     if (logger.isLoggable(FINEST)) {
diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
index 72c4383d2..5c9af0bcf 100644
--- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
+++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
@@ -49,8 +49,10 @@ import java.net.SocketAddress;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Function;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
+import javax.servlet.http.HttpServletRequest;
 
 /**
  * Builder to build a gRPC server that can run as a servlet. This is for advanced custom settings.
@@ -64,6 +66,8 @@ import javax.annotation.concurrent.NotThreadSafe;
 @NotThreadSafe
 public final class ServletServerBuilder extends ForwardingServerBuilder<ServletServerBuilder> {
   List<? extends ServerStreamTracer.Factory> streamTracerFactories;
+  private Function<HttpServletRequest, String> methodNameResolver =
+          req -> req.getRequestURI().substring(1); // remove the leading "/"
   int maxInboundMessageSize = DEFAULT_MAX_MESSAGE_SIZE;
 
   private final ServerImplBuilder serverImplBuilder;
@@ -98,7 +102,7 @@ public final class ServletServerBuilder extends ForwardingServerBuilder<ServletS
    * Creates a {@link ServletAdapter}.
    */
   public ServletAdapter buildServletAdapter() {
-    return new ServletAdapter(buildAndStart(), streamTracerFactories, maxInboundMessageSize);
+    return new ServletAdapter(buildAndStart(), streamTracerFactories, methodNameResolver, maxInboundMessageSize);
   }
 
   /**
@@ -176,6 +180,15 @@ public final class ServletServerBuilder extends ForwardingServerBuilder<ServletS
     throw new UnsupportedOperationException("TLS should be configured by the servlet container");
   }
 
+  /**
+   * Specifies how to determine gRPC method name from servlet request.
+   * The default strategy is using {@link HttpServletRequest#getRequestURI()} without the leading slash.
+   */
+  public ServletServerBuilder methodNameResolver(Function<HttpServletRequest, String> methodResolver) {
+    this.methodNameResolver = checkNotNull(methodResolver);
+    return this;
+  }
+
   @Override
   public ServletServerBuilder maxInboundMessageSize(int bytes) {
     checkArgument(bytes >= 0, "bytes must be >= 0");

And then in application code

public class MyServlet extends GrpcServlet {
    public MyServlet() {
        super(new ServletServerBuilder()
                .addService(new MyService())
                .methodNameResolver(req -> req.getRequestURI().substring(req.getContextPath().length() + 1))
                .buildServletAdapter());

@long76 would that work for you?

[ejona86]

@panchenko, yeah, I agree the use case is legitimate. But I'm also wary without reminding myself the difference between all the different servlet path-related strings (context, servlet name, path info, path translated, etc). I mainly just remember there were many ways of doing this sort of thing wrong, and it had seemed the "proper" way was rarely known online (at the time, decade ago).

[ejona86]

FWIW, I do think I'd prefer simple configuration for common use cases. This seems common enough that it doesn't seem like it should deserve an lambda to configure. As an old-school servlet user, this seems more like something that would have been nicest to configure in web.xml.

[long76]

@panchenko i think about this way(more universal), need to check. I decide remove only context path to expect user mistake and match grpc default functional - how i know non servlet usage ways don't allow user override method name out-of-the-box. Your solution may make servlet more prefer as more flexible tool but servlet still experimental.

[panchenko]

The lambda approach is more flexible — for example, if someone wants to strip a different prefix rather than contextPath.

Using a servlet parameter doesn’t really fit here (whether in web.xml or annotations), since GrpcServlet can’t be fully set up that way. The actual service implementation has to come from user code, so we should just make it fully configurable there.

For servlet-based apps, the cleanest way lifecycle-wise would be:

• implement ServletContainerInitializer
• create servlets with something like: GrpcServlet servlet = new ServletServerBuilder().addService(myServiceImpl).buildServlet()
• and register them programmatically

[long76]

@panchenko sorry for long answer, your solution works, feel free to create MR. i provide some other solution here

[panchenko]

@long76 thanks for confirming, and just to reiterate, have you considered approaches which don't require changes in the library code?

1. servlet context path = gRPC package name + gRPC service name
2. separate virtual host in your servlet container for gRPC service code
3. running gRPC service in another process

[long76]

@long76 thanks for confirming, and just to reiterate, have you considered approaches which don't require changes in the library code?

1. servlet context path = gRPC package name + gRPC service name
2. separate virtual host in your servlet container for gRPC service code
3. running gRPC service in another process

1. in urlPatterns? yes, but in req still full path
2. no, we want save current configuration
3. yes, but this does't suit us (need all in one)

[long76]

so, feel free to close this MR because you will provide yours or approve and merge this)

[panchenko]

@long76 In the first approach I mean do not change request path in nginx, but rename the context to match your gRPC service.

For example, for package my.example and service MyService the context name would be my.example.MyService
In such a setup it should work without changes in the library

[long76]

@long76 In the first approach I mean do not change request path in nginx, but rename the context to match your gRPC service.

For example, for package my.example and service MyService the context name would be my.example.MyService In such a setup it should work without changes in the library

i see, it's should work, but we have many services and add for all uncomfortable)