Add IComparable functionality to ValueComparer to support key ordering

[OleksandrKrutykh]

Description

The issue was discovered when migrating an existing project from EF Core 3.1 to EF Core 5.0. The project uses EF Core to work with a PostgreSQL database through Npgsql.EntityFrameworkCore.PostgreSQL provider.

The issue boils down to: If a CLR entity has a property of type System.Net.IPAddress (which is mapped to inet PostgreSQL data type), and there's a unique index defined for this property, an attempt to create a DB migration that includes the CLR entity will fail.

I don't know whether the issue is caused by the EF Core or by the Npgsql, so I'm creating the bug in the EF Core repository first. If the problem is not in the EF Core, I'll create a bug in Npgsql repository.

Steps to Reproduce

1. Open the minimal sample project IpAddressIndex.zip in Visual Studio.
2. Open Package Manager Console.
3. Execute Add-Migration Initial -Verbose in the console.

The command produces the following error:

Property 'Site.Address' cannot be used as a key because it has type 'IPAddress' which does not implement 'IComparable', 'IComparable' or 'IStructuralComparable'. Use 'HasConversion' in 'OnModelCreating' to wrap 'IPAddress' with a type that can be compared

Full command output: CommandOutput.txt

Version Information:

• EF Core version: 5.0.1
• Database provider: Npgsql.EntityFrameworkCore.PostgreSQL, v. 5.0.1
• Target framework: .NET Core 3.1, .NET 5
• Operating system: Windows 10 Enterprise 64-bit, v. 1903
• IDE: Visual Studio Professional 2019, v 16.8.3

The issue can be reproduced for the sample project both when it targets .NET Core 3.1 and when it targets .NET 5, provided that the versions of the NuGet packages in the .csproj file do not change. The issue cannot be reproduced if versions EF Core 3.x and Npgsql 3.x packages are used. The .csproj file in the sample project contains a commented out section that can be used to test that.

The issue cannot be reproduced if the index for the property is declared as non-unique.

[ajcvickers]

/cc @roji

[roji]

@ajcvickers this happens because .NET's IPAddress unfortunately doesn't implement IComparable/IStructuralComparable (which it probably should). As the error message suggests, it's possible to value-convert to a custom type that wraps IPAddress, but then the various method translators wouldn't work any more.

Does it make sense to add comparison functionality to ValueComparer instead?

[AIgin]

Same problem with PhysicalAddress.

Error:
Property 'EfMacAddress.Value' cannot be used as a key because it has type 'PhysicalAddress' which does not implement 'IComparable', 'IComparable' or 'IStructuralComparable'. Use 'HasConversion' in 'OnModelCreating' to wrap 'PhysicalAddress' with a type that can be compared.

[josh-power]

I'm seeing the same issue as Algin with a project I am currently from EF Core 2.0.2 to 5.0.2, as part of an overall upgrade to .NET 5.0. One option I have considered is implementing a sub-class from PhysicalAddress which does implement the 'IComparable<>' interface, and transitioning my project to use this type alongside a value converter. It's that or move away from PhysicalAddress' entirely, which is a massive headache for me.

[maknapp]

Is the IComparable requirement for .IsUnique() a breaking change that should be documented?

I have a project that is using both PhysicalAddress and IPAddress with unique indexes and it is blocking upgrade to .NET 5. I have tried extending PhysicalAddress with IComparable, added the .HasConversion(), and adding a type mapping.

When I add the conversion all I get is a different error:

System.InvalidOperationException: The property 'MyTestObject.MacAddress' is of type 'PhysicalAddress' which is not supported by the current database provider. Either change the property CLR type, or ignore the property using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.

Why would PhysicalAddress be tested for support when it is being "converted" to another type first?

---

public class CPhysicalAddress : PhysicalAddress, IComparable
{
    public CPhysicalAddress(byte[] address) : base(address)
    {
    }

    public int CompareTo(object obj)
    {
        if (obj is not CPhysicalAddress otherAddr)
        {
            return 0;
        }

        return ToString().CompareTo(otherAddr.ToString());
    }
}

builder.Property(x => x.MacAddress)
    .HasConversion(x => new CPhysicalAddress(x.GetAddressBytes()), x => x);

// npgsql
var mappingBuilder = new NpgsqlTypeMappingBuilder
{
    PgTypeName = "macaddr",
    NpgsqlDbType = NpgsqlDbType.MacAddr,
    ClrTypes = new[] { typeof(CPhysicalAddress) },
    TypeHandlerFactory = new ComparablePhysicalAddressHandlerFactory()
};
NpgsqlConnection.GlobalTypeMapper.RemoveMapping("macaddr");
NpgsqlConnection.GlobalTypeMapper.AddMapping(mappingBuilder.Build());

[roji]

Is the IComparable requirement for .IsUnique() a breaking change that should be documented?

