DapperLib / DapperLib/DapperAOT

Improving workflow for DBNulls on reference types

Open
#111 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
C#
Stars
472
Forks
43
Avg merge
1d 4h
Merged PRs (30d)
18

Description

Hi, I've posted this question yesterday on SO:
https://stackoverflow.com/questions/77769023/can-i-throw-an-exception-when-mapping-a-dbnull-with-dapper/77769044#77769044

I've checked out this lib on the example from the question and have found that it doesn't quite work in the way that IMO would be desirable (and also intended? from the answer)

Again to clarify, I use this model as a reference. PropetyOne and PropertyTwo work perfectly as expected with this, so my focus is only on PropertyThree that is a reference type

public class MyModel()
{
    public int PropertyOne { get; set; }
    public int? PropertyTwo { get; set; }
    public string PropertyThree { get; set; }
}

With NRTs enabled, this is the generated interceptor (tokens 2 and 5 are the ones responsible for PropertyThree)

          case 2:
              result.PropertyThree = reader.GetString(columnOffset);
              break;
          case 5:
              result.PropertyThree = GetValue<string>(reader, columnOffset);
              break;

Case 2 reads directly from the reader and the read won't throw an exception in case of DBNull (unlike its behaviour with value types), it'll return a null value. And even if we hit case 5, we go to CommandUtils.As and hit this bloc

        if (value is null or DBNull)
        {
            // if value-typed and *not* Nullable<T>, then: that's an error
            if (typeof(T).IsValueType && Nullable.GetUnderlyingType(typeof(T)) is null)
            {
                ThrowNull();
            }
            return default!;
        }

The logic looks only for a value type and since string isn't a value type it'll just return null and assign it, no exceptions thrown.

So, both cases are not correct because PropertyThree should not be null, and the generated code should in this case look as follows:

          case 2:
              result.PropertyThree = reader.IsDBNull(columnOffset) ? !!(throw SomeException)!! : reader.GetString(columnOffset);
              break;
          case 5:
              result.PropertyThree = reader.IsDBNull(columnOffset) ? !!(throw SomeException)!! : GetValue<string>(reader, columnOffset);
              break;

Without NRTs, we get this:

            case 2:
                result.PropertyThree = reader.GetString(columnOffset);
                break;
            case 5:
                result.PropertyThree = GetValue<string>(reader, columnOffset);
                break;

And sadly there doesn't appear to be a way to configure the property in a way that would generate an interceptor that would throw an exception in case NRTs are disabled.

IMO, it would be great to introduce an attribute to generate an interceptor for the case when NRTs are diabled.

  1. NRTs are enabled, we have reference type T => throw an exception in case of DBNull.
    Requires the abovementioned adjustments to the generated interceptor.

  2. NRTs are enabled, we have reference type T? => return null.
    Current logic works exactly like it.

  3. NRTs are disabled, we have reference type T, the property isn't marked by any attribute => return null.
    Current logic works like this.

  4. NRTs are disabled, we have reference type T, the property is marked with an attribute => throw an exception in case of DBNull.
    Requires the abovementioned adjustments to the generated interceptor and an additional attribute.

IMO, it would be good to have something lile "NotDBNullAttribute" for this. The naming is up to debate, but it would be good to have something like that, so the name doesn't interefere with System.Diagnostics.CodeAnalysis.NotNullAttribute and it'd be clear that we're dealing with db value mapping. I don't assume it'd be a good idea to use the NotNullAttribute from code analysis

Another attribute-based approach is to use System.ComponentModel.DataAnnotations.RequiredAttribute, so you'd have the same experience as EF in this. And IMO it'd make sense to have both available for use in the same way that the library uses DbValueAttribute and ColumnAttribute

Thanks!

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the generated interceptor cases shown in the issue and CommandUtils.As, then trace how DbValueAttribute, ColumnAttribute, and nullable reference types influence mapping. Done means the proposed attribute or RequiredAttribute behavior is defined for all four nullable-reference-type cases and DBNull handling is consistent in both generated interceptor paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend-api-design, database
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.