Skip to content
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

No easy way to determine which constraint failed. #35

Closed
mjamro opened this issue Mar 21, 2021 · 16 comments
Closed

No easy way to determine which constraint failed. #35

mjamro opened this issue Mar 21, 2021 · 16 comments

Comments

@mjamro
Copy link

mjamro commented Mar 21, 2021

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
Copy link
Owner

Giorgi commented Mar 21, 2021

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
Copy link

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
Copy link
Owner

Giorgi commented Feb 10, 2022

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

@STeeL835
Copy link

STeeL835 commented Feb 10, 2022

@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
Copy link
Owner

Giorgi commented Feb 10, 2022

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.

@Giorgi Giorgi closed this as completed Feb 10, 2022
@STeeL835
Copy link

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
Copy link
Owner

Giorgi commented Feb 14, 2022

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

@STeeL835
Copy link

@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
Copy link

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
Copy link
Owner

Giorgi commented Mar 15, 2024

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
Copy link
Owner

Giorgi commented Mar 16, 2024

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

image

image

@Giorgi
Copy link
Owner

Giorgi commented Mar 16, 2024

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

@NickStrupat
Copy link

NickStrupat commented Mar 16, 2024

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
Copy link
Owner

Giorgi commented Mar 16, 2024

@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
Copy link

Including Sqlite support?

@Giorgi
Copy link
Owner

Giorgi commented Mar 17, 2024

No, haven't implemented it for SQLite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants