dev@javaserverfaces.java.net

Re: Seeking Review: 1210 P2 Implicit Nav Query String Bug

From: Ryan Lubke <Ryan.Lubke_at_Sun.COM>
Date: Mon, 20 Jul 2009 09:43:22 -0700

Comments added to the issue.

On 7/18/09 7:03 PM, Ed Burns wrote:
> https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1210
>
> Issue: 1210
>
> This patch fixes several problems related to implicit navigation and
> redirect.
>
> SECTION: Modified Files
> ----------------------------
> M jsf-ri/src/com/sun/faces/application/NavigationHandlerImpl.java
>
> - REDIRECT_EQUALS_TRUE, INCLUDE_VIEW_PARAMS_EQUALS_TRUE
>
> As far as I can tell, the existing regexp didn't do exactly as
> expected. The simplified regexp, in concert with other changes, does.
>
> - Here's the big ticket item in this change-bundle, the bit about the questionMark.
>
> From what I can tell, this wasn't catching the question mark in the
> case of an actual implicit navigation. I've added a systest for this
> case.
>
> - Also, we need to gracefully handle the pathological case of the empty
> query string. This does so and includes a ProjectStage.Development too.
>
> - The two finds for the REDIRECT_EQUALS_TRUE and
> INCLUDE_VIEW_PARAMS_EQUALS_TRUE are independent. Fix the if to make
> them so.
>
> - Modify the m.group() to account for the new regexps
>
> M jsf-ri/src/com/sun/faces/LogStrings.properties
>
> - Add
>
> +jsf.navigation_invalid_query_string=JSF1069: Invalid query string in navigation outcome {0}
>
> I hope it's ok to just tack a new JSF**** message on to the end.
>
> M jsf-ri/src/com/sun/faces/resources/Messages.properties
> M jsf-ri/src/com/sun/faces/resources/Messages_en.properties
> M jsf-ri/src/com/sun/faces/resources/Messages_fr.properties
> M jsf-ri/src/com/sun/faces/resources/Messages_es.properties
> M jsf-ri/src/com/sun/faces/resources/Messages_de.properties
> M jsf-ri/src/com/sun/faces/util/MessageUtils.java
>
> - Add
>
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
>
> M jsf-ri/systest/src/com/sun/faces/systest/implicitnav/ImplicitNavigationTestCase.java
> A jsf-ri/systest/web/implicitnav/implicitNavRedirect.xhtml
> A jsf-ri/systest/web/implicitnav/implicitNavRedirect02.xhtml
>
> - automated test.
>
> SECTION: Diffs
>
> Index: jsf-ri/src/com/sun/faces/LogStrings.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/LogStrings.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/LogStrings.properties (working copy)
> @@ -113,6 +113,7 @@
> jsf.cannot_instantiate_component_error=JSF1068: Cannot instantiate component with component-type {0}
> jsf.application.legacy_facelet_viewhandler_detected=JSF1069: Disabling the JSF 2.0 Facelets ViewHandler as an older FaceletViewHandler, {0}, has been explicitly configured. \
> If this is not desired behavior, remove the older FaceletViewHandler and library from your application.
> +jsf.navigation_invalid_query_string=JSF1069: Invalid query string in navigation outcome {0}
> # the following three messages are duplicated in javax.faces.LogStrings.properties
> jsf.context.exception.handler.log_before={0} caught during beforePhase() processing of {1} : UIComponent-ClientId={2}, Message={3}
> jsf.context.exception.handler.log_after={0} caught during afterPhase() processing of {1} : UIComponent-ClientId={2}, Message={3}
> Index: jsf-ri/src/com/sun/faces/application/NavigationHandlerImpl.java
> ===================================================================
> --- jsf-ri/src/com/sun/faces/application/NavigationHandlerImpl.java (revision 7557)
> +++ jsf-ri/src/com/sun/faces/application/NavigationHandlerImpl.java (working copy)
> @@ -91,8 +91,8 @@
> * Flag indicated the current mode.
> */
> private boolean development;
> - private static final Pattern REDIRECT_EQUALS_TRUE = Pattern.compile("(?:\\?|&amp;|&)(faces-redirect=true(&|$))");
> - private static final Pattern INCLUDE_VIEW_PARAMS_EQUALS_TRUE = Pattern.compile("(?:\\?|&amp;|&)(includeViewParams=true(&|$))");
> + private static final Pattern REDIRECT_EQUALS_TRUE = Pattern.compile("(.*)(faces-redirect=true)(.*)");
> + private static final Pattern INCLUDE_VIEW_PARAMS_EQUALS_TRUE = Pattern.compile("(.*)(includeViewParams=true)(.*)");
>
>
> // ------------------------------------------------------------ Constructors
> @@ -467,23 +467,40 @@
> int questionMark = viewIdToTest.indexOf('?');
> String queryString;
> if (-1 != questionMark) {
> - queryString = viewIdToTest.substring(questionMark);
> - viewIdToTest = viewIdToTest.substring(0, questionMark);
> + int viewIdLen = viewIdToTest.length();
> + if (viewIdLen<= (questionMark+1)) {
> + if (logger.isLoggable(Level.SEVERE)) {
> + logger.log(Level.SEVERE, "jsf.navigation_invalid_query_string",
> + viewIdToTest);
> + }
> + if (development) {
> + String key;
> + Object[] params;
> + key = MessageUtils.NAVIGATION_INVALID_QUERY_STRING_ID;
> + params = new Object[]{viewIdToTest};
> + FacesMessage m = MessageUtils.getExceptionMessage(key, params);
> + m.setSeverity(FacesMessage.SEVERITY_WARN);
> + context.addMessage(null, m);
> + }
> + queryString = null;
> + viewIdToTest = viewIdToTest.substring(0, questionMark);
> + } else {
> + queryString = viewIdToTest.substring(questionMark + 1);
> + viewIdToTest = viewIdToTest.substring(0, questionMark);
>
> - Matcher m = REDIRECT_EQUALS_TRUE.matcher(queryString);
> - if (m.find()) {
> - isRedirect = true;
> - queryString = queryString.replace(m.group(1), "");
> -
> + Matcher m = REDIRECT_EQUALS_TRUE.matcher(queryString);
> + if (m.find()) {
> + isRedirect = true;
> + queryString = queryString.replace(m.group(2), "");
> + }
> m = INCLUDE_VIEW_PARAMS_EQUALS_TRUE.matcher(queryString);
> if (m.find()) {
> isIncludeViewParams = true;
> - queryString = queryString.replace(m.group(1), "");
> + queryString = queryString.replace(m.group(2), "");
> }
> }
>
> if (queryString != null&& queryString.length()> 0) {
> - queryString = queryString.substring(1);
> String[] queryElements = Util.split(queryString, "&amp;|&");
> for (int i = 0, len = queryElements.length; i< len; i ++) {
> String[] elements = Util.split(queryElements[i], "=");
> Index: jsf-ri/src/com/sun/faces/resources/Messages.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/resources/Messages.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/resources/Messages.properties (working copy)
> @@ -112,6 +112,7 @@
> com.sun.faces.MODELUPDATE_ERROR=Model Update failure for value ''{0}'' in model''{1}''.
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME=Unable to find matching navigation case from view ID ''{0}'' for outcome ''{1}''
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION=Unable to find matching navigation case from view ID ''{0}'' for action ''{1}'' with outcome ''{2}''
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
> com.sun.faces.NAMED_OBJECT_NOT_FOUND_ERROR=Expression Error: Named Object: ''{0}'' not found.
> com.sun.faces.NOT_NESTED_IN_FACES_TAG_ERROR=Not nested in a UIComponentTag Error for tag with handler class: ''{0}''.
> com.sun.faces.NOT_NESTED_IN_TYPE_TAG_ERROR=The {0} tag should be nested within a tag that is associated with a component of type {1}.
> Index: jsf-ri/src/com/sun/faces/resources/Messages_en.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/resources/Messages_en.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/resources/Messages_en.properties (working copy)
> @@ -113,6 +113,7 @@
> com.sun.faces.NAMED_OBJECT_NOT_FOUND_ERROR=Expression Error: Named Object: ''{0}'' not found.
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME=Unable to find matching navigation case from view ID ''{0}'' for outcome ''{1}''
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION=Unable to find matching navigation case from view ID ''{0}'' for action ''{1}'' with outcome ''{2}''
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
> com.sun.faces.NOT_NESTED_IN_FACES_TAG_ERROR=Not nested in a UIComponentTag Error for tag with handler class: ''{0}''.
> com.sun.faces.NOT_NESTED_IN_TYPE_TAG_ERROR=The {0} tag should be nested within a tag that is associated with a component of type {1}.
> com.sun.faces.NO_DTD_FOUND_ERROR=Unable to locate DTD with PUBLIC ID ''{0}'' at path ''{1}''.
> @@ -168,4 +169,4 @@
>
> com.sun.faces.JS_RESOURCE_WRITING_ERROR=Can''t write the JavaScript file to client.
>
> -com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> \ No newline at end of file
> +com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> Index: jsf-ri/src/com/sun/faces/resources/Messages_fr.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/resources/Messages_fr.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/resources/Messages_fr.properties (working copy)
> @@ -105,6 +105,7 @@
> com.sun.faces.NULL_CONFIGURATION=No Configuration loaded for the application.
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME=Unable to find matching navigation case from view ID ''{0}'' for outcome ''{1}''
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION=Unable to find matching navigation case from view ID ''{0}'' for action ''{1}'' with outcome ''{2}''
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
> com.sun.faces.ERROR_OPENING_FILE=Can''t open configuration file: ''{0}''.
> com.sun.faces.ERROR_REGISTERING_DTD=Can''t register DTD: ''{0}''.
> com.sun.faces.INVALID_INIT_PARAM=Invalid value: ''{0}'', for initialization parameter: ''{1}''. Acceptable values are 'true' or 'false'. Defaulting to 'false'.
> @@ -164,4 +165,4 @@
> com.sun.faces.JS_RESOURCE_WRITING_ERROR=Can''t write the JavaScript file to client.
> com.sun.faces.EVAL_ATTR_UNEXPECTED_TYPE=Evaluation of expression for attribute ''{0}'' resulted in unexpected type. Expected {1}, but received {2}.
>
> -com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> \ No newline at end of file
> +com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> Index: jsf-ri/src/com/sun/faces/resources/Messages_es.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/resources/Messages_es.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/resources/Messages_es.properties (working copy)
> @@ -105,6 +105,7 @@
> com.sun.faces.NULL_CONFIGURATION=No Configuration loaded for the application.
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME=Unable to find matching navigation case from view ID ''{0}'' for outcome ''{1}''
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION=Unable to find matching navigation case from view ID ''{0}'' for action ''{1}'' with outcome ''{2}''
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
> com.sun.faces.ERROR_OPENING_FILE=Can''t open configuration file: ''{0}''.
> com.sun.faces.ERROR_REGISTERING_DTD=Can''t register DTD: ''{0}''.
> com.sun.faces.INVALID_INIT_PARAM=Invalid value: ''{0}'', for initialization parameter: ''{1}''. Acceptable values are 'true' or 'false'. Defaulting to 'false'.
> @@ -165,4 +166,4 @@
> com.sun.faces.JS_RESOURCE_WRITING_ERROR=Can''t write the JavaScript file to client.
> com.sun.faces.EVAL_ATTR_UNEXPECTED_TYPE=Evaluation of expression for attribute ''{0}'' resulted in unexpected type. Expected {1}, but received {2}.
>
> -com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{1}'' component has been defined within the view.
> \ No newline at end of file
> +com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{1}'' component has been defined within the view.
> Index: jsf-ri/src/com/sun/faces/resources/Messages_de.properties
> ===================================================================
> --- jsf-ri/src/com/sun/faces/resources/Messages_de.properties (revision 7557)
> +++ jsf-ri/src/com/sun/faces/resources/Messages_de.properties (working copy)
> @@ -103,6 +103,7 @@
> com.sun.faces.CONFIG_RENDERER_REGISTRATION_MISSING_RENDERKIT=Unable to register renderers with RenderKit using id {0}. RenderKit does not exist.
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME=Unable to find matching navigation case from view ID ''{0}'' for outcome ''{1}''
> com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION=Unable to find matching navigation case from view ID ''{0}'' for action ''{1}'' with outcome ''{2}''
> +com.sun.faces.NAVIGATION_INVALID_QUERY_STRING=Invalid query string in outcome''{0}''
> com.sun.faces.NULL_CONFIGURATION=No Configuration loaded for the application.
> com.sun.faces.ERROR_OPENING_FILE=Can''t open configuration file: ''{0}''.
> com.sun.faces.ERROR_REGISTERING_DTD=Can''t register DTD: ''{0}''.
> @@ -164,4 +165,4 @@
> com.sun.faces.JS_RESOURCE_WRITING_ERROR=Can''t write the JavaScript file to client.
> com.sun.faces.EVAL_ATTR_UNEXPECTED_TYPE=Evaluation of expression for attribute ''{0}'' resulted in unexpected type. Expected {1}, but received {2}.
>
> -com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> \ No newline at end of file
> +com.sun.faces.RESOURCE_TARGET_NOT_AVAILABLE="One or more resources have the target of ''{0}'', but no ''{0}'' component has been defined within the view.
> Index: jsf-ri/src/com/sun/faces/util/MessageUtils.java
> ===================================================================
> --- jsf-ri/src/com/sun/faces/util/MessageUtils.java (revision 7557)
> +++ jsf-ri/src/com/sun/faces/util/MessageUtils.java (working copy)
> @@ -304,6 +304,8 @@
> "com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME";
> public static final String NAVIGATION_NO_MATCHING_OUTCOME_ACTION_ID =
> "com.sun.faces.NAVIGATION_NO_MATCHING_OUTCOME_ACTION";
> + public static final String NAVIGATION_INVALID_QUERY_STRING_ID =
> + "com.sun.faces.NAVIGATION_INVALID_QUERY_STRING";
> public static final String OUTCOME_TARGET_BUTTON_NO_MATCH =
> "com.sun.faces.OUTCOME_TARGET_BUTTON_NO_MATCH";
> public static final String OUTCOME_TARGET_LINK_NO_MATCH =
> Index: jsf-ri/systest/src/com/sun/faces/systest/implicitnav/ImplicitNavigationTestCase.java
> ===================================================================
> --- jsf-ri/systest/src/com/sun/faces/systest/implicitnav/ImplicitNavigationTestCase.java (revision 7557)
> +++ jsf-ri/systest/src/com/sun/faces/systest/implicitnav/ImplicitNavigationTestCase.java (working copy)
> @@ -42,6 +42,8 @@
> import com.gargoylesoftware.htmlunit.html.HtmlSubmitInput;
> import com.sun.faces.htmlunit.AbstractTestCase;
> import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
> +import com.gargoylesoftware.htmlunit.html.HtmlButtonInput;
> +import com.gargoylesoftware.htmlunit.html.HtmlTextInput;
>
> public class ImplicitNavigationTestCase extends AbstractTestCase {
>
> @@ -150,4 +152,62 @@
>
>
> }
> +
> +
> + public void testImplicitNavigationWithredirect() throws Exception {
> +
> + HtmlPage page = getPage("/faces/implicitnav/implicitNavRedirect.xhtml");
> + String text;
> + final String SEARCH_TEXT = "SEARCH_TEXT";
> +
> + // case 1, h:button, make sure the input value is lost.
> + HtmlTextInput input = (HtmlTextInput) page.getElementById("input");
> + input.setValueAttribute(SEARCH_TEXT);
> + HtmlButtonInput buttonButton = (HtmlButtonInput) page.getElementById("httpGet");
> +
> + page = buttonButton.click();
> + text = page.asText();;
> + assertTrue(!text.contains(SEARCH_TEXT));
> +
> + // case 2 h:commandButton that does redirect. Make sure a redirect is
> + // performed and the value is lost.
> + client.setRedirectEnabled(false);
> + boolean exceptionThrown = false;
> + page = getPage("/faces/implicitnav/implicitNavRedirect.xhtml");
> + input = (HtmlTextInput) page.getElementById("input");
> + input.setValueAttribute(SEARCH_TEXT);
> + HtmlSubmitInput submitButton = (HtmlSubmitInput) page.getElementById("httpPostRedirect");
> + try {
> + page = submitButton.click();
> + } catch (FailingHttpStatusCodeException e) {
> + assertEquals(302, e.getStatusCode());
> + exceptionThrown = true;
> + }
> + assertTrue(exceptionThrown);
> + client.setRedirectEnabled(true);
> + page = submitButton.click();
> + text = page.asText();;
> + assertTrue(!text.contains(SEARCH_TEXT));
> +
> + // case 3 h:commandButton with empty query string
> + page = getPage("/faces/implicitnav/implicitNavRedirect.xhtml");
> + input = (HtmlTextInput) page.getElementById("input");
> + input.setValueAttribute(SEARCH_TEXT);
> + submitButton = (HtmlSubmitInput) page.getElementById("httpPostInvalidQueryString");
> + page = submitButton.click();
> + text = page.asText();
> + assertTrue(text.contains(SEARCH_TEXT));
> + assertTrue(text.contains("Invalid query string in outcome\'implicitNavRedirect02?\'"));
> +
> + // case 4: h:commandButton, regular post.
> + page = getPage("/faces/implicitnav/implicitNavRedirect.xhtml");
> + input = (HtmlTextInput) page.getElementById("input");
> + input.setValueAttribute(SEARCH_TEXT);
> + submitButton = (HtmlSubmitInput) page.getElementById("httpPost");
> + page = submitButton.click();
> + text = page.asText();;
> + assertTrue(text.contains(SEARCH_TEXT));
> +
> + }
> +
> }
> Index: jsf-ri/systest/web/implicitnav/implicitNavRedirect.xhtml
> ===================================================================
> --- jsf-ri/systest/web/implicitnav/implicitNavRedirect.xhtml (revision 0)
> +++ jsf-ri/systest/web/implicitnav/implicitNavRedirect.xhtml (revision 0)
> @@ -0,0 +1,17 @@
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml"
> + xmlns:h="http://java.sun.com/jsf/html"
> + xmlns:f="http://java.sun.com/jsf/core">
> +<h:head>
> +<title>A Simple JavaServer Faces 2.0 View</title>
> +</h:head>
> +<h:body>
> +<h:form prependId="false">
> +<p>Enter first name:<h:inputText id="input" value="#{test1.stringProperty}" /></p>
> +<p><h:button id="httpGet" value="HTTP GET" outcome="implicitNavRedirect02" /></p>
> +<p><h:commandButton id="httpPostRedirect" value="HTTP POST with Redirect" action="implicitNavRedirect02?faces-redirect=true" /></p>
> +<p><h:commandButton id="httpPostInvalidQueryString" value="HTTP POST with invalid query string" action="implicitNavRedirect02?" /></p>
> +<p><h:commandButton id="httpPost" value="HTTP POST" action="implicitNavRedirect02" /></p>
> +</h:form>
> +</h:body>
> +</html>
> Index: jsf-ri/systest/web/implicitnav/implicitNavRedirect02.xhtml
> ===================================================================
> --- jsf-ri/systest/web/implicitnav/implicitNavRedirect02.xhtml (revision 0)
> +++ jsf-ri/systest/web/implicitnav/implicitNavRedirect02.xhtml (revision 0)
> @@ -0,0 +1,15 @@
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml"
> + xmlns:h="http://java.sun.com/jsf/html">
> +<h:head>
> +<title>Page 02</title>
> +</h:head>
> +<h:body>
> +<p>Page 02</p>
> +<p>Value of First name:&lt;#{test1.stringProperty}&gt;.</p>
> +
> +<p>The value will be empty if either of the "HTTP POST with Redirect" or the
> + "HTTP GET" buttons were pressed. If the "HTTP POST" button was pressed,
> + the value will not be empty.</p>
> +</h:body>
> +</html>
>
>