I don't think there was a breaking change here, AFAIK it has been this way since forever (but @ajcvickers can confirm).

Apart from that, see below for a sample showing how to use the wrapper correctly with a value converter. You should not be doing at the Npgsql level (i.e. with the GlobalTypeMapper) - this is purely an EF-level workaround.

Workaround

await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    static ILoggerFactory ContextLoggerFactory
        => LoggerFactory.Create(b => b.AddConsole().AddFilter("", LogLevel.Information));

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseNpgsql(@"Host=localhost;Username=test;Password=test")
            .EnableSensitiveDataLogging()
            .UseLoggerFactory(ContextLoggerFactory);

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>(e =>
        {
            e.Property(b => b.Address).HasConversion(a => a.Value, a => new CPhysicalAddress { Value = a });
            e.HasIndex(b => b.Address).IsUnique();
        });
    }
}

public class Blog
{
    public int Id { get; set; }
    public CPhysicalAddress Address { get; set; }
}

public class CPhysicalAddress : IComparable<CPhysicalAddress>
{
    public PhysicalAddress Value { get; set; }

    public int CompareTo(CPhysicalAddress other)
        => ToString().CompareTo(other.ToString());
}

[maknapp]

It has not been this way forever. I am running .NET core 3.1 with several unique IPAddress and PhysicalAddress columns without issue. IComparable seems to only be required after updating to .NET 5.

Thanks for the example. The error message makes it seem like conversion should happen the opposite way. I do not like the idea of changing property types on the data models as it will require changes in many other places, but I'll give it a try.

[maknapp]

Wrapping all unique IPAddress and PhysicalAddress columns also requires changing the types used in every query. This is not practical in my case.

While adding IComparable to those types would fix this, would it also make sense to add a way to register a compare function for custom types? Or is this issue not that common?

---

Comparable IPAddress (for those who can use this workaround)

public class CIPAddress : IComparable<CIPAddress>
{
    public IPAddress Value { get; set; }
    
    public int CompareTo(CIPAddress other)
    {
        if (other is null)
        {
            return 1;
        }
        
        var aa = Value.GetAddressBytes();
        var bb = other.Value.GetAddressBytes();
        
        var cmp = aa.Length.CompareTo(bb.Length);
        if (cmp != 0)
        {
            return cmp;
        }

        for (var i = 0; i < aa.Length; i++)
        {
            cmp = aa[i].CompareTo(bb[i]);
            if (cmp != 0)
            {
                return cmp;
            }
        }

        return 0;
    }
}

[roji]

@maknapp this issue is precisely to allow defining a compare function on ValueComparer, which would be the proper fix for this. But as you suspect, this issue simply isn't that common - most databases have a pretty restricted set of supported types, and only Npgsql natively supports IPAddress/PhysicalAddress.

[maknapp]

I took a look at the source to see if I would be able contribute this fix. ValueComparer seems to be added directly to a property, not to a type. So every IPAddress property would require adding modelBuilder.Property(x => x.Ip).Metadata.SetValueComparer(...)? Is there a way to assign a ValueComparer/IComparable to a type once?

Would adding IComparable directly to the abstract ValueComparer also be a breaking change for anyone who has implemented ValueComparer?

I also created an issue to add IComparable to those classes (dotnet/runtime#56627) to discuss that change also.

[roji]

@maknapp

Is there a way to assign a ValueComparer/IComparable to a type once?

This should be made possible by pre-convention model configuration, though am not sure if we've specifically added support for value comparers (cc @AndriySvyryd).

Would adding IComparable directly to the abstract ValueComparer also be a breaking change for anyone who has implemented ValueComparer?

Ideally, the base ValueComparer could provide a default implementation (which would match today's behavior), and allow it to be overridden.

I also created an issue to add IComparable to those classes (dotnet/runtime#56627) to discuss that change also.

Thanks, though I suspect adding comparers to the BCL types just to help EF Core wouldn't be accepted easily - ideally there would be a case for sorting these types regardless of EF.

[roji]

Note the (reasonable) decision in dotnet/runtime#56627 - there's no standard ordering for IP addresses or MAC (hardware) addresses. So even if we add the ability to define comparison on value comparers (which isn't a bad idea), I'm not sure it makes sense for EF Core to provide a default behavior here (exactly as it doesn't make sense for the BCL to provide one).

Am not very knowledgeable in this area, but the original ask here is for a unique index - @ajcvickers any way of making that happen without client-side comparison/IComprable behavior?

[ajcvickers]

@roji We need to be able to order in the update pipeline to reduce the chance of deadlocks. It doesn't matter what that order is as long as its deterministic. /cc @AndriySvyryd

[roji]

@ajcvickers right... So since any arbitrary deterministic ordering is OK, it does seem to make sense to implement something at the EF Core level (once this issue is done), rather than at the BCL level.

[ajcvickers]

@roji Agreed.