Allow RequestHandler* to return subclasses of Response

[kmruiz]

With complex APIs is desirable to have more typesafe way of handling responses, because the response type can be seen as documentation. Consider this request handler:

Completes<Response> createSchema(Object requiredParam) {
   return schemaRegistry.createSchema(requiredParam).andThen(this::buildResponse);
}

It's would be impossible to know the response type before digging into the implementation (and taking a look at buildResponse). The suggested improvement would make explicit the response type:

// CreatedSchemaResponse.java
public class CreatedSchemaResponse extends Response {
   public CreatedSchemaResponse(String location) { 
      super(ResponseStatus.Ok, Headers.of("Location", location)); // convenience constructor
   }
}

// SchemaResource.java
Completes<CreatedSchemaResponse> createSchema(...) {
}

WDYT?

[VaughnVernon]

@kmruiz Sounds good. WDYT @aleixmorgadas ?

[ghost]

It looks great to me ^^ Does this improvement need a change on the ResourceHandler to allow contravariance?

[kmruiz]

@aleixmorgadas Probably we will need to change all the ResourceHandlers to force the return type to be covariant because Java generics are invariant by default.

This would also be a really good opportunity for a one-time contribution, so I will label it.

[ghost]

Definitely, let's help a new contributor join Vlingo ^^

…

On Sun, Mar 3, 2019, 18:40 Kevin Mas Ruiz ***@***.***> wrote: @aleixmorgadas <https://github.com/aleixmorgadas> Probably we will need to change all the ResourceHandlers to force the return type to be covariant because Java generics are invariant by default. This would also be a really good opportunity for a one-time contribution, so I will label it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFQzdQ1xxJzC4dJMHtD1E9EXKrLExPbPks5vTAkdgaJpZM4baFbs> .

[aantoniadis]

@kmruiz Would it be sufficient if we change RequestHandler to RequestHandler<R extends Request> and use the generic type in the abstract method like that:
abstract Completes<R> execute(...) or do you something different in mind?

edit: I believe that it won't be enough. RequestHandler#executeRequest might return the result of defaultErrorResponse() which is certainly not the subtype of Response that is used in the generic type. Maybe we should consider returning the errors using something like Either.

Treating Response and Errors the same will introduce problems if we want to use the most specific type.

[kmruiz]

@aantoniadis

You are right, probably only changing the RequestHandler wouldn't be enough if we want the error handling also typesafe (as we don't have anything like ADTs sadly right now).

I see two options, one is as you said, using something similar to Either. Right now we have Outcome<F, S>

The other option would be to throw exceptions from the controller (Outcome<F, T> already supports that, using the get() method here) and use the error handler mechanism for those exception.

What do you think @aantoniadis, @VaughnVernon , @aleixmorgadas ?

[aantoniadis]

I prefer the solution with Outcome. Trying to match the appropriate error handler to the exception thrown, might result in unexpected behaviors since you have to deal with the hierarchy of exceptions and also have a "tiebreaker" mechanism in place. Moreover, it wouldn't be easy to treat domain errors differently than internal errors (such as NPEs), since the latter can match a "generic" error handler.
Finally, the construction of the error response DTOs is a responsibility of the RequestHandler; therefore it should only leave the serialization of the DTOs for the other layers.

[ghost]

IMO, it's worth to start adding Outcome into vlingo-http at this stage.

@aantoniadis, glad you share your thoughts about how to design it ^^. I would suggest start with the simplest design to accomplish. We can do a patch for typed response type and then a new patch for typed error type as well.

If you think it's worth to do it in the same patch, up to you, just suggesting in case it helps to make smaller patches. You choose ^^

[aantoniadis]

IMO, it's worth to start adding Outcome into vlingo-http at this stage.

@aantoniadis, glad you share your thoughts about how to design it ^^. I would suggest start with the simplest design to accomplish. We can do a patch for typed response type and then a new patch for typed error type as well.

If you think it's worth to do it in the same patch, up to you, just suggesting in case it helps to make smaller patches. You choose ^^

For Outcome to work, we need to change the Failure type a bit. Currently, it only gets subtypes of Throwable so RequestHandler#defaultErrorResponse, which returns Response, cannot be used. Is there a particular reason that we want failures to be subtype of Throwable? Most implementations of Either don't have this restriction.

edit: of course we can wrap the failure Response to Throwable but this is going to make the unwrapping more complex.

[VaughnVernon]

@aleixmorgadas @kmruiz ^^^ Please comment when possible.

[ghost]

For Outcome to work, we need to change the Failure type a bit. Currently, it only gets subtypes of Throwable so RequestHandler#defaultErrorResponse, which returns Response, cannot be used. Is there a particular reason that we want failures to be subtype of Throwable? Most implementations of Either don't have this restriction.

IMO, we shouldn't think about Outcome as an Either, in that case we probably would used Either directly.

I, or someone, will explain later when I've more time, the reasons behind Outcome. Meanwhile, I would suggest map RequestHandler#defaultErrorResponse into Outcome<?, Response>. Even though it's an Error, the resolution of the Actor method it's the expected one.

I wouldn't like to stop you to do the things you think are more appropriate to design this solution, I would say, share some ways this feature could be used to clarify your ideas with others if you think it's worth to, otherwise a Pull Request with the tests + implementation is enough.

[kmruiz]

When we designed Outcome<F, S> the idea was to handle possible failures in a typesafe way (it would be like a typesafe Try[T] in Scala), and it's the main reason why F should extend Throwable.

Even if I would agree that extending Throwable might not be needed always, having domain errors with the stack trace is really useful when logging. Also doesn't necessarilly pollute the hierarchy as it would be just the base trait of the ADT.

In case that we want to change the signature of Outcome<F, S> we should define the tradeoffs and decide based on that tradeoffs (probably the benefits are not enough).

Do any of you see a way where we can start working on this task without changing Outcome<F, S>? Is the initial proposal not good enough for some initial spikes on the code? I personally agree with @aleixmorgadas that we should aim to have small patches, and more in the current situation, that a big change would affect a couple of modules.

[aantoniadis]

I opened PR #30 in order to have some code as a reference for our discussion. The code is not ready to be merged. As you can see there, I had to wrap the errors in ErrorResponse, since Outcome only gets Throwables as we have already discussed (btw I totally agree with @kmruiz that Outcome is more like Try).

@aleixmorgadas I am not sure what you mean with your suggestion. Currently, RequestHandler#defaultErrorResponse returns a Completes, although it could return just a Response. Check here how I handle it. The problem, of course, is that this type of error is a Response and is different from the subtype of the Response that each handler will define.
The ErrorReponse wrap/unwrap doesn't feel right.

On a different note, if we decide that RequestHandler#executes will return Completes<Outcome<ResponseError, T>> then the method executeAction.get() probably needs some extra handling in case the Completes fails in order to convert the Completes to Completes<Outcome<,>>. What do you think?

[VaughnVernon]

@kmruiz Please see my #26 (comment)

[kmruiz]

Solved by #26