Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ActionException's for the Rest Dispatcher #735

Closed
wants to merge 5 commits into from

Conversation

BenDol
Copy link
Contributor

@BenDol BenDol commented Sep 19, 2015

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
Copy link
Contributor Author

BenDol commented Sep 22, 2015

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

@Chris-V
Copy link
Member

Chris-V commented Nov 2, 2015

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.

}

public ActionException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
Copy link
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
Copy link
Contributor Author

BenDol commented Nov 3, 2015

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
Copy link
Contributor Author

BenDol commented Nov 3, 2015

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
Copy link
Contributor Author

BenDol commented Dec 7, 2015

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

@christiangoudreau
Copy link
Member

Going to be reviewed, sorry about this!

@Chris-V
Copy link
Member

Chris-V commented Jan 4, 2016

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
Member

@BenDol can we help you with that PR ?

@BenDol
Copy link
Contributor Author

BenDol commented Jun 5, 2016

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