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 18:19:49 GMT
Sure, Carlin -- thanks!
Rich

Carlin Rogers wrote:

>Yes, I agree. I think I have a fix for all this
>(*BEEHIVE-1037<https://issues.apache.org/jira/browse/BEEHIVE-1037>)
>*and will check it into the beehive svn repository. I'll add a comment to
>the bug with the svn revision number so you give it a review if you want.
>Thanks again for your help.
>
>On 1/10/06, Rich Feit <richfeit@gmail.com> wrote:
>  
>
>>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