beehive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rich Feit <richf...@gmail.com>
Subject Re: Question about BEEHIVE-1024
Date Tue, 10 Jan 2006 16:03:40 GMT
I was thinking the latter -- replacing getModuleConfig() with 
ensureModuleConfig() in processMapping().  What do you think?

Carlin Rogers wrote:

>Do you mean, add a call to InternalUtils.ensureModuleConfig() from the
>FlowController.reinitialize() method during the create() process of the
>FlowController where the old initModuleConfig() call used to be? Or, were
>you thinking that it went in the processMapping to replace the call to
>getModuleConfig().
>
>On 1/10/06, Rich Feit <richfeit@gmail.com> wrote:
>  
>
>>I see -- thanks for clarifying.  I *think* that we should just change to
>>using InternalUtils.ensureModuleConfig() at that point.  It's meant to
>>be used in any code path where a desired module might still be
>>unregistered (and as so, getModuleConfig() is actually very rarely
>>used).  I should have switched to ensureModuleConfig(), not
>>getModuleConfig(), in that checkin.  Does that sound alright to you?
>>
>>Rich
>>
>>Carlin Rogers wrote:
>>
>>    
>>
>>>I haven't checked the setting or value of _moduleConfig. I've just been
>>>comparing the old code path to the new changes while running this
>>>      
>>>
>>convoluted
>>    
>>
>>>scenario. I noticed that in the old code, that if _moduleConfig was null
>>>      
>>>
>>in
>>    
>>
>>>initModuleConfig(), it would see if it was already an attribute of the
>>>context. Then, if not, AutoRegisterActionServlet.ensureModuleRegistered()
>>>was called.
>>>
>>>In turn, it called...
>>>AutoRegisterActionServlet.registerModule(), which called...
>>>AutoRegisterActionServlet.initModuleConfig().
>>>
>>>This initModuleConfig() does a servletContext.setAttribute() with the new
>>>module config, as does the
>>>      
>>>
>>AutoRegisterActionServlet.ensureModuleRegistered()
>>    
>>
>>>after registerModule() returns. The ModuleConfig object is returned from
>>>ensureModuleRegistered() and assigned to _moduleConfig.
>>>
>>>I've just noticed that in this scenario, when I'm in processMapping()
>>>      
>>>
>>that a
>>    
>>
>>>call to InternalUtils.getModuleConfig() for the GlobalApp module config
>>>      
>>>
>>will
>>    
>>
>>>now return null.
>>>
>>>Does that make sense? Sorry if this isn't so clear.
>>>
>>>Thanks,
>>>Carlin
>>>
>>>On 1/9/06, Rich Feit <richfeit@gmail.com> wrote:
>>>
>>>
>>>      
>>>
>>>>Hey Carlin,
>>>>
>>>>Just want to make sure I'm understanding here.  Are you saying that the
>>>>original call to initModuleConfig() (the one I removed) also registered
>>>>the module in the ServletContext, as a side effect?  It was only
>>>>supposed to set the reference to _moduleConfig, which is now always
>>>>taken care of in getModuleConfig().  Is the problem happening because
>>>>the module isn't registered in the ServletContext, or because
>>>>_moduleConfig is somehow null?
>>>>
>>>>Rich
>>>>
>>>>Carlin Rogers wrote:
>>>>
>>>>
>>>>
>>>>        
>>>>
>>>>>Hey Rich,
>>>>>
>>>>>Thanks for the reply and the information on the
>>>>>InternalUtils.avoidDirectResponseOutput() function.
>>>>>
>>>>>I looked at the issue some more and it turns out that for the scenario
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>I'm
>>>>
>>>>
>>>>        
>>>>
>>>>>investigating, there's another revision that is also impacting the
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>behavior.
>>>>
>>>>
>>>>        
>>>>
>>>>>In revision 351812,
>>>>>          
>>>>>
>>http://svn.apache.org/viewcvs.cgi?rev=351812&view=rev,
>>    
>>
>>>>>for BEEHIVE-1017, the FlowController.reinitialize() method was modified
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>so
>>>>
>>>>
>>>>        
>>>>
>>>>>that in no longer calls initModuleConfig(), which ensured that the
>>>>>          
>>>>>
>>module
>>    
>>
>>>>>config was registered (attribute of the context).
>>>>>
>>>>>Now, when the initial page flow of the portal scenario is opened in a
>>>>>portlet, the GlobalApp module config is not added to the servlet
>>>>>          
>>>>>
>>context
>>    
>>
>>>>>attributes. Then when the unhandled action is hit and we fall into
>>>>>processMapping(), the call to InternalUtils.getModuleConfig() for the
>>>>>GlobalApp module config will be null and we wouldn't even be able to
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>check
>>>>
>>>>
>>>>        
>>>>
>>>>>for an action that was declared as "unknown".
>>>>>
>>>>>Unfortunately, I'm still trying to understand why the call to from
>>>>>FlowController.reinitialize() to initModuleConfig() was removed.
>>>>>
>>>>>I've logged a JIRA issue on this and am trying to figure out what is
>>>>>          
>>>>>
>>the
>>    
>>
>>>>>best way to resolve the problem. See
>>>>>http://issues.apache.org/jira/browse/BEEHIVE-1037
>>>>>The bug description might be a better illustration of the scenario I'm
>>>>>trying to solve.
>>>>>
>>>>>Let me know if you have some more thoughts about how best to resolve
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>this.
>>>>
>>>>
>>>>        
>>>>
>>>>>Many thanks,
>>>>>Carlin
>>>>>
>>>>>On 1/9/06, Rich Feit <richfeit@privatei.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>>>Hey Carlin,
>>>>>>
>>>>>>Sorry for the delay.  I agree that we should be checking for an
>>>>>>"unknown" action mapping in the global app module, so if you're
>>>>>>suggesting making that change, I agree.
>>>>>>
>>>>>>My answer to the rest is a little more involved:
>>>>>>
>>>>>>  - There's already a rough mechanism for avoiding direct response
>>>>>>output.  In InternalUtils, there's avoidDirectResponseOutput().  If
>>>>>>that's called on the request, then InternalUtils.sendError() will
>>>>>>            
>>>>>>
>>throw
>>    
>>
>>>>>>an exception instead of writing to the response.  This is what I think
>>>>>>should be happening here -- we should be calling sendError().
>>>>>>
>>>>>>  - I think that two things should probably change here: 1)
>>>>>>InternalUtils.avoidDirectResponseOutput() should be replaced with
a
>>>>>>            
>>>>>>
>>flag
>>    
>>
>>>>>>in PageFlowRequestWrapper, and 2) strutsLookup() should just set this
>>>>>>flag off the bat.  We shouldn't be writing to the response *ever*
>>>>>>            
>>>>>>
>>during
>>    
>>
>>>>>>strutsLookup().
>>>>>>
>>>>>>Let me know what you think (and if you have any questions about this).
>>>>>>
>>>>>>Rich
>>>>>>
>>>>>>Carlin Rogers wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>Just wanted to note that the difference in the behavior is also
>>>>>>>              
>>>>>>>
>>related
>>    
>>
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>to a
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>struts merge  where the struts module config has an action defined
>>>>>>>              
>>>>>>>
>>with
>>    
>>
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>the
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>"unknown" attribute (making it like a default). I think the missing
>>>>>>>condition is that we check to see if the GlobalApp has the action
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>config
>>>>
>>>>
>>>>        
>>>>
>>>>>>>              
>>>>>>>
>>>>>>but
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>we don't check any of the action configs on the global app to
see if
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>they're
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>"unknown".
>>>>>>>
>>>>>>>So, If the global app includes a Struts Merge and that struts
module
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>config
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>includes an unknown action, we'll never hit it.
>>>>>>>
>>>>>>>Carlin
>>>>>>>
>>>>>>>On 1/5/06, Carlin Rogers <carlin.rogers@gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>>>Hey Rich,
>>>>>>>>
>>>>>>>>Hope your work is going well!
>>>>>>>>
>>>>>>>>I have a question about svn revision 356056 (
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>http://svn.apache.org/viewcvs.cgi?rev=356056&view=rev
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>) checked in as a fix for BEEHIVE-1024. It seems that it changed
the
>>>>>>>>behavior of PageFlowRequestProcessor.processMapping() and
how we
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>handle
>>>>
>>>>
>>>>        
>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>an
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>unknown action. With this change, the code path for an unknown
>>>>>>>>                
>>>>>>>>
>>action
>>    
>>
>>>>>>>>                
>>>>>>>>
>>>>in
>>>>
>>>>
>>>>        
>>>>
>>>>>>>>processMapping() fails the new check to see if it is in the
>>>>>>>>                
>>>>>>>>
>>globalApp
>>    
>>
>>>>>>>>(...globalApp.findActionConfig(path) != null). We drop to
the else
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>statement
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>and into a call to processUnresolvedAction() which uses the
>>>>>>>>DefaultExceptionsHandler class and eventually writes out the
HTML of
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>our
>>>>
>>>>
>>>>        
>>>>
>>>>>>>>action not found error message directly to the response. I
think
>>>>>>>>                
>>>>>>>>
>>this
>>    
>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>looks
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>OK. However, having the error message written to the response
may
>>>>>>>>                
>>>>>>>>
>>not
>>    
>>
>>>>>>>>                
>>>>>>>>
>>>>be
>>>>
>>>>
>>>>        
>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>the
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>desired behavior for something like a portal using a call
to
>>>>>>>>PageFlowUtils.strutsLookup(). What do you think?
>>>>>>>>
>>>>>>>>If we leave the fix as is, could we use the
>>>>>>>>PageFlowRequestWrapper.isScopedLookup() condition to determine
if
>>>>>>>>                
>>>>>>>>
>>this
>>    
>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>is
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>from strutsLookup() or not before calling processUnresolvedAction().
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>I.E
>>>>
>>>>
>>>>        
>>>>
>>>>>>>              
>>>>>>>
>>>>>>.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>>do something different for an unknown action in a strutsLookup()?
>>>>>>>>                
>>>>>>>>
>>Just
>>    
>>
>>>>>>>>curious.
>>>>>>>>
>>>>>>>>Thanks,
>>>>>>>>Carlin
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>            
>>>>>>
>>>>>
>>>>>          
>>>>>
>>>
>>>      
>>>
>
>  
>

Mime
View raw message