Improvement request related to exception handling

[dilbertside]

Hi,

I am facing a problem related to an error thrown like org.springframework.dao.DataIntegrityViolationException when trying to deleting a DB entity.

This error is expected and it is not a big issue or a bug. Why?

Because the Data Model to map the business model, has some constraint to avoid data loss by negligence.
So the client has to go through some steps so the entity could be deleted properly. Other user constraints make it difficult to do it through a proper workflow.
For legal reasons, every single deletion is audit trailed and error occurring have to be stored.

So come to my problem, when EDS catch the error in RouterController.handleException, it by-passes the web app to give a direct answer to the front-end.

EDS gives the opportunity to add some customization through Configuration.setExceptionToMessage but it is too generic and the net is too wide.
I can add DataIntegrityViolationException in the map but it will not help in a case of a business rule break and/or a real bug. It is too coarse.

And to make things bad, we cannot take quick action as the webapp is not aware what went wrong (except through the eventual log).
We would like to track the error and not send a straight notification to the user but a more elaborate answer.

My question is would it be possible to handover exception handling to application logic and not the library logic?

I see at least 2 possible solutions:

remove the try catch in RouterController.router so the error handling centralization can be made as Spring propose with its @ControllerAdvice and ResponseEntityExceptionHandler? cf http://static.springsource.org/spring/docs/3.2.x/spring-framework-reference/htmlsingle/#new-in-3.2-webmvc-exception-handler-support

or inject an exception handler in the RouterController so to delegate the exception handling to another bean if any are configured?

Regards,

[ralscha]

Hi

The ControllerAdvice/ResponseEntityExceptionHandler looks like a very
interesting solution. The only problem I see with this is that you need
access to the Ext.Direct request because in the Ext.Direct response the
name, action and tid has to match the ones from the request.

The second solution looks like easier to implement. We could create an
interface

public interface EdsExceptionHandler {
handleException(BaseResponse response, Exception e) ;
}

and create a default implementation that contains the current exception
handling code.
Then we inject this bean into the RouterController or the Configuration
class.

On Thu, Aug 15, 2013 at 8:54 AM, dilbertside notifications@github.comwrote:

Hi,

I am facing a problem related to an error thrown like
org.springframework.dao.DataIntegrityViolationException when trying to
deleting a DB entity.

This error is expected and it is not a big issue or a bug. Why?

Because the Data Model to map the business model, has some constraint to
avoid data loss by negligence.
So the client has to go through some steps so the entity could be deleted
properly. Other user constraints make it difficult to do it through a
proper workflow.
For legal reasons, every single deletion is audit trailed and error
occurring have to be stored.

So come to my problem, when EDS catch the error in
RouterController.handleException, it by-passes the web app to give a direct
answer to the front-end.

EDS gives the opportunity to add some customization through
Configuration.setExceptionToMessage but it is too generic and the net is
too wide.
I can add DataIntegrityViolationException in the map but it will not help
in a case of a business rule break and/or a real bug. It is too coarse.

And to make things bad, we cannot take quick action as the webapp is not
aware what went wrong (except through the eventual log).
We would like to track the error and not send a straight notification to
the user but a more elaborate answer.

My question is would it be possible to handover exception handling to
application logic and not the library logic?

I see at least 2 possible solutions:

remove the try catch in RouterController.router so the error handling
centralization can be made as Spring propose with its @ControllerAdvice and
ResponseEntityExceptionHandler? cf
http://static.springsource.org/spring/docs/3.2.x/spring-framework-reference/htmlsingle/#new-in-3.2-webmvc-exception-handler-support

or inject an exception handler in the RouterController so to delegate the
exception handling to another bean if any are configured?

Regards,

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101
.

[dilbertside]

answers inline

On 08/15/2013 03:34 PM, Ralph Schaer wrote:

Hi

The ControllerAdvice/ResponseEntityExceptionHandler looks like a very
interesting solution. The only problem I see with this is that you need
access to the Ext.Direct request because in the Ext.Direct response the
name, action and tid has to match the ones from the request.
not really a problem:
in something like that
@ExceptionHandler({org.springframework.dao.DataIntegrityViolationException.class})
public @responsebody ResponseEntity
handleDataAccess(Exception ex, WebRequest request, HttpServletResponse
response) {}

WebRequest gives you every thing you want, so I rebuild an
ExtDirectResponse with the WebRequest in my current implementation for
requests out of EDS

The problem I see with this solution is backward compatibility.
We cannot remove the try/catch block as existing implementations, I
guess, rely on this behavior.
Maybe re-throwing the error (with writableStackTrace to false) will let
Spring handle the case...

The second solution looks like easier to implement. We could create an
interface

public interface EdsExceptionHandler {
handleException(BaseResponse response, Exception e) ;
}

and create a default implementation that contains the current exception
handling code.
Then we inject this bean into the RouterController or the Configuration
class.

this one would be backward compatible

[ralscha]

Then let's try the second solution. Do you want to give it a try or should
I start a branch and try changing the code?

On Thu, Aug 15, 2013 at 10:07 AM, dilbertside notifications@github.comwrote:

answers inline

On 08/15/2013 03:34 PM, Ralph Schaer wrote:

Hi

The ControllerAdvice/ResponseEntityExceptionHandler looks like a very
interesting solution. The only problem I see with this is that you need
access to the Ext.Direct request because in the Ext.Direct response the
name, action and tid has to match the ones from the request.
not really a problem:
in something like that

@ExceptionHandler({org.springframework.dao.DataIntegrityViolationException.class})
public @responsebody ResponseEntity
handleDataAccess(Exception ex, WebRequest request, HttpServletResponse
response) {}

WebRequest gives you every thing you want, so I rebuild an
ExtDirectResponse with the WebRequest in my current implementation for
requests out of EDS

The problem I see with this solution is backward compatibility.
We cannot remove the try/catch block as existing implementations, I
guess, rely on this behavior.
Maybe re-throwing the error (with writableStackTrace to false) will let
Spring handle the case...

The second solution looks like easier to implement. We could create an
interface

public interface EdsExceptionHandler {
handleException(BaseResponse response, Exception e) ;
}

and create a default implementation that contains the current exception
handling code.
Then we inject this bean into the RouterController or the Configuration
class.

this one would be backward compatible

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101#issuecomment-22690211
.

[dilbertside]

If you have some time, I would let you start.
My hand are very tight at moment and I still have the #96 to develop.
Or we can wait a bit, when I get some free time ahead, I push something.
Low priority, anyway

Thanks

[ralscha]

I created a new branch (issue101) and made some changes.

There is now an interface ch.ralscha.extdirectspring.controller.RouterExceptionHandler and a default implementation ch.ralscha.extdirectspring.controller.DefaultRouterExceptionHandler.
To configure your own exception handler, create a class, implement the interface and configure the class as a spring bean.

Right now the method signature looks like this:
void handleException(BaseResponse response, Exception e) ;

I can add additional parameters, like servlet request or Ext.Direct request, if your program requires these.

[dilbertside]

Thanks, that's almost perfect :-)

What I would suggest is to give the user the possibility to insert his
own additional objects in the result map.
or give access to the ExtDirectResponse, or give a map to the
handleException method.
I propose the first solution in my library fork
git@github.com:dilbertside/extdirectspring.git branch issue101

As suggested, I would need also the Ext.Direct request, Principal and
Locale, but It is maybe optional as HttpServletRequest gives access to them.

On 08/15/2013 07:08 PM, Ralph Schaer wrote:

I created a new branch (issue101) and made some changes.

There is now an interface
ch.ralscha.extdirectspring.controller.RouterExceptionHandler and a
default implementation
ch.ralscha.extdirectspring.controller.DefaultRouterExceptionHandler.
To configure your own exception handler, create a class, implement the
interface and configure the class as a spring bean.

Right now the method signature looks like this:
void handleException(BaseResponse response, Exception e) ;

