Exceptions in maven-project

[John Casey <jd...@commonjava.org>]

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

Re: Exceptions in maven-project

[Brett Porter <br...@apache.org>]

Hey John,

Interesting mail.

On 07/11/2007, at 7:04 AM, John Casey wrote:

> 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.

+1 :)

> In the case of the ProjectBuildingException, I believe the  
> profusion of constructors is bad;

I'm not quite sure why this is a problem? It is good to centralise  
any such conversion (if it is necessary), and the important thing is  
that the exception only contains one type so it is easy to represent  
it consistently.

> 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.

This last sentence sounds back to front - it was the URL that caused  
the problems, file conversions are fine? The toURI Javadoc in fact  
says that it is designed to ensure things get encoded properly from  
files :) The URL problem simply seems a bug in 1.4 that was fixed in  
1.5 and we have a workaround in place now.

I think it's good to capture the URI when you have it - but you  
certainly don't want all the conversions happening elsewhere - it  
seems to make sense to support both File and URI as inputs to the API  
(though URI need only be in private/protected methods), but only  
store it as a URI when it is stored (and do the conversion at the  
point of storage to avoid proliferating that code).

>
> 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 don't think readability of the exception class is particularly  
concerning - I would be looking at the usages of it and the  
javadoc :) But yeah, private constructors in an exception seem  
unnecessary, and frankly I think we could do away with the projectId  
and come up with a better way to represent a project between the URI  
and the ID. The ID gets passed around far more than necessary in the  
builder itself.

> 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.

I agree.

Here are the guidelines I think through off the top of my head when  
doing such things:
- don't be afraid to throw a runtimeexception if the caller shouldn't  
be expected to be able to deal with it and it is unrecoverable
- don't be afraid to rethrow the same, unwrapped exception if the  
caller would know what it means. For example, if you get passed a  
file, and it's not found, throw the FileNotFoundException, don't wrap  
it.
- use meaningful parameters in exceptions if they'll help diagnosis  
independently of the message
- if you're coding against an api and you catch an exception that you  
can't show to the user in a meaningful way right now, then rethrowing  
it won't help - something has already gone wrong at the level deeper.
- if you throw a wrapped exception, add information that helps to the  
message (or extra parameters) if appropriate. If the existing message  
is sufficient, reuse it (or maybe just throw the original exception  
as above). But if you don't know that the existing message will be  
sufficient, see rule above.
- make sure whatever you throw, the calling code will know what to do  
with it as meaningfully as you can in the current code.
- look in nested exceptions and traces for programming errors, not  
recoverable problems (eg, resource not found on the repository).  
Describe why it happened and you'll know where it happened and so  
will the user - and so there should be no need to propagate the  
nested exception.
- and all the usual things about being specific (don't toss and catch  
Exception and Throwable, etc).

>
> 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.

The error diagnoser you added was a cool way to figure some of this  
out, but it was kind of covering over the mistakes made elsewhere  
too :) Ideally, I don't think having to traverse the exception chain  
should be necessary based on the guidelines above because you should  
always know enough about what went wrong at each level to express it  
to the caller explicitly. If you think about a project building  
exception - someone using maven-project directly, maven-embedder, and  
the CLI should all be able to get the same, meaningful explanation of  
a particular failure, right?

WDYT?

Cheers,
Brett

--
Brett Porter - brett@apache.org
Blog: http://www.devzuz.org/blogs/bporter/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org