Bug/Regression doing a query with a jsonb column as string with 2.1.* #2049

lucax88x · 2024-03-05T19:46:23Z

Ciao!

I've reproduced the issue there:

So, if you have a postgresql db, and you use a jsonb column, and you use netwonsoft as deserializer such as:

NpgsqlConnection.GlobalTypeMapper.UseJsonNet(settings: settings);

and perform a a query like

 await conn.QueryFirstOrDefaultAsync<string>("select \"Content\" from public.\"Posts\"");

will throw an exception with

 ---> System.InvalidCastException: Object must implement IConvertible.
   at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
   at Dapper.SqlMapper.GetValue[T](DbDataReader reader, Type effectiveType, Object val) in /_/Dapper/SqlMapper.cs:line 1366
   --- End of inner exception stack trace ---
   at Dapper.SqlMapper.ThrowDataException(Exception ex, Int32 index, IDataReader reader, Object value) in /_/Dapper/SqlMapper.cs:line 3928
   at Dapper.SqlMapper.QueryRowAsync[T](IDbConnection cnn, Row row, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 496

This happens only with 2.1.*, if I revert to 2.0.151 everything works perfectly.

This is probably the PR that breaks it.

ps: without json.net it also work perfectly, it's just a combination of json.net and jsonb.

mgravell · 2024-03-05T21:03:22Z

I imagine that what's happening here is that npgsql is doing the deserialize, and giving us something that isn't a string (but rather: a rich object) - and we're expecting it to be a string. This is probably related to Dapper (vanilla) using the untyped data read API in some cases. Dapper.AOT does a better job of preferring typed data, so I imagine that Dapper.AOT would explicitly request a string, and receive a string. Any chance you can try the same query with Dapper AOT enabled? Or provide a full repro?

lucax88x · 2024-03-05T21:36:46Z

There's a github link with a full repro on top.

Clone the repo, and run the tests, it will fire up a db with test containers and run the query.

I can try with AOT tomorrow!

mgravell · 2024-03-06T09:35:53Z

See https://aot.dapperlib.dev/gettingstarted

mgravell · 2024-03-06T09:36:28Z

apologies, I overlooked the repro; taking a peek

mgravell · 2024-03-06T09:42:22Z

nit: await using var conn = new NpgsqlConnection(_connectionString);, but: great repro, thanks - looking

mgravell · 2024-03-06T09:44:23Z

I'm a little confused... it... works? (I haven't changed anything except the connection string)

and at the command-line:

   09:39:49  DapperBug.Tests  main   8.0.200   13.796s   
➜ dotnet test
  Determining projects to restore...
  All projects are up-to-date for restore.
  DapperBug -> C:\Code\dapper-bug\DapperBug\bin\Debug\net8.0\DapperBug.dll
  DapperBug.Tests -> C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll
Test run for C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.9.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: < 1 ms - DapperBug.Tests.dll (net8.0)

What should I be seeing?

mgravell · 2024-03-06T10:01:59Z

it is possible that this can be fixed with a one-liner in SqlMapper.cs L217:

-                [typeof(string)] = DbType.String,
+                [typeof(string)] = new(DbType.String, TypeMapEntryFlags.SetType | TypeMapEntryFlags.UseGetFieldValue),

can check as soon as we can repro

on a per-consumer-project basis, we can check this with

SqlMapper.AddTypeMap(typeof(string), DbType.String, true);

lucax88x · 2024-03-06T15:01:39Z

I'm a little confused... it... works? (I haven't changed anything except the connection string)

and at the command-line:

   09:39:49  DapperBug.Tests  main   8.0.200   13.796s   
➜ dotnet test
  Determining projects to restore...
  All projects are up-to-date for restore.
  DapperBug -> C:\Code\dapper-bug\DapperBug\bin\Debug\net8.0\DapperBug.dll
  DapperBug.Tests -> C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll
Test run for C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.9.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: < 1 ms - DapperBug.Tests.dll (net8.0)

What should I be seeing?

I'm a little confused... it... works? (I haven't changed anything except the connection string)

and at the command-line:

   09:39:49  DapperBug.Tests  main   8.0.200   13.796s   
➜ dotnet test
  Determining projects to restore...
  All projects are up-to-date for restore.
  DapperBug -> C:\Code\dapper-bug\DapperBug\bin\Debug\net8.0\DapperBug.dll
  DapperBug.Tests -> C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll
Test run for C:\Code\dapper-bug\DapperBug.Tests\bin\Debug\net8.0\DapperBug.Tests.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.9.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: < 1 ms - DapperBug.Tests.dll (net8.0)

What should I be seeing?

Wow.

This is extremely weird, I lost two hours for that.

So, stay with me:
update the git repo
run the compose from the root
docker compose -f docker-compose.yml up

now, you have 2 tests from the solution
-CreateTables
-RunAloneNotTogetherWithCreateTables

Try to run CreateTables ALONE so you have the structure.
Now run the RunAloneNotTogetherWithCreateTable ALONE and it should give the Exception above (now reproduced)

But, if for some reason, you try to run both of them together, CreateTables will fail because table is already there (which is OK), but the RunAloneNotTogetherWithCreateTables will be green (WTF?)!

Something must happening with assembly...

Gonna try with your map.

lucax88x · 2024-03-06T15:03:02Z

SqlMapper.AddTypeMap(typeof(string), DbType.String, true);

I can confirm it works.

mgravell · 2024-03-06T15:14:43Z

great; that's a workaround, and we'll get the code fixed for next release

mgravell · 2024-03-06T15:17:42Z

Npgsql.PostgresException : 3D000: database "test" does not exist

so... I don't think it is connecting to the containerized database, but... I think we understand the problem well enough now

lucax88x · 2024-03-06T15:18:40Z

Create the db in the container :)

lucax88x · 2024-03-06T15:19:13Z

thanks for the quick answer btw.

mgravell · 2024-03-06T20:59:07Z

a couple of subtle behaviour changes with exception messages, but : within tolerance (and in some cases, clearer)

lucax88x · 2024-03-14T12:50:54Z

@mgravell

I don't know where the problem is, but I'm noticing the same bug using npgsql 8 and dapper 2.0.151.

I did another repro using a online postgresql.

https://gist.github.com/lucax88x/69c6deb14b80c856271534de5c10e1ad

dapper + json.net
dapper without json.net

npgsql with json.net
ngpsql without json.net

Without the workaround (the SqlMapper one) the one with dapper and json.net fails.

So I think the regression didn't come with 2.1 but rather from 8.0 of npgsql.

mgravell · 2024-03-14T15:11:55Z

It looks like we still haven't shipped NuGet packages for this - will fix However: IIRC npgsql now marks this global API as deprecated, preferring a column-specific configuration, so I'm sure they'd say "we already fixed this: the fix is to stop using that API" (apologies if I'm misrepresenting this - I don't do much npgsql)

…

On Thu, 14 Mar 2024, 12:51 Luca Trazzi, ***@***.***> wrote: @mgravell <https://github.com/mgravell> I don't know where the problem is, but I'm noticing the same bug using npgsql 8 and dapper 2.0.151. I did another repro using a online postgresql. https://gist.github.com/lucax88x/69c6deb14b80c856271534de5c10e1ad dapper + json.net dapper without json.net npgsql with json.net ngpsql without json.net Without the workaround (the SqlMapper one) the one with dapper and json.net fails. So I think the regression didn't come with 2.1 but rather from 8.0 of npgsql. — Reply to this email directly, view it on GitHub <#2049 (comment)> or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMAFKXOWA7YXJ27DRADYYGMMJBFKMF2HI4TJMJ2XIZLTSOBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLLDTOVRGUZLDORPXI6LQMWWES43TOVSUG33NNVSW45FGORXXA2LDOOJIFJDUPFYGLKTSMVYG643JORXXE6NFOZQWY5LFU4YTMMJTGM2DLAVEOR4XAZNFNFZXG5LFUV3GC3DVMWVDEMJXGAYDEMJRGUZ2O5DSNFTWOZLSUZRXEZLBORSQ> . You are receiving this email because you were mentioned. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub> .

mgravell · 2024-03-14T15:17:57Z

https://github.com/DapperLib/Dapper/releases/tag/2.1.37

mgravell · 2024-03-14T22:03:29Z

This change will almost certainly be reverted; it broke other scenarios, where string is handled more flexibly. I'll check more options.

lucax88x · 2024-03-15T07:44:54Z

It looks like we still haven't shipped NuGet packages for this - will fix However: IIRC npgsql now marks this global API as deprecated, preferring a column-specific configuration, so I'm sure they'd say "we already fixed this: the fix is to stop using that API" (apologies if I'm misrepresenting this - I don't do much npgsql)
…
On Thu, 14 Mar 2024, 12:51 Luca Trazzi, @.***> wrote: @mgravell https://github.com/mgravell I don't know where the problem is, but I'm noticing the same bug using npgsql 8 and dapper 2.0.151. I did another repro using a online postgresql. https://gist.github.com/lucax88x/69c6deb14b80c856271534de5c10e1ad dapper + json.net dapper without json.net npgsql with json.net ngpsql without json.net Without the workaround (the SqlMapper one) the one with dapper and json.net fails. So I think the regression didn't come with 2.1 but rather from 8.0 of npgsql. — Reply to this email directly, view it on GitHub <#2049 (comment)> or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMAFKXOWA7YXJ27DRADYYGMMJBFKMF2HI4TJMJ2XIZLTSOBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLLDTOVRGUZLDORPXI6LQMWWES43TOVSUG33NNVSW45FGORXXA2LDOOJIFJDUPFYGLKTSMVYG643JORXXE6NFOZQWY5LFU4YTMMJTGM2DLAVEOR4XAZNFNFZXG5LFUV3GC3DVMWVDEMJXGAYDEMJRGUZ2O5DSNFTWOZLSUZRXEZLBORSQ . You are receiving this email because you were mentioned. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

if you check the second repro I'm not using the global one, so they didn't fix, also because the problem is occuring only with dapper

mgravell · 2024-03-15T08:55:57Z

@roji can I summon your wisdom here? Problem is JSON deserialize kicking in when using the untyped read API, but actually wanting a string; there are long reasons (fixed in AOT) why typed read is a PITA. Anything more I can do the detect this problem before it happens?

roji · 2024-03-15T10:10:56Z

Hey @mgravell... Is this the repro (from the OP)? That's odd - by default, Npgsql should absolutely return a string for an untyped read of a PG jsonb.

If I I tried to repro this without Dapper (similar to the gist in #2049 (comment)) and couldn't (see below) - is it possible to get an ADO.NET-only code sample that shows what Dapper is doing when stuff fails?

Re NpgsqlConnection.GlobalTypeMapper being obsolete, that shouldn't have anything to do with this; we now recommend using the newer DbDataSource API to create connections, and to configure plugins such as Json.NET there, so:

var builder = new NpgsqlDataSourceBuilder("Host=localhost;Username=test;Password=test");
builder.UseJsonNet();
await using var dataSource = builder.Build();

await using var conn = await dataSource.OpenConnectionAsync();

Attempted ADO-only repro

#pragma warning disable CS0618 // Type or member is obsolete
NpgsqlConnection.GlobalTypeMapper.UseJsonNet();
#pragma warning restore CS0618 // Type or member is obsolete

await using var conn = new NpgsqlConnection("Host=localhost;Username=test;Password=test");
await conn.OpenAsync();

Console.WriteLine("Creating and seeding data");

await conn.ExecuteAsync("""
                        drop table if exists posts;
                        create table public.posts
                        (
                            postid  integer generated by default as identity
                        constraint "PK_posts"
                        primary key,
                            title   text  not null,
                        content jsonb not null
                            );

                        insert into public.posts (postid, title, content) values (1, 'title', '{"Prop1": "2", "Prop2": 2}');
                        """);

await using (var command = new NpgsqlCommand("select content from public.posts", conn))
await using (var reader = await command.ExecuteReaderAsync())
{
    while (await reader.ReadAsync())
    {
        Console.WriteLine($"Content typed: {reader.GetFieldValue<string>(0)}");
        Console.WriteLine($"Content untyped: {reader.GetValue(0)}");
    }
}

// The following indeed throws:
_ = await conn.QueryFirstOrDefaultAsync<string>("select content from public.posts");

lucax88x · 2024-03-15T10:28:35Z

@roji this is a simpler repro using both ado and dapper.

https://gist.github.com/lucax88x/69c6deb14b80c856271534de5c10e1ad

roji · 2024-03-15T10:49:33Z

@lucax88x the ADO tests in that gist pass, only dapper_should_work_with_json_net seems to fail.

mgravell · 2024-04-12T09:35:53Z

crossref #2070

* revert #2050 - see #2049 for more details

mgravell · 2024-04-12T10:25:28Z

https://github.com/DapperLib/Dapper/releases/tag/2.1.42

mgravell mentioned this issue Mar 6, 2024

string and byte[] : add UseGetFieldValue #2050

Merged

mgravell closed this as completed in #2050 Mar 6, 2024

mgravell mentioned this issue Mar 14, 2024

Querying int columns as strings has broken in 2.1.37 #2056

Closed

mgravell reopened this Mar 14, 2024

mgravell added a commit that referenced this issue Apr 12, 2024

revert #2050 - see #2049 for more details

eb4a503

mgravell added a commit that referenced this issue Apr 12, 2024

revert #2050 - see #2049 for more details (#2070)

fa9c96a

* revert #2050 - see #2049 for more details

mgravell closed this as completed Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/Regression doing a query with a jsonb column as string with 2.1.* #2049

Bug/Regression doing a query with a jsonb column as string with 2.1.* #2049

lucax88x commented Mar 5, 2024 •

edited by mgravell

mgravell commented Mar 5, 2024

lucax88x commented Mar 5, 2024 •

edited

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024 •

edited

mgravell commented Mar 6, 2024 •

edited

lucax88x commented Mar 6, 2024

lucax88x commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

lucax88x commented Mar 6, 2024

lucax88x commented Mar 6, 2024

mgravell commented Mar 6, 2024

lucax88x commented Mar 14, 2024

mgravell commented Mar 14, 2024 via email

mgravell commented Mar 14, 2024

mgravell commented Mar 14, 2024 •

edited

lucax88x commented Mar 15, 2024

mgravell commented Mar 15, 2024

roji commented Mar 15, 2024 •

edited

lucax88x commented Mar 15, 2024

roji commented Mar 15, 2024

mgravell commented Apr 12, 2024

mgravell commented Apr 12, 2024

Bug/Regression doing a query with a jsonb column as string with 2.1.* #2049

Bug/Regression doing a query with a jsonb column as string with 2.1.* #2049

Comments

lucax88x commented Mar 5, 2024 • edited by mgravell

mgravell commented Mar 5, 2024

lucax88x commented Mar 5, 2024 • edited

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024 • edited

mgravell commented Mar 6, 2024 • edited

lucax88x commented Mar 6, 2024

lucax88x commented Mar 6, 2024

mgravell commented Mar 6, 2024

mgravell commented Mar 6, 2024

lucax88x commented Mar 6, 2024

lucax88x commented Mar 6, 2024

mgravell commented Mar 6, 2024

lucax88x commented Mar 14, 2024

mgravell commented Mar 14, 2024 via email

mgravell commented Mar 14, 2024

mgravell commented Mar 14, 2024 • edited

lucax88x commented Mar 15, 2024

mgravell commented Mar 15, 2024

roji commented Mar 15, 2024 • edited

lucax88x commented Mar 15, 2024

roji commented Mar 15, 2024

mgravell commented Apr 12, 2024

mgravell commented Apr 12, 2024

lucax88x commented Mar 5, 2024 •

edited by mgravell

lucax88x commented Mar 5, 2024 •

edited

mgravell commented Mar 6, 2024 •

edited

mgravell commented Mar 6, 2024 •

edited

mgravell commented Mar 14, 2024 •

edited

roji commented Mar 15, 2024 •

edited