Nordstrom / xrpc

Simple, production ready Java API server built on top of functional composition.
Apache License 2.0
17 stars 17 forks source link

Proposal to integrate with xio #129

Open pdex opened 6 years ago

pdex commented 6 years ago

What?

I'd like to integrate the Request/Response classes from xio into xrpc. Here is the full list of classes I'd like to see integrated:

FullRequest.java and StreamingRequest.java descend from Request.java; they represent the two different kinds of requests that can be modeled with the netty http objects.

A full request is one that can be serviced without any further processing (i.e. HTTP GET). A streaming request requires multiple callbacks to be fully processed (i.e. HTTP POST with a large payload).

FullResponse.java and StreamingResponse.java are the response counterparts to the two request classes.

A full response is one that can be serviced immediately (i.e. status 404), while a streaming response is one that requires multiple callbacks to be fully processed (i.e. status 200 with a large payload).

Http1ServerCodec.java and Http2ServerCodec.java are chanel handlers do the work of translating back and forth between Request/Response objects and netty http objects.

Why?

Currently the XrpcRequest object is a mixture of several concerns. I would like to replace it with a mixture of channel handlers and the Request and Response interfaces:

  1. The Http1ServerCodec and Http2ServerCodec wrap existing http 1/2 request objects with objects that conform to the Request interface
  2. The Http1ServerCodec and Http2ServerCodec translate objects that conform to the Response interface into http 1/2 response objects.
  3. The netty concepts of FullHttpRequest, HttpRequest, FullHttpResponse, and HttpResponse are a bit muddled in XrpcRequest. We should be a clear about when we expect the user to handle a full request vs a streaming request. The FullRequest and StreamingRequest interfaces will allow us to do that.
  4. XrpcRequest has methods for constructing responses, these should probably be handled by a builder such as ResponseBuilders.

How?

Initially this should be done with a custom pipeline, so that we can find any issues and address them in xio. Once we're happy with the state of this ecosystem we will need to make some breaking changes to xrpc.

For instance com.nordstrom.xrpc.server.Handler will need to take a Request object and return a Response object.

public interface Handler {
  HttpResponse handle(XrpcRequest xrpcRequest) throws IOException;
}
andyday commented 6 years ago

the only reason we provide methods on XrpcRequest to construct responses is because it contains the ByteBufAllocator. If we can use the unpooled alloc or get the allocator in another way these can be moved out. XrpcRequest is currently functioning as the Request and a request Context object all in one. It would be good to revise this...

pdex commented 6 years ago

@andyday how do we feel about passing in a ResponseBuilder that has a reference to the allocator?

andyday commented 6 years ago

on the surface I think its fine. the name might need refining and we've been trying hard to minimize the handler signature to make it as simple as possible. there are also some helper methods on XrpcRequest such as value(), query(), etc which provide enhanced access to elements of the request. we'll need to sort out where they'll go as well.

andyday commented 6 years ago

we also may need a place to attach session/cache values to the request. we could do this on the request itself or pass a separate Context object which could also have the channel allocator and response methods. i have no great ideas as of yet...

pdex commented 6 years ago

I think all of these things are up for debate on the PR

jkinkead commented 6 years ago

I like the idea of having a more structured request & response. I think this organizational model makes sense.

I think the existing xio classes aren't doing precisely the same thing as the xrpc classes; in particular, the HTTP/2 request & response classes are missing the body. I haven't delved very deeply into the other parts of the code.