You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Emmanuel Bourg <eb...@micropole-univers.com> on 2004/02/26 12:19:17 UTC
Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel, Craig
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.
Emmanuel Bourg
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?
>
> Regards,
>
> Simon
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] CallMethodRule order [bug12997] attn:
Emmanuel, Craig
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Sat, 2004-02-28 at 00:32, Emmanuel Bourg wrote:
> Simon Kitching wrote:
>
> > The question in my mind is still: what use cases require the
> > CallMethodRule to fire when the params are available?
>
> Ok I remember why I needed this feature. I wanted to parse a file of
> i18n labels and insert them in a database. The file looked like this :
>
> <messages>
> <message key="global.cancel">
> <label lang="en">Cancel</label>
> <label lang="fr">Annuler</label>
> </message>
> <message key="global.apply">
> <label lang="en">Apply</label>
> <label lang="fr">Appliquer</label>
> </message>
> </messages>
>
> I first pushed a "MessageFactory" on the stack with the following
> interface :
>
> interface MessageFactory {
> void setKey(String key);
> void addLabel(String lang, String label);
> }
>
> and the parsing rules were :
> - call setKey() on messages/message using the key attribute
> - call addLabel() on messages/message/lang. This method issued the
> INSERT in the database, using the key previously specified.
>
>
> I was then surprised to find in my database :
>
> global.cancel=Apply
>
> That's how I discovered that the call to setKey() was issued after the
> call to addLabel().
>
> I admit I could have done it differently, but that illustrates the case
> of a rule fired on a child element depending on a context parameter set
> by a rule fired at the beginning of the parent element.
Hi Emmanuel,
I've added an additional test, on the lines of my email of yesterday.
I hope this satisfies your original concerns about the test changes.
See CallMethodRuleTestCase, methods "testOrderNestedPartA" and
"testOrderNestedPartB" for more info, esp. the javadoc comments.
Or see the commit email :-)
I can see how easy it would be to fall into the trap you outline above.
However, as you say, there are a number of alternative solutions. I have
added a comment to the javadoc for class CallMethodRule to warn of this
behaviour. I think this is sufficient myself...
<begin javadoc>
Note that the target method is invoked when the <i>end</i> of the tag
the CallMethodRule fired on is encountered, <i>not</i> when the last
parameter becomes available. This implies that rules which fire on tags
nested within the one associated with the CallMethodRule will fire
before the CallMethodRule invokes the target method. This behaviour is
not configurable.
<end javadoc>
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel, Craig
Posted by Emmanuel Bourg <eb...@micropole-univers.com>.
Simon Kitching wrote:
> The question in my mind is still: what use cases require the
> CallMethodRule to fire when the params are available?
Ok I remember why I needed this feature. I wanted to parse a file of
i18n labels and insert them in a database. The file looked like this :
<messages>
<message key="global.cancel">
<label lang="en">Cancel</label>
<label lang="fr">Annuler</label>
</message>
<message key="global.apply">
<label lang="en">Apply</label>
<label lang="fr">Appliquer</label>
</message>
</messages>
I first pushed a "MessageFactory" on the stack with the following
interface :
interface MessageFactory {
void setKey(String key);
void addLabel(String lang, String label);
}
and the parsing rules were :
- call setKey() on messages/message using the key attribute
- call addLabel() on messages/message/lang. This method issued the
INSERT in the database, using the key previously specified.
I was then surprised to find in my database :
global.cancel=Apply
That's how I discovered that the call to setKey() was issued after the
call to addLabel().
I admit I could have done it differently, but that illustrates the case
of a rule fired on a child element depending on a context parameter set
by a rule fired at the beginning of the parent element.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel, Craig
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On 27 Feb 2004, at 11:17, Simon Kitching wrote:
<snip>
> I'm not sure whether the param stack should be regarded as an external
> API for digester, though. While we can fix all param rules
> (CallParamRule, ObjectParamRule, etc) we would break any variants of
> these written by users if we do this. Unfortunately I think this is not
> acceptable.
i think that probably the behaviour of the param stack needs to be
backward compatible.
the limitation of only having two stacks is one that the named stacks
stuff was intended to address. maybe it'd be possible to use named
stacks to add this functionality which preserving backwards
compatibility.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel,
Craig
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Sat, 2004-02-28 at 11:39, Simon Kitching wrote:
> 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.
Hmm.. maybe I have thought of a way to do this. I'm still not entirely
sure this "call method when params available" is a good idea, but it
might be implementable as follows:
Have the digester's param stack be a stack of instances of the following
class, instead of a stack of Object[]:
Class Invocation {
private CallMethodRule rule;
private Object target;
private Object[] params;
private int nParamsRemainingToSet;
public void setParam(int index, Object paramValue) {
params[index] = paramValue;
--nParamsToSet;
if (nParamsRemainingToSet == 0) {
rule->invokeMethod(target, params);
}
}
}
The above is only rough, but gives the idea I hope. The CallMethodRule
creates one of these on begin() and pushes it onto the param stack. At
that time, it knows the target object (on top of object stack).
I think it's also a nicer API than the current code which accesses the
raw param array.
I'm not sure whether the param stack should be regarded as an external
API for digester, though. While we can fix all param rules
(CallParamRule, ObjectParamRule, etc) we would break any variants of
these written by users if we do this. Unfortunately I think this is not
acceptable.
The question in my mind is still: what use cases require the
CallMethodRule to fire when the params are available?
One difficulty I have struct myself is that when using SetNextRule to
set up parent/child relations, the "obvious" rule ordering causes an
*uninitialised" child to be passed to the parent rather than an
initialised one because both CallMethodRule and SetNextRule do their
work in end(), and end() methods fire in the reverse order they were
added to the digester.
example:
digester.addObjectCreate("foo/bar", Widget.class);
digester.addCallMethodRule("foo/bar", "setId", 1);
digester.addCallParamRule("foo/bar", 0, "id");
digester.addSetNext("foo/bar");
this is wrong; the setNext executes before the setId method, so the
Widget is uninitialised when passed to its parent. Swapping the
CallMethodRule and SetNextRule order fixes this, but is definitely
non-intuitive.
Thoughts?
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] CallMethodRule order [bug12997] attn: Emmanuel,
Craig
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
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