Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES_SPEC_PUBLIC-787
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Critical Critical
Assignee: Hanspeter Duennenberger
Reporter: Hanspeter Duennenberger
Votes: 2
Watchers: 3
Operations

If you were logged in you would be able to see more operations.
javaserverfaces-spec-public

Restore ViewScope before templates are processed with buildView

Created: 23/Apr/10 01:44 AM   Updated: 06/Apr/12 03:43 PM   Resolved: 16/Aug/11 10:17 PM
Component/s: Facelets/VDL
Affects Version/s: 2.0
Fix Version/s: 2.2

Time Tracking:
Issue & Sub-Tasks
Issue Only
Not Specified

File Attachments: 1. Text File changebundle-787-workaround.txt (2 kB) 27/Jul/11 08:31 PM - rogerk
2. Text File changebundle.txt (6 kB) 10/Jun/11 01:55 PM - Ed Burns
3. Text File changebundle.txt (7 kB) 08/Jun/11 03:00 PM - Hanspeter Duennenberger
4. Text File JAVASERVERFACES_SPEC_PUBLIC-787-restore-ViewScope-before-buildView.patch (6 kB) 07/Jun/11 06:27 AM - Hanspeter Duennenberger
5. Zip Archive JAVASERVERFACES_SPEC_PUBLIC-787.zip (15 kB) 12/Aug/11 01:39 PM - Hanspeter Duennenberger

Environment:

Operating System: All
Platform: All

Issue Links:
Dependency
 
Related
 

Issuezilla Id: 787
Status Whiteboard:

size_medium importance_large draft

Tags:
Participants: Ed Burns, Hanspeter Duennenberger, Rinner23 and rogerk

  • Sub-Tasks:
  • All
  • Open

 Description  « Hide

Restoring the view with partial state saving enabled, the state is attached to
the component tree after the view-definition is processed (aka the tree is built
from the template). But sometimes the application of the view-definition might
depend on state from the previous request. An example: we have a dialog system
which remembers across requests which dialog is active.

So, to be backwards compatible with JSF 1.2, we need a way to store this state
across requests - it can not be part of the component state, cause the component
state is applied too late.

Our solution - and also recommendation for the next version of the spec - is
that the ViewScope (or the complete state of the ViewRoot component, but
ViewScope is sufficient) is restored before buildView() is called. That would
allow components with dynamic behavior to keep state in the ViewScope.

best regards,

Martin and Hanspeter

=====================

Here what we do currently: we decorated ViewDeclarationLanguage.buildView like
this (ok, for simplification we restored complete ViewRoot state):

public void buildView(FacesContext context, UIViewRoot root) throws
IOException {

if(context.getCurrentPhaseId()== PhaseId.RESTORE_VIEW) { //restore the view-scope (part of the UIViewRoot state-saving process) //_before_ we run the templates RenderKit renderKit = context.getRenderKit(); ResponseStateManager rsm = renderKit.getResponseStateManager(); Object[] rawState = (Object[]) rsm.getState(context, root.getViewId()); //noinspection unchecked final Map<String, Object> state = (Map<String,Object>) rawState[1]; Object viewRootState = state.get(root.getClientId()); root.restoreState(context, viewRootState); }

delegate.buildView(context, root);
}



Ed Burns added a comment - 19/May/10 07:07 AM

I talked to Martin and Hanspeter about this. I agree with restoring the ViewMap first, but I don't know
if it will be possible in practice. Marking as P2 to make sure we get to it.


Ed Burns added a comment - 24/May/10 12:55 PM

take ownership.


Ed Burns added a comment - 27/May/10 12:06 PM

Behavior change, move to 2.1.


Ed Burns added a comment - 22/Jun/10 09:44 PM

triage


Ed Burns added a comment - 24/Jun/10 02:41 PM

GlassFish 3.1 M6 at the latest.


Ed Burns added a comment - 10/Sep/10 01:31 PM

Move these to 2.2


Hanspeter Duennenberger added a comment - 14/Nov/10 02:41 PM

Created an attachment (id=337)
Proposed patch to restore viewScpe before buildView


Hanspeter Duennenberger added a comment - 14/Nov/10 02:43 PM

Hi Ed.

Finally I found some time to develop a patch for this. It works fine with CS JSF.
Please have a look at it (I hope you find some time for that .

regards
Hanspeter


Hanspeter Duennenberger added a comment - 26/May/11 01:51 PM

taking ownership to develop patch for review and commit.


Hanspeter Duennenberger added a comment - 07/Jun/11 06:27 AM

Hello.

This is a reworked patch to restore ViewScope before the templates are processed in buildView().

Until now the full state including ViewScope is restored in UIViewRoot.restoreState(). But this method must only be called after the templates have been processed. To allow restore of ViewScope before processing the templates, I introduced a new method called UIViewRoot.restoreViewScopeState(FacesContext fc, Object state) to restore ViewScope only. In UIViewRoot.restoreState(..) viewScope will only be restored if not done before.

I am aware that it seems weird that ViewScope is state saved in the normal UIViewRoot.saveState(..) method and restored with restoreViewScopeState(..). But for saving the state there was no need to change the behaviour unless we want to redesign this in more depth which would involve a pair of method to save/restore ViewScope.

We use this solution in CS JSF since about 1/2 year and it worked with partial- and full state saving.

regards
Hanspeter


Ed Burns added a comment - 08/Jun/11 08:07 AM

This is a nice and simple patch. Naturally, I wonder if there is a root problem (no pun intended) not specific to just the UIViewRoot, where a component needs to have some state restored to it after the tree has been built from the template, but before the partial state restoration traversal. If the UIViewRoot has this problem, then perhaps other components may have it as well?

Do we need to generalize this or is it sufficient to handle the view scope alone?

Arguments in favor of accepting the special case approach directly from
Hanspeter's 20110607 patch.

1. The manner in which the pre-restoration-traversal state is obtained
is specific to the implementation of the component. Therefore, we
must own the implementation of the component. In the case of
UIViewRoot, we do. If we wanted to generalize it, we would need
additional API and the state management API is already too complex.

2. An other use-case for pre-restoration-traversal state is dynamic
components. However, these are handled specifically elsewhere in the
implementation.

3. The UIViewRoot is unique among all other components in that you don't
need a template to obtain a reference to it. This uniqueness
warrants special case API.

Arguments in favor of denying Hanspeter's 20110607 patch in favor of a
request for a more general solution.

1. The additional API required would not be that large. Here is a sketch.
Introduce a new ComponentSystemEvent: PreRestoreStateEvent. Modify
the contract of the existing UIComponent.processEvent() to handle
this event. Override it in the case of UIViewRoot to do the action
currently impleneted in Hanspeter's 20110607 patch.

Hanspeter, can you please cook up a version of the patch that uses the
PreRestoreStateEvent approach?


Ed Burns added a comment - 08/Jun/11 09:15 AM

For what it's worth, all 2160 automated tests passed with this patch applied.


Hanspeter Duennenberger added a comment - 08/Jun/11 01:38 PM

Hi Ed.

Interesting considerations and I really like more generalized solutions, but I think UIViewRoot is unique for this matter:

1. UIViewRoot is not created from templates and there is also no possibility to do special work in a UIViewRootTagHandler.

2. The state in question is the ViewScope - all other components don't have theyr own scope to restore.

3. PreRestoreStateEvent on the component would be about the same time as PostAddToViewEvent on the component. It's after component was constructed and added to the view but before restore state is processed - restore state is processed after the full component tree was built.

4. In my opinion there is no way to restore some component state before the component is created - and during component creation TagHandler may be used for such action or directly after component construction PostAddToView component event may be used.

5. PreRestoreStateEvent processing would probably cause another tree walk, which some of the EG members will not like.

Ed, I don't see the need for that elaboration.

Regards
Hanspeter


Ed Burns added a comment - 08/Jun/11 01:59 PM

Ok, thanks for considering the alternate idea.

I'm r=edburns for adding this to the trunk.


Hanspeter Duennenberger added a comment - 08/Jun/11 03:00 PM

final changebundle before commit


Ed Burns added a comment - 09/Jun/11 07:13 AM

r=edburns. Please add the output from svn commit, including all the "Sending..." lines and the line containing the svn revision number to this issue.

Ed


Hanspeter Duennenberger added a comment - 09/Jun/11 01:08 PM

Committed to trunk.

Sending jsf-api/src/main/java/javax/faces/component/UIViewRoot.java

Sending jsf-api/src/test/java/javax/faces/component/UIViewRootTestCase.java

Sending jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

Transmitting file data ......

Committed revision 9139.

(had to reproduce this manually - next time I know)


Ed Burns added a comment - 10/Jun/11 01:55 PM

Additional attribution and spec language


Ed Burns added a comment - 10/Jun/11 02:00 PM

Sending jsf-api/src/main/java/javax/faces/component/UIViewRoot.java
Sending jsf-api/src/main/java/javax/faces/view/StateManagementStrategy.java
Sending jsf-api/src/main/java/javax/faces/view/package.html
Transmitting file data ...
Committed revision 9149.


Rinner23 added a comment - 14/Jul/11 06:50 PM

Before I try a merge do you think this would be safe to work into 2.0.4b9? This would be a great feature for us as it would allow viewscope to be used instead of Session in some of our nasty dynamic components

Cheers!

Matt


Hanspeter Duennenberger added a comment - 14/Jul/11 09:04 PM

Hi Matt,

we used that on our custom built JSF RI 2.0 before 2.0.4 without any problem. So you should be save doing the same.

regards
Hanspeter


Rinner23 added a comment - 18/Jul/11 10:29 PM

Actually, this change does appear to have a problem on 2.0.4b9. After applying the patch, we do have the viewscope at the correct time, but for some reason we are seeing two PostAddToViewEvents being fired during RENDER_RESPONSE but only during the very first time the page is requested.

The second PostAddToViewEvent has the component in a bad state, with no children.

If I refresh the page, everything works as normal. The only way to cause the problem again is to bounce the server, and again on the first page refresh the additional PostAddToViewEvent is fired.

Matt


Rinner23 added a comment - 26/Jul/11 01:54 PM

Any thoughts on this? I've tried looking through the source code, but I'm currently really too ignorant of the details of the system to figure out why this might happen...

I think it must be related to how the system deals with the templates for the first time, like when it compiles/caches(correct?) them or something its throwing that additional PostAddToViewEvent. I've confirmed that if I remove the code, the additional event goes away as expected.

To work around this, I'm serializing my object graph into a hidden input field and restoring on the decode() of my custom composite component (manually grabbing the value from request).

This seems to work, and allows us to then dynamically create our component tree during PostAddToView based on the data serialized in that field from request to request (which can change).

Thanks for any input on this!

Regards,

Matt


Hanspeter Duennenberger added a comment - 26/Jul/11 02:43 PM

Hi Matt,
sorry, I did not have time to look into that.

The changes from this issue only affect restoreState in UIViewRoot and restoreView in StateManagementStrategyImpl - so I can hardly believe it affects the first time a page is rendered. I assume the double delivered PostAddToViewEvent is due to something else.

Debug into your PostAddToViewEvent handling will show where the event comes from. Make sure the second one is not from a resource request - which would explain why the component delivered by the event does not have any children or parent.

regards
Hanspeter


Rinner23 added a comment - 26/Jul/11 05:16 PM

I did check this, and there are no resources being requested.. It is simply a custom composite component in an otherwise empty page. Now, I am registering for the PostAddToViewEvent inside my components constructor, not in any page backing bean.

It is strange, that when I revert the changes and re-compile the extra event goes away with the exact same page.

As noted above I'm still very new to this technology having come across from ASP.NET, but it does have some similarities and I am working hard to get a better understanding.

Thanks,

Matt


rogerk added a comment - 27/Jul/11 06:05 PM

This patch breaks a fundamental dynamic components test. Here it is (and the exception):

Test Case: Simple View

<h:form prependId="false" id="dynamicForm">

<h:panelGroup id="group">
<h:commandButton value="Add Component" action="#{testManagedBean.addComponent}"/>
</h:panelGroup>

</h:form>

The action in the managed bean dynamically created an HTMLOutputText component and adds it as a child to the panelGroup ("group"):

public void addComponent() { FacesContext ctx = FacesContext.getCurrentInstance(); UIComponent group = ctx.getViewRoot().findComponent("dynamicForm" + UINamingContainer.getSeparatorChar(ctx) + "group"); HtmlOutputText output = new HtmlOutputText(); output.setValue("OUTPUT"); group.getChildren().add(output); }

The first button click adds it fine (after the button). The next button click produces the following exception:

java.lang.ClassCastException: com.sun.faces.application.view.StateHolderSaver cannot be cast to [Ljava.lang.Object;
at javax.faces.component.UIViewRoot.restoreViewScopeState(UIViewRoot.java:1693)
at com.sun.faces.application.view.StateManagementStrategyImpl.restoreView(StateManagementStrategyImpl.java:240)
at com.sun.faces.application.StateManagerImpl.restoreView(StateManagerImpl.java:211)
at com.sun.faces.application.view.ViewHandlingStrategy.restoreView(ViewHandlingStrategy.java:123)
at com.sun.faces.application.view.FaceletViewHandlingStrategy.restoreView(FaceletViewHandlingStrategy.java:452)
at com.sun.faces.application.view.MultiViewHandler.restoreView(MultiViewHandler.java:148)
at com.sun.faces.lifecycle.RestoreViewPhase.execute(RestoreViewPhase.java:192)
at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:101)
at com.sun.faces.lifecycle.RestoreViewPhase.doPhase(RestoreViewPhase.java:116)
at com.sun.faces.lifecycle.LifecycleImpl.execute(LifecycleImpl.java:118)
at javax.faces.webapp.FacesServlet.service(FacesServlet.java:635)


rogerk added a comment - 27/Jul/11 07:32 PM

The problem is that dynamic components are saved as StateHolderSaver objects.
You can see that logic in StateManagementStrategyImpl class (saveView method).
And in UIViewRoot.restoreViewScopeState (line 1693) we are blindly casting that to Object[]..
So, in this case, the first button press creates the dynamic component and it is saved as
a StateHolderSaver.


rogerk added a comment - 27/Jul/11 08:30 PM

Reopening as this has issues with dynamic components.


rogerk added a comment - 27/Jul/11 08:31 PM

Changes for workaround.


rogerk added a comment - 27/Jul/11 08:35 PM

I've committed the workaround to the trunk:
Sending jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java
Transmitting file data .
Committed revision 9226.


Hanspeter Duennenberger added a comment - 28/Jul/11 09:11 AM

Hi Roger,

thanks for that update - but I think the workaround is really just a workaround. I wonder why the dynamically added component changes the state structure. In UIViewRoot.saveState() - the saved state is an Object array with size 2 - element 0 is assigned with super.saveState() and element 1 is storing the viewScope as attachedState. But in case of the dynamically added component that seems no longer true - so that would mean the dynamically added component corrupts the state structure on UIViewRoot level.

I'll try to reproduce that problem and analyse it - hope I find time for it.

regards
Hanspeter


rogerk added a comment - 28/Jul/11 01:16 PM

I spoke incorrectly when I said dynamic components were saved as StateHolderSaver objects - obviously that is not true. StateHolderSaver is a helper class in Mojarra. During saveView, dynamic components are added to the state map as StateHolderSaver objects. You can see this in StateManagementStrategyImpl.saveView.
So StateHolderSaver is used to save/restore state for dynamic components.
In restoreView, dynamic components are handled after non-dynamic components. The state map will also contain StateHolderSaver objects (used for the restoration of dynamic components). So, at the time you call:
viewRoot.restoreViewScopeState(context, stateObj);
you cannot assume stateObj will always be the state object itself.


Hanspeter Duennenberger added a comment - 12/Aug/11 01:37 PM

Hi Ed and Roger.

I tried to reproduce the error with the example Roger has given - but was not able to reproduce the exact same problem. But I have realized that h:form prependId="false" may lead to duplicate id's - see http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1032.

Looking at what tree visitor does in StateManagementStrategyImpl.saveView I somewhat understood the problem - in case a certain component was added dynamically, it's state is packed into a StateHolderSaver. But to cause the problem that Roger has pointed out it would be necessary to dynamically add UIViewRoot - is that possible at all or are we hunting a phantom problem?

I think the problem Roger ran into was based on the problem that UIForm with prependId="false" generated an id for the dynamically added component that was the same as the UIViewRoot's id. Then of course the saveView() treeVisitor assumes UIViewRoot was added dynamically and wraps it's state within a StateHolderSaver - and voila - we have the problem. However, I was not able to reproduce that.

Big question: is it possible to have UIViewRoot as a dynamically added component?

I was not able to reproduce the problem - if Roger can reproduce it, please provide me exact information of how to reproduce.

Unfortunatley on my windows machine I cannot run test.with.container.refresh anymore ...


Hanspeter Duennenberger added a comment - 12/Aug/11 01:39 PM

Testcase I tried to reproduce the problem - no success so far.


Hanspeter Duennenberger added a comment - 12/Aug/11 02:19 PM

Finally with some help from Roger I was abler to reproduce the problem - but only after removing the patch for http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1032 UIForm.createUiqueId() which may produce non-unique clientId's in case prependId is false.

The dynamically added component is actually added to a panelGroup - and get's the clientId "j_id1" - which is the same as teh UIViewRoot's clientId. Now StateManagementStrategyImpl gets confused and handles UIViewRoot as a dynamically added component - and wraps it's state into a StateSaveHolder. And then next restoreState fails.


rogerk added a comment - 15/Aug/11 03:32 PM

Tests ran successfully on my hosted machine.

r=rogerk


Hanspeter Duennenberger added a comment - 16/Aug/11 10:17 PM - edited

resolved as of JAVASERVERFACES_SPEC_PUBLIC-1032 - see there


Ed Burns added a comment - 19/Oct/11 04:42 PM

Marking closed per most recent comments.


Ed Burns added a comment - 06/Apr/12 03:43 PM

Sending jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
Sending jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java
Sending jsf-ri/src/main/java/com/sun/faces/config/processor/FaceletTaglibConfigProcessor.java
Transmitting file data ...
Committed revision 9814.

http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1063 http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-787 http://java.net/jira/browse/JAVASERVERFACES-2369

SECTION: Modified Files
----------------------------
M jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

  • in restoreView(), note that the state map will never be null due to
    the assignment statement at the top of the method. Therefore, we
    shouldn't test for null to determine the presence or absence of a
    pre-existing state map.

M jsf-ri/src/main/java/com/sun/faces/config/processor/FaceletTaglibConfigProcessor.java

  • in createMethod(), replace one or more whitespace with a single whitespace.

M jsf-api/src/main/java/javax/faces/component/UIViewParameter.java

  • in getSubmittedValue(), remove unnecessary cast to String.