commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <si...@ecnetwork.co.nz>
Subject Re: [digester] plugins module ready for evaluation
Date Sun, 05 Oct 2003 21:53:25 GMT
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


Mime
View raw message