X Tutup
The Wayback Machine - https://web.archive.org/web/20201214181531/https://github.com/github/flit/pull/5
Skip to content

/ flit

forked from devork/flit
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

Jump to bottom

Support JAX-RS #5

Merged
merged 11 commits into from Aug 25, 2020
Merged

Support JAX-RS #5

merged 11 commits into from Aug 25, 2020

Conversation

@jlisam
Copy link
Collaborator

@jlisam jlisam commented Aug 20, 2020 •
edited

This PR implements a JAX-RS flavored Twirp RPC server generation. The PR also includes a JAX-RS runtime error mapper as well as a ProtobufMessageProvider that will allow reading and writing protobufs.

The generated code can be used in both Spring MVC/Boot and Undertow servers as long as they use a JAX-RS implementation instead of relying on the framework specific controllers.

Example:

syntax = "proto3";
package com.example.helloworld;
option java_package = "com.example.helloworld";

service HelloWorld {
    rpc Hello (HelloReq) returns (HelloResp);
    rpc HelloAgain (HelloReq) returns (HelloResp);
}

message HelloReq {
    string subject = 1;
}

message HelloResp {
    string text = 1;
}

The above protobuf will generate the following JAX-RS flavored code:

import com.google.protobuf.util.JsonFormat;
import java.io.InputStreamReader;
import java.lang.Exception;
import java.nio.charset.StandardCharsets;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
import javax.ws.rs.core.Context;

@Path("/twirp/com.example.helloworld.HelloWorld")
public class RpcHelloWorldResource {
  private final RpcHelloWorld service;

  public RpcHelloWorldResource(RpcHelloWorld service) {
    this.service = service;
  }

  @POST
  @Path("/Hello")
  public void handleHello(@Context HttpServletRequest request,
      @Context HttpServletResponse response) throws Exception {
    boolean json = false;
    final Helloworld.HelloReq data;
    if (request.getContentType().equals("application/protobuf")) {
      data = Helloworld.HelloReq.parseFrom(request.getInputStream());
    } else if (request.getContentType().startsWith("application/json")) {
      json = true;
      Helloworld.HelloReq.Builder builder = Helloworld.HelloReq.newBuilder();
      JsonFormat.parser().merge(new InputStreamReader(request.getInputStream(), StandardCharsets.UTF_8), builder);
      data = builder.build();
    } else {
      response.setStatus(415);
      response.flushBuffer();
      return;
    }
    Helloworld.HelloResp retval = service.handleHello(data);
    response.setStatus(200);
    if (json) {
      response.setContentType("application/json; charset=utf-8");
      response.getOutputStream().write(JsonFormat.printer().omittingInsignificantWhitespace().print(retval).getBytes(StandardCharsets.UTF_8));
    } else {
      response.setContentType("application/protobuf");
      retval.writeTo(response.getOutputStream());
    }
    response.flushBuffer();
  }

  @POST
  @Path("/HelloAgain")
  public void handleHelloAgain(@Context HttpServletRequest request,
      @Context HttpServletResponse response) throws Exception {
    boolean json = false;
    final Helloworld.HelloReq data;
    if (request.getContentType().equals("application/protobuf")) {
      data = Helloworld.HelloReq.parseFrom(request.getInputStream());
    } else if (request.getContentType().startsWith("application/json")) {
      json = true;
      Helloworld.HelloReq.Builder builder = Helloworld.HelloReq.newBuilder();
      JsonFormat.parser().merge(new InputStreamReader(request.getInputStream(), StandardCharsets.UTF_8), builder);
      data = builder.build();
    } else {
      response.setStatus(415);
      response.flushBuffer();
      return;
    }
    Helloworld.HelloResp retval = service.handleHelloAgain(data);
    response.setStatus(200);
    if (json) {
      response.setContentType("application/json; charset=utf-8");
      response.getOutputStream().write(JsonFormat.printer().omittingInsignificantWhitespace().print(retval).getBytes(StandardCharsets.UTF_8));
    } else {
      response.setContentType("application/protobuf");
      retval.writeTo(response.getOutputStream());
    }
    response.flushBuffer();
  }

}
jlisam added 5 commits Aug 19, 2020
@jlisam
Implement JAX-RS service generation
aabeebe
@jlisam
Bump gradle versions
3cd8160
@jlisam
Implement JAX-RS flit exception mapper
bac1dfc
@jlisam
Wrap protobuf in Response object
b3fbf0b
@jlisam
Add protobuf message provider
ec321b6
@jlisam jlisam requested review from BenEddy, josedab, ShaneSaww and zerowidth Aug 20, 2020
@jlisam jlisam changed the title Support jaxrs Support JAX-RS Aug 20, 2020
@josedab
josedab approved these changes Aug 21, 2020
View changes
Copy link

@josedab josedab left a comment

Thanks @jlisam for adding me to the review 🙇
I think so far the PR looks great 💯 Great job!
I added some thoughts/nitpicks: these are not blockers, just suggestions. Interested in knowing what you think.

You might want to consider rechecking the readme, there is a spot missing the jaxrs addition:
Screen Shot 2020-08-20 at 6 03 49 PM

