Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving workflow for DBNulls on reference types #111

Open
AntonZhernosek opened this issue Jan 7, 2024 · 0 comments
Open

Improving workflow for DBNulls on reference types #111

AntonZhernosek opened this issue Jan 7, 2024 · 0 comments
Labels
enhancement New feature or request

Comments

@AntonZhernosek
Copy link

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!

@AntonZhernosek AntonZhernosek added the enhancement New feature or request label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant