tmsmith / Dapper-Extensions

Dapper Extensions is a small library that complements Dapper by adding basic CRUD operations (Get, Insert, Update, Delete) for your POCOs. For more advanced querying scenarios, Dapper Extensions provides a predicate system. The goal of this library is to keep your POCOs pure by not requiring any attributes or base class inheritance.
1.78k stars 586 forks source link

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

Open spetz opened 2 years ago

spetz commented 2 years ago

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

valfrid-ly commented 2 years ago

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.

spetz commented 2 years ago

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?

valfrid-ly commented 2 years ago

I'll check it out

ghost commented 2 years ago

I am running into this issue as well.

ghost commented 2 years ago

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.

ALMMa commented 2 years ago

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.

avenmore commented 1 year ago

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.

norgie commented 1 year ago

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.

Spinnernicholas commented 1 year ago

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?

mattvb1 commented 1 year ago

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?

grazy27 commented 1 year ago

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

valfrid-ly commented 1 year ago

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

elena-obreja commented 8 months ago

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

Xustis commented 6 months ago

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