commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <si...@ecnetwork.co.nz>
Subject Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel, Craig
Date Fri, 27 Feb 2004 09:39:49 GMT
On Fri, 2004-02-27 at 00:19, Emmanuel Bourg wrote:
> Hi Simon, thank you for taking care of this issue. Actually I expected a 
> "real world" test case demonstrating the need for a specific order 
> rather than my own test case with the opposite assertion, but that's 
> fine :) I wish i had more time to investigate and isolate the Tomcat 
> issue, maybe I just missed a trivial side effect that was easily fixable.
> 
> I don't plan to (re)open the bug in a near future, I can't remember 
> exactly why I needed this feature at that time, but I think a 
> FactoryCreateRule would solve the issue I encountered.

I've had a good think about your proposed patch.

The issue with your proposal is that it assumes that at the moment when
all params are complete, the target object is on top of the digester
object stack. This is not always true.

When the CallMethodRule's begin fires, this essentially selects the
target object; the object on top of the object stack at the time the
CallMethodRule's begin fires is the one on which the method must
eventually be invoked.

With the existing rule, the CallMethodRule always fires when the object
stack has been restored to the state it was in when the CallMethodRule
executed its begin method.

But with your proposal, the CallMethodRule fires whenever the params are
complete, so there's no guarantee what object will be on the top of the
digester's object stack when that happens.

If, when the CallMethodRule "begin" fires, it could save the
top-of-stack object somewhere and always invoke the method on that
object then I think the issue would be resolved. But unfortunately, I
can't see any way of doing that. The CallMethodRule can't store the
target in a member attribute of the rule instance, because a single rule
instance can be mapped to multiple patterns. And if you store the target
object in any kind of stack, you get back to the same problem you had
initially.

Personally I'm not sure it was a good idea to allow a single Rule
instance to be mapped to multiple patterns. If we ever start on a
Digester2.0, this will be one of the issues I raise. And if this was
forbidden, then I think it would be possible to implement your approach.
But until then I think any such attempt is going to cause
backwards-compatibility problems.

Here's a test that I think would demonstrate the problem:

 <a>
   <b>bdata</b>
 </a>

 digester.addObjectCreate("a", Widget.class);
 digester.addCallMethod("a", "setChildData", 1);
 digester.addCallParam("a/b", 0); 

 digester.addObjectCreate("a/b", Gadget.class);

At the time the CallParamRule fires (when element b's body text is
encountered), the Digester object stack contains a Widget and a Gadget
in that order. So the CallMethodRule will try to call setChildData on
the top object: a Gadget.

You are right that the test case should be testing this fundamental
behaviour rather than the "side-effect" that my original patch tests
(CallMethodRule invocation order). When I get some time, I will add this
test case. [unless you point out a flaw in my reasoning].

Regards,

Simon


> Simon Kitching wrote:
> 
> > Hi Emmanuel,
> > 
> > Re your comments on bugzilla
> > http://nagoya.apache.org/bugzilla/show_bug.cgi?id=12997
> > 
> > I'd like to transfer this discussion to email rather than bugzilla form,
> > if that's ok. It's a pain typing and reading bugzilla comments :-)
> > 
> > I guess the problem is that the bugzilla entry is now addressing two
> > separate issues:
> > (a) 
> > A testcase fix is required to ensure that no patch changes the current
> > CallMethodRule invocation order, and
> > (b) 
> > an enhancement request from you (2002-10-30) to add a new rule or add an
> > optional feature to the existing CallMethodRule.
> > 
> > I've just fixed (a), but (b) is still pending it seems..
> > 
> > Your comment of 2002-10-30 said that the digester test cases were
> > running fine, and asked for a quick example that would break with your
> > patch. Well, that is clearly a flaw with the existing test cases, and
> > the test case change I recently committed provides that example you
> > asked for. The test you did with Tomcat (broke with your patch) shows
> > that the current CallMethodRule invocation order is deliberate and
> > should not change.
> > 
> > Are you intending to create an alternate version of your attached patch
> > which implements a new rule (eg CallMethodImmediateRule), or an
> > enhancement to CallMethodRule which adds this behaviour as *optional*
> > and *off-by-default*? I'm neutral on this myself. But if you do, it
> > might be nice to create a new Bugzilla entry so we can close this
> > existing one which is now rather confusing due to the two intermixed
> > issues.
> > 
> > I don't really understand Craig's comments re "hierarchical
> > structure that is isomorphic to a tree of beans that is being created".
> > 
> > However I do understand his comment re "backwards incompatibility problem",
> > which is what I have addressed here.
> > 
> > Comments?



---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message