Dapper-Extensions icon indicating copy to clipboard operation
Dapper-Extensions copied to clipboard

Using int type as identity key results in an error (library always try to convert the value to long)

Open spetz opened this issue 3 years ago • 14 comments

Hey,

I've found out the following issue when using int as a key and then trying to insert the record into the Postgres table (which is configured to be of identity type - so it's automatically incremented value by DB).

System.ArgumentException: Object of type 'System.Int64' cannot be converted to type 'System.Int32'.
   at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)

This is the sample entity:

public class MyType
{
    public int Id { get; set; }
}

and a mapping which doesn't really do anything

internal sealed class MyTypeMapping : ClassMapper<MyType>
{
    public MyTypeMapping()
    {
        Map(x => x.Id).Type(DbType.Int32).Key(KeyType.Identity);
        AutoMap();
    }
}

Seems that it all boils down to the following line - which always does assume that the ID will be of long type, even though it is int which is a valid type for the column (e.g. smallint, integer, biginteger).

https://github.com/tmsmith/Dapper-Extensions/blob/6482074a198cd5d089d0c6ef3b807caae3fe8669/DapperExtensions/DapperImplementor.cs#L624

spetz avatar Oct 20 '21 06:10 spetz

We have to use long as these identities can get very big.

If you do not apply "Type" to the mapping it should work normally.

valfrid-ly avatar Oct 20 '21 20:10 valfrid-ly

No, it doesn't work properly with or without mapping in any configuration possible. It's a bug which does prevent from using any other type than long for primary keys. I don't know why it was closed, as the issue was not resolved at all?

spetz avatar Oct 20 '21 20:10 spetz

I'll check it out

valfrid-ly avatar Oct 20 '21 20:10 valfrid-ly

I am running into this issue as well.

ghost avatar Dec 06 '21 20:12 ghost

So it seems as if this problem might be even deeper than just those lines. I went through and changed the supplied type parameter to dynamic and then pulled the value out of the resulting DapperRow and it was still set to long. From that I noticed this:

https://github.com/tmsmith/Dapper-Extensions/blob/6482074a198cd5d089d0c6ef3b807caae3fe8669/DapperExtensions/Sql/SqlServerDialect.cs#L21-L24

So it looks like it was even being cast at the SQL level. I even tried to remove this cast and keep the rest of the code changes and it still ran into an issue as it was then a System.Decimal. I am not sure how to move forward with this, as with the current structure I believe the only solution would be to have dynamic type parameters to pass into Dapper in the Insert method and I don't know a way to implement this or if that would even be possible.

ghost avatar Dec 09 '21 15:12 ghost

I found a workaround.

If you change this line from DapperImplementor as well as this other line too to be like below, it would work out of the box.

if (triggerIdentityColumn != null)
{
    keyValue = Convert.ChangeType(InsertTriggered(connection, entity, transaction, commandTimeout, sql, triggerIdentityColumn, dynamicParameters), keyColumn.MemberType);
}
else
{
    keyValue = Convert.ChangeType(InsertIdentity(connection, transaction, commandTimeout, classMap, sql, dynamicParameters), keyColumn.MemberType);
}

So far I have been able to move without further issues, but I can't submit a PR so fast as I'm still validting the new version. But it's working on my end.

To have this working I am customizing the InstanceFactory as follows:

DapperExtensions.DapperExtensions.InstanceFactory = MyFactory;
private IDapperImplementor MyFactory(IDapperExtensionsConfiguration config) => new MyCustomImplementor(new MyCustomSqlGenerator(config));

Just copy DapperImplementor (MyCustomImplementor) and SqlGenerator (MyCustomSqlGenerator) and add fix this straight on the MyCustomImplementor. You don't need to copy the generator, but it would help to troubleshoot, in case something else comes in...


EDIT: Found a better place to put the conversion which also fixes the returned value from the expression, and not only the key value when populating back the model.

ALMMa avatar Dec 19 '21 11:12 ALMMa

Wow, this is a show-stopping bug. As someone who wanted to use this library, spent time installing, learning the basics, had this error, debugged to understand and eventually got here, it's disappoint to see that this was reported a year ago and fixed six months ago but hasn't been released. Filed under "abandonware" and uninstalled.

avenmore avatar Oct 14 '22 07:10 avenmore

I have to agree with @avenmore. Seeing that this issue was fixed a year ago but not yet released makes us question whether to continue using DapperExtensions or to use something else. Our use case is porting a bunch of web-apis from full framework to .Net6. This bug stopped us dead in our tracks.

norgie avatar Nov 02 '22 11:11 norgie

It looks like the insert succeeds even when the exception occurs, at least with SQL Server. A quick, but unappealing workaround is to catch and ignore the exception.

I was unable to resolve the issue even by changing the table definition on the database or by modifying the model class. The very confusing thing is that even if I declare the identity field as a long, it still gives an error.

public int ID { get; set; } System.ArgumentException: Object of type 'System.Int64' cannot be converted to type 'System.Int32'.

public Int64 ID { get; set; } or public long ID { get; set; } Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: Cannot implicitly convert type 'long' to 'int'. An explicit conversion exists (are you missing a cast?)

I'm using SQL Server and get the same exception whether I declare the ID identity field as int or bigint.

Is this exception occurring after the insert, when it is attempting to set the identity field in the provided model instance?

Spinnernicholas avatar Nov 30 '22 17:11 Spinnernicholas

Following on from @avenmore and @norgie. This is blocking us from upgrading to the latest version of this package. Is this issue going to be fixed? If not, then what other similar libraries have people migrated to?

mattvb1 avatar Jan 22 '23 23:01 mattvb1

Same for us. We're blocked as old version doesn't support DeleteAll by Predicate when there're no PK specified, and we cannot update because the latest has this bug with long

grazy27 avatar Apr 12 '23 11:04 grazy27

I'm sorry guys... have been very busy and some goals here are very delayed

valfrid-ly avatar Apr 12 '23 15:04 valfrid-ly

Looks like it has been about 3 years. Is there still a plan to fix this issue?

elena-obreja avatar Nov 27 '23 10:11 elena-obreja

Is there any solution for this currently? I have used this library in the past in another company and when using it now it gives me this error and I don't know how to solve it

Xustis avatar Jan 16 '24 17:01 Xustis