Fixing problem: validation errors ignored on redirects. Closes #476 #725

renanigt · 2014-08-02T14:43:01Z

In conversation on Issue #476, @lucascs asked me to send this PR to start the bug fix.

Here the gist with the problematic controller as he asked me: Problematic Controller

garcia-jj · 2014-08-02T15:56:03Z

vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultResult.java

@@ -27,6 +27,7 @@
 import javax.inject.Inject;
 import javax.servlet.http.HttpServletRequest;

+import org.jboss.weld.exceptions.IllegalStateException;


Use java.lang.IllegalStateException instead

Really. Thanks. :-)

lucascs · 2014-08-05T02:06:46Z

vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultResult.java

+	                        + "validator.onErrorUse(page()).of(AnyController.class).anyMethod();\n"
+	                        + "or any view that you like.\n"
+	                        + "If you didn't add any validation error, it is possible that a conversion error had happened.");
+	    }


Done. thanks @lucascs . ;-)

lucascs · 2014-08-05T02:09:01Z

What is the problem the controller is causing?

if it's returning a 500 with the message you configured, it's the expected behavior

renanigt · 2014-08-05T02:22:23Z

The only problem that I saw, is that when using something like:

@Post("/add")
public void add(Usuario usuario) {
    validator.addIf(Strings.isNullOrEmpty(usuario.getLogin()), new SimpleMessage("Login", "Login inválido."));

    result.redirectTo(this).index();
}

The error is only this exception: https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java#L98

But with the approach of this PR and using something like result.use(json()).from ("test").serialize(); instead of result.redirectTo(this).index();, before exception above, I got either this exception:

net.vidageek.mirror.exception.ReflectionProviderException: Could not invoke method add
    net.vidageek.mirror.provider.java.PureJavaMethodReflectionProvider.invoke(PureJavaMethodReflectionProvider.java:45)
    net.vidageek.mirror.invoke.MethodHandlerByMethod.withArgs(MethodHandlerByMethod.java:54)
    br.com.caelum.vraptor.observer.ExecuteMethod.execute(ExecuteMethod.java:87)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:606)
    org.jboss.weld.injection.MethodInjectionPoint.invokeOnInstanceWithSpecialValue(MethodInjectionPoint.java:93)
    org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:266)
    org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:253)
    org.jboss.weld.event.ObserverMethodImpl.notify(ObserverMethodImpl.java:232)
    org.jboss.weld.event.ObserverNotifier.notifyObserver(ObserverNotifier.java:169)
    org.jboss.weld.event.ObserverNotifier.notifyObserver(ObserverNotifier.java:165)
    org.jboss.weld.event.ObserverNotifier.notifyObservers(ObserverNotifier.java:119)
    org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:112)
    org.jboss.weld.event.EventImpl.fire(EventImpl.java:83)
    br.com.caelum.vraptor.core.DefaultInterceptorStack.next(DefaultInterceptorStack.java:78)
    br.com.caelum.vraptor.interceptor.ExceptionHandlerInterceptor.intercept(ExceptionHandlerInterceptor.java:75)
    br.com.caelum.vraptor.interceptor.ExceptionHandlerInterceptor$Proxy$_$$_WeldClientProxy.intercept(Unknown Source)
    br.com.caelum.vraptor.core.ToInstantiateInterceptorHandler.execute(ToInstantiateInterceptorHandler.java:58)
    br.com.caelum.vraptor.core.DefaultInterceptorStack.next(DefaultInterceptorStack.java:83)
    br.com.caelum.vraptor.interceptor.FlashInterceptor.intercept(FlashInterceptor.java:98)
    br.com.caelum.vraptor.interceptor.FlashInterceptor$Proxy$_$$_WeldClientProxy.intercept(Unknown Source)
    br.com.caelum.vraptor.core.ToInstantiateInterceptorHandler.execute(ToInstantiateInterceptorHandler.java:58)
    br.com.caelum.vraptor.core.DefaultInterceptorStack.next(DefaultInterceptorStack.java:83)
    br.com.caelum.vraptor.core.DefaultInterceptorStack.start(DefaultInterceptorStack.java:93)
    br.com.caelum.vraptor.core.DefaultInterceptorStack$Proxy$_$$_WeldClientProxy.start(Unknown Source)
    br.com.caelum.vraptor.observer.RequestHandlerObserver.handle(RequestHandlerObserver.java:86)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:606)
    org.jboss.weld.injection.MethodInjectionPoint.invokeOnInstanceWithSpecialValue(MethodInjectionPoint.java:93)
    org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:266)
    org.jboss.weld.event.ObserverMethodImpl.sendEvent(ObserverMethodImpl.java:253)
    org.jboss.weld.event.ObserverMethodImpl.notify(ObserverMethodImpl.java:232)
    org.jboss.weld.event.ObserverNotifier.notifyObserver(ObserverNotifier.java:169)
    org.jboss.weld.event.ObserverNotifier.notifyObserver(ObserverNotifier.java:165)
    org.jboss.weld.event.ObserverNotifier.notifyObservers(ObserverNotifier.java:119)
    org.jboss.weld.event.ObserverNotifier.fireEvent(ObserverNotifier.java:112)
    org.jboss.weld.event.EventImpl.fire(EventImpl.java:83)
    br.com.caelum.vraptor.VRaptor.doFilter(VRaptor.java:119)

