DO NOT REPLY [Bug 35096] - [Digester][PATCH] Cannot add multiple CallMethodRules for the same pattern

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

DO NOT REPLY [Bug 35096] - [Digester][PATCH] Cannot add multiple CallMethodRules for the same pattern

Bugzilla from bugzilla@apache.org
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG?
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=35096>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND?
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=35096





------- Additional Comments From [hidden email]  2005-05-27 23:03 -------
(In reply to comment #2)
> Hi Rahul,
> Thanks very much for offering a patch for this. It is always good to see
people
> offering solutions rather than just problems :-).

I used Digester a couple of times recently, I found it quite nifty. Logically,
the next step was to try to fix the next problem reported on the mailing list.

> I do have some concerns about this approach though, that mean I personally
would
> prefer not to see this patch committed as-is. If you wished to work on it
some
> more, I'd be happy to help. However CallMethodRule has acted this way since
the
> very first release, so I don't see it being worthwhile to delay the next
release
> to get this in.

Sure, I can do multiple iterations based on feedback. Release descisions are
upto the Digester's RM.

> Of course others may see it differently; if any other digester committers are
> listening, please speak up!
> As I note below, a user rule should be able to do anything a built-in rule
can.
> So if this problem is solvable, then it should be solvable via a bunch of new
> Rule classes that can be downloaded separate from the commons-digester
release.
> That would be a good thing I think as people can use them and give feedback
> before the new functionality becomes part of the digester core. I think it
quite
> feasable to put such "extras" code up on the apache site somewhere for
people to
> download.

Thats one approach. I, personally, would never bother to download any extras
unless:
a) They were well-publicized
b) The problems they solve were clearly articulated
c) They solved a problem I was encountering
The revolutionary approach would be to demonstrate full backward
compatibility, and include in the distro.

> The concept of a stack of arrays was, I think, rather ugly in the first
place.
> Extending this by having a stack of lists of arrays isn't, in my opinion, the
> way to go.

OK, thats water under the bridge, here are the minimal requirements going
forward:
1) First, allow a call method rule to create its scratch space
2) Allow subsequent call param rules to populate the appropriate scratch space
3) Finally, allow the call method rule to consume (and release) the scratch
space
And this, without tripping on other call method rules (for the same pattern,
or otherwise).

> And while it really will be necessary to somehow link CallParamRule objects
to
> their associated CallMethodRule I don't think using integer indexes are the
> solution. It just feels clumsy.

I'm moderately surprised, primarily, due to the existence of paramIndex. I had
a similar reaction when I saw paramIndex, since I don't specify where an
argument should go on the parametes stack when making a function call, I just
supply the arguments in the right order. So I wondered why that index exists,
and figured out (as I wrote some Digesters), that one may want to order the
addRules by patterns, and in complex schemas, there are times when you
encounter params 'out of order' as you traverse the document structure. Now,
the trouble is, this was designed as a one-dimensional space on the parameters
stack in Digester (which is why it fails today), when it is, indeed, a two-
dimensional space. One index per dimension.

> Having any kind of rule treated "special" by the digester core is also
> undesirable. It is much nicer to have a core that just provides generic rule
> services, and then the rule classes are layered on top. Ok, apart from all
those
> factory methods that are also on the Digester class. Having this layering
means
> that a user rule can do anything that a built-in rule can. Currently this is
> true, and I'd like to see it stay that way.

My mistake, should have been more explicit in the TODO I added on top of the
instanceof. I do not want any special treatment for CallMethodRule, the rogue
bits in the core can be easily shifted to CallMethodRule. The purpose I put
them there, was to socialize those and get feedback of the form "hey, here's a
better way of figuring out this is the n'th callMethodRule added for pattern
foo/bar". We can put that bit in CallMethodRule, the setDigester isn't exactly
a side-effect free setter anyway.

> I was experimenting a while ago with the following:
> *
> having an interface (MethodInvoker or somesuch name) that the CallMethodRule
> implements.
> *
> having XXParamRule objects obtain the Rules object from the digester and
iterate
> over the list of Rule objects until they find themself. The immediately
> preceding MethodInvoker rule object is then the one that they associate
themself
> with. This can be done when setDigester is called, ie once only.

This can always be the default behavior when the callMethodRule index is not
specified. But again, I recommend that it be possible for a CallParamRule to
point to the CallMethodRule that it is refering too (I used index in document
order). If you buy paramIndex, this is a straight sell. Anytime we introduce a
default behavior which cannot be overridden, we're just digging ourselves
another hole. I recently had to call methods with interleaved params occuring
in the document structure, ofcourse, I had to tackle it with custom rules.
Which brings me to a broad comment, based on my use of Digester, in its
current state, its very useful for flat schemas, other than that you end up
writing a lot of custom rules.

> *
> having the MethodInvoker interface define method
>    setParameter(int i, Object value)
> that the various ParamRule objects could then call, rather than directly
> accessing a "parameter stack" data structure.

+1 for local storage

> *
> having each CallMethodRule (which implements MethodInvoker) manage its own
stack
> of parameter data. This could still be a stack of arrays internally, but it
is
> now a data structure private to the class, which is better than the current
> paramstack object which is exposed everywhere.
> The bit about XXParamRule classes scanning the list of rules to find the
> previous CallMethodRule means that code like this:
>   digester.addCallMethod(....);  // rule 1
>   digester.addCallParam(...);   // automatically associates with rule 1
>   digester.addCallMethod(....);  // rule 2
>   digester.addCallParam(...);   // automatically associates with rule 2
> works in an intuitive fashion. A CallParamRule associates with the most
recently
> added CallMethodRule.

But:
digester.addCallMethod(....);  // rule 1
digester.addCallParam(...);   // does not automatically associate with param 1
(1 relative)
digester.addCallParam(...);   // does not automatically associate with param 2
digester.addCallParam(...);   // does not automatically associate with param 3
Catch my drift?

> None of this should need changes to the digester core as far as I can
see;<snip>

Yes, didn't intend to cast any doubt on that.

> I haven't felt the enthusiasm to test out the idea, look for flaws, implement
> it, add unit tests, etc. for digester 1.x series.
> I certainly do intend to do *something* to address the existing
CallMethodRule
> issue though for the digester 2.x series. [which has no release date planned
yet].

I can propose a few iterations, if you wish, as and when I get some time.

(In reply to comment #3)
> One minor note: when attaching files, type=patch should only be used for text
> files that can be fed to the "patch" program, eg generated by "svn diff"
or "cvs
> diff". Zip files containing patches should be type=binary. Thanks.

Let me know if you want me to re-attach.

-Rahul


--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]