Filters concept improvement proposal in regard to error handling

[Dominik Dorn]

Hi!

The filter concept in Play (2.4 / 2.5) needs to be improved IMHO. 

Filters work good, if every thing works, we don't have exceptions and are able to follow "the happy path". 

Every filter gets a RequestHeader and ultimately returns a Future[Result]

A problem shows itself as soon as an error arises somewhere in the chain or in the final action. Right now, we can handle the error by recovering on the result of nextFilter(). The problem is, that we lose any changes that happened to the RequestHeader down the chain. 

I suggest we adjust the Filter Api (or create a new one with adapters for the existing classes), where nextFilter() does not simply return a Future[Result], but more like a Either[ProcessingError, Future[Result]] (or a more appropriate data structure than either), where ProcessingError might be something like

case class ProcessingError(e: Throwable, rh: RequestHeader, errorArgs: Map[String, Any]) 

Why we/I need this? 

I tried to integrate a "CorrelationIdFilter", a filter that creates a uniqueId for every request, adjusts the requestHeader so I have it available in all stages further down the chain (and in my action) and adding it as header to the result when receiving the response from nextFilter(..). 
This works quite well if everything works like it should. The problem arises if an error happens, because now to make sure my correlationId is added to the response returned to the browser I have to handle the failure by doing a recover, where I have to/(should?) delegate to the ErrorHandler. 

When calling the ErrorHandler from my filter, I only have the RequestHeader that was available by the time it entered my filter - so all the changes made by other filters down the chain are lost. 

If I have more than one filter that wants to add something to the output (even when errors happened), I have a real problem. 

Ultimately I want to be able to handle an Error in my filter, but still be able to pass the (modified) error up the chain, until it reaches the end of the chain where the default (or specified) ErrorHandler should handle the case. 

The only alternative right now to calling the ErrorHandler myself from my filter is to encapsulate the Exception with something like a CorrelationIdExceptionWrapper and unwrapping it again in the ErrorHandler. 

Perfect would be, if we could further adjust the ProcessingError class to something like 

case class ProcessingError(e: Throwable, rh: RequestHeader, errorArgs: Map[String, Any], recordingResult: Result)
where the recordingResult "records" all the changes made to it and allow them to be applied to the (or any other) Result that the ErrorHandler creates. 

Something slightly similar like my proposal already exists in the form of HttpRequestHandler, where we have this:

def handlerForRequest(request: RequestHeader): (RequestHeader, Handler)
The Problem is, that we don't have a chaining functionality of this (or no way to configure that - at least I could not find it).

What do you think? 

Thanks,
Dominik 

--

Dominik Dorn
http://dominikdorn.com | http://twitter.com/domdorn
XING: https://www.xing.com/profile/Dominik_Dorn
LINKEDIN: http://at.linkedin.com/pub/dominik-dorn/66/b42/bb1/

[Rich Dougherty]

Hi Dominik

Another idea would be to change the EssentialFilter code so it wraps any exceptions it throws with a new type of exception that contains the RequestHeader.

E.g. FilterException(f: Filter, rh: RequestHeader, cause: Throwable)

This would allow you to programmatically get the RequestHeader information up the entire filter chain.

We could optionally change Play's dev mode error display code to unpack these exceptions to display info about both the original and modified RequestHeaders.

Cheers

Rich

--

Rich Dougherty

Engineer, Lightbend, Inc

@richdougherty

[James Roper]

Hi Dominik,

I don't think what you're proposing actually solves the problem.  What if a filter throws an exception?  Then a new ProcessingError is created with the request header from the filter before it, losing all the changes from the filters after it, and all the args results etc.  Maybe it may make things nicer for your particular use case, but as a general purpose solution, it hasn't actually made any progress on the problem, and certainly in my view doesn't justify the additional complexity it adds to the types that filters need to deal with.

If a filter needs to impact the error handling, then the filter has to do the error handling.  So, that means any filter that wants to do this should catch errors and invoke the errorHandler, passing in its modified request header, and maybe modifying the returned result.  Filters up the chain will then continue to contribute to the materialised error result as normal.  If that doesn't suit you, then I would suggest capturing the information in an exception - after all, the result of a future is not actually T, it's Try[T], which is essentially Either[Throwable, T].  If your filters can ensure that the Throwable is of type ProcessingError, then you have your solution already without any changes to the API.

Regards,

James

--
You received this message because you are subscribed to the Google Groups "Play framework dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framework-...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

James Roper
Software Engineer

Lightbend – Build reactive apps!
Twitter: @jroper