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.

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

Restore ViewScope before templates are processed with buildView #787

Closed
eclipse-faces-bot opened this issue Apr 23, 2010 · 31 comments
Closed

Comments

@eclipse-faces-bot
Copy link

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);
}

Environment

Operating System: All
Platform: All

Affected Versions

[2.0]

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
Reported by dueni

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
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.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
take ownership.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
Behavior change, move to 2.1.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
triage

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
GlassFish 3.1 M6 at the latest.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
Move these to 2.2

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
Created an attachment (id=337)
Proposed patch to restore viewScpe before buildView

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
taking ownership to develop patch for review and commit.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
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?

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
For what it's worth, all 2160 automated tests passed with this patch applied.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
Ok, thanks for considering the alternate idea.

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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
final changebundle before commit

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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)

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
Additional attribution and spec language

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
@edburns said:
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.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rinner23 said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rinner23 said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rinner23 said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
dueni said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rinner23 said:
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

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rogerk said:
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)

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rogerk said:
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.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rogerk said:
Reopening as this has issues with dynamic components.

@eclipse-faces-bot
Copy link
Author

@glassfishrobot Commented
rogerk said:
Changes for workaround.

@eclipse-faces-bot
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants