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.

Sign up for GitHub

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

checking if result is used on DefaultLogicResult #804

Merged
merged 3 commits into from
Sep 22, 2014
Merged

checking if result is used on DefaultLogicResult #804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec28ad3
checking if result is used DefaultLogicResult
Turini Sep 18, 2014
ae9ec35
adding a debug log
Turini Sep 22, 2014
9e65b7f
simple test case
Turini Sep 22, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions vraptor-core/src/main/java/br/com/caelum/vraptor/view/DefaultLogicResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.slf4j.LoggerFactory;

import br.com.caelum.vraptor.Get;
import br.com.caelum.vraptor.Result;
import br.com.caelum.vraptor.controller.ControllerMethod;
import br.com.caelum.vraptor.controller.DefaultControllerMethod;
import br.com.caelum.vraptor.controller.HttpMethod;
Expand Down Expand Up @@ -68,17 +69,20 @@ public class DefaultLogicResult implements LogicResult {
private final FlashScope flash;
private final MethodInfo methodInfo;

private Result result;

/**
* @deprecated CDI eyes only
*/
protected DefaultLogicResult() {
this(null, null, null, null, null, null, null, null, null);
this(null, null, null, null, null, null, null, null, null, null);
}

@Inject
public DefaultLogicResult(Proxifier proxifier, Router router, MutableRequest request, HttpServletResponse response,
Container container, PathResolver resolver, TypeNameExtractor extractor, FlashScope flash, MethodInfo methodInfo) {
Container container, PathResolver resolver, TypeNameExtractor extractor, FlashScope flash, MethodInfo methodInfo, Result result) {
this.proxifier = proxifier;
this.result = result;
this.response = unproxifyIfPossible(response);
this.request = unproxifyIfPossible(request);
this.router = router;
Expand All @@ -97,24 +101,26 @@ public DefaultLogicResult(Proxifier proxifier, Router router, MutableRequest req
@Override
public <T> T forwardTo(final Class<T> type) {
return proxifier.proxify(type, new MethodInvocation<T>() {

@Override
public Object intercept(T proxy, Method method, Object[] args, SuperMethod superMethod) {
try {
logger.debug("Executing {}", method);
ControllerMethod old = methodInfo.getControllerMethod();
methodInfo.setControllerMethod(DefaultControllerMethod.instanceFor(type, method));
Object result = method.invoke(container.instanceFor(type), args);
Object methodResult = method.invoke(container.instanceFor(type), args);
methodInfo.setControllerMethod(old);

Type returnType = method.getGenericReturnType();
if (!(returnType == void.class)) {
request.setAttribute(extractor.nameFor(returnType), result);
request.setAttribute(extractor.nameFor(returnType), methodResult);
}
if (!response.isCommitted()) {
String path = resolver.pathFor(DefaultControllerMethod.instanceFor(type, method));
logger.debug("Forwarding to {}", path);
request.getRequestDispatcher(path).forward(request, response);
if (response.isCommitted() || result.used()) {
Copy link

Choose a reason for hiding this comment

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

result.used() always returns true.. no more forward :(

Copy link
Member

Choose a reason for hiding this comment

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

You're right! 💣 💣 💣

Do you want to open a pull request fixing this?

Copy link

Choose a reason for hiding this comment

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

sure! tks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really needed checking for result.used() ?

Atual VRaptor's version probably is breanking many application since forward isn't working.

return null;
Copy link
Member

Choose a reason for hiding this comment

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

I'd log debug here, at least.

}
String path = resolver.pathFor(DefaultControllerMethod.instanceFor(type, method));
logger.debug("Forwarding to {}", path);
request.getRequestDispatcher(path).forward(request, response);
return null;
} catch (InvocationTargetException e) {
propagateIfPossible(e.getCause());
Expand Down
4 changes: 3 additions & 1 deletion vraptor-core/src/test/java/br/com/caelum/vraptor/view/DefaultLogicResultTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,13 @@ public class DefaultLogicResultTest {
private @Mock TypeNameExtractor extractor;
private @Mock RequestDispatcher dispatcher;
private @Mock FlashScope flash;
private @Mock Result result;

private Proxifier proxifier;

private MethodInfo methodInfo;


public static class MyComponent {
int calls = 0;

Expand Down Expand Up @@ -114,7 +116,7 @@ public void setup() {
proxifier = new JavassistProxifier();
methodInfo = new MethodInfo(new ParanamerNameProvider());
this.logicResult = new DefaultLogicResult(proxifier, router, request, response, container,
resolver, extractor, flash, methodInfo);
resolver, extractor, flash, methodInfo, result);
Copy link
Member

Choose a reason for hiding this comment

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

no tests for result.used?

Copy link

Choose a reason for hiding this comment

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

E isso quebou o forward :'(

Copy link
Contributor

Choose a reason for hiding this comment

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

Qual o problema com o forward ?

Copy link

Choose a reason for hiding this comment

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

Sempre que eu faço o forward o used vem true.

Copy link
Member

Choose a reason for hiding this comment

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

@andytsunami Abra uma issue explicando o cenário completo para que possamos entender o que acontece. Ou se preferir, mande um email para a nossa mailing list.

Copy link

Choose a reason for hiding this comment

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

Abri a issue #825
Obrigado pela atenção dispensada.

}

@Test
Expand Down