[Proposal] Change API to encourage using HttpResponse instead of HttpResponseWriter

[anuraaga]

Currently we have a convenience abstract class, AbstractHttpService which takes care of HTTP request method dispatch to user service methods. The user methods have a signature like

protected void doGet(ServiceRequestContext ctx, HttpRequest req, HttpResponseWriter res);

where res is always an instance of DefaultHttpResponse.

I propose we consider changing these to not pass in a HttpResponse implementation and leave it to the user, i.e.,

protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req);

Reasons for this change are

Having HttpResponseWriter passed in isn't actually all that convenient.

Consider a common pattern

if (!checkContentType()) {
  res.respond(BAD_CONTENT_TYPE);
  return;
}
if (!checkArgs()) {
  res.respond(BAD_REQUEST);
  return;
}
res.respond(aggregatedHttpMessage);

is actually more lines than

if (!checkContentType()) {
  return HttpResponse.of(BAD_CONTENT_TYPE);
}
if (!checkArgs()) {
  return HttpResponse.of(BAD_REQUEST);
}
return HttpResponse.of(aggregatedHttpMessage);

and if the user actually does need a streaming response, it's just two lines to construct a streaming-type response and return it. I think in practice, passing in the writer rarely wins and often loses in terms of code complexity.

Provides more flexibility

As mentioned on #846, it is good to be able to select response implementation based on what the response actually is, often for performance reasons. For example, in the above examples all HttpResponse.of could be implemented using a non-streaming, optimized version of HttpResponse, without the user having to know the details. This allows us to experiment with implementations in the background without being tied to user code. All we needed was an HttpResponse, but restricting this interface to HttpResponseWriter cuts down on options, and as described above doesn't seem to have much benefit.

Encourages good practices

One option would be to not make this change and expect users that want to be able to select HttpResponse implementation to implement HttpService. But as method dispatch logic in AbstractHttpService is quite useful in most cases, many users will probably continue to use it and miss out on any work we do on HttpResponse. In particular, I suspect that most user code will be optimized if they switch from HttpResponseWriter to DeferredHttpResponse + FixedHttpResponse.

Removes some duplication

Currently, we have this comment, and annoying duplication of APIs among many objects

// Note: Ensure we provide the same set of `respond()` methods with the `of()` methods of
//       HttpResponse and AggregatedHttpMessage for consistency.

If we promote the pattern that HttpResponseWriter is only really for streaming, not for full responses, then we can remove all the respond methods from it, improving the situation a little.

Possibly less cognitive load

This could just be me, but I have always find myself get tripped up a little by the method signature inconsistency between AbstractHttpService and DecoratingService, both of which I commonly use. returning responses also feels more natural than writing a "pre-closed" response.

---

We could make HttpResponse the general entry-point to creating responses by a pattern like this.

interface HttpResponse {

  static HttpResponse of(status, content) {
    return FixedHttpResponse.of(status, content);
  }

  static DeferredHttpResponse deferred() {
    return new DeferredHttpResponse();
  }

  static HttpResponseWriter streaming() {
    return new DefaultHttpResponse();
  }
}

---

Of course, this change is a pretty breaking change - the compatibility issue may by itself be a blocker. However, it might not be so bad.

• I could do a global refactoring of a codebase trivially in IntelliJ using this replacement template

• Otherwise, we could provide a separate base class with the alternate signatures, and optionally migrate to it being the standard over a few versions of armeria.

[minwoox]

Such a great idea. I totally agree with you especially for the part that removes the duplication and has same signature with the Service.

Just a little bit worried about complaint from our customers though. :-)

[trustin]

+1 to the proposal as well. Less duplication without losing brevity is awesome. What do you think @imasahiro @kojilin and other users?

[imasahiro]

Sounds good to me~

[anuraaga]

Thanks for the comments, glad the new API seems reasonable. Want to confirm - are people ok with a full break of the API, or is a transition with something like AbstractHttpService2 a better idea? If we were 1.0, the latter would be required, and is a fairly common pattern in Java libraries, but as we're not, I would want to have buy-in from the users if we skipped a transition period.

[anuraaga]

Implemented the API change, somehow without any hard breaks :)