fast-member icon indicating copy to clipboard operation
fast-member copied to clipboard

ObjectReader, AllowDBNull and non-nullable reference types

Open AdamWillden opened this issue 4 years ago • 4 comments

Hi,

I'm using ObjectReader to allow me to assert behaviour in tests that use an IDataReader. We're using c#8 with non-nullable reference types.

The detection of nullability does not cater for this: allowNull = !(memberType.IsValueType && memberType == tmp);

See the example model below as to my desired output.

public class A
{
    public string Foo { get; set; } //AllowDBNull expected false, actual true (FAIL)
        = string.Empty;

    public string? Bar { get; set; } //AllowDBNull expected true, actual true (PASS)
        = string.Empty;

    public int Fizz { get; set; } //AllowDBNull expected false, actual false (PASS)
        = int.MaxValue;

    public int? Buzz { get; set; } //AllowDBNull expected true, actual true (PASS)
        = int.MaxValue;
}

Detecting nullability for reference types is a bit of a pain - what are your thoughts around handling this? Desired?


Thank you for efforts creating and maintaining this package, no doubt you don't get enough praise. I've only just become aware of this package and expect to be using it in runtime code very soon. Thank you again and keep up the good work 👍

AdamWillden avatar Mar 03 '20 22:03 AdamWillden

Hmmm. Interesting. I haven't updated anything to account for C#8, and IIRC the nullable attribute mechanism is incredibly awkward and subtle, but - I do agree with you that we should investigate this.

On Tue, 3 Mar 2020, 22:12 Adam Willden, [email protected] wrote:

Hi,

I'm using ObjectReader to allow me to assert behaviour in tests that use an IDataReader. We're using c#8 with non-nullable reference types.

The detection of nullability https://github.com/mgravell/fast-member/blob/a5499dd5228608310e5e50201c23aae43784498d/FastMember/ObjectReader.cs#L77 does not cater for this: allowNull = !(memberType.IsValueType && memberType == tmp);

See the example model below as to my desired output.

    public class A

    {

        public string Foo { get; set; } //AllowDBNull expected false, actual true (FAIL)

            = string.Empty;



        public string? Bar { get; set; } //AllowDBNull expected true, actual true (PASS)

            = string.Empty;



        public int Fizz { get; set; } //AllowDBNull expected false, actual false (PASS)

            = int.MaxValue;



        public int? Buzz { get; set; } //AllowDBNull expected true, actual true (PASS)

            = int.MaxValue;

    }

Detecting nullability for reference types is a bit of a pain - what are your thoughts around handling this? Desired?

Thank you for efforts creating and maintaining this package, no doubt you don't get enough praise. I've only just become aware of this package and expect to be using it in runtime code very soon. Thank you again and keep up the good work 👍

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mgravell/fast-member/issues/82?email_source=notifications&email_token=AAAEHMDMFISXZZNJ6ZZSBODRFV6FNA5CNFSM4LAUPZSKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ISFS37A, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMBTFDIOWJW3Q4ORF6DRFV6FNANCNFSM4LAUPZSA .

mgravell avatar Mar 03 '20 23:03 mgravell

@mgravell no kidding about the nullable attribute mechanism, that in itself is a project to find the most reliable and performant method. I knew this when raising the ticket 😁 If there's anyone up to such a challenge... 😉

I've seen a few sources relating to nullable reference type checks and they're all pretty hideous and by extension (I imagine) not very good in relation to performance.

AdamWillden avatar Mar 04 '20 09:03 AdamWillden

Just a heads up. NullabilityInfoContext makes this a lot easier and less hideous now.

brandonryan avatar Feb 23 '24 04:02 brandonryan

Dapper AOT now has a build-time object reader implementation. I think it is probably time to simply see whether we can move people to a different code base, designed with the current needs in mind.

mgravell avatar Feb 23 '24 19:02 mgravell