javax.faces.ViewState + PPR does not work out for cross form ppr cases

[glassfishrobot]

Following problem

<h:form id="a">
    <h:commandButton action="#{TestBean.action}" value="submit"/>
</h:form>

<h:form id="b">
    <h:commandLink value="ajax ReRender" 
        onclick="jsf.ajax.request(this,event,{execute:'b a',render:'a'}); return false;"/>
</h:form>

Cannot work out because, the viewstate is returned as separate viewstate block
(in both implementations the does not pass the viewState in the
update block).

Now the specification says:

If an update element is found in the response with the identifier
javax.faces.ViewState:

<update id="javax.faces.ViewState">
   <![CDATA[...]]>
</update>

locate and update the submitting form's javax.faces.ViewState value with the
CDATA contents from the response.

Which means in this special case that the viewState for form a is lost.
Mojarra has fixed this to some degree by setting the viewstate if a direct form
render on a happens.

However if you do following:

<h:panelGroup id="myGroup">
    <h:form id="a">
        <h:commandButton action="#{TestBean.action}" value="submit"/>
    </h:form>
</h:panelGroup>

<h:form id="b">
    <h:commandLink value="ajax ReRender" 
        onclick="jsf.ajax.request(this,event,{execute:'b a',render:'myGroup'}); return false;"/>
</h:form>

Mojarra also fails.

The problem here lies clearly with the spec, I am not sure why the viewstate is
only updated to the issuing form.

Either all forms must be updated or at least the forms which are processed both
within the execute and render parts.

I also opened a discussion on the open mailing list regarding this, since this is
a usecase which can happen quite often in a typical rich client scenario where a
lot of detachments can happen to satisfy ie6 and multiple forms are the norm if
you have floating frames.

Environment

Operating System: All
Platform: Macintosh

Affected Versions

[2.0]

[glassfishrobot]

Reported by werpu12

[glassfishrobot]

Issue-Links:
is duplicated by
JAVASERVERFACES-3436
is related to
JAVASERVERFACES_SPEC_PUBLIC-1421
is related to
JAVASERVERFACES_SPEC_PUBLIC-1093
JAVASERVERFACES-1715

[glassfishrobot]

@edburns said:
Agree for inclusion in 2.0 Rev a

[glassfishrobot]

@edburns said:
take ownership.

[glassfishrobot]

@edburns said:
I agree we should update all forms. Move to 2.1.

[glassfishrobot]

@edburns said:
triage

[glassfishrobot]

@edburns said:
Change target milestone.

[glassfishrobot]

werpu12 said:
Actually I personally think the only possible solution here is to offload this
to the server, instead of issuing we have to
extend the protocol here so that the client is notified which form and viewstate
element needs to be updated.
I am not sure if updating all forms automatically really resolves the issue,
because in a portlet scenario this does not work out, how about client side
state saving or even worse if someone introduces multiple viewroots
programmatically just as portlets do?

[glassfishrobot]

rogerk said:
triage

[glassfishrobot]

frederickkaempfer said:
The same problem occurs (in Mojarra 2.1.3) when the entire ViewRoot is replaced via ajax. In that case none of the forms get their view state updated. You need to submit a form at least twice in order to trigger the execution of the full JSF lifecycle. The first time only a new ViewState field is created in the submitted form.

At least all forms contained in the rendered section need to have their view state field updated. In the current implementation a ViewState field is only created if the render target is it self a form (not a parent of forms).

Updating all forms on the page is also a possibility though strictly speaking forms not contained in the section of the Ajax request are not part of the newly generated view state.

Possible duplicates of this bug I found so far:
http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1024
http://java.net/jira/browse/JAVASERVERFACES-2199

[glassfishrobot]

frederickkaempfer said:
I have created a fix for the issue, which updates the ViewState field for forms which are children of the specified render targets.

Furthermore it handles the case where render="@ALL" is used. In this case all forms in the document will have their ViewState field updated. Previously this caused forms to loose their ViewState.

Note that this fix does not update all forms in the document, but only those which have been re-rendered (thus are part of the newly generated view on the server).

[glassfishrobot]

File: changebundle.txt
Attached By: frederickkaempfer

[glassfishrobot]

werpu12 said:
Actually I have similar fixes for myfaces, I basically update all forms which have render targets in or forms which are part of a render target.
Additionally to that I added a update all config option which is outside of the spec.
But nevertheless.

But that is just a hack in my opinion. The root problem is in the protocol.
The protocol simply needs to deliver a list of form ids which need a viewstate update.
So I personally think the only viable long term solution to this problem simply is to update the protocol of jsf 2.2 in this regar ala dkghsfkjgh or for backwards compatibility reasons introduce an entirely new tag like ....

[glassfishrobot]

rogerk said:
Your workarounds are fine for now, but clearly this needs to be a spec change for the better.

[glassfishrobot]

frederickkaempfer said:
If it was safe to assume that the ViewState does not change during a partial request the logic can be simplified by updating ALL forms in the document (not just render targets).

In Mojarra's ServerSideStateHelper (line 200) this assumption holds true, but I did not find anything in the spec enforcing this.

Additionally this would also take care of any Form dynamically added to the view or any render ids added via facesContext.getPartialViewContext().getRenderIds().add(...);

Right now only the "javax.faces.partial.render" argument is consulted which is indeed suboptimal.

I can change the workaround to simply update all forms in the page, which would also make it less of a "Hack". What do you think?

[glassfishrobot]

rogerk said:
I was thinking along those same lines..
In which case we would not need to introduce a new response element as Werner mentioned.
It's been sometime since we looked at this - it has been discussed before.
Are there any portlet implications (namespace related) by doing this (that was brought up - but could be for a different issue)..

[glassfishrobot]

werpu12 said:
Actually there are portlet implications, hence i stayed away from updating all forms automatically, but made it a config option you can turn on in myfaces.

The implication is that different portlets can host different forms under different viewroots and hence have different viewstates.
Now the main problem is, we dont have any viewroot information. So it comes down to following:
a) either send the viewstate info with an altered protocol i mentioned

• safe since every form must have a unique id in the dom no double ids are allowed

b) add viewroot information and then update all forms under the corresponding viewroot - should be safe as well but is in my opinion not as clean as option a) and probably causes an alteration of the protocol as well (ala introduction of a viewroot element) or on the api side. I am not sure if we have a scenario where we have different viewstates under one viewroot though. I never really bothered to look deeper into this aspect of the jsf spec.

c) Try to move forward with the usual hacks - worst option of all

In my personal experience this multi form issue has been a huge issue for the users, I had about 5-10 requests on the myfaces mailinglist where users had problems and thought it was an issue of the implementation.

[glassfishrobot]

frederickkaempfer said:
It's true that updating all forms won't work in any case where there are multiple viewroots involved. Anyhow I was hoping to find a solution that is usable even with the 2.1 and 2.0 spec versions, because this bug is not an enhancement, but currently breaks a couple of basic Ajax features in JSF. To summarize them quickly:

1. Specifying parents of forms as render targets.
2. Using the @ALL render target on any page with multiple forms.
3. Replacing the entire viewroot, for example via <h:commandLink action="myNavigationOutcome"><f:ajax/></h:commandLink>
4. Dynamically adding RenderIds that include forms in JSF Lifecycle with facesContext.getPartialViewContext().getRenderIds().add(...)

The workaround I provided should at least make 1-3 work again. Updating all forms in the document would make 1-4 work, but as you said will break Portlet compatibility.

There is another option to consider that does not require a protocol change:

Before the doUpdate function is called for any of the changes read the view state information at the end of the partial response document and save it in a variable in the jsf.ajax.response function. the viewstate can then be passed to any call of doUpdate function, which can then take care of creating view state fields where it is appropriate. This way we would not have to redundantly send the render ids, as they already provided as the ids.

[glassfishrobot]

frederickkaempfer said:
Here is a changebundle that updates the ViewState for each form element that is replaced during the ajax update.

I wonder if a Spec change is even necessary because: If a form is processed during an update it means that its ViewState field has to be updated (or created) as well. With this patch it is now done in the doUpdate function. On the other hand, if a form is not re-rendered there is no compelling reason to update its ViewState field, because if it had been changed on the server-side this would not be visible in the browser.

So following this logic, specifying the "updateIds" in a new protocol element will always replicate the render ids which are already contained in the tags (or any of the forms contained in them, which can also be found using JavaScript, like it is done in the provided patch).

Do you think this is still too "hack"-ish?

[glassfishrobot]

File: changebundle.txt
Attached By: frederickkaempfer

[glassfishrobot]

werpu12 said:
Unfortunately things are not that simple. I cannot speak for mojarra, but I assume it is similar, but myfaces has a meta information in the viewstate which also pinpoints to the viewid (and the state history as well) so if you do not update a second form even if you dont have a rerender there, you basically at a submit from that form can get a cannot find viewid once it drops out of the view history.

So we have two problems
a) update all forms automatically is prevented due to the fact that it breaks in a portlet environment

