Allow HasConversion/ValueConverters to convert nulls

[MarkGodwin]

There are significant problems when executing queries that either convert nulls in the database to non-nulls in code or vice-versa. Therefore, we have marked this feature as internal for EF Core 6.0. You can still use it, but you will get a compiler warning. The warning can be disabled with a #pragma. See https://github.com/dotnet/efcore/issues/26230 for more information.

---

When using FromSql to create a custom filtered data set, any column conversions defined on the DbQuery object don't get applied.

I think the column conversions should be included automatically around the sub-query supplied to FromSql, similar to how a sub-query is used if more predicates are added to the IQueryable.

    public class Model
    {
        public int Id { get; set; }
        public bool Flag { get; set; }
    }

    public partial class MyDbContext : DbContext
    {
        public DbQuery<Model> Models { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
                    modelBuilder.Query<Models>(
                        entity =>
                        {
                            entity.ToView("dbo.Models");
                            // Convert null Flag to false on way in
                            entity.Property(e => e.Flag).HasConversion<bool?>(f => f, t => t ?? false);
                        });
        }
    }

    // ...

    var models = dbContext.Models.FromSql("SELECT * FROM dbo.Models").ToList();

This, I think, should result in SQL similar to this being executed...

SELECT [e].[Id], COALESCE([e].[Flag], 0) AS [Flag]
FROM (
    SELECT * FROM dbo.Models) AS [e];

But, in reality, the plain SQL from FromSql is executed

SELECT * FROM dbo.Models

EF Core version: 2.1.4 Database Provider: Microsoft.EntityFrameworkCore.SqlServer Operating system: Windows 7 IDE: Visual Studio 2017 15.8.5

[ajcvickers]

@MarkGodwin Value conversions are not translated to SQL--that feature is tracked by #10861. Instead, the value is read from the database and then converted. This is because the primary purpose of this feature it to allow use of types that cannot be directly read from or written to the database.

Also, value converters do not generally allow the conversion of null to some other value. This is because the same value converter can be used for both nullable and non-nullable types, which is very useful for PK/FK combinations where the FK is often nullable and the PK is not. We will discuss whether or not to add support for converting nulls.

[MarkGodwin]

Sorry, I can see that I am guilty of making assumptions that I didn't verify before raising the issue. I misinterpreted the documentation about where value converters are executed, and then jumped to conclusions about why it wasn't working in my particular case.

The inability to translate null is also very clearly described in the documentation.

I... assume... a correctly implemented value converter would be executed when mapping the results of a FromSql. So I'll close this off immediately.

Apologies again.

[space-alien]

The inability to translate nulls leads to some pretty unintuitive behaviour:

modelBuilder.Entity<Person>()
    .Property(p => p.Address).HasConversion(
        address => JsonConvert.SerializeObject(address),     // Address is a value object
        str => JsonConvert.DeserializeObject<Address>(str));

The above is mapped to a nullable string column in the DB. If Address is null, a NULL is inserted into the row and vice versa. So far so good.

But the results of following query include entities where Address is null!

_context.People.Where(c => c.Address != null).ToList()

[Skulblaka]

I ran into a problem when I tried to add a ValueConverter<float, float?> that converts float.NaN to null and vice versa since SQL Server's real data type does not support NaN. The generated columns for the properties with the converter did not allow null values; I guess this is related?

[ajcvickers]

@Skulblaka Yes.

[UdiAzulay]

any success to use conversion from non null to nullable type? (without excluding the property using NonMapped)

[UdiAzulay]

found an ugly solution using ValueConverter<T, T?>(optionally) and the below reflection field update

prop.Metadata.GetType().GetField("_isNullable", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic).SetValue(prop.Metadata, true);

[AndriySvyryd]

After this is fixed columns with a non-nullable value converter should be marked as non-nullable:

        public static bool IsColumnNullable([NotNull] this IProperty property)
            => !property.IsPrimaryKey()
               && (property.DeclaringEntityType.BaseType != null
                   || (property.IsNullable
                       && property.GetValueConverter()?.ProviderClrType.IsNullableType() != false)
                   || IsTableSplitting(property.DeclaringEntityType));

See https://github.com/aspnet/EntityFrameworkCore/issues/18592#issuecomment-546492893

Also consider setting the database default to the converted null

[UdiAzulay]

seems that the above suggested ugly workaround only works for "_fastQuery" and fails with "_shapedQuery" so i had to put another ugly solution for that:

    static EntityContext()
    {
        var localMethod = typeof(EntityContext).GetMethod("TryReadValue", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic);
        typeof(Microsoft.EntityFrameworkCore.Metadata.Internal.EntityMaterializerSource).GetField("TryReadValueMethod", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.Public)
            .SetValue(null, localMethod);
    }

    private static TValue TryReadValue<TValue>(in Microsoft.EntityFrameworkCore.Storage.ValueBuffer valueBuffer, int index, Microsoft.EntityFrameworkCore.Metadata.IPropertyBase property)
    {
        var ret = valueBuffer[index];
        if (ret == null && property is Microsoft.EntityFrameworkCore.Metadata.Internal.PropertyBase ip)
            return default(TValue);
        return (TValue)ret;
    }

now add the bellow line for each db null field (which is not null in application level) prop.Metadata.GetType().GetField("_isNullable", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic).SetValue(prop.Metadata, true);

and the null values will be converted to default(type) in all cases.

please add supported solution for nullable to non nullable conversion for cases like i have (400 poorly designed tables which i am not allowed to change )

[ajcvickers]

See also #22542

[roji]

Note: our current built-in converters perform null checks internally although that probably isn't necessary - because checks are also done outside, in SanitizeConverter (so we're doing null checking twice). When doing this issue, we should probably clean this up,

[dziedrius]

I understand reasoning for not passing Null to converter, but in that case we need some other way to handle it.

I think quite common approach is to use non breaking DDL changes, for example: I have a table with millions of rows in AWS Postgres Serverless and I need to add new boolean column. I can't add non nullable column with default value, because in AWS Postgres Severless that locks table for minutes (I think new Postgres versions fixed that, but Serverless is stuck on old version). So I'm adding a field as nullable boolean in database, but there's no need to treat it as nullable in code, as null and false are treated the same. I can do workaround like this:

    // entity code:
    public bool? _Flag { get; set; }
    public bool Flag
    {
        get => _Flag.GetValueOrDefault();
        set => _Flag = value;
    }
    // db context:
   modelBuilder.Entity<Foo>(entity =>
        {
            entity.Property("_Flag").HasColumnName("Flag");
            entity.Ignore(e => e.Flag);
        });

but it is a bit ugly.

[roji]

I can't add non nullable column with default value, because in AWS Postgres Severless that locks table for minutes

It sounds a bit odd that adding a non-nullable column would lock for minutes, but a nullable column wouldn't lock at all... If that really is so, you can also try to add a nullable column, make sure no row actually contains a null value in it, and then alter that column to be non-nullable.

In any case, if your column doesn't actually contain any nulls (regardless of whether it's defined as nullable or not), then you don't really need a value converter - just map it to a CLR bool directly. This feature is more about allowing nulls to be value-converted to something other than nulls.

[vernou]

In our case, we can have unspecified date, for example order without fixed delivery date.

We want manage this in C# with nullable date, but in the database with no nullable column for performance reason where unspecified date is stored with DateTime.Max (or DateTime.Min according to filter need).

This feature seems perfect for this need.

[dziedrius]

@roji

It sounds a bit odd that adding a non-nullable column would lock for minutes, but a nullable column wouldn't lock at all... If that really is so, you can also try to add a nullable column, make sure no row actually contains a null value in it, and then alter that column to be non-nullable.

For example:

ALTER TABLE your_table
    ADD COLUMN new_column integer NOT NULL DEFAULT 0;

this will execute with access exclusive lock and we have basically to rewrite whole table because of default value. It is possible to split that into:

