Skip to content

feat[ServletAdapter]: Ability remove context path when get method name #11730

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

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
protected void doPost(HttpServletRequest request, HttpServletResponse response)
throws IOException {
if (ServletAdapter.isGrpc(request)) {
servletAdapter.doPost(request, response);
servletAdapter.doPost("helloworld.Greeter/SayHello", request, response);
} else {
response.setContentType("text/html");
response.getWriter().println("<p>Hello non-gRPC client!</p>");
Expand Down
13 changes: 12 additions & 1 deletion servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/5066")
public class GrpcServlet extends HttpServlet {
private static final long serialVersionUID = 1L;
public static final String REMOVE_CONTEXT_PATH = "REMOVE_CONTEXT_PATH";

private final ServletAdapter servletAdapter;

Expand All @@ -58,6 +59,16 @@
return serverBuilder.buildServletAdapter();
}

private String getMethod(HttpServletRequest req) {
Boolean removeContextPath = Boolean.parseBoolean(getInitParameter(REMOVE_CONTEXT_PATH));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be done per-request. We should do this in init instead, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree, fix it soon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Boolean/boolean/

String method = req.getRequestURI();
if (removeContextPath) {
// remove context path used in application server
method = method.substring(req.getContextPath().length());

Check warning on line 67 in servlet/src/main/java/io/grpc/servlet/GrpcServlet.java

View check run for this annotation

Codecov / codecov/patch

servlet/src/main/java/io/grpc/servlet/GrpcServlet.java#L67

Added line #L67 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this mostly the same as req.getPathInfo()? I wouldn't expect to need to do any string manipulation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, at first I thought the same thing. only getContextPath() needed. see https://coderanch.com/t/610432/certification/getContextPath-getServletPath-getPathInfo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you are making it relative to the deployment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah)

}
return method.substring(1); // remove the leading "/"
}

@Override
protected final void doGet(HttpServletRequest request, HttpServletResponse response)
throws IOException {
Expand All @@ -67,7 +78,7 @@
@Override
protected final void doPost(HttpServletRequest request, HttpServletResponse response)
throws IOException {
servletAdapter.doPost(request, response);
servletAdapter.doPost(getMethod(request), request, response);
}

@Override
Expand Down
13 changes: 7 additions & 6 deletions servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@
* process it, and transforms the gRPC response into {@link HttpServletResponse}. An adapter can be
* instantiated by {@link ServletServerBuilder#buildServletAdapter()}.
*
* <p>In a servlet, calling {@link #doPost(HttpServletRequest, HttpServletResponse)} inside {@link
* javax.servlet.http.HttpServlet#doPost(HttpServletRequest, HttpServletResponse)} makes the servlet
* backed by the gRPC server associated with the adapter. The servlet must support Asynchronous
* Processing and must be deployed to a container that supports servlet 4.0 and enables HTTP/2.
* <p>In a servlet, calling {@link #doPost(String, HttpServletRequest, HttpServletResponse)} inside
* {@link javax.servlet.http.HttpServlet#doPost(HttpServletRequest, HttpServletResponse)} makes
* the servlet backed by the gRPC server associated with the adapter. The servlet must support
* Asynchronous Processing and must be deployed to a container that supports servlet 4.0
* and enables HTTP/2.
*
* <p>The API is experimental. The authors would like to know more about the real usecases. Users
* are welcome to provide feedback by commenting on
Expand Down Expand Up @@ -110,7 +111,8 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOExc
* <p>Do not modify {@code req} and {@code resp} before or after calling this method. However,
* calling {@code resp.setBufferSize()} before invocation is allowed.
*/
public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting this breaks compatibility. If you have to modify the helloworld example (hard-coding a string no less!), then that clearly is API-breaking.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree that hard-coded string is bad but for now i dont found better solution, i need more time or you can suggest smth

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow the difficulty.

public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
  doPost(req.getRequestURI().substring(1), req, resp);
}

public void doPost(String method, HttpServletRequest req, HttpServletResponse resp)
throws IOException {
checkArgument(req.isAsyncSupported(), "servlet does not support asynchronous operation");
checkArgument(ServletAdapter.isGrpc(req), "the request is not a gRPC request");

Expand All @@ -119,7 +121,6 @@ public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOEx

AsyncContext asyncCtx = req.startAsync(req, resp);

String method = req.getRequestURI().substring(1); // remove the leading "/"
Metadata headers = getHeaders(req);

if (logger.isLoggable(FINEST)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public void scheduledExecutorService() throws Exception {
ServletServerBuilder serverBuilder =
new ServletServerBuilder().scheduledExecutorService(scheduler);
ServletAdapter servletAdapter = serverBuilder.buildServletAdapter();
servletAdapter.doPost(request, response);
servletAdapter.doPost("hello/world", request, response);

verify(asyncContext).setTimeout(1);

Expand Down
Loading