-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce SqlState on DbException for standard cross-database errors #35601
Comments
I like the idea, but the fact that it already exists on two of them makes implementation awkward if you keep the name
something like (reduced): abstract class DbException : Exception
{
public string SqlState => GetSqlState();
protected virtual string GetSqlState() => "";
}
class FooDbException : DbException
{
protected override string GetSqlState() => "AB123";
// the thing they already declared on their public API
public new string SqlState => GetSqlState();
} |
We already have that problem with the new "Async" methods: #28596 For providers that already implemented those methods (before they were added in Is there a strong need to preserve binary compatibility when replacing a DLL that targets |
Yeah, I basically think the same as @bgrainger (this happened even earlier with Scale/Precision introduced at some point on DbParameter in .NET Framework), and I've dealt with it in the same way: you multitarget to the new TFM and override there (I don't see a need for binary compat because it's a new TFM), and keep the property as-is for older TFMs. You end up with some #ifs but it's not a huge deal. |
Fair enough. I can't help thinking this could cause a bait-and-switch problem if some intermediate library that uses the existing properties doesn't also update to multi-target, but: it seems like there's an existing pattern here. |
@mgravell that's definitely true, I just haven't seen it happen... It would have to be an Npgsql-specific (or MySqlConnector-specific) library that depends on this specific property, and doesn't update to multitarget. Possible, just not very likely... |
@roji what about moving the proposed SQLState property into a new abstract class called DbError. From there it could be implemented from all other DbError based classes like SqlError. It would be a fresh start I believe this way it's not stepping on the toes of existing properties. Oracle driver has OracleError which is almost the same as SqlError class |
@rgarrison12345 as written above, the fact that SqlState already exists on some providers' DbExceptions doesn't seem like a problem here, definitely not enough to warrant introducing a whole new type... This has already happened several times in the history of System.Data, and can pretty easily be taken care of via multitargeting. Also, most database providers don't seem to have a notion of an error that's distinct from the exception (PostgreSQL, MySQL, Sqlite...) - and it's definitely not part of the SQL standard. It seems to me that the concept would only add API complexity with no real benefits... |
namespace System.Data.Common
{
public partial class DbException
{
public virtual string? SqlState { get; }
}
} |
I may be missing something, but from the docs, it doesn't look like |
@hangy can you point to the place in the docs which suggests that? DbException is part of ADO.NET, along with DbConnection and DbCommand, which are very much about SQL databases (e.g. DbCommand has a textual CommandText which contains SQL). |
Maybe I was thrown off by the prefix and the fact that some of the classes in the |
@cheenamalhotra Thanks! I'm aware that classes in namespaces like |
Provider tracking issues
This new API has been merged, following are tracking issues for implementation in the different providers:
Proposal
Following discussion with @cheenamalhotra in #34798, this proposes adding a nullable string SqlState property on DbException:
The SQL standard defines SQLSTATE, which is an 5-letter error code scheme for database errors; the first two characters express an error class, and the last 3 a subclass. Since the codes themselves are defined in the standard, it is possible to detect certain errors (e.g. unique constraint violation) in a portable way, regardless of the specific database being used. Databases are also free to add their own errors under special categories. Both JDBC and ODBC expose errors via SQLSTATE.
Unfortunately, databases do not implement this standard in a very reliable way:
Although only 2 our of the 4 databases examined above implement SQLSTATE natively, there seems to be enough value in exposing this property; hopefully this would also provide a reason for the others to map to SQLSTATE, providing more standardized behavior.
Notes
/cc @David-Engel @cheenamalhotra @bgrainger @bricelam @ajcvickers @mgravell @FransBouma @stephentoub @terrajobst
The text was updated successfully, but these errors were encountered: