Skip to content

Improve ActionException's for the Rest Dispatcher#735

Closed
BenDol wants to merge 5 commits into
ArcBees:masterfrom
BenDol:bd_exhandler
Closed

Improve ActionException's for the Rest Dispatcher#735
BenDol wants to merge 5 commits into
ArcBees:masterfrom
BenDol:bd_exhandler

Conversation

@BenDol

@BenDol BenDol commented Sep 19, 2015

Copy link
Copy Markdown
Contributor

This gives better understanding of issues during transport.

Exception Hierarchy:

|-- Exception
     -- ActionException           -- RestActionException
                |-- ActionResponseException
                |     -- ActionDeserializationException                 -- ActionSerializationException

ActionException.java

This exception now takes a TypedAction<?> (which has been made Serializable).

Note: There have been issues with variables in Exceptions for the RPC implementation, will need to know what the limitations are if there are any.

RestActionException.java

Used as convenience for type casting the TypedAction<?> directly as RestAction<?>. Extends the ActionException class.

ActionResponseException.java

Used in the response phase providing most of the Response information in the exception. ActionDeserializationException extends this class.

ActionSerializationException.java

Used in the serialization of an action request, mostly when a SerializationException is thrown.

ActionDeserializationException.java

Used during the deserialization of a response from the server, only thrown when the response is unable to be deserialized by the object mapper. This class extends ActionResponseException providing the Response information with it.

…ails.

This gives better understanding to what has gone wrong during transport.
@BenDol

BenDol commented Sep 22, 2015

Copy link
Copy Markdown
Contributor Author

Added an example to the carstore sample application ArcBees/GWTP-Samples#91

@Chris-V

Chris-V commented Nov 2, 2015

Copy link
Copy Markdown
Member

I'm not sure I can see the need to have both ActionResponseException and ActionDeserializationException.

Imo the action instance should be available through the deserialization as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the use of this overload

Passing the Response in deserializeValue in order to provide information in the exception.
@BenDol

BenDol commented Nov 3, 2015

Copy link
Copy Markdown
Contributor Author

Well a response exception is when there is a response problem not a deserialization problem, so if the server returns an unsuccessful status code it throws the ActionResponseException with the response information. The deserialization exception extends the response exception because its part of the response phase and has response information.

@BenDol

BenDol commented Nov 3, 2015

Copy link
Copy Markdown
Contributor Author

This allows me to catch response specific errors and handle them in my exception handler. I.e. security exceptions where I notify the user of insufficient rights.

@BenDol

BenDol commented Dec 7, 2015

Copy link
Copy Markdown
Contributor Author

Is this PR going to be used or reviewed to be used? Would be helpful for me.

@christiangoudreau

Copy link
Copy Markdown
Member

Going to be reviewed, sorry about this!

@Chris-V

Chris-V commented Jan 4, 2016

Copy link
Copy Markdown
Member

It would be interesting to have RestActionException take the RestAction<?> in argument, and maybe the callback. I can see potential uses for automated retry mechanisms.

Also, in DefaultResponseDeserializer, I believe throwing a BadStatusCodeException (that extends ActionResponseException) would help prevent bad handling. ie: if someone tests for ActionResponseException before testing for ActionDeserializationException, then the first test will match and may prevent the second one for reaching.

RestActionException and ActionResponseException can be made abstract to help understanding they are top-level exceptions.

Can we get rid of the "Action" prefix. This may cause clashes with other exception names, though those prefixes annoy me to no end and I'm trying to get rid of them as time goes on ;)

@olafleur

Copy link
Copy Markdown
Member

@BenDol can we help you with that PR ?

@BenDol

BenDol commented Jun 5, 2016

Copy link
Copy Markdown
Contributor Author

Hey Chris, since this PR is not going to be merged can you help me understand how the new exception handling should work? I currently use this for catching my exceptions in my latest project and would need to rewrite it to work the way you have redesigned it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants