Jesus M. Rodriguez wrote:
On Thu, Mar 26, 2009 at 11:25 AM, Devan Goodwin <[email protected]> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Wed, 25 Mar 2009 22:45:46 -0400
Jesus Rodriguez <[email protected]> wrote:
This is more of a note to self, but I'd figured I'd share with
everyone. We need to revisit our exception classes, the api has a
very rich set of exceptions that can be useful by other parts of the
code.
Most folks don't want to use them because they live in an xmlrpc
package, and we could probably move them to a more common location.
Another reason is not all of the api exceptions are localized (usually
the older ones).
Another thing I noticed was that the exceptions that are localized
none of them use the FaultExceptions constructor which accepts the
localized parameters:
public FaultException(int error, String lbl, String messageId,
Object [] args)
{ super(LocalizationService.getInstance().getMessage(messageId,
args)); this.errorCode = error; this.label = lbl;
}
We seem to do it the hardway :)
...
super(1062, "Invalid Arch" , LocalizationService.getInstance().
getMessage("api.system.invalidarch", new Object [] {arch}));
This could've been easily written as:
super(1062, "Invalid Arch", "api.system.invalidarch", new Object []
{arch}));
Honestly, I would do away with the messageid altogether and use
the exception label as the messageid. So the ctor would change to:
super(1062, "Invalid Arch", new Object [] {arch}));
And in FaultException (top example) it would change to this:
public FaultException(int error, String lbl, Object [] args) {
super(LocalizationService.getInstance().getMessage(lbl, args));
this.errorCode = error;
this.label = lbl;
}
Oh back to the organization, I think we should move the Exceptions to
com.redhat.rhn.common.errors. except for maybe the truly specific
errors which could remain in the xmlrpc package.
Thoughts?
Are we talking about about localizing and defining error codes
for exceptions throughout the application here?
Not really. The localization comment was that we seem to do it more
than one way. :) It's easy to think I meant to localize throughout the
application because I talked about reusing the same exceptions.
They're actually 2 separate topics :)
1) localization should be consistent and required for API exceptions
2) reusing exceptions (regardless of localization) was we have exceptions
that some areas of the code use but others don't.
I always considered these relatively API specific, and in general an
Exception isn't something I expect to bubble up to the UI and need
localization. Normally we just see Internal Server Exception in the UI
right? And then the admin consults the logs to find the actual
exception details. To me this doesn't really need localization, and I
would prefer to avoid the overheard.
Actually Internal Server Error or Exception is bad. We never want to
see those. Right now I think we have PermissionException,
BadParameterException, and one more IIRC.
Basically my point is that we can use ExceptionHandlers in Struts
to forward users to error pages (the pages can have whatever
on them).
I'm not real keen on getting into fine grained exceptions for
everything that could go wrong either, I feel this is required in the
API to some extent as users writing scripts on the other end need
them, but elsewhere I only like to do this if there's a specific reason,
i.e. I need to detect that a specific problem happened and handle it
somewhere else. Otherwise I prefer to avoid the class bloat and re-use
generic exceptions.
+1 I can agree to that.
If however we are thinking about doing localization + error codes all
over, we probably want to come up with a better way for defining the
codes. It's manual today and not great, probably need some tools for
finding free codes, defining the subject matter ranges, and detecting
dupes.
Totally agree with that. Partha mentioned using the FaultException without
using specific exceptions. Now I'm thinking off the cuff, maybe we create
a registry that has all of the error codes, labels, and messageids. Then
we change FaultException to know about this registry (if it isn't just part
of that class) and throw FaultExceptions by label.
For example, instead of doing throw new NoSuchPackageException();
we do throw new FaultException("noSuchPackage"); Then FaultException
will know how to lookup the errorcode, etc. for that. This would actually
remove ALL of the api exceptions. Hrm. sounds promising. The benefit
of this would be all of the error codes would be in a single place.
Now back to the API, maybe we're doing ok here with exception handling
and keep the 'global' exceptions to a minimum.
Even with the API, I like the idea of using FaultException defined by a
registry over the current implementation. Currently, there are ~90+ of
these exception handlers under the xmlrpc namespace and that is sure to
grow as the application grows. Depending on how the registry is
defined, it would simplify selecting available error codes and might
even be useful if we wanted to generate some documentation that could be
referenced by API docs. Currently, the API docs mentions things like
returns "int - 1 on success, exception thrown otherwise."; however,
there is no detail on specific exceptions they might want to look for.
jesus
_______________________________________________
Spacewalk-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/spacewalk-devel
_______________________________________________
Spacewalk-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/spacewalk-devel
