Improved support for value-converted arrays and lists

[Dresel]

As discussed here, some query combinations including Contains do not work if ValueConverters are used (throwing Couldn't find array or element type mapping in ArrayAnyAllExpression). If configured via HasConversion, everything works as expected.

public class Program
{
    private static void Main(string[] args)
    {
        ServiceCollection serviceCollection = new();
        serviceCollection.AddDbContext<MyContext>(options =>
        {
            options.LogTo(Console.WriteLine);

            options.UseNpgsql("Server=localhost;Port=5432;Database=test;UserID=postgres;Password=postgres;");

            options.ReplaceService<IValueConverterSelector, TypedIdValueConverterSelector>();
        });

        ServiceProvider buildServiceProvider = serviceCollection.BuildServiceProvider();

        MyContext context = buildServiceProvider.GetRequiredService<MyContext>();
        context.Database.EnsureCreated();

        // Works
        ////context.EntitiesA.Where(x => context.EntitiesB.Contains(x.EntityB)).ToList();

        ////EntityBId[] ids = context.EntitiesB.Select(x => x.Id).ToArray();
        ////context.EntitiesA.Where(x => ids.Contains(x.EntityB.Id)).ToList();

        // Does not work
        List<EntityB> list = context.EntitiesB.ToList();
        context.EntitiesA.Where(x => list.Contains(x.EntityB)).ToList();

        ////EntityB[] list = context.EntitiesB.ToArray();
        ////context.EntitiesA.Where(x => list.Contains(x.EntityB)).ToList();

        ////List<EntityBId> ids = context.EntitiesB.Select(x => x.Id).ToList();
        ////context.EntitiesA.Where(x => ids.Contains(x.EntityB.Id)).ToList();
    }
}

public class TypedIdValueConverterSelector : NpgsqlValueConverterSelector
{
    private readonly ConcurrentDictionary<(Type ModelClrType, Type ProviderClrType), ValueConverterInfo> converters = new();

    public TypedIdValueConverterSelector(ValueConverterSelectorDependencies dependencies) : base(dependencies)
    {
    }

    public override IEnumerable<ValueConverterInfo> Select(Type modelClrType, Type? providerClrType = null)
    {
        foreach (var converter in base.Select(modelClrType, providerClrType)) yield return converter;

        Type underlyingModelType = Nullable.GetUnderlyingType(modelClrType) ?? modelClrType;

        if (underlyingModelType.IsAssignableTo(typeof(EntityAId)))
            yield return converters.GetOrAdd((typeof(EntityAId), typeof(int)), _ =>
            {
                return new ValueConverterInfo(typeof(EntityAId), typeof(int),
                    valueConverterInfo => new EntityAIdValueConverter(valueConverterInfo.MappingHints));
            });

        if (underlyingModelType.IsAssignableTo(typeof(EntityBId)))
            yield return converters.GetOrAdd((typeof(EntityBId), typeof(int)), _ =>
            {
                return new ValueConverterInfo(typeof(EntityBId), typeof(int),
                    valueConverterInfo => new EntityBIdValueConverter(valueConverterInfo.MappingHints));
            });
    }
}

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

    public DbSet<EntityA> EntitiesA { get; set; }

    public DbSet<EntityB> EntitiesB { get; set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        // Using HasConversion instead of ValueConverters works
        ////modelBuilder.Entity<EntityA>().HasKey(x => x.Id);
        ////modelBuilder.Entity<EntityA>().Property(x => x.Id)
        ////    .HasConversion(id => id.Value, value => new EntityAId {Value = value});

        ////modelBuilder.Entity<EntityB>().HasKey(x => x.Id);
        ////modelBuilder.Entity<EntityB>().Property(x => x.Id)
        ////    .HasConversion(id => id.Value, value => new EntityBId { Value = value });
    }
}

public class EntityA
{
    public EntityAId Id { get; set; }

    public EntityB EntityB { get; set; }
}

public class EntityB
{
    public EntityBId Id { get; set; }
}

public class EntityAId
{
    public int Value { get; set; }
}

public class EntityBId
{
    public int Value { get; set; }
}

public class EntityAIdValueConverter : ValueConverter<EntityAId, int>
{
    public EntityAIdValueConverter(ConverterMappingHints mappingHints = null) :
        base(id => id.Value, value => new EntityAId {Value = value}, mappingHints)
    {
    }
}

public class EntityBIdValueConverter : ValueConverter<EntityBId, int>
{
    public EntityBIdValueConverter(ConverterMappingHints mappingHints = null) :
        base(id => id.Value, value => new EntityBId {Value = value}, mappingHints)
    {
    }
}

[roji]

Running your simple errors at model building time with "The entity type 'EntityA' requires a primary key to be defined.". I'm using EF Core 5.0, which version are using?

[Dresel]

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL" Version="5.0.2" />
  </ItemGroup>

</Project>

What if you uncomment the HasKey statements?

[roji]

Now I get "The property 'EntityA.Id' is of type 'EntityAId' which is not supported by the current database provider.".

Please, help me out by submitting a runnable code sample (or point to a github repo) which actually works on your end...

[Dresel]

Sorry, I didn't mean to steal your time, but the code above works for me as described. I sounds like the TypedIdValueConverterSelector is missing? Anyway I have copied everything and pasted it into a repository. I have also asked a colleague to double check.

The following

git clone https://github.com/SpatialFocus/efcorepg-issue-1716 temp
dotnet run --project .\temp\EFCorePGIssue1716\EFCorePGIssue1716.csproj

leads to the Couldn't find array or element type mapping in ArrayAnyAllExpression.

[roji]

Thanks, I can see the exception now - will take a look.

[roji]

This is quite winding/complex, but here are my findings :smile:

First, there's a problem with the value converter; it adds supports for value-converting Entity1AId and Entity1BId, but not their corresponding array and List types (Entity1AId[], List<Entity1AId> etc.). If you look at NpgsqlValueConverterSelector, you can see the logic adding proper support for these (it's not trivial).

The logic is missing because TypedIdValueConverterSelector extends NpgsqlValueConverterSelector, so the former adds its conversions on top of the latter's... The order should be reversed, with NpgsqlValueConverterSelector adding array/list conversions for the non-array conversions added to TypedIdValueConverterSelector. Unfortunately, because this is an inheritance hierarchy, that doesn't work; we could refactor NpgsqlValueConverterSelector to wrap another selector passed through the constructor (composition instead of inheritance), which would allow this kind of thing. But if you copy-paste the code from NpgsqlValueConverterSelector you should be able to get it to work.

There's still the question of why things work with the explicit HasConversion configuration (without TypedIdValueConverterSelector). Well, they actually don't :) In this mode, the SQL generated is:

SELECT e."Id", e."EntityBId"
FROM "EntitiesA" AS e
LEFT JOIN "EntitiesB" AS e0 ON e."EntityBId" = e0."Id"
WHERE e0."Id" IN (8, 9)

This is the default SQL generated by EF Core, and is not what Npgsql usually generates; try to just do Contains with a list of ints or similar, and you should see something like WHERE e0."Id" = ANY (@p). This is a superior translation, since the entire array/list is passed as a parameter to PG, whereas the default EF one expands the array into constants in the SQL, creating various types of perf issues. In effect, Npgsql fails to translate this to its own (efficient) construct, and EF falls back to its default (less efficient) translation.

Finally, Npgsql fails to translate this because of a shortcoming: it's because the array's CLR type isn't known to Npgsql, since the value converter in HasConversion only lives on the property. Value convertors returns by the ValueConverterSelector, on the other hand, add global support for the CLR type everywhere, so Npgsql does attempt to translate, but fails because the selector doesn't have the proper array mappings as above. The problematic line is https://github.com/npgsql/efcore.pg/blob/main/src/EFCore.PG/Storage/ValueConversion/NpgsqlValueConverterSelector.cs.

I'll keep this open for looking into a fix for this.

[Dresel]

Thanks for the insight, this makes sense. Wish I had more time to deep dive into frameworks like this 😄

I will try the modified / copied NpgsqlValueConverterSelector workaround later, but I'm sure it will work out 👍

[roji]

Note (mainly to myself).

For array properties with value converters, in some scenarios we require to have the element converter. For example, when doing .Where(b => b.SomeArrayPropertyWithValueConverter.Contains(someParam), the parameter has no type mapping, so we need to infer it from the array - including an element value converter to do the conversion. Up to now, such array properties had a mapping with the proper converter, but pointed to an ElementMapping without a converter at all; so this could not be translated.

In order to make all this work, element mappings of array mappings with a converter themselves must have the proper converter. This means that we can't allow users to set up array conversions directly - as lambdas operating on arrays - since it's impossible derive an element converter from that. So we force users to provide an element converter when setting up the array property (breaking change), and build the array conversion over that ourselves; this new API is called HasPostgresArrayConversion.

[Dresel]

Just read and tried to understand your explanation. Did I understand it correctly that with this fix I would still have to copy / paste your selector code (which - just to point out - is not an problem for me) to make sure array converters are created for my custom type converters?

[Dresel]

By the way the modified version of the NpgsqlValueConverterSelector works flawlessly - I just hardcoded the inner selector because I wasn't able to find a good way to register decorations within the DbContext configuration (ReplaceService is limited and I don't know how to deal with InternalServiceProvider / ApplicationServiceProvider) 😬

[roji]

Just read and tried to understand your explanation. Did I understand it correctly that with this fix I would still have to copy / paste your selector code (which - just to point out - is not an problem for me) to make sure array converters are created for my custom type converters?

Yeah, #1719 only fixes the last problem detailed above, and doesn't refactor NpgsqlValueConverterSelector (which is a less severe thing and can be worked around as you saw). If you feel this is important, please open another issue to track it - hopefully at some point I can take a look.