From 7b5b4d449576ce6ca3f6c1a2d758c7a5353e5292 Mon Sep 17 00:00:00 2001 From: Erwin Vervaet Date: Thu, 5 Jun 2008 11:22:07 +0000 Subject: [PATCH] Code review and cleanup. FlowPhaseListener.cleanupResources() is now protected. --- spring-webflow/changelog.txt | 1 + .../executor/jsf/FlowPhaseListener.java | 99 ++++++++++--------- 2 files changed, 56 insertions(+), 44 deletions(-) diff --git a/spring-webflow/changelog.txt b/spring-webflow/changelog.txt index 7cb92207..de603d5a 100644 --- a/spring-webflow/changelog.txt +++ b/spring-webflow/changelog.txt @@ -13,6 +13,7 @@ Package org.springframework.webflow.action Package org.springframework.webflow.executor.jsf * Statics on FlowFacesUtils are now public: they're supposed to be well known names. +* FlowPhaseListener.cleanupResources() is now protected. Changes in version 1.0.5 (03.10.2007) ------------------------------------- diff --git a/spring-webflow/src/main/java/org/springframework/webflow/executor/jsf/FlowPhaseListener.java b/spring-webflow/src/main/java/org/springframework/webflow/executor/jsf/FlowPhaseListener.java index 09f60453..6881a401 100644 --- a/spring-webflow/src/main/java/org/springframework/webflow/executor/jsf/FlowPhaseListener.java +++ b/spring-webflow/src/main/java/org/springframework/webflow/executor/jsf/FlowPhaseListener.java @@ -97,7 +97,7 @@ public class FlowPhaseListener implements PhaseListener { /** * A helper for handling arguments needed by this phase listener to restore and launch flow executions. - * + *

* This helper is responsible for two main things: *

    *
  1. Helping in the restoration of the "current" FlowExecution by extracting arguments from the request. @@ -115,7 +115,7 @@ public class FlowPhaseListener implements PhaseListener { *
  2. Generating external URLs to redirect to on a ExternalRedirect repsonse. * *
- * How arguments are extracted and how URLs are generated can be customized by setting a custom {{@link #setArgumentHandler(FlowExecutorArgumentHandler) argument handler}. + * How arguments are extracted and how URLs are generated can be customized by setting a custom argument handler. */ private FlowExecutorArgumentHandler argumentHandler = new RequestParameterFlowExecutorArgumentHandler(); @@ -195,16 +195,16 @@ public class FlowPhaseListener implements PhaseListener { /** * Sets the JSF view id mapper used by this phase listener. The {@link ViewIdMapper} provides a mechanism to convert - * a logical Spring Web Flow application view name into a JSF view id.
- * + * a logical Spring Web Flow application view name into a JSF view id. + *

* JSF view ids are important to this phase listener: it uses them to check whether the current view has changed, - * and if a new view needs to be created and activated by delegating to the application's {@link ViewHandler}.
- * + * and if a new view needs to be created and activated by delegating to the application's {@link ViewHandler}. + *

* A view handler typically treats a JSF view id as the physical location of a view template encapsulating a page * layout. The JSF view id normally specifies the physical location of the view template minus a suffix. View * handlers typically replace the suffix of any view id with their own default suffix (e.g. ".jsp" or ".xhtml") and - * then try to locate a physical template view.
- * + * then try to locate a physical template view. + *

* The {@link ViewIdMapper} provides the ability to customize how SWF view name is mapped to a JSF view id that will * be passed to the ViewHandler. The default value for the view id mapper is a {@link DefaultViewIdMapper} which * just returns the SWF viewId as-is. @@ -262,6 +262,12 @@ public class FlowPhaseListener implements PhaseListener { } } + // internal processing logic + + /** + * Restores the flow exeuction identified by given request, making it available to other JSF artifacts while + * processing the request. + */ protected void restoreFlowExecution(FacesContext facesContext) { JsfExternalContext context = new JsfExternalContext(facesContext); if (argumentHandler.isFlowExecutionKeyPresent(context)) { @@ -277,8 +283,8 @@ public class FlowPhaseListener implements PhaseListener { FlowExecution flowExecution = repository.getFlowExecution(flowExecutionKey); if (logger.isDebugEnabled()) { logger.debug("Loaded existing flow execution with key '" + flowExecutionKey - + "' due to browser access " - + "[either via a flow execution redirect or direct browser refresh]"); + + "' due to browser access" + + " [either via a flow execution redirect or direct browser refresh]"); } FlowExecutionHolderUtils.setFlowExecutionHolder(new FlowExecutionHolder(flowExecutionKey, flowExecution, lock), facesContext); @@ -289,6 +295,8 @@ public class FlowPhaseListener implements PhaseListener { lock.unlock(); throw e; } + + // in the normal case, unlock will happen later, or failing that the FlowSystemCleanupFilter } catch (FlowExecutionAccessException e) { // thrown if access to the execution could not be granted handleFlowExecutionAccessException(e, facesContext); @@ -304,8 +312,8 @@ public class FlowPhaseListener implements PhaseListener { ViewSelection selectedView = flowExecution.start(createInput(context), context); holder.setViewSelection(selectedView); if (logger.isDebugEnabled()) { - logger.debug("Launched a new flow execution due to browser access " - + "[either via a flow redirect or direct browser URL access]"); + logger.debug("Launched a new flow execution due to browser access" + + " [either via a flow redirect or direct browser URL access]"); } } } @@ -455,13 +463,11 @@ public class FlowPhaseListener implements PhaseListener { } } - // private helpers - - private JsfExternalContext getCurrentContext() { - return (JsfExternalContext) ExternalContextHolder.getExternalContext(); - } - - private void cleanupResources(FacesContext context) { + /** + * Cleans up any allocated flow system resources and clears out the external context holder. + * @param context the faces context + */ + protected void cleanupResources(FacesContext context) { if (logger.isDebugEnabled()) { logger.debug("Cleaning up allocated flow system resources"); } @@ -469,6 +475,29 @@ public class FlowPhaseListener implements PhaseListener { ExternalContextHolder.setExternalContext(null); } + // private helpers + + private void generateKey(JsfExternalContext context, FlowExecutionHolder holder) { + FlowExecution flowExecution = holder.getFlowExecution(); + if (flowExecution.isActive()) { + // generate new continuation key for the flow execution before rendering the response + FlowExecutionKey flowExecutionKey = holder.getFlowExecutionKey(); + FlowExecutionRepository repository = getRepository(context); + if (flowExecutionKey == null) { + // it is a new conversation - generate a brand new key + flowExecutionKey = repository.generateKey(flowExecution); + FlowExecutionLock lock = repository.getLock(flowExecutionKey); + lock.lock(); + // set that the flow execution lock has been acquired + holder.setFlowExecutionLock(lock); + } else { + // it is an existing conversation - get the next key + flowExecutionKey = repository.getNextKey(flowExecution, flowExecutionKey); + } + holder.setFlowExecutionKey(flowExecutionKey); + } + } + private void updateViewRoot(FacesContext facesContext, String viewId) { UIViewRoot viewRoot = facesContext.getViewRoot(); if (viewRoot == null || hasViewChanged(viewRoot, viewId)) { @@ -501,30 +530,9 @@ public class FlowPhaseListener implements PhaseListener { keyHolder.setFlowExecutionKey(flowExecutionKey); } - private void generateKey(JsfExternalContext context, FlowExecutionHolder holder) { - FlowExecution flowExecution = holder.getFlowExecution(); - if (flowExecution.isActive()) { - // generate new continuation key for the flow execution before rendering the response - FlowExecutionKey flowExecutionKey = holder.getFlowExecutionKey(); - FlowExecutionRepository repository = getRepository(context); - if (flowExecutionKey == null) { - // it is a new conversation - generate a brand new key - flowExecutionKey = repository.generateKey(flowExecution); - FlowExecutionLock lock = repository.getLock(flowExecutionKey); - lock.lock(); - // set that the flow execution lock has been acquired - holder.setFlowExecutionLock(lock); - } else { - // it is an existing conversation - get the next key - flowExecutionKey = repository.getNextKey(flowExecution, flowExecutionKey); - } - holder.setFlowExecutionKey(flowExecutionKey); - } - } - /** - * Utility method needed needed only because we can not rely on JSF RequestMap supporting Map's putAll method. Tries - * putAll, falls back to individual adds. + * Utility method needed only because we cannot rely on JSF RequestMap supporting Map's putAll method. Tries putAll, + * falls back to individual adds. * @param targetMap the target map to add the model data to * @param map the model data to add to the target map */ @@ -532,8 +540,7 @@ public class FlowPhaseListener implements PhaseListener { try { targetMap.putAll(map); } catch (UnsupportedOperationException e) { - // work around nasty MyFaces bug where it's RequestMap doesn't - // support putAll remove after it's fixed in MyFaces + // work around nasty MyFaces bug where it's RequestMap doesn't support putAll Iterator it = map.entrySet().iterator(); while (it.hasNext()) { Map.Entry entry = (Map.Entry) it.next(); @@ -542,6 +549,10 @@ public class FlowPhaseListener implements PhaseListener { } } + private JsfExternalContext getCurrentContext() { + return (JsfExternalContext) ExternalContextHolder.getExternalContext(); + } + private FlowDefinitionLocator getLocator(JsfExternalContext context) { return FlowFacesUtils.getDefinitionLocator(context.getFacesContext()); }