RepoDB icon indicating copy to clipboard operation
RepoDB copied to clipboard

Bug: Int Enum with Flags can't read from database if more than one flag is set

Open SpaceOgre opened this issue 3 years ago • 10 comments

Bug Description

I'm using int Enums with the Flags attribute and inserting with ExecuteNonQueryAsync and AnonymouseType is working but when I try and read a row that have more than one flag set I get the following crash.

It might be connected to #624.

This is how i do my setup:

RepoDb.SqlServerBootstrap.Initialize();
RepoDb.Converter.EnumDefaultDatabaseType = DbType.Int32;

And call to database:

await connection.ExecuteQueryAsync<Customer>("SELECT * FROM dbo.Customer");

Exception Message:

System.ArgumentNullException: Value cannot be null. (Parameter 'value')
   at System.Enum.TryParse(Type enumType, String value, Boolean ignoreCase, Boolean throwOnFailure, Object& result)
   at lambda_method89(Closure , DbDataReader )
   at RepoDb.Reflection.DataReader.ToEnumerableAsync[TResult](DbDataReader reader, IEnumerable`1 dbFields, IDbSetting dbSetting, CancellationToken cancellationToken)
   at RepoDb.DbConnectionExtension.ExecuteQueryAsyncInternalForType[TResult](IDbConnection connection, String commandText, Object param, Nullable`1 commandType, String cacheKey, Nullable`1 cacheItemExpiration, Nullable`1 commandTimeout, IDbTransaction transaction, ICache cache, CancellationToken cancellationToken, String tableName, Boolean skipCommandArrayParametersCheck)

Schema and Model:

Please share to us the schema of the table (not actual) that could help us replicate the issue if necessary.

CREATE TABLE dbo.Customer
(
  [CustomerNumber] int NOT NULL IDENTITY(1,1) PRIMARY KEY CLUSTERED
  , [ContractOwner] int NOT NULL
);

And also the model that corresponds the schema.

    public abstract record Customer
    {
        public int CustomerNumber { get; init; }
        public ContractOwner ContractOwner { get; init; }
    }

    [Flags]
    public enum ContractOwner
    {
        None = 0,
        Alfa = 1,
        Beta = 2,
        Other = 4
    }

Library Version:

Example: RepoDb v1.12.7 and RepoDb.SqlServer v1.1.3

SpaceOgre avatar Apr 16 '21 13:04 SpaceOgre

Just tested it with using nvarchar instead if int and that works, so it seems to be a problem with Flags and Enums as int.

SpaceOgre avatar Apr 16 '21 13:04 SpaceOgre

Something weird is going on I think, if I just change the database column from int to nvarchar then i works without doing anything else, RepoDb.Converter.EnumDefaultDatabaseType = DbType.Int32; is still set. So the value in the database is still an int but the type is nvarchar.

SpaceOgre avatar Apr 16 '21 13:04 SpaceOgre

Interesting. We will investigate and get back to you in relation to this. Is this a blocker on your side? Or, can you utilize the NVARCHAR type for now?

mikependon avatar Apr 16 '21 19:04 mikependon

No blocker, the NVARCHAR works for now :) and nothing that is going to production anytime soon.

SpaceOgre avatar Apr 19 '21 08:04 SpaceOgre

@mikependon I might have to bump this to a bit more critical, we are now having problems with BulkInsertAsync and this. When we do our normal insert we don't insert the flag column, that is handled later via an update like this:

        public async Task UpdateContractOwner(int customerNumber, ContractOwner contractOwner)
        {
            using var connection = new SqlConnection(ConnectionString);
            await connection.ExecuteNonQueryAsync(@"
                UPDATE dbo.Customer SET
                    ContractOwner = @contractOwner
                WHERE CustomerNumber = @customerNumber"
            , new { customerNumber, contractOwner });
        }

This inserts an int even though it is an NVARCHAR in the database, which is what we want. I guess this works is because there is no tabel mapping going on.

But we also have a migration stage that is using BulkInsertAsync with Entity mapping, this is now inserting the names of the flags rather than the int value and we are getting a crash since we only did NVARCHAR(2). We can ofc increase the NVARCHAR to a value that can fit all combinations but we really want to use int.

Is there some way we can force the BulkInsert to use Int event though the database is using NVARCHAR? The [TypeMap(DbType.Int32)] attribute does not help.

SpaceOgre avatar May 03 '21 11:05 SpaceOgre

By default, the library is using the string types when pushing the data into the database, specifically if it uses the Execute<Methods> and if your column type in the data database is NVARCHAR. If you uses the Execute<Methods> with INT column type in the database, it should throw an exception (unless you explicitly set the default enum database type to DbType.Int32 as explained here).

Note: You do not need to define the database type if you are using the model-based methods.

In your case, we cannot do any further for BulkInsert as it is the default behavior of the underlying mechanism which is the ADO .NET. I would strongly suggest that you use either of the approaches below.

  • Make a model that has an INT property and use it for bulk insert (you can also use anonymous type)
  • Or, consider using the string type enumeration when saving to database (there should be no more action item here, as per your case)

But since you mentioned that when you call the ExecuteNonQueryAsync it saves an INT type, it sounds to me that you are using the older version of the library, I really believe that this should not be the behavior. The Execute<Methods> should pass the string representation of the ENUM into the database as explained above. Or, ensure to use the latest beta version.

mikependon avatar May 03 '21 14:05 mikependon

Oops, wait, I can replicate your problem if the enum is marked as Flags. We will look into this.

mikependon avatar May 03 '21 14:05 mikependon

I think, the ideal fix for this should save the string representation of the bit-wise enum (i.e: "Apple | Banana | Orange") if the underlying column is of NVARCHAR type, otherwise, it will save the INT type. This is now true to the normal enumeration, which is not a bit-wise.

mikependon avatar May 04 '21 07:05 mikependon

Yes that is how I would like for it to work as well, but I think it needs to respect the DbType set by Fluent, Attribut, Default etc as well.

SpaceOgre avatar May 04 '21 07:05 SpaceOgre

Just ran into this as well.

Given the following enum, and an int database column with value 5 (so A+C).

[Flags]
enum ExampleFlagEnum
{
    A = 1 << 0,
    B = 1 << 1,
    C = 1 << 2
}

Throws an error:

   at System.Enum.EnumResult.SetFailure(ParseFailureKind failure, String failureParameter)
   at System.Enum.TryParseEnum(Type enumType, String value, Boolean ignoreCase, EnumResult& parseResult)
   at System.Enum.Parse(Type enumType, String value, Boolean ignoreCase)
   at RepoDb.Reflection.DataReader.<ToEnumerableAsync>d__1`1.MoveNext()

I've narrowed that down to the following code:

https://github.com/mikependon/RepoDB/blob/744a54309b441dd9a45260e3d20370241074aba7/RepoDb.Core/RepoDb/Reflection/Compiler/Compiler.cs#L641-L655

Which generates the following expression: System.Enum.GetName(typeof(ExampleFlagEnum), 5). But, value 5 is not an enum value (because it's a combination), so GetName returns null. This gets passed to ConvertExpressionToEnumExpressionForString(...) which calls System.Enum.Parse(...) on that null value and crashes.

What's the reason for going from a database int value via Enum.GetName to string, and then to Parse to get the enum value, instead of just casting directly? A simple call to Enum.ToObject(...) would suffice for conversion from numeric values regardless of actual underlying type of the enum.

w5l avatar Mar 02 '22 08:03 w5l