I can add additional parameters, like servlet request or Ext.Direct
request, if your program requires these.

—
Reply to this email directly or view it on GitHub
#101 (comment).

[ralscha]

Thanks. Looks good.

Integrated your changes in my branch and build a new SNAPSHOT
https://oss.sonatype.org/content/repositories/snapshots/ch/ralscha/extdirectspring/1.3.6-SNAPSHOT/extdirectspring-1.3.6-20130816.040326-7.jar

[dilbertside]

I forgot to address the 2 other cases poll and handleMethodCall in
RouterController
It is done now in last push.

I tested it, perfect.

Thank you very much

[ralscha]

This breaks a few unit tests because the library does not expect a response
in the exception case.

On Fri, Aug 16, 2013 at 6:41 AM, dilbertside notifications@github.comwrote:

I forgot to address the 2 other cases poll and handleMethodCall in
RouterController
It is done now in last push.

I tested it, perfect.

Thank you very much

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101#issuecomment-22747314
.

[dilbertside]

which one poll or handleMethodCall ?

Because actually when I proceeded to implement some logic I noticed
handleMethodCall was called and result "success" was not set to false,
so it ended up on the client side to be interpreted as success true and
extjs never run the failure handler.

I can work on the unit test to make them work.

[ralscha]

I like your change that the exception handler can set a result. But this breaks a few unit tests and it could break some code if somebodys program expect a null result.

To solve this I added the MethodInfo to the signature of the exception handler method. This way the default exception handler only sends a result back if it's a FORM_POST method call.

Object handleException(MethodInfo methodInfo, BaseResponse response, Exception e, HttpServletRequest request) ;

[dilbertside]

Hmmm! more tricky than I thought.

I'm running some tests on STORE_MODIFY and I noticed
Ext.data.proxy.Server.processResponsethere is a extjs bug as if we set
a result message it is lost when the call to
result = reader.read(me.extractResponseData(response));
is made.

So in the current implementation of EDS, it is fine when you return null
by default as every thing else you can set is ignored except records and
success property.

However, I will file a bug report with them, to ask why the
result.message is lost, but still it doesn't matter for EDS as the
webapp can takeover the responsibility to ignore the exception or handle
it differently (in that case, it is important that the STORE_MODIFY send
a success false otherwise the sync failure handler will never be called).

Your latest change didn't break my customized exception handling, so we
can assume unless other testers say otherwise, it is fine.

[dilbertside]

Forget about the extjs bug. I was wrong about the fact that the message
sent by the server in result map is lost.

Down the line in result = reader.read(me.extractResponseData(response));

in fact in
http://localhost/ext4.2/docs/source/Reader2.html#Ext-data-reader-Reader-method-readRecords

we have something like

if (me.messageProperty) {
message = me.getMessage(data);
}

and by default messageProperty is undefined. (same code in 4.1.3 and 4.2.1)

So I was wondering if the model generator could add optionally that as
it already write
reader:{
root:"records"
}
it could be
reader:{
root:"records",
messageProperty: "message"
}

Most of my extjs model now are generated with the generator, so it would
make sense to add that property or I need to hack the reader to add a
default one.

Should I make a new improvement request issue and would it be a valuable
addition to the model generator?

On 08/16/2013 03:38 PM, Ralph Schaer wrote:

I like your change that the exception handler can set a result. But
this breaks a few unit tests and it could break some code if somebodys
program expect a null result.

To solve this I added the MethodInfo to the signature of the exception
handler method. This way the default exception handler only sends a
result back if it's a FORM_POST method call.

|Object handleException(MethodInfo methodInfo, BaseResponse response, Exception e, HttpServletRequest request) ;
|

—
Reply to this email directly or view it on GitHub
#101 (comment).

[ralscha]

Should I make a new improvement request issue and would it be a valuable
addition to the model generator?
Yes please create a new issue.
I guess the easiest solution is adding a new property 'messageProperty' to the @model annotation.

[ralscha]

All the changes are now merged into the 'next' branch.

[dilbertside]

thanks a lot.