Fixed issue 733 Getting wrong path when trying to use logic forwardTo

[adolfoweloy]

The getFilteredControllerBaseName method was created to filter baseName that may be using proxified controller base name. Without this filter, extractControllerFromName wouldn't be able to extract Controller suffix.

[garcia-jj]

@adolfoweloy, thank you to help us with your pull request.

Your solution is very nice, but only coverage cases that proxy generated class uses *Controller name. But proxy can have other names, and we need to coverage this cases too.

There is a class called CDIProxies in the package br.com.caelum.vraptor.proxy. The method extractRawTypeIfPossible will extract the properly class to us.

So instead of extract the name using a regex, you can only change the line 68 of DefaultPathResolver to like this:

From: method.getController().getType().getSimpleName() to extractRawTypeIfPossible(method.getController().getType()).getSimpleName().

https://github.com/adolfoweloy/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/view/DefaultPathResolver.java#L68

What you think about?

Thank you.

[lucascs]

A better place to do this fix is right here:
https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/core/AbstractResult.java#L58-L75

instead of controller.getClass(), you can use the method @garcia-jj said.

[adolfoweloy]

I think it's really a better solution.
The change I've made will be stuck at that regex and if any new situation appears we would need to add new rules.
So what do you think about this at AbstractResult.java:

    @Override
    @SuppressWarnings("unchecked")
    public <T> T redirectTo(T controller) {
        return (T) redirectTo(CDIProxies.extractRawTypeIfPossible(controller.getClass()));
    }

    @Override
    @SuppressWarnings("unchecked")
    public <T> T forwardTo(T controller) {
        return (T) forwardTo(CDIProxies.extractRawTypeIfPossible(controller.getClass()));
    }

    @Override
    @SuppressWarnings("unchecked")
    public <T> T of(T controller) {
        return (T) of(CDIProxies.extractRawTypeIfPossible(controller.getClass()));
    }

I've just ran maven tests and and my app and that's ok

[garcia-jj]

Yes, your code looks nice. Can you commit this changes? So when you commit and push this changes, will appers here in this pull request.

[adolfoweloy]

Sorry @garcia-jj
I've made a new pull request using a new branch name. Any problem?

[garcia-jj]

There are way to add a text case for this code?

[lucascs]

It would be cool to add a test case.

you can try to use the proxifier to generate a proxy for a Controller and pass it to the method.

[adolfoweloy]

The class under test would be DefaultResult using a mocked Controller proxified by proxifier?

[lucascs]

You would add tests like these:
https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultResultTest.java#L136-L166

which don't use mocked controllers

[adolfoweloy]

I've added some tests like @lucascs suggested.
What do you think?

[lucascs]

actually you should verify this:

verify(logicResult).redirectTo(RandomController.class);

That's what you want.

[adolfoweloy]

Hello @lucascs.
I'm trying to figure out how to effectively test it but having no success :(
When verifying as you suggested, tests are failing. So I realize that I can't invoke redirectTo or forwardTo using a proxified controller through proxifier.proxify(RandomController.class).

When debugging an application executing at Wildfly8, I saw that our controller is an instance of something similar to ExampleController$Proxy$__$$_Weld****. And such an instance is created by invoking beanManager.getContext inside InstantiateObserver.

Do you think it's better to keep talking about it at caelum-vraptor-dev google group? Because I'm having doubts about the source code ;)

Thanks

[csokol]

That's because CDIProxies.unproxifyIfPossible is prepared to unproxify CDI proxies. In your test, you are using javassist to generate the proxy. Is it possible to build this proxy with Weld api? Or you can create a mock of TargetInstanceProxy.

[csokol]

Another possible solution would be to inject CDIProxies as a dependency and mock it in your test. But than extractRawTypeIfPossible should become an instance method. What do you think?

[lucascs]

My point was just to change this line from:

verify(logicResult).redirectTo(CDIProxies.extractRawTypeIfPossible(randomProxy.getClass()));

to

verify(logicResult).redirectTo(RandomController.class);

does the test break if you do this?

[csokol]

It breaks because randomProxy is not a CDI proxy, so the extractRawTypeIfPossible will return the proxyed class (something like RandomController$..javassist..etcetc), and not the unproxied RandomController.class

[Turini]

I've fixed it on this PR #740. After merging it I'll release a new VR version. ok?

[adolfoweloy]

Thanks Turini. I saw the fix and that sounds nice for me.

[Turini]

You're welcome :) I didn't suggest the fix here because we need this PR merged to release RC2, so I've provided a fast fix instead. Thanks a lot for your contribution.

[garcia-jj]

I guys. This pull request causes a StackOverflowError when I use a redirect like this:

public void store(Setting setting) {
    settingService.store(setting);
    result.redirectTo(this).edit();
}

Caused by: java.lang.StackOverflowError
    at java.io.PrintStream.println(PrintStream.java:824) [rt.jar:1.8.0]
    at org.jboss.stdio.StdioContext$DelegatingPrintStream.println(StdioContext.java:486)
    at br.com.caelum.vraptor.core.AbstractResult.redirectTo(AbstractResult.java:62) [vraptor-4.1.0-RC3-SNAPSHOT.jar:]

After some minutes I see that we forgot to add a cast to Class<T>. Without this, the current redirect(T controller) will redirect to yourself until a StackOverflowError happens. See this line: https://github.com/caelum/vraptor4/pull/734/files#diff-7158fea18fe672727e918c1d04830fe4R68

I'll send a pull request after some tests in my app.