npgsql / npgsql

Npgsql is the .NET data provider for PostgreSQL.
http://www.npgsql.org
PostgreSQL License
3.33k stars 821 forks source link

"Date" default binding to "DateTime", and not "DateOnly" - causing issues with dynamic binding with Dapper #5792

Closed B345TM45T3R closed 3 months ago

B345TM45T3R commented 3 months ago

Steps to reproduce

Using Dapper, perform a SELECT query on data that contains columns of PostgreSQL type Date with untyped generic <dynamic>. for example: var result = await conn.QueryFirstOrDefaultAsync<dynamic>("SELECT my_date_col FROM my_table_name"); Returns: date_col = {2014/09/10 00:00:00}

The issue

I've spent a couple of days looking into an issue. Our system uses Npgsql and Dapper. I've looked through a lot of the current issues and the collaboration with Dapper and Npgsql with regard to the DateOnly support issue. And I'm not entirely sure which library I should be logging this issue against. From what I can see it is an issue from the driver's side.

The trouble I'm having:

I'm encountering what is written in a comment in the one of the Dapper tests: tests/Dapper.Tests/DateTimeOnlyTests.cs - PostgreSQL Date binding dynamically to DateTime instead of DateOnly:

// untyped, observation is that these come back as DateTime and TimeSpan
Assert.Equal(date, DateOnly.FromDateTime((DateTime)row.Date));
Assert.Equal(time, TimeOnly.FromTimeSpan((TimeSpan)row.Time));

We're needing untyped <dynamic> to return as DateOnly, otherwise the added 'midnight' Time component then triggers behaviour similar to this issue: #4471: Writing DateTime values to a Date field is storing the previous date because I guess the added zero/midnight timestamp gets subtracted from (in the case of SAST), to adjust back to UTC, before storing the (now previous day's) date in the DB, and then the time is truncated.

Examples: (where date_col is type Date in PostgreSQL)

Untyped (what we need to use) var result = await conn.QueryFirstOrDefaultAsync<dynamic>("SELECT date_col FROM table_name"); Returns: date_col = {2014/09/10 00:00:00}

If I bind it to int to make sure it fails, I get: var result = await conn.QueryFirstOrDefaultAsync<int>("SELECT date_col FROM table_name"); Returns: Error parsing column 0 (date_col=2014/09/10 00:00:00 - DateTime)

This seems to show that the Npgsql to CLR mapping is giving Dapper DateTime as the CLR to work with, which is in line with Npgsql docs.

Based on:

