Nicola Ken Barozzi wrote:
Since you know that we have dozens of places in the code with this thing, do you care posting a dozen of them here?
Thanks for taking the time to do it. Now, let's try to analyze and see what is the real use of them.
AbstractCommandLineEnvironment.java:
} catch (ProcessingException pe) { throw new CascadingIOException("ProcessingException: " + pe, pe); }
This is the whole relevant code snippet with my comment below (as in mail replies) the commented sections:
/**
* Redirect the client to a new URL
*/
public void redirect(boolean sessionmode, String newURL)
throws IOException {//NKB: Here it says that a redirect can throw only an IOException. // What does this mean? It means that the caller of this can only handle // a *generic* error on the IO, that is usually abort what it was doing. // Hmmm...
[...]
// FIXME: this is a hack for the links view
if (newURL.startsWith("cocoon:")
&& this.getView() != null
&& this.getView().equals(Constants.LINK_VIEW)) {[...]
Source redirectSource = null;
try {
redirectSource = this.resolveURI(newURL);
SourceUtil.parse( this.manager, redirectSource, ls);//NKB: Here it says that it has to resolve an URL, but the process // is not necessarily something that has success.
} catch (SourceException se) {
throw new CascadingIOException("SourceException: " + se, se);
} catch (SAXException se) {
throw new CascadingIOException("SAXException: " + se, se);
} catch (ProcessingException pe) {
throw new CascadingIOException("ProcessingException: " + pe, pe);
} finally {
this.release( redirectSource );
}
[...]
//NKB: Here what we are doing is saying: // " Some finer grained exceptions can occur, but we won't do anything // about it. Instead I'll tell my parent that I cannot do my work."
Note a couple of things here:
1 - Reading the code I have no idea whatsoever about the ProcessingException: we could as easily have made it Exception that it has the same (lack of) meaning.
2 - The redirect takes an URL as a String, which is wrong. A method should do one thing, not many. If it does many, it can fail in many ways, but here there is only one way it can fail now: IOException.
So because of this the method should be:
public void redirect(boolean sessionmode, String newURL)
throws IOException, SourceException {or
public void redirect(boolean sessionmode, Source newURL)
throws IOException {The latter is definately preferrable.
In fact, for example, SitemapSource throws a MalformedURLException, which should be called and thus dealt with by the caller.
3 - It redirects a SAX stream and cannot internally fail for an IOException... then why does it throw one?
So because of this the method should be:
public void redirect(boolean sessionmode, String newURL)
throws SAXException {If it were so, we would not have try/catch.
The problem with error handling here is that the contract is wrong.
HTMLTransformer.java (this is wicked!):
} catch (ProcessingException e) { e.printStackTrace(); }
/**
* React on endElement calls that contain a tag to be
* tidied and run Jtidy on it, otherwise passthru.
*
* @see org.xml.sax.ContentHandler#endElement(java.lang.String, java.lang.String, java.lang.String)
*/
public void endElement(String uri, String name, String raw)
throws SAXException {
if (this.tags.containsKey(name)) {
String toBeNormalized = this.endTextRecording();
try {
this.normalize(toBeNormalized);
} catch (ProcessingException e) {
//NKB: Here we are trying to normalize a tag, but can fail. // What we want to do it not to fail even if the single step to // *try* to clean it, fails.
e.printStackTrace();
}
}
super.endElement(uri, name, raw);
}Hence it should be:
} catch (ProcessingException e) {
logger.debug("Cleaning tag failed. Continuing... ",e);
}DefaultHandlerManager.java:
} catch (ProcessingException se) {
throw new ConfigurationException("Exception during configuration of handler: " + name, se);
}
[Below I snipped all ProcessingExceptions that actually have little meaning. The wrong thing in particular happens because we catch a generic ProcessingException and narrow it, like above...]
> } catch (ProcessingException pe) {
> throw new SAXException(pe);
> }
>
> CachingSource.java (does it three times):
>
> catch(ProcessingException e) {
> throw new CascadingIOException("Failure storing response.", e);
> }[Or probably a contract mismatch instead, like above.]
[...]
That's 23, then I stopped pasting. But considering all the catch clauses for ProcessingException and its subclasses we have:
ProcessingException: 64 occurences ConnectionResetException: 4 occurences ResourceNotFoundException: 10 occurrences
But ProcessingException is just one example. Avalon's CascadingException has 14 children exception classes. For instance, org.apache.avalon.framework.configuration.ConfigurationException, which is heavily used by CForms for doing things like:
throw new ConfigurationException("Convertor defined in plain attribute unavailable.", e);
Now, don't tell me that distributing an incorrect configuration file is not a programming error. Do contracts also cover the case of programming errors?
Yes, when there are different roles in deployment. The contract is between the coder and the deployer, and usually they are different.
I suspect this should be a runtime exception. Better yet, when we have Java 1.4 as a requirement, this should be:
assert false, "Convertor defined in plain attribute unavailable.";
?
In any case, what I see is that Cocoon has gotten many contracts wrong. In particular it has been coded using generic Cocoonish exceptions that were meant to gobble up the source exceptions from the start. In fact we can say that what seems now as an error was deemed in the beginning as a feature, that was probably done to achieve that RuntimeExceptions do rather than to have what we have now.
I would like to note BTW that my -1 is a vote, not a veto. It is based on the fact that I don't believe that using RuntimeExceptions will bring us that cleaness we need without unwanted consequences, but this is just MHO and I may well again be wrong, as it happens :-)
--
Nicola Ken Barozzi [EMAIL PROTECTED]
- verba volant, scripta manent -
(discussions get forgotten, just code remains)
---------------------------------------------------------------------