renanigt · 2014-08-05T02:25:07Z

@csokol opened a PR with something like this problem with exception, I don't know if can solves this issue.

See: #728 and #729.

lucascs · 2014-08-05T02:36:10Z

is there any caused by or root cause for this exception?

renanigt · 2014-08-05T04:33:28Z

is there any caused by or root cause for this exception?

No.

I tested the @csokol's solution, and solved this problem...

renanigt · 2014-08-06T02:02:24Z

@lucascs do you think that we need check just for Results.json() and Results.xml() since using Results.http(), request.sendRedirect(...) and result.redirectTo(...).someMethod() we get the correct behavior ? Or we need just check for validator.hasErrors() and throw exception to all cases ?

lucascs · 2014-08-06T02:05:51Z

I'd rather throw the exception as soon as possible.

so for all result.use

renanigt · 2014-08-06T02:10:31Z

@lucascs since the PR #729 fix the problem with exception that I cited before, should I need wait the merge and update my branch before another changes or can I going ahead ?

lucascs · 2014-08-06T02:52:28Z

go ahead... write some unit tests for it

maybe you can remove the exception thrown on ExecuteMethodObserver also.

renanigt · 2014-08-06T04:16:28Z

If I remove the exception thrown on ExecuteMethodObserver don't will cause conflict with PR #729 since both are changing the same class ?

renanigt · 2014-08-06T04:38:51Z

Making some tests, now the problem is because Validator uses Result to make the redirects, forwards...

So, I can't just add this check on method use.

I'll do another tests and take a look about how I can fix the problem, but if do you have some suggestions, I think you. rs

lucascs · 2014-08-06T04:41:28Z

I mean this exception, that is the same you've thrown:

https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java#L98-L103

maybe we should move this whole if to result#use:
https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java#L89-L104

can you do it please and test if it works?

renanigt · 2014-08-06T04:43:01Z

Yes, I can. 😃

lucascs · 2014-08-06T04:53:22Z

about the cyclic problem, we can fix it by clearing validation#errors here:
https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/validator/DefaultValidator.java#L144

before returning the viewsFactory

Not sure if I like the solution though... What do you think? @garcia-jj @Turini

renanigt · 2014-08-06T05:00:10Z

Maybe a solution for the cyclin problem, if do you agree, I can do it. @lucascs @garcia-jj @Turini

Moving the check for validation erros to use method, I got the error but still with the ReflectionProviderException that is caused here: https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/observer/ExecuteMethod.java#L87

So we still need the PR #729.

Turini · 2014-08-06T11:34:37Z

I'm from mobile, so I didn't look at all code. But about cyclic problem you can just
"lazy injected" the bean with Instance<?>. CDI will handle the problem as expected.

renanigt · 2014-08-06T12:13:20Z

@Turini sorry if I didn't understand your point and as you said you're from mobile and didn't look at all code.