You can read as other types by calling NpgsqlDataReader.GetFieldValue<T>() [docs](https://www.npgsql.org/doc/types/basic.html#:~:text=The%20default%20type,GetFieldValue%3CT%3E().) ,

I found the Dapper fix/support for this issue and registered these (hoping that Dapper might be able to use them on the untyped <dynamic> calls too):

Dapper.SqlMapper.AddTypeMap(typeof(DateOnly), System.Data.DbType.Date, true); 
Dapper.SqlMapper.AddTypeMap(typeof(TimeOnly), System.Data.DbType.Time, true);

And then the below example works (previously returned default DateOnly struct): var result = await conn.QueryFirstOrDefaultAsync<DateOnly>("SELECT date_col FROM table_name"); Returns: date_col = {2014/09/10}

But <dynamic> still maps to DateTime.

So my question is, am I missing something? How can I get Npgsql or Dapper to use DateOnly as the default dynamic binder for Date columns? I'd really like to change/override Npgsql's default mapping of DbType to CLR for Date when creating the NpgsqlDataSource, but have been unable to work out how to do that. Is that possible currently? Like similar to mapping Enums and composites.

Thanks!

Further technical details

Npgsql version: 8.0.3 PostgreSQL version: 16.2 Operating system: Win 11 Pro 64-bit Dapper version: 2.1.35

roji commented 3 months ago

Yes, this is by design; Npgsql reads PG date as DateTime by default, both because of the breaking change it would be to change the default, and because DateOnly was introduced only recently in .NET, and we shouldn't return different types for different TFMs targets og Npgsql. I could see us doing the breaking change at some point, as we're dropping support for older TFMs without DateOnly.

The general recommendation is to tell Npgsql which type you want (i.e. not use Dapper dynamic).

NinoFloris commented 3 months ago

As @roji mentioned we are considering changing defaults in 9.0 (see https://github.com/npgsql/npgsql/issues/5328). It would be a breaking change and we should provide a way for users to opt-out and return to the old behavior.

We do have an 'unstable' public extensibility layer in Npgsql - mainly for plugins - that can be used to add and override mappings. Something like this should do the trick for your case:

using System;
using System.Diagnostics.CodeAnalysis;
using Npgsql.Internal;
using Npgsql.Internal.Postgres;

[Experimental("NPG9001")]
sealed class DateOnlyDefaultResolverFactory : PgTypeInfoResolverFactory
{
    public override IPgTypeInfoResolver CreateResolver() => new Resolver();
    public override IPgTypeInfoResolver? CreateArrayResolver() => new ArrayResolver();

    class Resolver : IPgTypeInfoResolver
    {
        TypeInfoMappingCollection? _mappings;
        protected TypeInfoMappingCollection Mappings => _mappings ??= AddMappings(new());

        public PgTypeInfo? GetTypeInfo(Type? type, DataTypeName? dataTypeName, PgSerializerOptions options)
            => type == typeof(object) ? Mappings.Find(type, dataTypeName, options) : null;

        static TypeInfoMappingCollection AddMappings(TypeInfoMappingCollection mappings)
        {
            mappings.AddStructType<DateOnly>("pg_catalog.date",
                static (options, mapping, _) => options.GetTypeInfo(typeof(DateOnly),
                    // Hack to resolve data type name to oid until https://github.com/npgsql/npgsql/pull/5737 is merged
                    options.GetArrayElementTypeId(new DataTypeName("pg_catalog.date").ToArrayName()))!, isDefault: true);

            return mappings;
        }
    }

    sealed class ArrayResolver : Resolver, IPgTypeInfoResolver
    {
        TypeInfoMappingCollection? _mappings;
        new TypeInfoMappingCollection Mappings => _mappings ??= AddMappings(new(base.Mappings));

        public new PgTypeInfo? GetTypeInfo(Type? type, DataTypeName? dataTypeName, PgSerializerOptions options)
            => type == typeof(object) ? Mappings.Find(type, dataTypeName, options) : null;

        static TypeInfoMappingCollection AddMappings(TypeInfoMappingCollection mappings)
        {
            mappings.AddStructArrayType<DateOnly>("pg_catalog.date");
            return mappings;
        }
    }
}

You can register it by calling datasourcebuilder.AddTypeInfoResolverFactory(new DateOnlyDefaultResolverFactory()); or globally via the obsolete NpgsqlConnection.GlobalTypeMapper.AddTypeInfoResolverFactory.

B345TM45T3R commented 3 months ago

Oh ok, cool thank you very much.

The general recommendation is to tell Npgsql which type you want (i.e. not use Dapper dynamic).

Ok thanks. We do that where possible, but part of the system allows some dynamic table configuration and handling, and so for that part we've been using dynamic binding.

@NinoFloris Thanks for that! I tried implementing the code Resolver factory you sent, and registering it (both ways), but for some reason it was ending up with a stack overflow? I'm not sure why - breakpoints in the lambda's all hit and seemed to work fine.

So I've coded a workaround to modify the affected DateTime to DateOnly after the data load - it might not be the ideal/most performant solution, but it'll work fine - it's not hit all the time (only on single record returns, and if there are date columns in the DDL).

Bit curious about the stack overflow now. But otherwise happy to close the issue.

NinoFloris commented 3 months ago

Ahaha that serves me right for not testing it. I've modified my previous post to contain the working version.

roji commented 3 months ago

Duplicate of https://github.com/npgsql/npgsql/issues/5328