Recently, a couple of us have been making modifications to the maven-
project exceptions, and I think it's gotten to the point where we
need to step back and discuss what we're trying to accomplish here.
I'll start with the changes I'm making, and a couple of other things
I've noticed that I'd like some clarification on.
My own changes in these classes have focused on exposing constructors
that only accept the specific types of exception parameters that are
currently passed into these project exceptions. One notable instance
of this is the ProjectBuildingException. In this case, there are
quite a few constructors, since there are currently a lot of
different types of exceptions that can be wrapped by a
ProjectBuildingException. The point of this exercise is to improve
the readability of the code, and allow someone browsing through the
source to know what the potential exception chains actually are,
without requiring a full static analysis of maven's entire codebase.
It has the added benefit of forcing us to look at these exception
chains a little bit, and figure out where we might be able to
consolidate exception classes, subclass exceptions, and attach better
context information at a given exception's construction. In the case
of the ProjectBuildingException, I believe the profusion of
constructors is bad; however, I likewise believe that the profusion
of exception types wrapped by ProjectBuildingException is less than
ideal. For the record, I don't mind supporting protected constructors
to allow controlled subclassing of these exceptions, as long as we
don't open up the potential for:
super( message, cause )
in a subclass of ProjectBuildingException. This destroys the context
of this exception, and makes it very difficult to construct a
coherent error message at the end of the build.
---
The other thing I've been struggling with lately is the use of
java.net.URI internally within the ProjectBuildingException. I've
done static analysis of maven's core, and the URI instance is not
used intact anywhere in the code. While, taken with the super-POM's
location, this field type might make a sort of pure sense, I don't
see the value of incurring additional instability here. We're
converting pom locations from (and, supposedly, to) java.io.File
instances using information in this exception, but with the new
changes, we're adding all sorts of new complexity in the code by
constructing File instances so we can call toUri() on them...which in
some cases, causes the build to tank because the file path has a
space in it.
On top of this, we have protected (and private) constructors in this
class just to allow support the DRY principle on two fields
(projectId and pomUri). This means that, taken with the file
construction used to support the (now deprecated) string-based
constructors for pomLocation, we have two layers of indirection
feeding a private constructor. This seems geared to avoid duplication
of calls to set these two fields in the large number of new
constructors, but the way I see it, it's a massively over-normalized
approach, and it makes this class much harder to read with little or
no corresponding benefit. IMO, until we have more than two fields to
set, private constructors on these sorts of exceptions will only
confuse their readability.
---
I guess maybe what we really need here is a better discussion of
exactly how we support the user through errors. I've been thinking
about this stuff quite a bit lately, and I realize it's not enough to
have the information for the topmost and bottom-most exceptions in
the chain. Instead, each place where an exception is wrapped, it's
wrapped because we're traversing a layer in the architecture (or, it
should be this way, in any event). This means that there is a new
layer of context information associated with the site where the new
wrapper is constructed, and this additional context can be critical
in making the error easier to understand. Things like certain
pertinent parameters, project instances that were in play when the
error occurred, extra plugin information pertinent to the error, and
so forth all need to be captured.
When it comes to actually traversing the tree of possible exceptions
and constructing a coherent error message for the user, we also need
to have some reasonable assurance that we have 90% of the possible
exception chains accounted for. If new functionality results in new
exception chains, we should have to *think* about the reporting
implications of adding these new chains. To me, this means forcing
the developer to deliberately enable the new chain by adding a
constructor for the new sub-exception in the wrapper exception. It's
not perfect, obviously, but it should help us remember that we need
to add reporting code for this new variant of the wrapper.
Thoughts?
---
John Casey
Committer and PMC Member, Apache Maven
mail: jdcasey at commonjava dot org
blog: http://www.ejlife.net/blogs/john
rss: http://feeds.feedburner.com/ejlife/john