I think when @lucascs said about cyclic problem is because Validator uses Result to make its redirects, forwards... so if we just check for validator.hasErrors() on result.use to throw exception, we always get the exception since Validator will use Result.

Can we resolve this with Instance<?> ?

Thanks a lot. 😃

Turini · 2014-08-06T12:58:14Z

hum, now I see. In this case Instance<Validator> could resolve only if Validator
had @Dependent scope (this is not the case). I think we can use Lucas's solution.

renanigt · 2014-08-06T13:04:47Z

@garcia-jj do you agree too ?

If so, can I implement it ? @lucascs @Turini @garcia-jj

Thanks. :-)

renanigt · 2014-08-29T03:48:58Z

Done. What do you think ? @lucascs @Turini @garcia-jj

I did some tests with a POC and worked fine.

renanigt · 2014-08-29T03:54:23Z

@Turini,
I really didn't understand why the cyclic problem not occurred, since we have this:
https://github.com/renanigt/vraptor4/blob/validationIgnoredOnRedirects/vraptor-core/src/main/java/br/com/caelum/vraptor/validator/DefaultValidator.java#L81

https://github.com/renanigt/vraptor4/blob/validationIgnoredOnRedirects/vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java#L64

DefaultValidator injects Messages and Messages injects Validator.

May you explain me ?

Thanks a lot.

Turini · 2014-08-29T09:55:25Z

This is really simple. Normal scoped beans are "lazy injected". When you inject such bean it will be just a proxy object, the real instance will be called only at the moment when you performed your first action. CDI impl takes care of it, you don't need to use Instance<?> for this specific case. (sorry about the short answer, I'm from mobile right now)

rponte · 2014-08-29T11:26:46Z

@Turini,

This kind of feature in CDI it's interesting but it's weird if you think in
terms of design of the classes. Have you tried to instantiate a class with
circular dependency? I know we can decouple using interfaces but it's still
weird, at least for me. I just think we should avoid it if possible!

As @Turini said, CDI just handles circular reference when at least one of
the beans has normal scope
http://docs.jboss.org/cdi/spec/1.0/html/injectionelresolution.html!

The container is required to support circularities in the bean dependency

graph where at least one bean participating in every circular chain of
dependencies has a normal scope, as defined in Section 6.3, “Normal
scopes and pseudo-scopes”
http://docs.jboss.org/cdi/spec/1.0/html/contexts.html#normalscope. The
container is not required to support circular chains of dependencies where
every bean participating in the chain has a pseudo-scope.

On Fri, Aug 29, 2014 at 6:55 AM, Rodrigo Turini notifications@github.com
wrote:

