DapperLib / Dapper

Dapper - a simple object mapper for .Net
https://www.learndapper.com/
Other
17.54k stars 3.68k forks source link

Sqlite: InvalidCastException if the first value in a column is NULL followed by any other value #642

Open CheeseSucker opened 7 years ago

CheeseSucker commented 7 years ago

It seems like Dapper uses the first value in a column to decide what type the rest of the values will have. If the first value is NULL, this fails spectacularly.

I have set up a repository demonstrating this bug: https://github.com/CheeseSucker/DapperSqliteBug

Tested dapper versions: 1.50.0, 1.50.2, 1.50.3-beta1

Relevant code:

using (var connection = new SqliteConnection("Data Source=:memory:"))
{
    connection.Open();
    connection.Execute("CREATE TABLE MyTable (MyValue INTEGER)");
    connection.Execute("INSERT INTO MyTable (MyValue) VALUES (4)");
    connection.Execute("INSERT INTO MyTable (MyValue) VALUES (NULL)");
    connection.Execute("INSERT INTO MyTable (MyValue) VALUES (4)");

    // This is fine
    var result1 = connection.Query<MyModel>("SELECT * FROM MyTable");

    // This is also fine
    var result2 = connection.Query<MyModel>("SELECT * FROM MyTable ORDER BY MyValue IS NULL ASC");

    // This will fail because NULL is the first value in the column
    var result3 = connection.Query<MyModel>("SELECT * FROM MyTable ORDER BY MyValue IS NULL DESC");

    // InvalidCastException has been encountered before this line
    connection.Close();
}
class MyModel
{
    public long MyValue;
}
mgravell commented 7 years ago

This sounds horribly familiar, and iirc when I got to the bottom of it last time, it was essentially a provider bug. Let me see if I can find the more detailed chain.

On 17 Nov 2016 4:20 pm, "CheeseSucker" notifications@github.com wrote:

It seems like Dapper uses the first value in a column to decide what type the rest of the values will have. If the first value is NULL, this fails spectacularly.

I have set up a repository demonstrating this bug: https://github.com/CheeseSucker/DapperSqliteBug

Tested dapper versions: 1.50.0, 1.50.2, 1.50.3-beta1

Relevant code:

using (var connection = new SqliteConnection("Data Source=:memory:")) { connection.Open(); connection.Execute("CREATE TABLE MyTable (MyValue INTEGER)"); connection.Execute("INSERT INTO MyTable (MyValue) VALUES (4)"); connection.Execute("INSERT INTO MyTable (MyValue) VALUES (NULL)"); connection.Execute("INSERT INTO MyTable (MyValue) VALUES (4)");

// This is fine var result1 = connection.Query("SELECT * FROM MyTable");

// This is also fine var result2 = connection.Query("SELECT * FROM MyTable ORDER BY MyValue IS NULL ASC");

// This will fail because NULL is the first value in the column var result3 = connection.Query("SELECT * FROM MyTable ORDER BY MyValue IS NULL DESC");

// InvalidCastException has been encountered before this line connection.Close(); }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/642, or mute the thread https://github.com/notifications/unsubscribe-auth/AABDsLRN9GLK3RG9F_VVMalt7Q6DH6-dks5q_H7HgaJpZM4K1gWz .

CheeseSucker commented 7 years ago

329 looks a bit similar. Could these problems be related?

mgravell commented 7 years ago

Actually the one I was thinking of is #552, but that is mysql; if this is sqlite, I need to investigate from scratch. It could be similar, it could be unrelated. For the record, dapper doesn't do anything with the first row; instead, it asks the reader what the column types are. If the reader (provider-specific) does something stupid, that's when we get trouble.

hakon-knowit commented 7 years ago

I think it is indeed the same issue.

Just like in the MySQL issue, SQlite will return a different value for GetFieldType() while iterating a result set: https://github.com/aspnet/Microsoft.Data.Sqlite/issues/300

Unfortunately, this is by design and so is unlikely to change.

mgravell commented 7 years ago

Dammit. That's extremely vexing. I need to think on this. There is no perfect fix.

On 21 Nov 2016 4:44 pm, "Håkon Trandal" notifications@github.com wrote:

I think it is indeed the same issue.

Just like in the MySQL issue, SQlite will return a different value for GetFieldType() while iterating a result set: aspnet/Microsoft.Data.Sqlite#300 https://github.com/aspnet/Microsoft.Data.Sqlite/issues/300

Unfortunately, this is by design and so is unlikely to change.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/642#issuecomment-261994026, or mute the thread https://github.com/notifications/unsubscribe-auth/AABDsPmlefUHuHnSwOpPuzEzgTDaJc-pks5rAcqIgaJpZM4K1gWz .

mgravell commented 7 years ago

The best I can figure off the top of my head is a global AssumeColumnsAreStronglyTyped property that defaults to true but that which can be elected to false (and we'd emit guidance to do so in the invalid cast scenario); if false, all value conversions go via a Read{type} method that converts between a wide range of known primitives. So if you have a float, for example, it could work from any int* types, double, decimal, string, etc. With the fist line "if(val is float) return (float)val". Or perhaps more generally:

obj.Prop = val is TheType ? (TheType)val : ReadTheType(val);

Thoughts?

On 21 Nov 2016 6:10 pm, "Marc Gravell" marc.gravell@gmail.com wrote:

Dammit. That's extremely vexing. I need to think on this. There is no perfect fix.

On 21 Nov 2016 4:44 pm, "Håkon Trandal" notifications@github.com wrote:

I think it is indeed the same issue.

Just like in the MySQL issue, SQlite will return a different value for GetFieldType() while iterating a result set: aspnet/Microsoft.Data.Sqlite#300 https://github.com/aspnet/Microsoft.Data.Sqlite/issues/300

Unfortunately, this is by design and so is unlikely to change.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/642#issuecomment-261994026, or mute the thread https://github.com/notifications/unsubscribe-auth/AABDsPmlefUHuHnSwOpPuzEzgTDaJc-pks5rAcqIgaJpZM4K1gWz .

CheeseSucker commented 7 years ago

That sounds fine to me. The setting should probably be tied to a DbConnection in case more than one database is used.

What are the drawbacks to doing this?

mgravell commented 7 years ago

The drawback is that it will be slightly slower. I'm loathe to try to tie it to the connection type, because there isn't really a good API for that which reliably exposes the actual provider, especially when tools exist that work as decorators (meaning: GetType() isn't a good option). In this context, I'd rather just have all the connections pay the slight check/conversion overhead.

Marc

On 23 November 2016 at 08:52, CheeseSucker notifications@github.com wrote:

That sounds fine to me. The setting should probably be tied to a DbConnection in case more than one database is used.

What are the drawbacks to doing this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/642#issuecomment-262460263, or mute the thread https://github.com/notifications/unsubscribe-auth/AABDsPpxp2Fkq3ID-3Ygja95uR_PO0P2ks5rA_7PgaJpZM4K1gWz .

Regards,

Marc

CheeseSucker commented 7 years ago

Sounds good!

canton7 commented 6 years ago

Is there any progress on this? As far as I can make out, this is stopping me from using Dapper in a netcore app.

enantiomer2000 commented 6 years ago

I also am experiencing this problem with the Microsoft.Data.Sqlite library and Dapper. I switched to Microsoft.Data.Sqlite because the System.Data.Sqlite library doesn't work with linux without recompiling the Interop.Sqlite dll which is quite troublesome. I fix for this would be awesome. My datasets are not very large so a slight performance hit is not so huge a deal for me.

mlaily commented 6 years ago

Well, it seems I have the same issue. Any news? I see there is a pull request (https://github.com/StackExchange/Dapper/pull/720), but it does not merge anymore... :/

DaveInCaz commented 6 years ago

This question on SO seems to be a result of this issue: https://stackoverflow.com/questions/39980840/sqlite-using-dapper-error-parsing-column-unable-to-cast-object-of-type-system

DaveInCaz commented 6 years ago

Little example of why this is a problem in my case.

In one sqlite table, a column has values like this:

field123
--------
70.1
70
69.8
...

Since sqlite will only treat the value "70" as a integer, never a floating point (even if I inserted the value "70.0") this exposes this Dapper casting problem. There does not appear to be any workaround in the DB.

DaveInCaz commented 6 years ago

If you use a SQL CAST() on the column having the problem, you can work around this issue. This has been mentioned elsewhere but not yet in this issue, FYI for future readers.

aidanw commented 6 years ago

I am having this issue and casting did not help.

With my current data I can sort so I don't get nulls in the first row... but it is a shaky solution!

kardkovacsi commented 6 years ago

I think it would be better to state on the website that Dapper DOES NOT support SQLite so people will not invest time and money into a non-working package.

abbfmst commented 6 years ago

Sorry @mgravell but this issue is still there. Does this comment here in Microsoft.Data.Sqlite help any further?
https://github.com/aspnet/Microsoft.Data.Sqlite/issues/300#issuecomment-402779146

Otherwise the suggestion from @kardkovacsi and @hsorbo seems to be fair, as I'm hit with this issue while developing a quick and simple Web API using SQLite and Dapper. (DECIMAL column parsed as INT by error)

ravimpatel commented 5 years ago

I created a type handler for double? , it seems to work. Assumption here is that if user has specified type as double, it should attempt to convert it to double. As a user I should not be type a memo field as double of course. Is this an acceptable workaround?

`

    public class NullableDoubleHandler : SqlMapper.TypeHandler<double?>{
    protected NullableDoubleHandler() {}
    public static readonly NullableDoubleHandler Default = new NullableDoubleHandler();
    public override void SetValue(IDbDataParameter parameter, double? value)
    {
        if (value.HasValue)
        {
            parameter.Value = value.Value;
        }
        else
        {
            parameter.Value = DBNull.Value;
        }
    }
    public override double? Parse(object value)
    {
        if (value == null || value is DBNull) return null;

        return Convert.ToDouble(value);
    }
} `
vitidev commented 5 years ago

The error still exists and seems never to be fixed. Thanks @ravimpatel for a good solution.

Bunnywood commented 2 years ago

Seems this issue is still there as i've just had the same problem. That's over 5 years! Don't get me wrong, I have recently started using Dapper and I like the simplicity, but this is a pain. Shall be trying the @ravimpatel solution.

mlaily commented 2 years ago

Seems this issue is still there as i've just had the same problem. That's over 5 years! Don't get me wrong, I have recently started using Dapper and I like the simplicity, but this is a pain. Shall be trying the @ravimpatel solution.

If you want a more generic solution and are not afraid to maintain your own fork, PR #720 is a nice alternative. It does not merge anymore, but I regularly rebase it on top of the most recent tag for my own use here: https://github.com/mlaily/Dapper/commits/patch-720

To use it, simply SqlMapper.AssumeColumnsAreStronglyTyped = false;.

BTW, I wonder if the new STRICT mode in SQLite changes anything in regard to the current issue...

Bunnywood commented 2 years ago

Hi, Thanks for the suggestion @mlaily, but managing a fork for a small issue on an even smaller project is not an option for me. Looks good though, maybe it will make it into the official build eventually.

I did try the solution from @ravimpatel but it did not work for my exact situation.

What did eventually work for me, was to change the SQL so I forced a double value output using COALESCE(Value, 0.0)

Hopefully that will help anyone else encountering this issue in the future.

CarlVerret commented 1 year ago

I've just ran into this problem this morning with my unit tests, in my case, it was a SELECT statement that involves a SUM ( [decimal column] that sometimes returns values that SQLite / Dappes considers as int and sometime decimal. I worked around adding this : SELECT SUM (0.0+ MyColumn) and now it works.

anandjaisy commented 1 year ago

Is there any workaround for the case of NULL. In the unit test case I am mocking the query

string delete_query = "SELECT TOP 1 * FROM MarketplaceItem";
var marketplaceItem = context.MarketplaceItem.AsNoTracking().FirstOrDefault();
_fakedbConnection.SetupQuery(delete_query).Returns(marketplaceItem);

The marketplaceItem is NULL on the case if no records are there, however in one of the service layout I have a dapper query as

controller

Marketplace.MarketplaceItem marketplaceItem =
                await connection.QueryFirstOrDefaultAsync<Marketplace.MarketplaceItem>("SELECT TOP 1 * FROM MarketplaceItem");

Since the return value is NULL facing an exception as

System.Data.DataException: Error parsing column 0 (Name=<null>)
 ---> System.Reflection.TargetException: Non-static method requires a target.
   at System.Reflection.MethodBase.ValidateInvokeTarget(Object target)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.PropertyInfo.GetValue(Object obj)
   at NSubstitute.DbConnection.MockQuery.<>c__DisplayClass29_0.<ExecuteReader>b__7(CallInfo ci)
   at NSubstitute.Core.ReturnValueFromFunc`1.ReturnFor(CallInfo info)
   at NSubstitute.Core.CallResults.ResultForCallSpec.GetResult(ICall call, ICallInfoFactory callInfoFactory)
   at NSubstitute.Core.CallResults.TryGetResult(ICall call, Object& result)
   at NSubstitute.Routing.Handlers.ReturnConfiguredResultHandler.Handle(ICall call)
   at NSubstitute.Routing.Route.Handle(ICall call)
   at NSubstitute.Core.CallRouter.Route(ICall call)
   at NSubstitute.Proxies.CastleDynamicProxy.CastleForwardingInterceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at NSubstitute.Proxies.CastleDynamicProxy.ProxyIdInterceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.DbDataReaderProxy.get_Item(Int32 ordinal)
   at Deserializeb55939c3-2c31-4e08-bd4b-606d843266ec(DbDataReader)
   --- End of inner exception stack trace ---
   at Dapper.SqlMapper.ThrowDataException(Exception ex, Int32 index, IDataReader reader, Object value) in /_/Dapper/SqlMapper.cs:line 3878
   at Deserializeb55939c3-2c31-4e08-bd4b-606d843266ec(DbDataReader)
   at Dapper.SqlMapper.QueryRowAsync[T](IDbConnection cnn, Row row, Type effectiveType, CommandDefinition command)
   at LegalRegTech.MasterData.AuthorityAppService.DeleteAuthorityFromMarketplace(Int32 id)

Is there any workaround for this ?

mgravell commented 1 year ago

In the DapperAOT spike (not ready for use!) I'm using a different approach re column parsing which should mean that we get better results with this scenario. I'll see if we can add the examples here into the AOT integration tests.

Retrofitting these changes into Dapper "vanilla" would be prohibitively hard,and I do not currently propose to do such. But if DapperAOT fixes it with zero changes to your code... that sounds good, no?

mgravell commented 1 year ago

@anandjaisy if you're only reading one row , that sounds like a different thing

anandjaisy commented 1 year ago

var marketplaceItem = context.MarketplaceItem.AsNoTracking().FirstOrDefault();

@mgravell Yes I am reading the single row, is there any workaround for this? I am really stuck on this.

mgravell commented 1 year ago

@anandjaisy you've shown me zero of the Dapper related code, so: I can't see what you're actually doing. If you could show me the relevant Dapper related code, and any types that you're using as parameters or query types, I can probably help. Even better: show what the exception is in an integration test rather than a mocked test. Honestly, it looks like the problem here is the mock layer, in which case... "don't do that"?

anandjaisy commented 1 year ago

@mgravell Below is the Dapper code, when I do have a value the exception is not thrown, when the value is null I am getting that exception

startup.cs

services.AddTransient<IDapperContextBuilder,DapperContextBuilder>(serviceProvider =>
{
    var sqlConnection = new LegalRegTechDbConnectionBuilder(_hostingEnvironment).CreateSqlConnection(_appConfiguration.GetConnectionString("Default"));
    return new DapperContextBuilder(sqlConnection);
});
public class DapperContextBuilder: IDapperContextBuilder
{
    private readonly IDbConnection _sqlConnection;
    public DapperContextBuilder(IDbConnection sqlConnection)
    {
        _sqlConnection = sqlConnection;
    }
    public IDbConnection  CreateConnection()
    {
      return _sqlConnection;
     }
}
public async Task UpdateStatus()
{
    using var connection = _dapperContextBuilder.CreateConnection();
    Marketplace.MarketplaceItem marketplaceItem =
                await connection.QueryFirstOrDefaultAsync<Marketplace.MarketplaceItem>("SELECT TOP 1 * FROM MarketplaceItem");
}
mgravell commented 1 year ago

Looks fine. Does it work when using a real ADO.NET connection? In the stack-trace, the errors seemed to all be in the mocking infrastructure, which is "not me". So does it actually work against a connection?

Also, is there anything unusual in your POCO (MarketplaceItem)?

anandjaisy commented 1 year ago

Looks fine. Does it work when using a real ADO.NET connection? In the stack-trace, the errors seemed to all be in the mocking infrastructure, which is "not me". So does it actually work against a connection?

Also, is there anything unusual in your POCO (MarketplaceItem)?

Yes, it does work against the real connection. There is nothing unusual on the POCO there are [ForeignKey("xxxx")]something like that

mgravell commented 1 year ago

If it works against the actual connection, then this is a topic for your mock library maintainers. Sorry, but there's nothing we can do to cause or resolve this.