Re: Code Review Request: (JAVASERVERFACES-3593) Navigation from method-call-node to method-call-node does not work

From: zhijun Ren <>
Date: Wed, 21 Jan 2015 16:28:34 +0800

Hi Ed,

Please help to review the following changes(changebundle attached), once
you approved I will do delivery to make the Hudson build clean:

1. Putting back your additional changes in NavigationHandlerImpl but
with a little change of me, because
com.sun.faces.test.javaee6web.flowinxmlandjava.Spec730IT failed at a
StackOverFlowExcetion, the little change is in method findSwitchMatch()
as following:

/*boolean matched=false;*
                 for (SwitchCase cur : cases) {
                     if (cur.getCondition(context)) {
                         outcome = cur.getFromOutcome();
                  // 2. the default outcome
                 if (*!matched || *outcome == null) {
                     outcome = switchNode.getDefaultOutcome(context);

2. My another change is in
because of the typos of the original test author.

3. My third change is that the following 2 test cases(of that 25
navigation cases) are commented out because of the failure, I will
uncomment them once the issue are fixed.

There will be no failed tests related to my change after above change
committed cause 2 failed tests are removed.


On 1/21/15, 2:47, Edward Burns wrote:
> Hello Zhijun,
> Comments below.
>>>>>> On Mon, 19 Jan 2015 16:50:51 +0800, zhijun Ren<> said:
> ZR> Hi Ed and Manfred,
> ZR> I have committed the code, I need note something here to avoid surprising:
> Thanks. Let me say first that you have done exactly as we agreed in the
> meeting on Thursday.
> ZR> 1. For the fixing code in class NavigationHandlerImpl, I only keep the
> ZR> code that handle case that start from method call node and removed the
> ZR> code that handles the case start from switch node due to it fails the
> ZR> cases which starting from switch node both legacy and newly-added;
> Due to the noted complexity of this code, I'm going to ask that you put
> the removed code back, and hopefully it won't break any additional
> tests. If it does we must roll back the entire commit r14211. Having
> the code there may aid in the refactoring process which I discuss later
> in this email.
> ZR> 2. I have do the Hudson testing against weblogic and the failed tests are:
> ZR> /./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.263 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT
> ZR> ./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.167 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT
> ZR> ./javaee7/cdiBeanValidator/target/failsafe-reports/com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.txt:Tests
> ZR> run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.179 sec<<<
> ZR> FAILURE! - in com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT
> ZR> /
> ZR> 3. I have do the Hudson testing against glassfish and the failed tests are:
> ZR> /./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.231 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT
> ZR> ./javaee6web/flowTraversalCombinations/target/failsafe-reports/com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt:Tests
> ZR> run: 5, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.166 sec<<<
> ZR> FAILURE! - in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT/
> ACTION: => For your items 2 and 3, I know I had directed you to commit
> these known failing tests. Instead of doing that, please svn remove
> them and re-add them as you make them work while fixing issues 3694 and
> 3695.
> ZR> 4. Cause I didn't find failed test cases starting from method call node,
> ZR> so I set the bug #3593 as resolved;
> Yes, we agreed to this.
> ZR> 5. I filed jira bugs for the failed Tests:
> ZR> <>: for
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.ReturnNaviToOtherNodesIT.testReturnNaviToFlowCallNode
> ZR> <>: for tests in
> ZR> com.sun.faces.test.javaee6web.flowtraversalcombinations.SwitchNaviToOtherNodesIT.txt/
> We agreed to this as well.
> ZR> /6. about com.sun.faces.test.javaee7.cdibeanvalidator.Issue3014IT.txt
> ZR> for Weblogic, I will observe the result in Hudson today, if still
> ZR> failed, I will file a bug also.
> Can you please explain how you came upon this observation? Was it
> failing before and now suddenly started working?
>>>>>> On Tue, 20 Jan 2015 14:14:18 +0800, zhijun Ren<> said:
> ZR> When I tried to fix #3694 and #3695, I found there are lot bad smells in
> ZR>, such as duplicated code and many if-else
> ZR> statements.
> Yes, you are right. The flow related code in NavigationHandler is
> indeed a source of technical debt. I agree it needs to be refactored
> into a state machine.
> ZR> The biggest concern for me is there is many recursive calls, directly or
> ZR> indirectly, which can easily cause Stack Overflow error or exceptions.
> ZR> The code does not obey OO design principles and is very fragile.
> Yes, I agree, it is fragile.
> ZR> So I think we need to refactor the code with State Design Pattern and
> ZR> other Patterns to remove the bad smells, do you agree?
> Yes. I would like to do this work in two big stages, with sub-stages as
> well.
> I. Incremental improvement: do this now
> 0. Put back removed code from commit of 3593, assuming it doesn't break
> any additional tests.
> 1. Get 3694 and 3695 working
> 2. Make it so there are no @Ignored flow related tests on trunk.
> II. Flow refactoring: do this later, in concert with MVC
> Zhijun, we are trying to see if the Faces Flow feature can be re-used
> via CDI from MVC. This work is still in the early discussion phase.
> It makes the most sense to do this technical-debt reducing refactoring
> in concert with this effort. However, at this point, I can still see
> some early work that can be done now.
> 1. Come up with a brief list of requirements. Chief among them must
> be the need to not change the behavior of the NavigationHandler for
> non flow related invocations. An equally important requirement is
> the need for the solution to work outside of JSF in MVC.
> 2. Come up with a state machine design that does work within the "JSF
> only" context, as a fallback in case the flow feature is dropped
> from MVC.
> So, please do take care of part I above now.
> Thanks, Zhijun.
> Ed