Handling ValidationExceptions in interceptors #873

csokol · 2014-11-10T16:02:07Z

Please do not merge it yet, it's a work in progress related with #858

My idea here is to extract classes to better deal with exception handling so then we could reuse the exception handling code to develop that @ValidationInterceptor feature that @lucascs proposed.

I've implemented a Try class, similar to the scala's Try monad. Is not a proper monad here, but I think it's a nice abstraction to handle exceptions, what do you think? @lucascs @Turini @garcia-jj

garcia-jj · 2014-11-10T16:18:45Z

vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExceptionHandler.java

+
+import static org.slf4j.LoggerFactory.getLogger;
+
+public class ExceptionHandler {


We already have an exception handler. What you think to improve then instead of creating a new one?

this is not the same idea of the other one, I guess...

So a rename is welcome to avoid mistakes.

I'll try to find a better name...

I've renamed to ExecuteMethodExceptionHandler, what do you think @garcia-jj?

asouza · 2014-11-10T16:55:53Z

I really liked the Try class. For me, it is a nice abstraction and could be
reused by VRaptor's users in their business logic.

On Mon, Nov 10, 2014 at 2:02 PM, csokol notifications@github.com wrote:

Please do not merge it yet, it's a work in progress related with #858
#858

My idea here is to extract classes to better deal with exception handling
so then we could reuse the exception handling code to develop that
@ValidationInterceptor feature that @lucascs https://github.com/lucascs
proposed.

I've implemented a Try class, similar to the scala's Try monad. Is not a
proper monad here, but I think it's a nice abstraction to handle
exceptions, what do you think? @lucascs https://github.com/lucascs
@Turini https://github.com/Turini @garcia-jj

https://github.com/garcia-jj

You can merge this Pull Request by running

git pull https://github.com/csokol/vraptor4 catching-interceptors-validation-errors

Or view, comment on, or merge it at:

#873
Commit Summary

refactoring exception handling code with a a Try class

File Changes

M
vraptor-core/src/main/java/br/com/caelum/vraptor/core/ToInstantiateInterceptorHandler.java
https://github.com/caelum/vraptor4/pull/873/files#diff-0 (15)

A
vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExceptionHandler.java
https://github.com/caelum/vraptor4/pull/873/files#diff-1 (34)

M
vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java
https://github.com/caelum/vraptor4/pull/873/files#diff-2 (63)

A vraptor-core/src/main/java/br/com/caelum/vraptor/util/Try.java
https://github.com/caelum/vraptor4/pull/873/files#diff-3 (56)

M
vraptor-core/src/test/java/br/com/caelum/vraptor/observer/ExecuteMethodTest.java
https://github.com/caelum/vraptor4/pull/873/files#diff-4 (2)

Patch Links:

https://github.com/caelum/vraptor4/pull/873.patch

https://github.com/caelum/vraptor4/pull/873.diff

—
Reply to this email directly or view it on GitHub
#873.

Alberto Souza

www.caelum.com.br
www.codecrushing.com/products/book-play-framework-java
www.leanpub.com/playframeworknapratica
www.alots.wordpress.com/

garcia-jj · 2014-11-10T17:47:33Z

vraptor-core/src/main/java/br/com/caelum/vraptor/util/Try.java

+
+		@Override
+		public Exception getException() {
+			throw new UnsupportedOperationException();


May be null is better than this exception, because

Success means that have no exception, so return null is right;

in the view we can do something like ${not empty try.exception}. If this exception we need to write more code: ${try.failed ? try.exception : null};

I like the query method :) to know about the state of the object.

On Mon, Nov 10, 2014 at 3:47 PM, Otávio Garcia notifications@github.com
wrote:

In vraptor-core/src/main/java/br/com/caelum/vraptor/util/Try.java:

public static class Success extends Try {

private final T result;

public Success(T result) {

this.result = result;

}

@Override

public boolean failed() {

return false;

}

@Override

public Exception getException() {

throw new UnsupportedOperationException();

May be null is better than this exception, because

Success means that have no exception, so return null is right;

in the view we can do something like ${not empty try.exception}. If
this exception we need to write more code: ${try.failed ? try.exception :
null};

—
Reply to this email directly or view it on GitHub
https://github.com/caelum/vraptor4/pull/873/files#r20098586.

Alberto Souza

www.caelum.com.br
www.codecrushing.com/products/book-play-framework-java
www.leanpub.com/playframeworknapratica
www.alots.wordpress.com/

I usually try to avoid passing nulls around. What do you thinkg of returning an empty Optional? (from guava)

I can't see why too complex. There is no reason to export an 3rd part library in this case. If there is no exception, null can be welcome.

we should avoid nulls if we can...

IMHO, as a framework, we should not expose 3rd part library in our public API. Java Optional is only available in Java 8. So null are welcome here. We already use null in other places like converters and more.

Instead of adding a 3rd part library in our public API I prefer this UnsupportedOperationException so.

I'd also avoid to expose Guava's Optional in our public API.

I'll keep the UnsupportedOperationException, then.

👍

What you think to implement Try.failed() to return always false and getException() and getResult() to always throws the Exception?

So in the Success you need to only implements getResult(), and in the Failure you need only to override failed() and getException().

The same approaching is used in AbstractList. All methods are implemented with some common behavior, and subclasses only define a new behaviors as needed.

Turini · 2014-11-11T12:29:15Z

I really liked the Try class. For me, it is a nice abstraction and could
be reused by VRaptor's users in their business logic.

👍 and it becomes even better for java 8 users. It's a nice addition

Turini · 2014-11-11T12:33:26Z

vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java

@@ -58,55 +60,50 @@

 	private Event<MethodExecuted> methodExecutedEvent;
 	private Event<MethodReady> methodReady;


please, can you add final for other fields too?

Turini · 2014-11-11T12:42:43Z

after code is done, can you add some docs about it? thks

garcia-jj · 2014-11-11T12:56:14Z

I like this pull request. Code are too pretty and as @Turini said, its a nice addition the Try class. Thank you.

@Turini Whats the target release? Since this not change our public API can be merged in the current release, right? What you think?

Turini · 2014-11-11T13:17:42Z

Since this not change our public API can be merged in the current release,
right? What you think?

Sure, we can merge it now. But we'll need to hold a little more the 4.2 final
to ensure its working as expected (if possible deploy it on mamute and other apps)

csokol · 2014-11-11T17:58:22Z

Do you want to merge it right now? I still need to implement the validation exception logic around interceptors. Should I do it in another PR?

garcia-jj · 2014-11-11T18:04:02Z

I vote to merge right now, because do not change public API.

Just for label: this pull request conflicts with #848, that needs to be refactored after this merge.

garcia-jj · 2014-11-11T18:05:49Z

Or due this is a new feature, and we are working in a patch version, we can generate a new version 4.1.2 today or tomorrow, and merge this pull request in 4.2, that is a better place to add new features.

But I'm not sure what's the best way.

csokol · 2014-11-11T18:32:53Z

I've finished implementing the validation exceptions handling around interceptor calls, using the idea of @lucascs with a new @ValidatedInterceptor annotation. Can I push this update in this PR?

Turini · 2014-11-12T11:46:46Z

Or due this is a new feature, and we are working in a patch version, we
can generate a new version 4.1.2 today or tomorrow, and merge this pull
request in 4.2, that is a better place to add new features.

@garcia-jj I agree merging it in 4.2, since its a new feature and not a bugfix.
About the release, we need to fix the current bugs (i) before doing it. I'll try to
work on it as soon as possible. (i) #876 and #832. The #867 was just merged

Turini · 2014-11-12T11:48:30Z

I've finished implementing the validation exceptions handling around
interceptor calls [...] Can I push this update in this PR?

@csokol, sure! Just let us know when its done to review it again. Thanks

csokol · 2014-11-12T14:03:39Z

I've just updated the PR with the code to handle validation exceptions in
interceptors annotated with @ValidatedInterceptor, can you guys review it?

On Wed Nov 12 2014 at 9:48:32 AM Rodrigo Turini notifications@github.com
wrote:

I've finished implementing the validation exceptions handling around
interceptor calls [...] Can I push this update in this PR?

@csokol https://github.com/csokol, sure! Just let us know when its done
to review it again. Thanks

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

garcia-jj · 2014-11-12T14:05:59Z

I don't remember: why we need this new annotation?

csokol · 2014-11-12T17:36:32Z

The whole idea of this PR was to deal with ValidationExceptions inside
interceptors. Currently, if an interceptor does something like:
validator.onErrorRedirect(..). The exception comes up and the request end
with a 500.

The idea from @lucascs was to create a new annotation and the users should
use it to define what interceptors should be treated differently to support
validation errors.

I don't remember: why we need this new annotation?

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

lucascs · 2014-11-13T00:31:56Z

well, we don't NEED this new annotation, but since we already have some annotations that "generate" code (like @AcceptsWithAnnotation), @ValidationInterceptor would "generate" the try...catch(ValidationException) around the interceptor.

csokol · 2014-11-13T13:57:55Z

Exactly :-)

Another option is to assume that this will be default behavior from now on
and remove the annotation.

Turini · 2014-11-24T18:40:27Z

Another option is to assume that this will be default behavior from
now on and remove the annotation.

There is a downside for doing it? (something like performance issues)

csokol · 2014-11-26T13:49:58Z

I don't see any performance issue with this wrapping all the interceptors around this try catch. Actually it should be more efficient since we won't need to lookup for the annotation of each interceptor executed.

Turini · 2014-11-26T13:55:53Z

And so do I. +1 for keep all interceptors validated by defaul.

lucascs · 2014-11-28T02:01:11Z

it makes sense... since ExecuteMethod is not an interceptor anymore, I don't see a problem doing this try..catch for every interceptor.

garcia-jj · 2014-12-10T20:38:07Z

vraptor-core/src/main/java/br/com/caelum/vraptor/util/Try.java

+			return e;
+		}
+	}
+