This is really simple. Normal scoped beans are "lazy injected". When you
inject such bean it will be just a proxy object, the real instance will be
called only at the moment when you performed your first action. CDI impl
takes care of it, you don't need to use Instance<?> for this specific case.
(sorry about the short answer, I'm from mobile right now)

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

Turini · 2014-08-29T11:34:13Z

I just think we should avoid it if possible!

I agree about avoiding it, but to use Instance<?> will change nothing.
We need a better design for this solution. What do you suggest?

rponte · 2014-08-29T11:45:49Z

Hi @Turini,

Why not put assertAbsenceOfErrors
https://github.com/renanigt/vraptor4/blob/validationIgnoredOnRedirects/vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java#L121
method inside of Validator or another class? It's a little bit weird having
this method inside of Messages. Maybe I'm missing something, but what do
you think?

On Fri, Aug 29, 2014 at 8:34 AM, Rodrigo Turini notifications@github.com
wrote:

I just think we should avoid it if possible!

I agree about avoiding it, but to use Instance<?> will change nothing.
We need a better design for this solution. What do you suggest?

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

Turini · 2014-08-29T11:59:49Z

extracting assertAbsenceOfErrors to a new bean can be better, but it won't
avoid the cyclic dependency, it'll just hide it :) makes sense? this is because the
Messages will need the new component that will need the Validator that need
Messages again. What do you think about it @lucascs and @garcia-jj?

rponte · 2014-08-29T12:06:34Z

Turini,

What I mean is that the place of assertAbsenceOfErrors method seems like
it's not in the right class. Which class is invoking the
assertAbsenceOfErrors method?

On Fri, Aug 29, 2014 at 8:59 AM, Rodrigo Turini notifications@github.com
wrote:

extracting assertAbsenceOfErrors to a new bean can be better, but it
won't
avoid the cyclic dependency, it'll just hide it :) makes sense? this is
because the
Messages will need the new component that will need the Validator that
need
Messages again. What do you think about it @lucascs
https://github.com/lucascs and @garcia-jj https://github.com/garcia-jj?

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt · 2014-08-29T12:09:03Z

Thanks for the explanation @Turini and @rponte !

@rponte assertAbsenceOfErrors is invoked by DefaultResult and ExecuteMethod.

rponte · 2014-08-29T12:14:45Z

Hi @renanigt,

As you can see, DefaultResult and ExecuteMethod use Messages only to
call assertAbsenceOfErrors
method, when they could use Validator instead. This way we could put
assertAbsenceOfErrors method inside Validator and avoid circular dependency
at all.

What do you think?

On Fri, Aug 29, 2014 at 9:09 AM, Renan Montenegro notifications@github.com
wrote:

Thanks for the explanation @Turini https://github.com/Turini and @rponte
https://github.com/rponte !

@rponte https://github.com/rponte assertAbsenceOfErrors is invoked by
DefaultResult and ExecuteMethod.

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt · 2014-08-29T12:17:39Z

@rponte makes sense for me. I agree with you.

What do tou think @lucascs @Turini @garcia-jj ?

rponte · 2014-08-29T12:18:08Z

Humm.. I see now, Validator uses Result too so we end up with another
circular dependency. I think we should extract a new class just to verify
the absence of errors and use it wherever it's needed.

On Fri, Aug 29, 2014 at 9:14 AM, Rafael Ponte rponte@gmail.com wrote:

Hi @renan,

As you can see, DefaultResult and ExecuteMethod use Messages only to call assertAbsenceOfErrors
method, when they could use Validator instead. This way we could put
assertAbsenceOfErrors method inside Validator and avoid circular
dependency at all.

What do you think?

On Fri, Aug 29, 2014 at 9:09 AM, Renan Montenegro <
notifications@github.com> wrote:

Thanks for the explanation @Turini https://github.com/Turini and
@rponte https://github.com/rponte !

@rponte https://github.com/rponte assertAbsenceOfErrors is invoked by
DefaultResult and ExecuteMethod.

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt · 2014-08-29T12:19:15Z

@rponte True, I didn't see that.

rponte · 2014-08-29T12:22:03Z

Well, dealing with circular dependency it's not so easy as we thought :-)

On Fri, Aug 29, 2014 at 9:19 AM, Renan Montenegro notifications@github.com
wrote:

@rponte https://github.com/rponte True, I didn't see that.

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

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt · 2014-08-29T12:31:36Z

I think the best solution is extract the assertAbsenceOfErrors method to a new bean.

@Turini if we extract it to a new bean, Messages don't will need the new component since the assertAbsenceOfErrors method is invoked just by DefaultResult and ExecuteMethod.

renan · 2014-08-29T12:59:26Z

Wrong @renan?

renanigt · 2014-08-29T13:03:17Z

Wrong @renan?

I think so. 😃

lucascs · 2014-08-29T14:28:54Z

vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java

+				} catch (ValidationException e) {
+					log.debug("Some validation errors occured: {}", e.getErrors());
+				}
+			}


you don't need validator here.

you already have the errors (getErrors()), just use it.

and remove validator dependency.

no more cyclic dependency ;)

you can change this whole if on line 123 to:

log.debug("Some validation errors occured: {}", getErrors());

slf4j already does the if internally.

@lucascs what about validator.onErrorUse(nothing()); ?

True @lucascs, now I see your point. 😃

Thanks.

it's from the time we didn't have validator.getErrors() method, so we had to catch the exception to be able to get the errors. VRaptor 3 inheritance ;)

Thanks a lot @lucascs. 😃

renanigt · 2014-08-29T17:17:59Z

Done. What do you think ? @lucascs @Turini @garcia-jj @rponte

lucascs · 2014-08-29T17:22:40Z

vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultResultTest.java

@@ -238,7 +248,7 @@ public void shouldDelegateToStatusOnPermanentlyRedirectToControllerInstance() th
 		result.permanentlyRedirectTo(new RandomController());

 		verify(status).movedPermanentlyTo(RandomController.class);
-
+		verify(messages).assertAbsenceOfErrors();


leave this verify only for the method which is checking this behavior. No need to repeat it on all.

renanigt · 2014-08-29T18:34:50Z

Done @lucascs !

renanigt · 2014-08-29T23:15:32Z

For @lucascs this issue can be merged.

What do you think ? @Turini @garcia-jj

Turini · 2014-08-30T18:42:44Z

sounds nice, thanks

Fixing problem: validation errors ignored on redirects. Closes #476

Fixing problem: validation ignored on redirects.

cff51ec

renanigt mentioned this pull request Aug 2, 2014

Validation error is ignored on redirects #476

Closed

renanigt changed the title ~~Fixing problem: validation ignored on redirects. Closes #476~~ Fixing problem: validation errors ignored on redirects. Closes #476 Aug 2, 2014

garcia-jj reviewed Aug 2, 2014
View reviewed changes

Importing the correct IllegalStateException.

c96221c

Turini added the bug label Aug 4, 2014

lucascs reviewed Aug 5, 2014
View reviewed changes

Fixing indentation.

f543fa9

renanigt mentioned this pull request Aug 5, 2014

unwrapping cause of ReflectionProviderException #729

Merged

renanigt added 2 commits August 6, 2014 01:07

Checking for validator erros on method use from Result.

07912a9

Minor refactoring.

64c3c07

Improvements based on Lucas's suggestions.

9869b7c

lucascs reviewed Aug 29, 2014
View reviewed changes

Improvements based on Lucas's suggestion.

ae22e99

lucascs reviewed Aug 29, 2014
View reviewed changes

Improving tests.

bb4c60a

Turini added a commit that referenced this pull request Aug 30, 2014

Merge pull request #725 from renanigt/validationIgnoredOnRedirects

dbe7799

Fixing problem: validation errors ignored on redirects. Closes #476

Turini merged commit dbe7799 into caelum:master Aug 30, 2014

renanigt deleted the validationIgnoredOnRedirects branch September 2, 2014 12:46

Fixing problem: validation errors ignored on redirects. Closes #476 #725

Fixing problem: validation errors ignored on redirects. Closes #476 #725

Conversation

renanigt commented Aug 2, 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

lucascs commented Aug 5, 2014

renanigt commented Aug 5, 2014

renanigt commented Aug 5, 2014

lucascs commented Aug 5, 2014

renanigt commented Aug 5, 2014

renanigt commented Aug 6, 2014

lucascs commented Aug 6, 2014

renanigt commented Aug 6, 2014

lucascs commented Aug 6, 2014

renanigt commented Aug 6, 2014

renanigt commented Aug 6, 2014

lucascs commented Aug 6, 2014

renanigt commented Aug 6, 2014

lucascs commented Aug 6, 2014

renanigt commented Aug 6, 2014

Turini commented Aug 6, 2014

renanigt commented Aug 6, 2014

Turini commented Aug 6, 2014

renanigt commented Aug 6, 2014

renanigt commented Aug 29, 2014

renanigt commented Aug 29, 2014

Turini commented Aug 29, 2014

rponte commented Aug 29, 2014

Turini commented Aug 29, 2014

rponte commented Aug 29, 2014

Turini commented Aug 29, 2014

rponte commented Aug 29, 2014

renanigt commented Aug 29, 2014

rponte commented Aug 29, 2014

renanigt commented Aug 29, 2014

rponte commented Aug 29, 2014

renanigt commented Aug 29, 2014

rponte commented Aug 29, 2014

renanigt commented Aug 29, 2014

renan commented Aug 29, 2014

renanigt commented Aug 29, 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

renanigt commented Aug 29, 2014

Choose a reason for hiding this comment

renanigt commented Aug 29, 2014

renanigt commented Aug 29, 2014

Turini commented Aug 30, 2014