I tried to add a comment on the PR but I cannot comment on any line (only where lines related to the diff happen 😞 )
plugin/src/main/java/com/flit/protoc/gen/server/jaxrs/RpcGenerator.java Show resolved Hide resolved
plugin/src/main/java/com/flit/protoc/gen/server/jaxrs/RpcGenerator.java Outdated Show resolved Hide resolved
plugin/src/main/java/com/flit/protoc/gen/server/jaxrs/RpcGenerator.java Outdated Show resolved Hide resolved
.../jaxrs/src/main/java/com/flit/runtime/jaxrs/ProtobufMessageProvider.java Outdated Show resolved Hide resolved
.../jaxrs/src/main/java/com/flit/runtime/jaxrs/ProtobufMessageProvider.java Outdated Show resolved Hide resolved
.../jaxrs/src/test/java/com/flit/runtime/jaxrs/FlitExceptionMapperTest.java Outdated Show resolved Hide resolved
jlisam added 5 commits Aug 21, 2020
@jlisam
Update Readme with jaxrs option
ecaafc1
@jlisam
Use consistent protobuf media type
da63de4
@jlisam
Refactor media type to use guava's MediaType constants
43f8b78
@jlisam
Extract to constants
3e65883
@jlisam
Merge branch 'master' into support-jaxrs
Loading status checks…
8381062
@ShaneSaww
ShaneSaww approved these changes Aug 21, 2020
View changes
Copy link

@ShaneSaww ShaneSaww left a comment

This looks pretty legit to me, nice work.

Let's get this merged and start getting some proto in fjord
@jlisam
Simplify jax-rs resource (removes the protobuf message provider)
Loading status checks…
13211d6
@ShaneSaww
ShaneSaww reviewed Aug 25, 2020
View changes
runtime/jaxrs/src/main/java/com/flit/runtime/jaxrs/FlitExceptionMapper.java
import org.slf4j.LoggerFactory;

@Provider
public class FlitExceptionMapper implements ExceptionMapper<FlitException> {

This comment has been minimized.

Sign in to view
@ShaneSaww

ShaneSaww Aug 25, 2020

Do we still need these ExceptionMappers?
I know that we're going to need some ExceptionMapper fir Dropwizard, but does it still make sense to have them in Flit or have the app build them as needed?

I can see the InvalidProtobufExceptionMapper being useful, but Idk about the FlitExceptionMapper as the generate code doesn't throw any FlitExceptions.

Also does it make sense to put Dropwizard only code in flit now? I believe that the theExceptionMapper currently only work with Dropwizard as I don't see any @ExceptionHandler(... annotations for springboot.

This comment has been minimized.

Sign in to view
@jlisam

jlisam Aug 25, 2020
Author Collaborator

@ShaneSaww I think we still need them. We could definitely move them over to fjord but the existing pattern is to declare the exception mappers for each server type (spring mvc, undertow and in this case jaxrs).

I can see the InvalidProtobufExceptionMapper being useful, but Idk about the FlitExceptionMapper as the generate code doesn't throw any FlitExceptions.

So the usefulness of these ExceptionMappers is to convert the FlitExceptions to the appropriate response. The application, in this case Fjord, would have to throw FlitException wrapping existing exceptions for this mapper to catch and convert to the response. In aqueduct, we actually wrap the exception to a FlitException here. The only difference is that Spring's exception handler covers both the mappers (Exception and InvalidProtocolException) in one class.

Also does it make sense to put Dropwizard only code in flit now? I believe that the theExceptionMapper currently only work with Dropwizard as I don't see any @ExceptionHandler(... annotations for springboot.

So if you look at the project outline, there are 3 packages under runtime:

  • jaxrs
  • spring
  • undertow

ExceptionMapper is a JAX-RS concept and will work in any of the frameworks (dropwizard, spring, undertow) as long as you are using the JAX-RS library instead of like Spring Rest/MVC. The equivalent of the ExceptionMapper for spring is FlitExceptionHandler, which we currently use.

This comment has been minimized.

Sign in to view
@ShaneSaww

ShaneSaww Aug 25, 2020

The application, in this case Fjord, would have to throw FlitException wrapping existing exceptions for this mapper to catch and convert to the response. In aqueduct, we actually wrap the exception to a FlitException here.

ohh, interesting. I fully didn't get that before. Just thinking about it, it makes some sense, but it's also a bit weird to me. It seems like a lot of work to get the correct response code(and error message) back to the client. But nothing we need to change right now.

So if you look at the project outline, there are 3 packages under runtime:

👍 I completely missed the spring runtime one.

ExceptionMapper is a JAX-RS concept and will work in any of the frameworks (dropwizard, spring, undertow) as long as you are using the JAX-RS library instead of like Spring Rest/MVC. The equivalent of the ExceptionMapper for spring is FlitExceptionHandler, which we currently use.

👍 Thanks for the explanation
@jlisam jlisam merged commit 99a203f into master Aug 25, 2020
1 check passed
1 check passed
build
Details
@jlisam jlisam deleted the support-jaxrs branch Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneSaww ShaneSaww

@josedab josedab

@zerowidth zerowidth

@BenEddy BenEddy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@jlisam @josedab @ShaneSaww
You can’t perform that action at this time.
X Tutup