ALTER TABLE your_table
    ADD COLUMN new_column integer;
ALTER TABLE your_table
    ALTER COLUMN new_column SET DEFAULT 0;

Where first will take milliseconds and second one won't need access exclusive lock, but probably you don't wish that in your migrations too, as seconds statement can still run very long and it would be better to take it out of migrations and do in batches.

In any case, if your column doesn't actually contain any nulls (regardless of whether it's defined as nullable or not), then you don't really need a value converter - just map it to a CLR bool directly. This feature is more about allowing nulls to be value-converted to something other than nulls.

Yes, but from SDLC perspective: 1) create migration to add nullable field 2) add nullable field to entity (because during release there will transition period when there will be nulls) 4) make sure all code that relates on that field handles nulls 5) apply migration 6) release new version to prod 7) set all values to default value for the new field in database 8) change nullable field in entity to non nullable, adjust related code 9) release again

If I could just say:

here's bool field, if in some case from database some null will appear just replace it with default value and all my rest code does not wish to know about it,

that would simplify my life quite a bit :)

[roji]

this will execute with access exclusive lock and we have basically to rewrite whole table because of default value.

I'm no ~SQL Server~ expert, but I don't see why the existence or not of a default value should change whether an exclusive table lock is needed or not - unless of course the default value is some long calculation which locks the table for much longer (but that shouldn't be the case with 0). But again, I'm no expert.

In any case, yeah, it's possible take care of this manually and/or in batches, to avoid locking up the database. And the nulls in value converters is indeed in the plan.

[ajcvickers]

Note for implementation: consider the case where the value converter changes the type from nullable to non-nullable, or vice versa. See #24534.

[roji]

Adding providers-beware because of the change from property.GetDefaultValue() != null to property.TryGetDefaultValue(out _).

[BickelLukas]

@ajcvickers I just updated to preview 5 but there is still an issue with this:

The database schema is managed outside of the scope of our application and booleans are represented as smallint where 0 or null means false and 1 means true (I know its fucked up but we have to deal with it).

I created a value converter like this:

    public class BooleanValueConverter : ValueConverter<bool, short?>
    {
        public BooleanValueConverter() : base(val => (short)(val ? 1 : 0), val => val != null && val != 0, convertsNulls: true)
        {
        }
    }

This is an test context with an entity:

    public class TestDbContext : DbContext
    {
        public TestDbContext(DbContextOptions<TestDbContext> options) : base(options)
        { }

        public DbSet<TestEntity> Entities => Set<TestEntity>();

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<TestEntity>().Property(x => x.Value)
                .HasConversion(new BooleanValueConverter());
        }
    }

    public class TestEntity
    {
        public int Id { get; set; }
        public bool Value { get; set; }
    }

When I query an entity with a null value I get the following error:

System.Data.SqlTypes.SqlNullValueException
Data is Null. This method or property cannot be called on Null values.
   at Microsoft.Data.SqlClient.SqlBuffer.ThrowIfNull()
   at Microsoft.Data.SqlClient.SqlBuffer.get_Int16()
   at Microsoft.Data.SqlClient.SqlDataReader.GetInt16(Int32 i)
   at lambda_method23(Closure , QueryContext , DbDataReader , ResultContext , SingleQueryResultCoordinator )
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()

An when I try to specify IsRequired(false) in the ModelBuilder I get the following error:

The property 'TestEntity.Value' cannot be marked as nullable/optional because the type of the property is 'bool' which is not a nullable type. Any property can be marked as non-nullable/required, but only properties of nullable types can be marked as nullable/optional.
   at Microsoft.EntityFrameworkCore.Metadata.Internal.Property.SetIsNullable(Nullable`1 nullable, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Internal.InternalPropertyBuilder.IsRequired(Nullable`1 required, ConfigurationSource configurationSource)
   at Microsoft.EntityFrameworkCore.Metadata.Builders.PropertyBuilder.IsRequired(Boolean required)
   at Microsoft.EntityFrameworkCore.Metadata.Builders.PropertyBuilder`1.IsRequired(Boolean required)

[ajcvickers]

@BickelLukas The ability of value converters to change the nullability of columns is tracked by #24685. I haven't been able to find a workaround for this case.

Beyond that, value converters do not handle cases where the database column has multiple different values that convert to the same value--for example, 0 and null both mapping to false in your model. This is because a query like this:

context.Entities.Where(e => !e.Value).ToList();

will be translated using the converter to:

SELECT [e].[Id], [e].[Value]
FROM [Entities] AS [e]
WHERE [e].[Value] = CAST(0 AS smallint)

This will return rows where the "false" value is 0, but not rows where the false value is null. This is not something we have plans to change, although it should be better documented.

[ajcvickers]

For anyone watching this issue: there are significant problems when executing queries that either convert nulls in the database to non-nulls in code or vice-versa. Therefore, we have marked this feature as internal for EF Core 6.0. You can still use it, but you will get a compiler warning. The warning can be disabled with a #pragma. See https://github.com/dotnet/efcore/issues/26230 for more information.

[SF-Simon]

Hello @ajcvickers , I'm trying to write a process to save any field type with JSON text fields.

However, to be compatible with nullable types, this HasConversion does not support the practice of empty types, and I cannot complete the work.

But I found that IConventionProperty.SetValueConverter is also set value conversion, and it also supports returning null.

            // propertyBuilder.HasConversion(converter);
            propertyBuilder.Metadata.SetValueConverter(converter);
            propertyBuilder.Metadata.SetValueComparer(comparer);

I don't use the A function, I use SetValueConverter and SetValueComparer separately.

I would like to ask, is this a solution? Or what is the difference between them? Is there anything else I need to pay attention to?

the API document: https://learn.microsoft.com/zh-cn/dotnet/api/microsoft.entityframeworkcore.metadata.imutableproperty.setvalueconverter?view=efcore-7.0

[douglasg14b]

When using strongly typed Ids this is REALLY important. This means we will have null Ids that are meant to be a strongly typed Id that can internally handle nulls.

I hope we seen this soon!!

[douglasg14b]

This post seems to imply that this is possible, but it's internal, but I'm not finding code examples for this internal API? How do we actually achieve this? Is this only through what @SF-Simon showed?

How can this be achieved in ConfigureConventions?

        protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
        {
            configurationBuilder
                .Properties<EventPlatform>()
                .HaveConversion<EventPlatformConverter>();

            configurationBuilder
                .Properties<EventPartner>()
                .HaveConversion<EventPartnerConverter>();
        }

public class EventPlatformConverter : ValueConverter<EventPlatform, string?>
{
    public EventPlatformConverter()
        : base(
            v => v.ToString(),
            v => new EventPlatform(v)) 
    { }
}

[ajcvickers]

@douglasg14b The API hasn't changed, it's just marked as internal because converting nulls only works correctly in very limited cases. This is because the database, .NET, and EF Core all treat nulls differently to other values, so if a null is converted then it stops behaving like a null. We may make this a non-internal API if we can find some other way to let people know it is a pit of failure, but it is unlikely we will do any other work in this area.

[chris-pie]

@douglasg14b The API hasn't changed, it's just marked as internal because converting nulls only works correctly in very limited cases. This is because the database, .NET, and EF Core all treat nulls differently to other values, so if a null is converted then it stops behaving like a null. We may make this a non-internal API if we can find some other way to let people know it is a pit of failure, but it is unlikely we will do any other work in this area.

Would you be interested in accepting PRs for this at all or no? I'm wondering if I should try tackling it but if it's not going to get merged anyway I'd rather work on something else.

[ajcvickers]

@chris-pie In the general case, this is a very involved fix that requires understanding how the query pipeline handles nulls and null semantics. Realistically, this would be a big, complex item to undertake for somebody on the team with a very good understanding of how both type mapping and queries work. That being said, if there is something specific you are planning to do, then let us know, and we will discuss.