Two empty lines here ✂️

nykolaslima · 2014-12-22T16:33:11Z

@csokol The PR cannot be merged due to conflicts. You need to solve it before merging

csokol · 2014-12-22T16:49:40Z

I'll rebase my branch and force a push into this branch

On Mon Dec 22 2014 at 2:33:13 PM Nykolas Laurentino de Lima <
notifications@github.com> wrote:

@csokol https://github.com/csokol The PR cannot be merged due to
conflicts. You need to solve it before merging

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

Turini · 2014-12-23T11:28:58Z

vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java

+	/**
+	 * @deprecated CDI eyes only
+	 */
+	protected ExecuteMethod() {


we don't need this, ExecuteMethod has dependent scope

Turini · 2014-12-23T11:40:58Z

@csokol this PR looks great, thanks a lot! Before merging, can you add a simple
paragraph on vraptor-site about this feature? And if possible javadoc Try class

lucascs · 2014-12-23T21:37:30Z

🎅

csokol · 2014-12-29T14:23:04Z

Just finished the docs, @Turini, can you take a look? I think is ready to

Turini · 2014-12-29T14:29:29Z

🚀

…errors Handling ValidationExceptions in interceptors

nykolaslima · 2016-05-05T23:15:52Z

@Turini can we make a relase with it? The ValidationException thrown from an interceptor is fixed by this PR but the last VRaptor release (4.2.0-RC3) doesn't have this fix.

Turini · 2016-05-05T23:37:32Z

hey @nykolaslima.

sure, I'm planning to do that tomorrow during the morning. Will let u know

Turini · 2016-05-09T14:46:35Z

done!

filipesg · 2017-05-24T20:49:12Z

Is this available on 4.2.0-RC5? I'm getting error 500 on this code:
validator.onErrorForwardTo("/url");
java.lang.RuntimeException: java.lang.String is final
It's inside an interceptor. I need to use any kind of extra annotation to catch this validation error?

garcia-jj reviewed Nov 10, 2014
View reviewed changes

Turini added the enhancement label Nov 11, 2014

Turini added this to the 4.2.0.Final milestone Nov 11, 2014

Turini reviewed Nov 11, 2014
View reviewed changes

garcia-jj reviewed Dec 10, 2014
View reviewed changes

csokol mentioned this pull request Dec 16, 2014

Validation errors on Interceptor are not handled by VRaptor #858

Closed

csokol added 8 commits December 22, 2014 14:55

refactoring exception handling code with a a Try class

76f67ae

Try should be generic

f7cd903

renaming method and adding exception message

4b17d6c

renaming class

b64e749

handling validation exception around interceptors

eadf45d

removing empty lines

646fc1f

catching validation errors for all interceptors and removing annotation

3603996

fixing import

9c1ee3e

csokol force-pushed the catching-interceptors-validation-errors branch from 05625dc to 9c1ee3e Compare December 22, 2014 17:10

Turini reviewed Dec 23, 2014
View reviewed changes

csokol added 5 commits December 29, 2014 11:33

removing pointless constructor

ad4b568

moving try to core

37d89fd

Try javadoc

7d8b1f5

fixing imports

59d03b5

Try docs on vraptor-site

9503b57

csokol added a commit that referenced this pull request Dec 29, 2014

Merge pull request #873 from csokol/catching-interceptors-validation-…

3268b17

…errors Handling ValidationExceptions in interceptors

csokol merged commit 3268b17 into caelum:master Dec 29, 2014

csokol deleted the catching-interceptors-validation-errors branch December 29, 2014 15:31


		import static org.slf4j.LoggerFactory.getLogger;

		public class ExceptionHandler {

		@@ -58,55 +60,50 @@

		private Event<MethodExecuted> methodExecutedEvent;
		private Event<MethodReady> methodReady;

Handling ValidationExceptions in interceptors #873

Handling ValidationExceptions in interceptors #873

Conversation

csokol commented Nov 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asouza commented Nov 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turini commented Nov 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turini commented Nov 11, 2014

garcia-jj commented Nov 11, 2014

Turini commented Nov 11, 2014

csokol commented Nov 11, 2014

garcia-jj commented Nov 11, 2014

garcia-jj commented Nov 11, 2014

csokol commented Nov 11, 2014

Turini commented Nov 12, 2014

Turini commented Nov 12, 2014

csokol commented Nov 12, 2014

garcia-jj commented Nov 12, 2014

csokol commented Nov 12, 2014

lucascs commented Nov 13, 2014

csokol commented Nov 13, 2014

Turini commented Nov 24, 2014

csokol commented Nov 26, 2014

Turini commented Nov 26, 2014

lucascs commented Nov 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nykolaslima commented Dec 22, 2014

csokol commented Dec 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turini commented Dec 23, 2014

lucascs commented Dec 23, 2014

csokol commented Dec 29, 2014

Turini commented Dec 29, 2014

nykolaslima commented May 5, 2016

Turini commented May 5, 2016

Turini commented May 9, 2016

filipesg commented May 24, 2017 • edited Loading

filipesg commented May 24, 2017 •

edited

Loading