mgravell / fast-member

Automatically exported from code.google.com/p/fast-member
Apache License 2.0
1.02k stars 141 forks source link

ObjectReader, AllowDBNull and non-nullable reference types #82

Open AdamWillden opened 4 years ago

AdamWillden commented 4 years ago

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 πŸ‘

mgravell commented 4 years ago

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, notifications@github.com 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 .

AdamWillden commented 4 years ago

@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.

brandonryan commented 8 months ago

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

mgravell commented 8 months ago

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.