b) not updating not rendered forms might break the state history information of the viewstate not updated

So a pure javascript fixup will work and might work under normal non portlet circumstances (the simplest one probably is simply to update all forms for a normal webapp) but it will break in corner cases like portlet environments. Thats one of the reasons why I implemted about three fallback modes for this problem in myfaces. So that if even one fallback fails the user can switch to another one which might suite to his environment.

[glassfishrobot]

frederickkaempfer said:
I think Mojarra does not even change the ViewState during partial requests.

Nevertheless I suppose there are two separate issues surfacing here that are remotely related. This patch should work in a Portlet scenario as well as the unpatched version and it fixes the 4 issues I mentioned earlier including the initial bug report. Contrary to updating all forms in the document, it doesn't make it any worse for the portlet scenario. But yes, it is a preliminary hack, if you consider the following change as the right way to handle the situation:

In your last comment you basically said that on each Ajax request all forms contained in a one ViewRoot have to be updated with new state information - at least in the case of MyFaces (it would work for Mojarra as well because the ViewState doesn't change):

• If we are in a "normal" (non-portlet) environment with just one ViewRoot, this means updating all forms contained in the entire document.

• If we are in a portlet environment it would mean updating all forms that are contained in the current ViewRoot.

So wouldn't the best course of action be your option b): make it possible to specify the view root element's id additionally in the partial response. If it is not specified or is set to a default value, update the entire document, if not only update forms contained in the viewroot element. This would also add the possibility to rerender a complete portlet with the special render target @ALL, which represents entire ViewRoot element. Sadly I don't know enough about the portlet spec to tell if this is a good idea and if this change is enough to support it properly.

Providing a list of update ids would then again be redundant, because the forms that need updating depend only on the ViewRoot that is currently - partly or in whole - redisplayed.

If this sounds like the right way to do it, then applying the changebundle will be counterproductive, because it is unnecessarily complex compared to updating all forms in the current ViewRoot. On the other hand it could even be backported to 2.0 or 2.1, it does not make things worse for portlets and it fixes the 4 critical bugs/problems i mentioned.

[glassfishrobot]

@edburns said:
Thank you for excellent and clear analysis. Given my desire to include sketches for #730, #517, and #802, in next week's EDR of the spec, I'm going to defer this til after JavaOne.

[glassfishrobot]

codylerum said:
This also is an issue with popup panels which often have their own form that when submitted rerenders content on the main page.

Since any forms on the main page did not participate in the submit they don't rerender with a viewstate and are unusable.

[glassfishrobot]

lotus118 said:
Added a javascript workaround that fixes this issue and can be executed via the browser onLoad event handler. There are 2 versions: a plain JS one, and a jQuerified one. Tested and confirmed for Mojarra 2.1.2 (and jQuery 1.7.x).
Mad propz to frederickkaempfer for the original JS code.

[glassfishrobot]

File: 790-js-workaround.txt
Attached By: lotus118

[glassfishrobot]

werpu12 said:
Sorry to crush some hopes here, but the posted solution is exactly the one I have in myfaces for the non portlet case (you can turn it on via a config param). And there it works, but it can only work in a non portlet environment, because there you have only one viewroot. So you can update all forms under document.

In a portlet environment you can have several viewroots under document with independend viewstates and independend forms. There the patch ultimately will break your portlet environment by applying wrong viewroots to other portlets. Thats because the which dom node is the viewroot info is simply missing on the client side and/or the protocol.

As I said before unless you have the viewroot info or you know which forms you update there is no way to resolve that issue for the portlet and non portlet case at the same time. The problem really is a problem of the protocol not the implementation.

[glassfishrobot]

rogerk said:
I agree with Wevner's earlier assessment - that to handle the portlet case - multiple independent view root (eaxh with one or more forms) - we do need a new "server to client" message protocol that draws the association of which forms the view state should apply.

[glassfishrobot]

lotus118 said:
werpu12 is right, the workaround I posted only works for the non-portlet case (and is only verified for mojarra 2.1.2)

[glassfishrobot]

sharath.naik said:
Updating the MultiViewHandler's [ public void writeState(FacesContext context) throws IOException ] seems to fix the issue. The change made is to add the view state hidden field for forms, even when is a ajax request.

What is the purpose of not writing the view state if it is a partial request in this method?

Attaching the custom ViewHandler to override this method. would this be a fix that can be considered to be moved to MultiViewHandler?

[glassfishrobot]

stiemannkj1 said:
Just fixed a few failures: https://github.com/stiemannkj1/mojarra/tree/fix-render-@all-JSF-SPEC-790.

The only failure I have left to fix is test/servlet30/ajax Issue3833IT (only for client-side state saving).

[glassfishrobot]

stiemannkj1 said:
balusc, Neil Griffin, I have completed the fix. I sent a PR for review here: https://github.com/jsf-spec/mojarra/pull/5

I've tested this fix with server-side and client-side state saving against the following modules:

• servlet40
• servlet31
• servlet30-isolated
• servlet30
• javaee8
• javaee7
• javaee6web
• javaee6

The only failure was in javaee8/importConstants which was failing before my change and is unrelated to my changes.

As per the instructions here, I also sent this patch to the issues@javaserverfaces.java.net.

Also note my comment from above:

Note that I had to tell JUnit to @ignore my tests since they require HtmlUnit 2.24-SNAPSHOT due to https://sourceforge.net/p/htmlunit/bugs/1815/ (the tests do pass with 2.24-SNAPSHOT though).

The bug report says that this bug in HtmlUnit is a regression that was introduced in 2.19, so downgrading to 2.18 might also work.

[glassfishrobot]

@BalusC said:
Thank you for clarifying your intention and refactoring hints in the PR. I have largely applied the desired changes, just made them a bit simpler: https://java.net/projects/mojarra/sources/git/revision/41a60c0d6bb0af295ee271180b8bac4d550cffee Please let me know if this solves everything in your side too.

I have added an IT on this too, but the currently used HtmlUnit version indeed fails on the desired IT. I'd rather not switch to a snapshot version. Leave it for now until HtmlUnit 2.24 is released.

As to javaee8/importConstants test, this will only fail when -Dwebapp.partialStateSaving VM argument is missing. That was my mistake and I've already altered the test on that.

[glassfishrobot]

ngriffin7a said:
@balusc: Thank you for applying the changes. We have been testing this extensively with Mojarra 2.3.0-m09-SNAPSHOT and Pluto 3.0-SNAPSHOT this week. However, we will need to perform additional testing next week. We will report back with more info.

[glassfishrobot]

stiemannkj1 said:
@balusc, I have tested your changes in portlets and they work correctly now. Thanks.

• Kyle

[glassfishrobot]

ngriffin7a said:
@balusc: As Kyle mentioned we have experienced good results with our testing. There was a problem in PrimeFaces but my PR fix for PrimeFaces Issue#1997 was just merged. I might submit a follow-up PR for PrimeFaces that makes a better distinction between parameter "namespace" and parameter "prefix", according to the comment you wrote in PR#5. There is also a namespace prefix issue with ICEfaces portlets but we can address that in the future by contributing a fix to the ICEfaces source code.

I am currently reviewing all of my comments in this thread in order to make sure everything has been addressed. I'll report back with my findings. Thanks.

[glassfishrobot]

stiemannkj1 said:
balusc (and Neil Griffin), I've sent a PR to fix a minor issue related to this ticket to vsingleton. If all the tests pass, then vsingleton will merge this PR into master.

• Kyle

[glassfishrobot]

ngriffin7a said:
@balusc:

Regarding my comments on 25-July-2016. Unless I'm missing it, the JavaDoc for ExternalContext#getRequestParameterMap() and ExternalContext#getRequestParameterValuesMap() do not mention the backward compatibility requirement. If you think it's a good idea, I can come up with some language and send you a pull request. If you don't think it's a good idea to modify the JavaDoc then that's fine too. This will only be a concern in a portlet environment anyway. The FacesBridge Spec contains language about how the bridge must deviate from the JavaDoc for ExternalContext and we can document it there.

Regarding my comments on Oct-17-2016, we fixed the bridge and PrimeFaces to handle the separatorChar. At this point I don't think it is necessary to bring back the context param com.sun.faces.namespaceParameters.

[glassfishrobot]

@BalusC said:
Which backward compatibility requirement exactly?

As long as UIViewRoot is a NamingContainer, everything will continue to work transparently and there's no need for an additional "namespace" layer as the NamingContainer already does all that job.

[glassfishrobot]

ngriffin7a said:
The "backward compatibility requirement" is potentially adding to the JavaDoc some text that requires JSF implementations to do what Mojarra is currently doing:

**RequestParameterMap.java**String mapValue = request.getParameter(mapKey);
if (mapValue == null && !mapKey.startsWith(getNamingContainerPrefix())) {
    // Support cases where enduser manually obtains a request parameter while in a namespaced view.
    mapValue = request.getParameter(getNamingContainerPrefix() + mapKey);
}

This goes back to your comments on 24-Jul-2016 when you asked "Do you think specifying in API is necessary?"

[glassfishrobot]

ngriffin7a said:
@balusc: When UIViewRoot is a NamingContainer, should the developer expect both the commandLink and commandButton in the example below to work? Right now, only the commandButton is working (since it has an AjaxBehavior).

<h:form id="a">
	<h:outputLabel for="foo" value="Enter a new value for foo:" />
	<h:inputText id="foo" value="#{testBean.foo}" />
	<h:commandButton action="#{testBean.action}" value="click me to execute 'form a' but re-render 'form b'">
		<f:ajax execute="a" render=":b" />
	</h:commandButton>
	<h:commandLink value="click me to execute 'form a' but re-render 'form b'"
		   onclick="jsf.ajax.request(this,event,{execute:'a',render:':b'}); return false;"/>
</h:form>
<h:form id="b">
	<h:outputText value="This is the value of 'b': #{testBean.foo}" />
</h:form>

Here is the rendered HTML output of the h:commandButton:

<input id="Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a:j_idt9" type="submit" name="Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a:j_idt9" value="click me to execute 'form a' and 'form b' but re-render 'form b'" onclick="mojarra.ab(this,event,'action','Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a','Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:b');return false">

After clicking the h:commandButton, here are the relevant XHR postback param names/values:

Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:javax.faces.partial.execute=Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a:j_idt9 Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a
Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:javax.faces.partial.render=Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:b

And here is the rendered HTML output of the h:commandlink, showing that the onclick simply passes through:

<a href="#" onclick="jsf.util.chain(this,event,'jsf.ajax.request(this,event,{execute:\'a\',render:\':b\'}); return false;','mojarra.jsfcljs(document.getElementById(\'Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a\'),{\'Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a:j_idt10\':\'Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:a:j_idt10\'},\'\')');return false">click me to execute 'form a' but re-render 'form b'</a>

After clicking the h:commandLink, here are the relevant XHR postback param names/values:

Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:javax.faces.partial.execute= a
Pluto_com_liferay_faces_issue_790_portlet_1__904823498_0_:javax.faces.partial.render=:b

[glassfishrobot]

@BalusC said:
Previous behavior in request parameter map is unspecified and Mojarra specific. I prefer to keep it as is to keep API more simple and less confusing to the enduser. The code stays there to maintain backwards compatibility with how Portlets previously dealt with namespaced parameters without using NamingContainer.

As to the <h:commandLink> fail, this is not caused by the fact that a <h:commandLink> is used instead of <h:commandButton>, but that a manual jsf.ajax.request() script is used instead of one generated by <f:ajax>. If you replace jsf.ajax.request() by <f:ajax>, then it will work fine, regardless of if you use a command link or button, because <f:ajax> will generate the correct execute and render options. However, the potential backwards compatibility issue with this kind of code with a manual jsf.ajax.request() is understood. I will add a check to jsf.js if all execute and render parameters are properly namespaced. Still, I'm here also not confident that this has to go in the spec as all of this is caused by initially incorrect usage of NamingContainer/namespaces. I'd rather not want to promote two different ways of using a feature as this is just confusing to endusers.

[glassfishrobot]

ngriffin7a said:

I prefer to keep it as is to keep API more simple and less confusing to the enduser

Sounds good to me.

However, the potential backwards compatibility issue with this kind of code with a manual jsf.ajax.request() is understood. I will add a check to jsf.js if all execute and render parameters are properly namespaced. Still, I'm here also not confident that this has to go in the spec as all of this is caused by initially incorrect usage of NamingContainer/namespaces.

I don't think it needs to go in the spec either. But thank you for fixing Mojarra to check for proper namespacing with a manual jsf.ajax.request().

On behalf of Kyle and all of us on the Liferay Faces team, thank you so much for all you did to complete this issue. As far as we are concerned it can be closed! Hooray!

[glassfishrobot]

@BalusC said:
The check has been committed: https://java.net/projects/mojarra/sources/git/revision/90587b22dd5c8d15793f29d6161ff8f658bde3c6

[glassfishrobot]

Marked as fixed on Wednesday, January 11th 2017, 12:44:07 pm

[glassfishrobot]

@BalusC said:
Yes, closed, hooray!

[glassfishrobot]

kfyten said:
A hearty thank-you from the ICEfaces team as well! Nice to see this one taken care of.

[glassfishrobot]

This issue was imported from java.net JIRA JAVASERVERFACES_SPEC_PUBLIC-790