Giorgi / EntityFramework.Exceptions

Strongly typed exceptions for Entity Framework Core. Supports SQLServer, PostgreSQL, SQLite, Oracle and MySql.
https://giorgi.dev/entity-framework/introducing-entityframework-exceptions/
Other
1.44k stars 68 forks source link

No easy way to determine which constraint failed. #35

Closed mjamro closed 2 years ago

mjamro commented 3 years ago

Problem

When entity has more than one constraint there is no easy way to dermine which constraint failed. You need to resort to underlying DB provider exception, which slightly defies the idea behind this library of having high-level exception handling.

Currently you need to do it like that:

   // Both username/email are required to be unique
    var user = new User
    {
        Username = username,
        Email = email,
    };

    await _dbContext.AddAsync(user);

    try
    {
        await _dbContext.SaveChangesAsync();
    }
    catch (UniqueConstraintException e)
    {
        if(e.InnerException is PostgresException pge)
        {
            if(pge.ConstraintName == "IX_User_Email")
            {
               //Unique email constraint failes
            }
            else if(pge.ConstraintName == "IX_User_Username")
            {
               //Unique username constraint failes
            }
        }
    }

Proposal

Add fields to the exception to hold additional information like name of the constraint that failed. DB Providers typically contain that in their exceptions, so it's only a matter of mapping them to classes like UniqueConstraintException

That would include:

    public string ConstraintName { get; }
    public string DataTypeName { get; }
    public string ColumnName { get; }
    public string TableName { get; }
    public string SchemaName { get; }

This would simplify the catch block like that:

   // Both username/email are required to be unique
    var user = new User
    {
        Username = username,
        Email = email,
    };

    await _dbContext.AddAsync(user);

    try
    {
        await _dbContext.SaveChangesAsync();
    }
    catch (UniqueConstraintException e)
    {
        if(e.ConstraintName == "IX_User_Email")
        {
           //Unique email constraint failes
        }
        else if(e.ConstraintName == "IX_User_Username")
        {
           //Unique username constraint failes
        }
    }

I can provide a PR if you approve.

Giorgi commented 3 years ago

Do all the DB Providers that this library supports contain all that information? I can't find it for SqlException. Haven't checked others

STeeL835 commented 2 years ago

I can't find it for SqlException.

Right, SqlException doesn't have such properties, but it has needed values in a message:

Cannot insert duplicate key row in object 'dbo.MyTable' with unique index 'UQ_MyTable_ColumnNameOrElse'. The duplicate key value is (duplicatevalue).
The statement has been terminated.

This package could try to extract these values from a message (or elsewhere) and just have corresponding properties nullable if not all providers have them in an exception.

Another option would be to store that info in a Data property (no key = no value provided by the exception) and TryGet.. methods, but this is less favorable option.

Giorgi commented 2 years ago

@STeeL835 Message is not reliable because it is different based on the language of the database server.

STeeL835 commented 2 years ago

@Giorgi You're right.. Well, it could be a "preview" feature, that doesn't guarantee the values (nullable properties still fit) - at least something.

Maybe SqlServerExceptionProcessorStateManager could have a configurable option to modify parsing template if server's language id isn't 1033? We can't check it from exception programmatically - that would need to be checked by users, but we can still check if template fits the actual exception message and log a warning (like "Exception details could not be parsed from a message. If current locale isn't English, template can be modified there.")

Giorgi commented 2 years ago

Given that it won't work in all cases even for SqlServer, the value added by those properties will be very low while the number of cases it won't work will be very high.

In my opinion parsing of error messages generated by different databases in different languages should be a separate project and not part of this library.

STeeL835 commented 2 years ago

Still don't understand, why not. People with providers that support these properties will be happy, people with providers like SqlServer will be able to use a workaround easily (assuming that if you know which DB you're working with, you know its language), and people with none of the options won't lose anything. This lib shouldn't parse all languages of course, but at least could give a tool for it (or an interface for other tools), since it already does the work of intercepting DbUpdateExceptions.

Giorgi commented 2 years ago

@STeeL835 Which database providers report the constraints/tables that caused the error?

STeeL835 commented 2 years ago

@STeeL835 Which database providers report the constraints/tables that caused the error?

For example NpgSql, like in the OP's example. Don't know about the others, though

NickStrupat commented 6 months ago

One solution that would be more reliable than parsing the message to extract the index name could be:

Get a list of all the index names from the IModel like so:

var indexNames = context.Model.GetEntityTypes().SelectMany(x => x.GetIndexes())
    .ToFrozenDictionary(x => x.GetDatabaseName()!, x => x.Properties);

Then you could search the error message for each of the index names, and match that way. It's not perfect but it'd solve at least for the language differences. Messages that don't contain the index name at all will of course not work.

Giorgi commented 6 months ago

That sounds like a good idea. It will not work for indexes that exist in the database but not in the model but that can be documented.

Giorgi commented 6 months ago

I tried this approach and it doesn't work for SQLite:

image

image

Giorgi commented 6 months ago

@NickStrupat The good news is that it works for all other databases.

NickStrupat commented 6 months ago

For Sqlite, it seems the unique constraint message follows the pattern of a , delimited list of TableName.ColumnName. Maybe location rules would result in different delimiters?

Anyway, a solution for Sqlite could be to cache a lookup structure where the keys are a list of the column names in each index, and then you could loop over those trying to match as many as possible in the message. Then you could use that key to look up the constraint name.

Something like this to make the lookup structure:

var indexNameLookup = context.Model.GetEntityTypes().SelectMany(x => x.GetIndexes()).ToFrozenDictionary(
    i => i.Properties.Select(p => i.DeclaringEntityType.GetTableName() + '.' + p.GetColumnName()).ToList(),
    i => i.GetDatabaseName()!
);
Giorgi commented 6 months ago

@NickStrupat @mjamro @STeeL835 This is now implemented and live in the 8.1.0 version of the library. See the ReadMe for more details.

NickStrupat commented 6 months ago

Including Sqlite support?

Giorgi commented 6 months ago

No, haven't implemented it for SQLite.