rhaazy wrote:
The situation that bought up this question was an issue with my client
supplying me with bad data. I have to run some sql that performs a
division operation and the denominator is an exchange rate. For some
reason the client sent me some data with some exchange rates set to
0.0000 which makes no sense at all... The only number I should get
thats not an actual exchange rate would be 1.0000. So I got some
divide by zero error that did exactly what you explained, exposed
sensitive data that definately should not be seen, and because of an
error I probably should have tested for, but didn't think to.
That last part is the tricky bit, of course. Was it an error or a bug? In
this case, a bug, because of rule #1 when dealing with external data:
external data cannot be trusted and leaving out verification is a bug.
If you cannot tell whether data is valid (the "I didn't think of checking
the exchange rate for 0" problem) then your program isn't really complete.
It should somehow include the knowledge of what's not acceptable input just
as it includes the knowledge of what to do with valid input. These are
usually two sides of the same coin.
So I was debating whether or not to add try-catch to just expose this
error or if I should add it to catch any error. If I add it to catch any
error, than I should add it to the rest of my code too....
In this situation, I'd do this:
1. Add a top-level exception handler that returns "internal system error" or
something equally vague to the client and logs the full exception details
(including stack) to a conspicuous place for you. This handler is not meant
to actually fix any problems -- it's just there to make sure we have a
regular interface to the outside world ("we will always return this or this
or this vague error that tells you nothing but tells me I messed up").
2. Fix the bug(s)! The client can supply any crazy thing it likes, and you
have to be able to handle it without being surprised. Ideally, you set up a
"firewall" where you can say: "beyond this point, I know my data is good
because I've checked it myself or produced it myself from checked data".
Exception handling may or may not come into play for point #2. Certainly,
this is almost certainly wrong:
try {
// Do things with the exchange rate
} catch (SqlException e) {
if (e.Errors[0].Number == 8134) { // Division by zero, can't you tell?
// Wait a minute, why didn't we just validate it before? Are we sure
it's the exchange rate that's causing the problem now, or some other
division in there?
}
}
But this is a possibility:
class RowParseException : Exception {
RowParseException(int rowNumber) { ... }
public int RowNumber { ... }
}
class ExchangeRateOutOfRangeException : RowParseException { ... }
...
if (exchangeRate <= 0) throw new
ExchangeRateOutOfRangeException(rowNumber, string.Format("Exchange rate
'{0}' is out of range.", exchangeRate));
You'd have a handler just before your top-level handler catching
RowParseExceptions and returning the problem to the client.
This is by far not the only way of doing it, which is why it's called an
error strategy and not error boilerplate. Throwing and catching exceptions
is fairly slow, which could be an issue if your service faces heavy load.
Also, the above approach will stop at the first sign of trouble. So you
could also do this (simplified because I'm lazy, don't use public fields but
readonly properties):
class ErrorRow {
Row Row;
RowParseException Error;
}
class ValidateRowsResult {
IEnumerable<RowGoodRows;
IEnumerable<ErrorRowErrorRows;
}
ValidateRowsResult ValidateRows(IEnumerable<RowinRows) {
List<RowgoodRows = new List<Row>();
List<ErrorRowerrorRows = new List<ErrorRow>();
...
foreach (Row row in rows) {
...
if (row.ExchangeRate <= 0) {
errorRows.Add(new ErrorRow(row, new
ExchangeRateOutOfRangeException(rowNumber, string.Format("Exchange rate
'{0}' is out of range.", exchangeRate))));
continue;
}
...
goodRows.Add(row);
}
return new ValidateRowsResult(goodRows, errorRows);
}
This approach makes it possible to ignore errors and continue. The number #1
selling point of exceptions is that they make it *impossible* to ignore
errors and continue -- you either explicitly handle them or processing is
going to stop. So it depends on what you're trying to achieve.
Here's yet another approach, which looks a lot like the very first one I
turned down but with a crucial difference:
try {
// Do things with an entire row
} catch (SqlException e) {
// We couldn't process this row, we don't know why, the database does
throw new RowStoreException(string.Format("Database error storing row
{0}.", row.Id), e);
}
...
foreach (Row row in rows) {
try {
processRow();
} catch (RowStoreException e) {
// Log the details of the error for ourselves
// Give the client e.Message -- we know it's safe since it's our
exception
}
}
The difference here is that we do not try to figure out exactly *why*
writing to the database failed -- that's the database's problem and
responsibility. We tell the client what we know, which is almost nothing,
and call it a day. This isn't very convenient for either you or the client,
but it's easy to write, and sometimes rapid delivery beats a thorough
middle-tier check.
If you give me more time I'm sure I can think of even more ways to implement
an error strategy here, but only you can decide what's right for your
application. Some people will doubtlessly claim either this way or that is
inherently superior to the others in every situation, but I don't believe
that for a second.
Your responses were very informative. I always appreciate the
feedback from the professionals who post on the various google group
programming forums.
If you really want to show your appreciation, do me a favor and don't call
Usenet "Google Groups". :-) You're not to blame, as Google isn't very candid
about it, but you're posting to a system that's much older than Google
itself, and not all of us access it through Google.
--
J.
