INVOKE_APPLICATION PhaseListener with redirect

[g***@public.gmane.org]

Hi all,
After spending the past couple of days researching these concepts I
have reached the opinion that I have detected a suboptimal condition
enforced by the JSF specification (v2.1), and wanted to sound off.
Since I do not have a direct conduit to the EG as a whole, I'm airing
my concern here in the hope of starting a discussion and seeing where
that leads.

Facts:
A: Per the javadoc of Application#getActionListener(), the default
ActionListener implementation should implement #processAction() such
that #handleNavigation() is called against the available
NavigationHandler.
B: Per section 7.4.2 of the specification, it is the responsibility
of the default NavigationHandler implementation to call
ExternalContext#redirect() when the resulting navigation is marked as
<redirect>true</redirect>, and subsequently to call
FacesContext#responseComplete().
C: Per section 12.3 of the specification: for a given PhaseId,
#afterPhase() should be invoked against any PhaseListener against which
#beforePhase() was also invoked.

By merging these bits of information together, one may deduce that when
the navigation implied by a given action results in a redirect, the
#afterPhase() invocations for the current phase (typically
INVOKE_APPLICATION) are made in the context of a response that has
already been completed. This is indeed my experience in the nontrivial
application I am building as well as in the distilled test app which
behaves consistently with the spec both on Mojarra and MyFaces. This
situation makes certain crosscutting logic difficult at best to
implement and thus makes me wonder if it has been considered by the EG.
Would it not be better to revise the behavior of the various
cooperating components such that the redirect is performed *after* the
current phase has completed, making up simply an alternative path in
the implementation of RENDER_RESPONSE? If not, why not? As yet the
most straightforward workaround I have come up with is an
ExternalContextWrapper that intercepts the #redirect() call to do the
necessary cleanup.

Thanks,
Matt

[Ed Burns]

MB> Would it not be better to revise the behavior of the various
MB> cooperating components such that the redirect is performed *after* the
MB> current phase has completed, making up simply an alternative path in
MB> the implementation of RENDER_RESPONSE? If not, why not? As yet the
MB> most straightforward workaround I have come up with is an
MB> ExternalContextWrapper that intercepts the #redirect() call to do the
MB> necessary cleanup.

The current specification and implementation of
ExternalContext.redirect() means "redirect NOW". I don't want to change
that since it seems likely some applications are depending on the
immediate nature of calling redirect(). However, you do have a good
point: the afterPhase() listeners are not being invoked early enough to
take action based on the redirect().

What would you think if we made it so redirect() retains its immediacy,
but we move up the invocation of the afterPhase() listeners so they
happen before the redirect() actually occurs?

Ed

[Matt Benson]

MB>  Would it not be better to revise the behavior of the various
MB> cooperating components such that the redirect is performed *after* the
MB> current phase has completed, making up simply an alternative path in
MB> the implementation of RENDER_RESPONSE?      If not, why not?  As yet the
MB> most straightforward workaround I have come up with is an
MB> ExternalContextWrapper that intercepts the #redirect() call to do the
MB> necessary cleanup.
The current specification and implementation of
ExternalContext.redirect() means "redirect NOW".  I don't want to change
that since it seems likely some applications are depending on the
immediate nature of calling redirect().  However, you do have a good
point: the afterPhase() listeners are not being invoked early enough to
take action based on the redirect().
What would you think if we made it so redirect() retains its immediacy,
but we move up the invocation of the afterPhase() listeners so they
happen before the redirect() actually occurs?

Hi, Ed; thanks for your attention to this. I mentioned to you on IRC
that I have some add-on code that implements my recommendation. In a
nutshell, this code:

* provides a custom ExternalContext, overriding redirect() to save
the requested redirect URL to the requestMap, calling renderResponse()
against the current FacesContext
* provides a custom Lifecycle, overriding render() such to execute
any redirect deferred by the custom ExternalContext rather than
calling through to the wrapped Lifecycle's render() implementation

These have the effect of allowing afterPhase() to be triggered while
the request is active. Additionally, I have created a custom
FacesContext that returns false from getResponseComplete() while a
redirect is pending. Having called renderResponse() from the custom
ExternalContext when the redirect is requested, I personally can't
think of anything else that will happen beyond following up the
afterPhase listeners properly, so this seems equivalent to your
suggestion to move up the invocation of the afterPhase() listeners.
RENDER_RESPONSE afterPhase() would still be called after the response
is completed, but this is IMO as it should be and should be the status
quo for non-redirect requests today. WDY (or anyone else) T about
this approach?

Matt

Ed
--
| homepage:               | http://ridingthecrest.com/

[Ed Burns]

[...]

MB> These have the effect of allowing afterPhase() to be triggered while
MB> the request is active.

[...]

This is the key thing. The rest is implementation details (ha!). But
seriously, are you ok with having a special case that makes the
"afterPhase()" listeners be invoked "early" in the case of a redirect?
More specifically, when ExternalContext.redirect() is called, any
afterPhase() listeners for the current phase must be executed
immediately, before the underlying servlet or portlet redirect methods
are invoked, or, in the case of ajax, any of the special ajax actions
are taken.

Is this ok?

Ed

[Matt Benson]

Post by Ed Burns
[...]
MB> These have the effect of allowing afterPhase() to be triggered while
MB> the request is active.
[...]
This is the key thing.  The rest is implementation details (ha!).  But
seriously, are you ok with having a special case that makes the
"afterPhase()" listeners be invoked "early" in the case of a redirect?
More specifically, when ExternalContext.redirect() is called, any
afterPhase() listeners for the current phase must be executed
immediately, before the underlying servlet or portlet redirect methods
are invoked, or, in the case of ajax, any of the special ajax actions
are taken.
Is this ok?

I think I have talked myself around to your approach as far as the
spec goes, the final reason being that redirect is properly a function
of the ExternalContext. When you're wrapping the ExternalContext,
it's okay IMO to snatch a call away from the "real" impl and deliver
it later, but handling it in this way IN the "real" impl would only
result in a bumpier API (to differentiate between "redirect later" and
"redirect now"), and certainly the simplicity of the interface
outweighs that of the implementation. The only other approach would
be to make the "redirect later" call against the FacesContext, with
the implementation calling "redirect now" against the ExternalContext
at the proper time, but this would break clients currently calling
ExternalContext.redirect() directly. While I would prefer this design
given a clean slate, that's not what we're working with here.

Thanks for the talk; do you need me to create the JIRA request?

Matt

Post by Ed Burns
Ed
--
| homepage:               | http://ridingthecrest.com/

[Ed Burns]

Yes, please. Create an issue in JAVASERVERFACES_SPEC_PUBLIC. Thank you.