You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Simon Kitching <si...@ecnetwork.co.nz> on 2003/10/01 07:59:10 UTC
[digester] plugins module ready for evaluation
Hi,
Many many moons ago, I proposed a "plugins" extension for digester.
It is now ready for the world [yes, yes, brave words I know :-]
I have created a bugzilla entry and attached the source to it.
See http://issues.apache.org/bugzilla/show_bug.cgi?id=23537
The total runtime components are:
11 classes totalling 19.7kb when the classes are jarred.
[digester is currently 109kb without it].
Other code attached to the bugzilla entry:
unit tests (12)
examples (2 complete toy apps).
No additional dependencies on other libraries are introduced.
The bugzilla .tgz files should be uncompressed in the digester source
root directory.
I hope this is of interest; I'm really happy with how it has turned out
and think it would be of use to a fair number of projects. Anyone up for
porting Ant to this ?? :-)
This *could* be a candidate for the new "optionals" module, but
personally I think it is too useful for that, and really belongs as a
sibling to xmlrules functionality in the core. Still, over to you
maintainers to debate that one.
Any feedback welcome.
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] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
patch applied. (sorry it's been quite a while.)
- robert
On Monday, October 6, 2003, at 12:00 AM, Simon Kitching wrote:
>>>
>>> i'm also a bit confused why PluginDeclarationRule throws
>>> ClassNotFoundException's when require attributes are missing from the
>>> xml.
>>> this seems a wrong to me. (i've left these for the moment since it's
>>> easier for people to examine the code when it's in cvs.) there are also
>>> a
>>> few Exception's thrown. i'd prefer specific subclasses to be thrown
>>> since
>>> this allows users (if they wish) to diagnose the original problem.
>>
>> Well, I chose ClassNotFound partly because the result of these mandatory
>> attributes being missing is that we don't know which class to
>> instantiate. Isn't that sort of like ClassNotFound? :-)
>>
>> Any suggestions for superior options are welcome. I agree it could be
>> improved. This code was written before the PluginInvalidInputException
>> class existed; would this be more appropriate?
>>
>> If there are any raw "Exception" classes being thrown, these are just
>> remnants of code before cleanup and should definitely be fixed.
>
> Attached is a patch to fix the locations where ClassNotFoundException
> and Exception are being thrown. Class PluginInvalidInputException is
> thrown instead.
>
> Patch is based from digester root directory.
>
> 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] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
> >
> > i'm also a bit confused why PluginDeclarationRule throws
> > ClassNotFoundException's when require attributes are missing from the xml.
> > this seems a wrong to me. (i've left these for the moment since it's
> > easier for people to examine the code when it's in cvs.) there are also a
> > few Exception's thrown. i'd prefer specific subclasses to be thrown since
> > this allows users (if they wish) to diagnose the original problem.
>
> Well, I chose ClassNotFound partly because the result of these mandatory
> attributes being missing is that we don't know which class to
> instantiate. Isn't that sort of like ClassNotFound? :-)
>
> Any suggestions for superior options are welcome. I agree it could be
> improved. This code was written before the PluginInvalidInputException
> class existed; would this be more appropriate?
>
> If there are any raw "Exception" classes being thrown, these are just
> remnants of code before cleanup and should definitely be fixed.
Attached is a patch to fix the locations where ClassNotFoundException
and Exception are being thrown. Class PluginInvalidInputException is
thrown instead.
Patch is based from digester root directory.
Regards,
Simon
Re: [digester] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
> On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
<snip>
>> 7. Logs
>>
>> at the moment, each plugin class uses it's own and the log cannot be set.
>> i'm not in favour of this pattern for several reasons. most rules
>> implementations use the digester log and i'd prefer to switch as many
>> classes as possible to use the log and give the others setters and
>> getters.
>
> I disagree with this one. I think having a different logger on each
> class is one of the most wonderful features of logging, giving much
> better control of output.
>
> Having all logging going via single Log object means that there is no
> control over what gets logged and what doesn't for different components
> of Digester. And Digester generates some serious log output when
> enabled!
agreed.
the drawback of this approach is that limits the contexts in which
digester plug-ins can be used. some frameworks have good reasons to want
to be able to control where the output of the log goes.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Tuesday, October 21, 2003, at 08:04 AM, Craig R. McClanahan wrote:
<snip>
> For classes inside, consistency with past practice overwhelms any
> principle about the desireability of the practice--otherwise, you totally
> screw up any existing Digester user that expects his or her current log
> settings to continue to work when we add additional classes to the
> package. I'm also going to -1 any proposed class that gratuitously
> breaks backwards compatiblity (even though it is not an issue that will
> cause compile-time behavior differences) in this way. It's too late to
> change this, even if there were good reasons.
craig: i'm not sure i'm parsing you correctly here. you mean too late to
change the digester conventions, right? (IMHO it's not too late to change
the plug-ins stuff.)
i'm not sure that i feel quite so strongly about logging symantics as you
do. (i probably wouldn't have committed the stuff without changes if i'd
thought the issues were black-and-white rather than worthy of debate.) the
plugin package is self-contained and any differences in symantics cannot
effect users of the other packages. they might have expectations about the
way that plugins will handling logging but i'm not sure that it will break
the configurations typically used.
i'd be happy to see the Rule implementations follow the standard
convention and log to Digester.getLog(). (but someone would need to go
through and ensure that the calls were correctly prefixed and that the
levels were appropriate given that it's a multi-purpose log.)
one issue is that there is a *lot* of logging in the plugins packages
(that is not intended as a criticism). it seems to me that a lot of this
will be needed by users trying to debug their plug-ins but may obfuscate
in other cases. i've taken a look and there isn't any real precedent for
logging in matching Rules. there may be case for directing logging
messages from matching Rules to a separate Log (in the same way that
digester has saxLog and Log) since matching Rules get called a *lot* and
logging would tend to obfuscate if the problem lies elsewhere.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by "Craig R. McClanahan" <cr...@apache.org>.
Simon Kitching wrote:
>On Tue, 2003-10-21 at 17:36, Craig R. McClanahan wrote:
>
>
>>Simon Kitching wrote:
>>
>>
>>>I'm looking at this from the point-of-view of a contributor to Digester,
>>>not a user (hence the dev list address). The problem is that in order to
>>>integrate with this solution, every class written for the Digester which
>>>needs to log output *must* do:
>>>
>>> digester.getLog().warn("oops")
>>>
>>>rather than
>>>
>>> Logger log = LogFactory.getLogger(ThisClass.class)
>>> ..
>>> log.warn("oops")
>>>
>>>
>>>
>>>
>>>
>>Why? The first approach is only required if you want your log messages
>>to go to the same Log instance that Digester is using. If you don't
>>care, don't bother.
>>
>>
>
>Hmm .. we don't seem to be connecting here.
>
>The original cause of this thread was that I wrote a new set of classes
>for Digester (plugins) and submitted them for inclusion in the base
>Digester distribution. In these classes I use a Log category created via
>LogFactory. Robert Donkin told me off (nicely) for not using
>digester.getLog().
>
>The easiest path for me is to accept Robert's solution and change the
>code (which has been committed by Robert to the digester cvs repository
>with the controversial code currently intact).
>
>However it seems to me that the current logging approach is broken (or
>at least very clumsy), for the reasons I have put forward in the
>previous emails [and are summarized at the end of this mail].
>
>Rather than change my code to comply with what I think is an incorrect
>API, I would like to discuss this and maybe come up with a better
>solution. Or maybe I will just learn why the current solution is
>necessary. So far my original questions have not been answered, though.
>
>It may well be that my proposed solution (just use commons-logging's
>LogFactory functionality) is broken too (does not support the needs of
>users like Tomcat and Avalon).
>
>
>
That explanation makes life really simple. The issues are *totally*
different for classes inside commons-digester (more specifically, inside
the org.apache.commons.digester) than outside.
For classes inside, consistency with past practice overwhelms any
principle about the desireability of the practice--otherwise, you
totally screw up any existing Digester user that expects his or her
current log settings to continue to work when we add additional classes
to the package. I'm also going to -1 any proposed class that
gratuitously breaks backwards compatiblity (even though it is not an
issue that will cause compile-time behavior differences) in this way.
It's too late to change this, even if there were good reasons.
For classes outside the org.apache.commons.digester package, do whatever
you want. Digester doesn't care, and I don't care.
Craig
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Tue, 2003-10-21 at 17:36, Craig R. McClanahan wrote:
> Simon Kitching wrote:
> >I'm looking at this from the point-of-view of a contributor to Digester,
> >not a user (hence the dev list address). The problem is that in order to
> >integrate with this solution, every class written for the Digester which
> >needs to log output *must* do:
> >
> > digester.getLog().warn("oops")
> >
> >rather than
> >
> > Logger log = LogFactory.getLogger(ThisClass.class)
> > ..
> > log.warn("oops")
> >
> >
> >
>
> Why? The first approach is only required if you want your log messages
> to go to the same Log instance that Digester is using. If you don't
> care, don't bother.
Hmm .. we don't seem to be connecting here.
The original cause of this thread was that I wrote a new set of classes
for Digester (plugins) and submitted them for inclusion in the base
Digester distribution. In these classes I use a Log category created via
LogFactory. Robert Donkin told me off (nicely) for not using
digester.getLog().
The easiest path for me is to accept Robert's solution and change the
code (which has been committed by Robert to the digester cvs repository
with the controversial code currently intact).
However it seems to me that the current logging approach is broken (or
at least very clumsy), for the reasons I have put forward in the
previous emails [and are summarized at the end of this mail].
Rather than change my code to comply with what I think is an incorrect
API, I would like to discuss this and maybe come up with a better
solution. Or maybe I will just learn why the current solution is
necessary. So far my original questions have not been answered, though.
It may well be that my proposed solution (just use commons-logging's
LogFactory functionality) is broken too (does not support the needs of
users like Tomcat and Avalon).
> Why? The first approach is only required if you want your log messages
> to go to the same Log instance that Digester is using. If you don't
> care, don't bother.
>
It doesn't make much sense to me to provide a framework with a way to
control *part* of the digester library's logging output. Someone who
uses the Plugins functionality would be bound to be confused if the
setLog() call redirected output generated by CallMethodRule instances,
but not output generated by PluginCreateRule instances...
Surely either *all* code that is part of the digester library (ie all
that stuff in cvs) should use digester.getLog().warn(...), or none of it
should.
> And, without a getLog() call, it would not even be *possible* to share
> the same Log instance, which is what the Avalon folks were complaining
> about.
Sharing a Log instance is a solution.
What exactly is the problem or requirement that the Avalon folk had?
Did they want to be able to redirect log output emitted by all instances
of classes within the org.apache.commons.digester package?
If so, isn't that achievable with a custom LogFactory class? Or even
with a standard LogFactory class, by retrieving the appropriate Log
object and configuring it? [this would require downcasting to the actual
Log implementation class as commons-logging does not provide Log object
configuration methods].
>
> >The first implication is that with this pattern, *every* object which
> >wants to log output *requires* a reference to whatever object is the
> >"holder" of the master Log object (a Digester instance in this case).
> >
> >
> >
> Even if this were a *must* requirement, how many objects n the Digester
> API don't have a Digester instance already? That's right, basically
> none of them :-).
Well, yes. However isn't this an issue that all commons components face?
Commons components are almost by definition intended to be called by
frameworks. And they *should* be using logging.
It is possible to say: hey, let's solve *our* problem by crafting a
solution that works for Digester alone. I feel that this is what the
current setLog() method does.
It does seem nicer, if possible, to come up with a solution to this
problem that can be applied to all commons components. Or better yet
steal a solution from some commons component which has "got it right"
with an approach that works for all libraries, not just one with certain
object relationship topologies (woo - big words!)
It means, for a start, that users of commons components have a single
consistent mechanism for controlling the logging of a commons component,
rather than "oh, Digester has a setLog methog, but Net has this
approach, and HiveMind has that approach".
I did look at collections, and found that they resolve the issue by not
having any logging at all :-). I've not had time to look into the other
components.
Have the Avalon team complained about logging in commons-net? Presumably
not. But if they do, and find that they can't put a setLog() method
anywhere in Net because there is no centrally-accessable object to store
the master Log object on, then what? A solution for Net which solves the
same problem, but is implemented in a totally different manner? ecch...
> Centralized logging for Digester is used by the standard Digester
> classes to minimize the number of log names you have to administer.
> Whether you utilize it in your own code (or whether your own code is
> even aware that the possibility exists) is totally up to you.
"Minimize the number of log names you have to administer".
Isn't that what Catalog hierarchies are for? Configure the
"org.apache.commons.digester" hierarchy and you have set defaults for
all Categories below it. One config entry qualifies as "minimize" to me
:-)
And by "configure", I mean either configure one of the standard Log
implementations (eg log4j's), or replace it with a custom one (such as
an AvalonLog object).
Quick summary of the issues I have with the current solution:
[1] all logging goes through one category so it is not possible to
isolate stuff like "log debug messages from RulesBase, info for
everything else".
[2] objects can't log anything until after setDigester is called.
[3] objects which don't have an explicit reference to a digester
(for example a custom org.xml.sax.ErrorHandler object) can't
log.
[4] the solution isn't consistent with solutions to the same
problem in other commons projects - and can't be, because
many of those projects won't have a place to store a central
Log instance.
[5] in applications which use many commons components, custom logging
will need to be configured for each one in turn, rather than
doing it once.
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] plugins module ready for evaluation
Posted by "Craig R. McClanahan" <cr...@apache.org>.
Simon Kitching wrote:
>On Tue, 2003-10-21 at 11:26, Craig R. McClanahan wrote:
>
>
>>Simon Kitching wrote:
>>
>>
>
>Regarding your "tomcat" example, I will have to have a think about this.
>I'm no expert on complex container architectures, nor on Tomcat, so if
>you and the Avalon team say the setLog() approach is the only way to go,
>I'll believe you.
>
>Nevertheless, the current approach *feels* wrong to me. I'm just trying
>to understand why it is necessary to do logging in this way.
>
>
>
>
>>If you go back in the commons-dev message archives, you'll find that the
>>reason setLog exists in the first place is because the Avalon folks (who
>>know a little about IoC :-) said they needed it. You seem to be arguing
>>the opposite position, which is kinda interesting. But, at the end of
>>the day, I can't see why having an optional method to set the Log object
>>a Digester instance can use causes you any grief if you never call it.
>>
>>
>
>The problem is not that the method exists on the Digester class.
>
>I'm looking at this from the point-of-view of a contributor to Digester,
>not a user (hence the dev list address). The problem is that in order to
>integrate with this solution, every class written for the Digester which
>needs to log output *must* do:
>
> digester.getLog().warn("oops")
>
>rather than
>
> Logger log = LogFactory.getLogger(ThisClass.class)
> ..
> log.warn("oops")
>
>
>
Why? The first approach is only required if you want your log messages
to go to the same Log instance that Digester is using. If you don't
care, don't bother.
And, without a getLog() call, it would not even be *possible* to share
the same Log instance, which is what the Avalon folks were complaining
about.
>The first implication is that with this pattern, *every* object which
>wants to log output *requires* a reference to whatever object is the
>"holder" of the master Log object (a Digester instance in this case).
>
>
>
Even if this were a *must* requirement, how many objects n the Digester
API don't have a Digester instance already? That's right, basically
none of them :-).
>This looks like undesirable coupling to me; I find it difficult to
>believe that many other projects would be *able* to implement this
>pattern. For example, the commons-net project: does every object have a
>reference to some "master" object that can be used to hold a central Log
>object?
>
>Also see the "xmlrules" module. This creates new Digester objects to
>parse the xmlrules input files, but doesn't copy the set Log object. So
>currently xmlrules is broken with respect to Log behaviour: there is no
>way for the "framework" to control the Log object used by the Digester
>instances created by xmlrules. While this could be considered a bug in
>xmlrules, I think it shows a flaw in the concept of a centralised "Log"
>object attached to some "master" object; the log control doesn't
>automatically propagate, but instead requires explicit code.
>
>The second implication of the "centralised Log object" pattern is that
>there is a single Category for all output generated by all classes in
>the Digester project. This seems a shame. It may be acceptable for a
>project of Digester size (20-30 classes), but I presume that there is a
>limit beyond which this would not be acceptable. Does Tomcat use a
>single Category to output all of its logging?
>
>
>
Centralized logging for Digester is used by the standard Digester
classes to minimize the number of log names you have to administer.
Whether you utilize it in your own code (or whether your own code is
even aware that the possibility exists) is totally up to you.
>Regards,
>
>Simon
>
>
>
Craig
>---------------------------------------------------------------------
>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] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Tue, 2003-10-21 at 11:26, Craig R. McClanahan wrote:
> Simon Kitching wrote:
Regarding your "tomcat" example, I will have to have a think about this.
I'm no expert on complex container architectures, nor on Tomcat, so if
you and the Avalon team say the setLog() approach is the only way to go,
I'll believe you.
Nevertheless, the current approach *feels* wrong to me. I'm just trying
to understand why it is necessary to do logging in this way.
> If you go back in the commons-dev message archives, you'll find that the
> reason setLog exists in the first place is because the Avalon folks (who
> know a little about IoC :-) said they needed it. You seem to be arguing
> the opposite position, which is kinda interesting. But, at the end of
> the day, I can't see why having an optional method to set the Log object
> a Digester instance can use causes you any grief if you never call it.
The problem is not that the method exists on the Digester class.
I'm looking at this from the point-of-view of a contributor to Digester,
not a user (hence the dev list address). The problem is that in order to
integrate with this solution, every class written for the Digester which
needs to log output *must* do:
digester.getLog().warn("oops")
rather than
Logger log = LogFactory.getLogger(ThisClass.class)
..
log.warn("oops")
The first implication is that with this pattern, *every* object which
wants to log output *requires* a reference to whatever object is the
"holder" of the master Log object (a Digester instance in this case).
This looks like undesirable coupling to me; I find it difficult to
believe that many other projects would be *able* to implement this
pattern. For example, the commons-net project: does every object have a
reference to some "master" object that can be used to hold a central Log
object?
Also see the "xmlrules" module. This creates new Digester objects to
parse the xmlrules input files, but doesn't copy the set Log object. So
currently xmlrules is broken with respect to Log behaviour: there is no
way for the "framework" to control the Log object used by the Digester
instances created by xmlrules. While this could be considered a bug in
xmlrules, I think it shows a flaw in the concept of a centralised "Log"
object attached to some "master" object; the log control doesn't
automatically propagate, but instead requires explicit code.
The second implication of the "centralised Log object" pattern is that
there is a single Category for all output generated by all classes in
the Digester project. This seems a shame. It may be acceptable for a
project of Digester size (20-30 classes), but I presume that there is a
limit beyond which this would not be acceptable. Does Tomcat use a
single Category to output all of its logging?
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] plugins module ready for evaluation
Posted by "Craig R. McClanahan" <cr...@apache.org>.
Simon Kitching wrote:
>On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote:
>
>
>>On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>>
>><snip>
>>
>>
>>
>>>Hmm .. but calling Digester.setLogger probably doesn't override the
>>>object known to the LogFactory...
>>>
>>>What exactly is the purpose of being able to set the Log object used by
>>>a class or instance?
>>>
>>>
>>so the instance used to log by digester can be set programmatically at
>>runtime. this is a useful feature because:
>>
>>1. it's a very convenient way to bypass the usual commons-logging
>>configuration infrastructure. for example, it's often easier (when
>>debugging) to setLogger programmatically on a particular instance than to
>>reconfigure everything.
>>
>>
>
>Easier than changing the logging config file for the appropriate logging
>implementation (eg the log4j.properties or log4j.xml file)?
>
>
>
No.
Consider a use case like Tomcat, where Digester is used both by the
container (to parse server.xml and web.xml files) and also by an
application (say, Struts reading struts-config.xml). It's entirely
reasonable to for someone to want the digester log output for the
container's use of Digester to go to the container's logging hierarchy,
and the webapp's use of Digester to log to a webapp hierarchy.
>>2. it allows digester to participate properly in frameworks based on
>>inversion-of-control. (frameworks of this type are configured and
>>controlled in a parent-child fashion. the Log to be used by a digester
>>should be controlled by the component owning digester)
>>
>>
>
>This approach requires:
>
>(a)
>The framework to call setLog on each component used by the framework
>which has a dedicated Log object.
>
*Allows* not *requires*. If you don't call setLog() you get the default
names.
> Imagine a framework which uses
>Digester + Net + CLI, where each component works like Digester,
>requiring the user to call setLog on each one in turn.
>
>(b)
>Requiring classes to log via some object stored on some "main" object in
>the component, like all classes in the Digester project are required to
>get the Log object from their owning Digester object. In fact, in many
>projects this will not be feasable; it only works in Digester because
>every object of interest happens to have a reference to an owning
>Digester object.
>
>
>
*Allows* not *requires*. If you don't call setLog() you get the default
names.
>Isn't it much cleaner for the calling framework to set up an appropriate
>implementation of org.apache.commons.loggging.Logfactory that does
>whatever the "framework" wants with respect to logging? If the calling
>framework wants all logging (regardless of category) to go to one
>destination, then it creates a LogFactory implementation which returns a
>single Log object always, and that Log object's implementation writes to
>the desired destination.
>
>
>
Can you show me how you'd solve the problem posed above about Tomcat?
Especially in the case where you're running in a restricted environment
where you are not allowed to modify the LogFactory in the container itself?
>All those getLog and setLog calls go away. Objects don't need to have
>references to the "master" object which holds the Log object to use.
>Objects log via their own Category, so that filtering can be applied
>appropriately.
>
>
>
>Thoughts??
>
>Regards,
>
>Simon
>
>
>
If you go back in the commons-dev message archives, you'll find that the
reason setLog exists in the first place is because the Avalon folks (who
know a little about IoC :-) said they needed it. You seem to be arguing
the opposite position, which is kinda interesting. But, at the end of
the day, I can't see why having an optional method to set the Log object
a Digester instance can use causes you any grief if you never call it.
Craig
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Monday, October 20, 2003, at 10:46 PM, Simon Kitching wrote:
> On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote:
>> On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>>
>> <snip>
>>
>>> Hmm .. but calling Digester.setLogger probably doesn't override the
>>> object known to the LogFactory...
>>>
>>> What exactly is the purpose of being able to set the Log object used by
>>> a class or instance?
>>
>> so the instance used to log by digester can be set programmatically at
>> runtime. this is a useful feature because:
>>
>> 1. it's a very convenient way to bypass the usual commons-logging
>> configuration infrastructure. for example, it's often easier (when
>> debugging) to setLogger programmatically on a particular instance than to
>> reconfigure everything.
>
> Easier than changing the logging config file for the appropriate logging
> implementation (eg the log4j.properties or log4j.xml file)?
this allows some things to be done easily that otherwise would be
difficult:
1a different instances can use different log systems;
1b different instances can use different log levels (this is particularly
useful in complex systems);
>> 2. it allows digester to participate properly in frameworks based on
>> inversion-of-control. (frameworks of this type are configured and
>> controlled in a parent-child fashion. the Log to be used by a digester
>> should be controlled by the component owning digester)
>
> This approach requires:
>
> (a)
> The framework to call setLog on each component used by the framework
> which has a dedicated Log object. Imagine a framework which uses
> Digester + Net + CLI, where each component works like Digester,
> requiring the user to call setLog on each one in turn.
this approach is advocated by many designers of (usually heavyweight)
frameworks. these kinds of framework have their own configuration
mechanisms which configure each component in turn.
> (b)
> Requiring classes to log via some object stored on some "main" object in
> the component, like all classes in the Digester project are required to
> get the Log object from their owning Digester object. In fact, in many
> projects this will not be feasable; it only works in Digester because
> every object of interest happens to have a reference to an owning
> Digester object.
yes and no :)
logging to a central object only works when a component has a central
object. the alternative for components which consist of loose graphs is
typically having setters and getters for each (principal) object.
> Isn't it much cleaner for the calling framework to set up an appropriate
> implementation of org.apache.commons.loggging.Logfactory that does
> whatever the "framework" wants with respect to logging? If the calling
> framework wants all logging (regardless of category) to go to one
> destination, then it creates a LogFactory implementation which returns a
> single Log object always, and that Log object's implementation writes to
> the desired destination.
1. this pattern has been found to be very powerful. allowing components to
be configure by setting properties is both common and clean.
2. IMHO library code should seek to work flexible with different designs
of framework.
> All those getLog and setLog calls go away. Objects don't need to have
> references to the "master" object which holds the Log object to use.
> Objects log via their own Category, so that filtering can be applied
> appropriately.
i'm very keen about leaving the door open for users to integrate digester
into frameworks using IOC and so i'm probably (sooner or later) going to
add getters and setters for each log.
the real question (as far as i'm concerned) is whether each class needs it'
s own logger (with a getter and setter) or whether it's better (in this
case) to follow the digester conventions and feed some (or all?) of these
into the central digester log. i'd be interested to hear other opinions on
this.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
An recurring problem(extracting one property out of an ArrayList containing a Bean)
Posted by Yansheng Lin <ya...@isogis.com>.
Hi,
Is there a method in the BeanUtils package that allows you to extract one
property as a String[] out of an ArrayList containing a collection of Beans?
The only thing I found that was close to what I want is:
public static String[] getArrayProperty(Object bean, String
name)
throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException {}
, but I need something that takes an ArrayList instead of a Bean Object....
Any hints appreciated!
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Tue, 2003-10-21 at 10:29, robert burrell donkin wrote:
> On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>
> <snip>
>
> > Hmm .. but calling Digester.setLogger probably doesn't override the
> > object known to the LogFactory...
> >
> > What exactly is the purpose of being able to set the Log object used by
> > a class or instance?
>
> so the instance used to log by digester can be set programmatically at
> runtime. this is a useful feature because:
>
> 1. it's a very convenient way to bypass the usual commons-logging
> configuration infrastructure. for example, it's often easier (when
> debugging) to setLogger programmatically on a particular instance than to
> reconfigure everything.
Easier than changing the logging config file for the appropriate logging
implementation (eg the log4j.properties or log4j.xml file)?
>
> 2. it allows digester to participate properly in frameworks based on
> inversion-of-control. (frameworks of this type are configured and
> controlled in a parent-child fashion. the Log to be used by a digester
> should be controlled by the component owning digester)
This approach requires:
(a)
The framework to call setLog on each component used by the framework
which has a dedicated Log object. Imagine a framework which uses
Digester + Net + CLI, where each component works like Digester,
requiring the user to call setLog on each one in turn.
(b)
Requiring classes to log via some object stored on some "main" object in
the component, like all classes in the Digester project are required to
get the Log object from their owning Digester object. In fact, in many
projects this will not be feasable; it only works in Digester because
every object of interest happens to have a reference to an owning
Digester object.
Isn't it much cleaner for the calling framework to set up an appropriate
implementation of org.apache.commons.loggging.Logfactory that does
whatever the "framework" wants with respect to logging? If the calling
framework wants all logging (regardless of category) to go to one
destination, then it creates a LogFactory implementation which returns a
single Log object always, and that Log object's implementation writes to
the desired destination.
All those getLog and setLog calls go away. Objects don't need to have
references to the "master" object which holds the Log object to use.
Objects log via their own Category, so that filtering can be applied
appropriately.
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] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
<snip>
> Hmm .. but calling Digester.setLogger probably doesn't override the
> object known to the LogFactory...
>
> What exactly is the purpose of being able to set the Log object used by
> a class or instance?
so the instance used to log by digester can be set programmatically at
runtime. this is a useful feature because:
1. it's a very convenient way to bypass the usual commons-logging
configuration infrastructure. for example, it's often easier (when
debugging) to setLogger programmatically on a particular instance than to
reconfigure everything.
2. it allows digester to participate properly in frameworks based on
inversion-of-control. (frameworks of this type are configured and
controlled in a parent-child fashion. the Log to be used by a digester
should be controlled by the component owning digester)
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] assertion mechanism throws Error [was: plugins module
ready for evaluation]
Posted by "Craig R. McClanahan" <cr...@apache.org>.
Simon Kitching wrote:
>On Tue, 2003-10-28 at 02:53, robert burrell donkin wrote:
>
>
>>On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>>
>>
>>
>>>On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
>>>
>>>
>><snip>
>>
>>
>>
>>>>3. Exception handling
>>>>
>>>>PluginAssertionError extends Error. i'm not very happy about this. IMHO
>>>>a
>>>>well behaved component should not throw any Error subclasses. i'd be
>>>>happier for PluginAssertionError to be renamed PluginAssertionException
>>>>and extend RuntimeException.
>>>>
>>>>
>>>Having PluginAssertionError extend Error was a very deliberate choice.
>>>It is modelled after the java 1.4 assertion mechanism, where an
>>>assertion failure generates AssertionError.
>>>
>>>PluginAssertionError is never thrown due to a mistake on the part of the
>>>user of the Plugins module, nor due to a mistake in the xml of any sort.
>>>It is only thrown when a bug in the plugins library is detected.
>>>
>>>
>>i probably should clarify my opinion: no well behaved component should
>>throw an Error in production. in many context's the throwing of an Error
>>in production can be pretty serious. in a J2EE context, a container may
>>mark a bean or a thread which throws an Error as dead. in a production
>>environment, this may cause problems with these pooled resources.
>>
>>in a way, though finding a bug in the plugins library in production may
>>irritate a user a little, i'm pretty sure that they will be very angry if
>>a bug in the plugins library causes their J2EE application to fail through
>>resources starvation. so, i'm not too sure what the best way to handle
>>this situation is but i don't think that throwing an Error is the right
>>thing to do. maybe, it could be replaced with a runtime.
>>
>>
>
>I see your point. However it seems to me that what you are arguing is
>that the *official* java assertion mechanism is fatally flawed, and will
>never be acceptable in Apache code. That might be right, but is a pretty
>significant decision.
>
>I just feel that the fellows who designed java assertions are pretty
>smart, and must have had a reason for deciding to define AssertionError
>instead of AssertionException. Rather than assume I'm smarter than them,
>I followed their lead on this issue.
>
>Do you think it is worth opening up this discussion more widely on
>commons-dev? What do the Avalon or Tomcat people think about the
>java 1.4 assertion model?
>
>
>
As a (basically) former Tomcat person, I expect most Tomcat developers
would consider the assertion mechanism irrelevant at this point in time,
because Tomcat needs to run on JDK 1.3 (and even 1.2).
>I understand it is not a pressing problem for Apache developers as
>assertions require a minimum of java1.4; while 1.2 or 1.3 support is
>required of apache projects it is only a theoretical problem - unless
>there are people like me who are fans of assertions.
>
>
>
I'm *not* a fan of assertions in my production code, at least partly
because of this issue. I'm a big fan of assertions (JUnit style) in my
unit tests.
>I'm happy to change to using a RuntimeException subclass if that is the
>general consensus.
>
>
>
+1.
IMHO, things like IllegalArgumentException and NullPointerExcepton are
not "Errors" for a reason; they are "Exceptions" -- caused by either the
user botching their use of your class, or your class botching its
specified contracts. In most cases, they represent conditions that you
should be able to, or should at least try to, recover from.
Error is (to quote the Javadocs) "a subclass of Throwable that indicates
serious problems that a reasonable application should not try to
catch." Lumping AssertionError into that definition is pretty
problematic -- I'd probably never knowingly run an app with assertions
enabled in production.
>Simon
>
>
>
Craig
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] assertion mechanism throws Error [was: plugins
module ready for evaluation]
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Tue, 2003-10-28 at 12:12, robert burrell donkin wrote:
> On Monday, October 27, 2003, at 10:41 PM, Simon Kitching wrote:
Just a note: I'm mostly on the side of a RuntimeException-based approach
already. I just proposed an Error-based exception to ensure the issue
was debated. A shame the "debate" is Robert & me so far..
If no-one can mount a good defence of the Java-1.4-like Error-based
approach, I will happily prepare a patch.
[Robert: If you could commit the logging patch submitted earlier today,
that would make it easier to create the patch, hint, hint :-].
>
> i like assertions but i don't want error's thrown from library code when
> it's used in production.
Well, I'll have a final stab at arguing the toss on this topic..
It seems the difference between the existing plugins code and the
equivalent implementation using java 1.4 assertions in "production" mode
is:
* the current plugins approach would throw Error, which may have nasty
side-effects on the container, vs
* the java 1.4 assertion mechanism would have been disabled, so the code
would zoom straight past the point at which the assertion would have
detected invalid state, and kept on going with unpredictable
consequences.
It's tough to have to choose between these options!
NB: If the java 1.4 assertions are not disabled, then the two
implementations behave identically.
Personally, I think the JSR assertion group got it wrong. Assertion
failures should indeed throw a RuntimeException subclass. Then it would
not be any great problem to leave assertions enabled in production code
(providing the performance hit was acceptable) without the nasty
problems that Robert described.
I didn't want to say so at first, because a "RuntimeException" approach
would probably have been accepted without comment, and I hoped to get
some comment for/against the two options.
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] assertion mechanism throws Error [was: plugins module ready for evaluation]
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Monday, October 27, 2003, at 10:41 PM, Simon Kitching wrote:
> On Tue, 2003-10-28 at 02:53, robert burrell donkin wrote:
>> On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>>
>>> On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
>>
>> <snip>
>>
>>>> 3. Exception handling
>>>>
>>>> PluginAssertionError extends Error. i'm not very happy about this.
>>>> IMHO
>>>> a
>>>> well behaved component should not throw any Error subclasses. i'd be
>>>> happier for PluginAssertionError to be renamed
>>>> PluginAssertionException
>>>> and extend RuntimeException.
>>>
>>> Having PluginAssertionError extend Error was a very deliberate choice.
>>> It is modelled after the java 1.4 assertion mechanism, where an
>>> assertion failure generates AssertionError.
>>>
>>> PluginAssertionError is never thrown due to a mistake on the part of the
>>> user of the Plugins module, nor due to a mistake in the xml of any sort.
>>> It is only thrown when a bug in the plugins library is detected.
>>
>> i probably should clarify my opinion: no well behaved component should
>> throw an Error in production. in many context's the throwing of an Error
>> in production can be pretty serious. in a J2EE context, a container may
>> mark a bean or a thread which throws an Error as dead. in a production
>> environment, this may cause problems with these pooled resources.
>>
>> in a way, though finding a bug in the plugins library in production may
>> irritate a user a little, i'm pretty sure that they will be very angry if
>> a bug in the plugins library causes their J2EE application to fail
>> through
>> resources starvation. so, i'm not too sure what the best way to handle
>> this situation is but i don't think that throwing an Error is the right
>> thing to do. maybe, it could be replaced with a runtime.
>
> I see your point. However it seems to me that what you are arguing is
> that the *official* java assertion mechanism is fatally flawed, and will
> never be acceptable in Apache code. That might be right, but is a pretty
> significant decision.
>
> I just feel that the fellows who designed java assertions are pretty
> smart, and must have had a reason for deciding to define AssertionError
> instead of AssertionException. Rather than assume I'm smarter than them,
> I followed their lead on this issue.
they were smart enough to allow assertions to be turned off for production
:)
i had considered suggesting a system property flag but it seems like it'd
require a lot of explanation for such a small feature.
> Do you think it is worth opening up this discussion more widely on
> commons-dev? What do the Avalon or Tomcat people think about the
> java 1.4 assertion model?
the place where this kind of thing used to be debated was on general.
(commons-dev is pretty high volume and i'd say it's a bit impolite posting
to everyone.) but there are quite a few people who read digester who might
like to jump in now.
> I understand it is not a pressing problem for Apache developers as
> assertions require a minimum of java1.4; while 1.2 or 1.3 support is
> required of apache projects it is only a theoretical problem - unless
> there are people like me who are fans of assertions.
i like assertions but i don't want error's thrown from library code when
it's used in production.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] assertion mechanism throws Error [was: plugins
module ready for evaluation]
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Tue, 2003-10-28 at 02:53, robert burrell donkin wrote:
> On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
>
> > On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
>
> <snip>
>
> >> 3. Exception handling
> >>
> >> PluginAssertionError extends Error. i'm not very happy about this. IMHO
> >> a
> >> well behaved component should not throw any Error subclasses. i'd be
> >> happier for PluginAssertionError to be renamed PluginAssertionException
> >> and extend RuntimeException.
> >
> > Having PluginAssertionError extend Error was a very deliberate choice.
> > It is modelled after the java 1.4 assertion mechanism, where an
> > assertion failure generates AssertionError.
> >
> > PluginAssertionError is never thrown due to a mistake on the part of the
> > user of the Plugins module, nor due to a mistake in the xml of any sort.
> > It is only thrown when a bug in the plugins library is detected.
>
> i probably should clarify my opinion: no well behaved component should
> throw an Error in production. in many context's the throwing of an Error
> in production can be pretty serious. in a J2EE context, a container may
> mark a bean or a thread which throws an Error as dead. in a production
> environment, this may cause problems with these pooled resources.
>
> in a way, though finding a bug in the plugins library in production may
> irritate a user a little, i'm pretty sure that they will be very angry if
> a bug in the plugins library causes their J2EE application to fail through
> resources starvation. so, i'm not too sure what the best way to handle
> this situation is but i don't think that throwing an Error is the right
> thing to do. maybe, it could be replaced with a runtime.
I see your point. However it seems to me that what you are arguing is
that the *official* java assertion mechanism is fatally flawed, and will
never be acceptable in Apache code. That might be right, but is a pretty
significant decision.
I just feel that the fellows who designed java assertions are pretty
smart, and must have had a reason for deciding to define AssertionError
instead of AssertionException. Rather than assume I'm smarter than them,
I followed their lead on this issue.
Do you think it is worth opening up this discussion more widely on
commons-dev? What do the Avalon or Tomcat people think about the
java 1.4 assertion model?
I understand it is not a pressing problem for Apache developers as
assertions require a minimum of java1.4; while 1.2 or 1.3 support is
required of apache projects it is only a theoretical problem - unless
there are people like me who are fans of assertions.
I'm happy to change to using a RuntimeException subclass if that is the
general consensus.
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Sunday, October 5, 2003, at 10:53 PM, Simon Kitching wrote:
> On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
<snip>
>> 3. Exception handling
>>
>> PluginAssertionError extends Error. i'm not very happy about this. IMHO
>> a
>> well behaved component should not throw any Error subclasses. i'd be
>> happier for PluginAssertionError to be renamed PluginAssertionException
>> and extend RuntimeException.
>
> Having PluginAssertionError extend Error was a very deliberate choice.
> It is modelled after the java 1.4 assertion mechanism, where an
> assertion failure generates AssertionError.
>
> PluginAssertionError is never thrown due to a mistake on the part of the
> user of the Plugins module, nor due to a mistake in the xml of any sort.
> It is only thrown when a bug in the plugins library is detected.
i probably should clarify my opinion: no well behaved component should
throw an Error in production. in many context's the throwing of an Error
in production can be pretty serious. in a J2EE context, a container may
mark a bean or a thread which throws an Error as dead. in a production
environment, this may cause problems with these pooled resources.
in a way, though finding a bug in the plugins library in production may
irritate a user a little, i'm pretty sure that they will be very angry if
a bug in the plugins library causes their J2EE application to fail through
resources starvation. so, i'm not too sure what the best way to handle
this situation is but i don't think that throwing an Error is the right
thing to do. maybe, it could be replaced with a runtime.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [digester] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Sun, 2003-10-05 at 00:43, robert burrell donkin wrote:
> hi simon
>
> i've committed your contribution (with a few changes). many thanks. would
> you like to write up something more detailed for the apache newsletter
> (ASAP)? (you'll find this on the wiki
> http://nagoya.apache.org/wiki/apachewiki.cgi?ApacheNewsletterDrafts/Issue2)
> ?
Excellent! Thanks for the encouragement to get this far.
I will write up an article for the newsletter today (noting that the
code is undergoing review).
>
> i have found a few issues that (whilst they did not stop the code being
> committed) i think should be resolved before we could think about
> releasing the plugins code. i'd like to encourage people to comment :)
Of course.
>
> 2. Re: PluginRules.add(String pattern, Rule rule)
>
> try {
> ((InitializableRule)rule).postRegisterInit(pattern);
> } catch (PluginConfigurationException e) {
> // Currently, Digester doesn't handle exceptions well
> // from the add method. The workaround is for the
> // initialisable rule to remember that its initialisation
> // failed, and to throw the exception when begin is
> // called for the first time.
> log.debug("Rule initialisation failed", e);
> // throw e; -- alas, can't do this
> return;
> }
>
> exception handling tends to be a difficult issue for reusable components.
> digester tends not to throw generic catchable exceptions. (the issue of
> exception handling has been debated ad nauseum on the commons list many
> times in many contexts.) IMHO the arguments in favour of adding a check
> exception to Rules.add are not strong enough to consider breaking
> backwards compatibility.
Fair enough. Maybe add exceptions to Rules methods for Digester 2.0??
:-)
>
> there are two reasonable approaches that plug-ins could take. the first
> (which is what the current code does) is to silently fail. IMHO this is
> probably the wrong behaviour in this case. why? well, it seems to me that
> a typical reason that the InitializableRule may throw an exception is that
> it is not able to function. this suggest to me that really in this case it
> is not unreasonable to assume that running any digestion from the current
> ruleset may well be caused to fail. it will be more difficult to recover
> and diagnose a problem at this time.
Amen to that. I think silent failure stinks as an option.
If you have a look at the code for the sole InitializableRule instance,
"PluginCreateRule", you will see that the current code does not
"silently fail". An exception is guaranteed to be generated; it just
happens at a later time than the problem is detected. It is still
impossible to use a misconfigured PluginCreateRule object without
getting an exception.
Ok, it does require the "InitializableRule" to recognise that the
exception it throws may not be honoured. However the alternative is to
throw a RuntimeException (which is discussed below), or to have the
postRegisterInit method not throw an exception at all, which to me
obscures the fact that it is recognised that initialisation may fail.
None of the options I came up with were terribly appealing; the current
implementation is what I think is the "least worst" option, given that
no method in the Rules interface can report initialisation errors.
It would be great to find a better solution....but I'm not sure a
RuntimeException is it (see below).
>
> the other approach is to throw a runtime exception subclass. i'd probably
> support this approach.
Currently, I (and many others I expect) do this:
try {
digester.parse(file);
}
catch(SAXException saxe) {
}
catch(IOException ioe) {
}
If an exception was generated that was a subclass of RuntimeException
and not one of the declared exceptions on the parse method I would be
rather unhappy, as it would break all my apps.
If Digester's parse method catches RuntimeException and wraps it, then
that would be ok. However it currently doesn't do so. Some xml parsers
*might* do this, but it isn't specified in the JAXP docs that they
*must* do so, as far as I can tell.
It also seems a little ugly to wrap an arbitrary exception as a
SAXException or an IOException.
If Digester.parse were declared to throw Exception, then I think the
approach of throwing a RuntimeException subclass would be more feasable.
> 2. compatibility with other kinds of Rules
>
> i'm not totally convinced that PluginRules will work with custom rules
> (but then again, i might be wrong). maybe this would be worth testing.
Yes, the behaviour of PluginRules with other Rules implementations may
well have some limitations.
What I expect is that PluginRules can be used in combination with any
other Rules implementation for every rule type except PluginCreateRule.
ie create a custom rules object, wrap a PluginRules around it, then
registering any rule type *except PluginCreateRule* with any pattern
appropriate for the nested Rules matching engine should be fine.
PluginCreateRules should (and currently must) be registered with
patterns containing no wildcards of any sort.
Whether it is possible to handle PluginCreateRule instances associated
with patterns containing wildcards is something that could be looked
into if people have a need for the feature.
Custom rules associated with plugged-in classes should never add any
patterns containing wildcards. Currently, the Rules implementation used
to match patterns associated with custom rules is *always* a RulesBase
instance. Again, if this feature is desired then it *might* be possible.
However IMHO it is a pretty exotic sort of thing to want to do...
>
> 3. Exception handling
>
> PluginAssertionError extends Error. i'm not very happy about this. IMHO a
> well behaved component should not throw any Error subclasses. i'd be
> happier for PluginAssertionError to be renamed PluginAssertionException
> and extend RuntimeException.
Having PluginAssertionError extend Error was a very deliberate choice.
It is modelled after the java 1.4 assertion mechanism, where an
assertion failure generates AssertionError.
PluginAssertionError is never thrown due to a mistake on the part of the
user of the Plugins module, nor due to a mistake in the xml of any sort.
It is only thrown when a bug in the plugins library is detected.
>
> i'm also a bit confused why PluginDeclarationRule throws
> ClassNotFoundException's when require attributes are missing from the xml.
> this seems a wrong to me. (i've left these for the moment since it's
> easier for people to examine the code when it's in cvs.) there are also a
> few Exception's thrown. i'd prefer specific subclasses to be thrown since
> this allows users (if they wish) to diagnose the original problem.
Well, I chose ClassNotFound partly because the result of these mandatory
attributes being missing is that we don't know which class to
instantiate. Isn't that sort of like ClassNotFound? :-)
Any suggestions for superior options are welcome. I agree it could be
improved. This code was written before the PluginInvalidInputException
class existed; would this be more appropriate?
If there are any raw "Exception" classes being thrown, these are just
remnants of code before cleanup and should definitely be fixed.
> 4. Delegate marker interface
>
> this appears to be have been removed between the original and the update.
> it was still referenced in the tests. i've removed the reference from the
> tests for now. is this right?
Oops. You did the right thing; the "update" removed the need for the
Delegate class.
>
> 5. Test cases
>
> these wouldn't run against the updated code. the reason was a problem with
> local names verses qnames (on my machine). i've adapted the code so that
> they'll run on my machine but see next for discussion about namespaces.
Hmm...must be a difference in the default xml parser. Thanks for fixing
the problem.
>
> 6. Namespaces
>
> it seems to me that there hasn't really been a lot of thought about
> namespaces. maybe more thought's needed on this subject before the plugin'
> s code could be released. one option might be to allow the names of the
> plug-in element to be varied by the user. another might be to create a
> namespace for the plugin element to live in.
Yep, all true. I am aware that hard-wiring the attribute names
"plugin-id" and "plugin-class" introduces a (small) risk of name
collision. It is also unfriendly to non-english-speaking users. I just
decided to see whether plugins was accepted before proposing additional
features :-).
The PluginCreateRule could have static methods to set the default names
for these attributes, plus non-static methods to set the attribute names
for specific instances:
public static void setDefaultPluginClassAttributeName(String name)
public static void setDefaultPluginIdAttributeName(String id)
public void setPluginClassAttributeName(String name)
public void setPluginIdAttributeName(String name)
PluginCreateRule.setDefaultPluginClassAttributeName("classe-de-plugin");
<widget classe-de-plugin="..."/>
Namespacing the attribute also seems quite a nice solution:
<widget plugin:class="..."/>
Of course this would need to be supported when setNamespaceAware is set
to true or false...
Maybe both could be supported...
Now that the code is in CVS, I will have a think about this one and come
up with a proposal, unless someone beats me to it...
> 7. Logs
>
> at the moment, each plugin class uses it's own and the log cannot be set.
> i'm not in favour of this pattern for several reasons. most rules
> implementations use the digester log and i'd prefer to switch as many
> classes as possible to use the log and give the others setters and getters.
I disagree with this one. I think having a different logger on each
class is one of the most wonderful features of logging, giving much
better control of output.
Having all logging going via single Log object means that there is no
control over what gets logged and what doesn't for different components
of Digester. And Digester generates some serious log output when
enabled!
For most logging implementations (log4j at least), unless logging is
explicitly configured otherwise, logging for category
"org.apache.commons.digester.plugins" will fall back to using the
setting for category "org.apache.commons.digester", which is the
digester log.
Hmm .. but calling Digester.setLogger probably doesn't override the
object known to the LogFactory...
What exactly is the purpose of being able to set the Log object used by
a class or instance?
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] plugins module ready for evaluation
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
hi simon
i've committed your contribution (with a few changes). many thanks. would
you like to write up something more detailed for the apache newsletter
(ASAP)? (you'll find this on the wiki
http://nagoya.apache.org/wiki/apachewiki.cgi?ApacheNewsletterDrafts/Issue2)
?
i have found a few issues that (whilst they did not stop the code being
committed) i think should be resolved before we could think about
releasing the plugins code. i'd like to encourage people to comment :)
1. code formatting
i've left simon's field naming convention alone for the time being since
it's not harmful. i'd prefer all of the code to follow the same pattern
and so would probably like to change it but i've committed it as-is to
give people a chance to take a look and offer an opinion.
2. Re: PluginRules.add(String pattern, Rule rule)
try {
((InitializableRule)rule).postRegisterInit(pattern);
} catch (PluginConfigurationException e) {
// Currently, Digester doesn't handle exceptions well
// from the add method. The workaround is for the
// initialisable rule to remember that its initialisation
// failed, and to throw the exception when begin is
// called for the first time.
log.debug("Rule initialisation failed", e);
// throw e; -- alas, can't do this
return;
}
exception handling tends to be a difficult issue for reusable components.
digester tends not to throw generic catchable exceptions. (the issue of
exception handling has been debated ad nauseum on the commons list many
times in many contexts.) IMHO the arguments in favour of adding a check
exception to Rules.add are not strong enough to consider breaking
backwards compatibility.
there are two reasonable approaches that plug-ins could take. the first
(which is what the current code does) is to silently fail. IMHO this is
probably the wrong behaviour in this case. why? well, it seems to me that
a typical reason that the InitializableRule may throw an exception is that
it is not able to function. this suggest to me that really in this case it
is not unreasonable to assume that running any digestion from the current
ruleset may well be caused to fail. it will be more difficult to recover
and diagnose a problem at this time.
the other approach is to throw a runtime exception subclass. i'd probably
support this approach.
2. compatibility with other kinds of Rules
i'm not totally convinced that PluginRules will work with custom rules
(but then again, i might be wrong). maybe this would be worth testing.
3. Exception handling
PluginAssertionError extends Error. i'm not very happy about this. IMHO a
well behaved component should not throw any Error subclasses. i'd be
happier for PluginAssertionError to be renamed PluginAssertionException
and extend RuntimeException.
i'm also a bit confused why PluginDeclarationRule throws
ClassNotFoundException's when require attributes are missing from the xml.
this seems a wrong to me. (i've left these for the moment since it's
easier for people to examine the code when it's in cvs.) there are also a
few Exception's thrown. i'd prefer specific subclasses to be thrown since
this allows users (if they wish) to diagnose the original problem.
4. Delegate marker interface
this appears to be have been removed between the original and the update.
it was still referenced in the tests. i've removed the reference from the
tests for now. is this right?
5. Test cases
these wouldn't run against the updated code. the reason was a problem with
local names verses qnames (on my machine). i've adapted the code so that
they'll run on my machine but see next for discussion about namespaces.
6. Namespaces
it seems to me that there hasn't really been a lot of thought about
namespaces. maybe more thought's needed on this subject before the plugin'
s code could be released. one option might be to allow the names of the
plug-in element to be varied by the user. another might be to create a
namespace for the plugin element to live in.
7. Logs
at the moment, each plugin class uses it's own and the log cannot be set.
i'm not in favour of this pattern for several reasons. most rules
implementations use the digester log and i'd prefer to switch as many
classes as possible to use the log and give the others setters and getters.
- robert
On Thursday, October 2, 2003, at 03:46 AM, Simon Kitching wrote:
> On Wed, 2003-10-01 at 17:59, Simon Kitching wrote:
>> Hi,
>>
>> Many many moons ago, I proposed a "plugins" extension for digester.
>>
>> It is now ready for the world [yes, yes, brave words I know :-]
>
> Follow-up comments:
>
> *
> The package overview is accessable via this direct link:
> http://issues.apache.org/bugzilla/showattachment.cgi?attach_id=8410
>
> *
> I always wanted class PluginRules to be a decorator for other Rules
> classes rather than reimplement RulesBase functionality. It hasn't been
> possible in the past, because I needed to use a CursorableLinkedList (or
> equivalent) to hold the rules. However I realised this morning in the
> shower (true!) that given recent changes to the way PluginCreateRule is
> implemented, it is now possible to do so.
>
> I have attached an updated (and smaller) code tarfile to the bugzilla
> entry http://issues.apache.org/bugzilla/show_bug.cgi?id=23537.
>
> *
> Currently, PluginCreateRules creates a new Rules instance each time it
> fires, to hold the custom rules associated with whatever concrete class
> the user has specified. As it is expected that the same concrete class
> will be reused often (just see the examples), a cache could be created
> to hold these rules objects. I would rather hold off doing this
> optimisation, though, until I know whether plugins will be accepted into
> the digester tree or not.
>
> *
> Sorry about my coding style creeping into the posted code a little.
> In particular, I add underscores to the end of instance variables, like:
> private String foo_;
> public void setFoo(String foo) { foo_ = foo; }
> Habits are hard to break...
>
> *
> Calls to log.debug still need to be wrapped in if(debug) for
> performance, like in the rest of Digester code.
>
> Cheers,
>
> 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] plugins module ready for evaluation
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
On Wed, 2003-10-01 at 17:59, Simon Kitching wrote:
> Hi,
>
> Many many moons ago, I proposed a "plugins" extension for digester.
>
> It is now ready for the world [yes, yes, brave words I know :-]
Follow-up comments:
*
The package overview is accessable via this direct link:
http://issues.apache.org/bugzilla/showattachment.cgi?attach_id=8410
*
I always wanted class PluginRules to be a decorator for other Rules
classes rather than reimplement RulesBase functionality. It hasn't been
possible in the past, because I needed to use a CursorableLinkedList (or
equivalent) to hold the rules. However I realised this morning in the
shower (true!) that given recent changes to the way PluginCreateRule is
implemented, it is now possible to do so.
I have attached an updated (and smaller) code tarfile to the bugzilla
entry http://issues.apache.org/bugzilla/show_bug.cgi?id=23537.
*
Currently, PluginCreateRules creates a new Rules instance each time it
fires, to hold the custom rules associated with whatever concrete class
the user has specified. As it is expected that the same concrete class
will be reused often (just see the examples), a cache could be created
to hold these rules objects. I would rather hold off doing this
optimisation, though, until I know whether plugins will be accepted into
the digester tree or not.
*
Sorry about my coding style creeping into the posted code a little.
In particular, I add underscores to the end of instance variables, like:
private String foo_;
public void setFoo(String foo) { foo_ = foo; }
Habits are hard to break...
*
Calls to log.debug still need to be wrapped in if(debug) for
performance, like in the rest of Digester code.
Cheers,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org