Fix configure smart enum for owned types

[mgraf1]

This PR attempts to fix #253 which impacts SmartEnum.EFCore.

The main issue is that when ConfigureSmartEnum is called, it sets the conversion for every entity with the following code

modelBuilder.Entity(entityType.Name).Property(property.Name).HasConversion(converter);

This breaks for owned entity types since modelBuilder.Entity marks the entity as non-owned. The solution proposed in this PR is to first check if the entity type is owned, and if it is, configure the property with

entityType.FindProperty(property.Name).SetValueConverter(converter);

This seems to work for owned entities which were previously causing ConfigureSmartEnum.

This PR also adds an extension method for the new ModelConfigurationBuilder which was introduced in EF Core 6. It can be used in placed of the ModelBuilder extension method. It can be used like this:

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.ConfigureSmartEnum();
}

[ardalis]

We could support both approaches for backward compatibility, right? Both the extension and the method that accepts the builder.

[mgraf1]

That is true. I can update the PR to also include the other approach.

[ardalis]

Sure, let's do that. Would you mind briefly showing the usage in the README file as well in the PR?

[mgraf1]

I updated the code to add the new ModelConfigurationBuilder extension while updating the older one to prevent it from breaking with owned entities. I also updated the README.

[fiseni]

Hey @mgraf1 @ardalis

I just noticed this PR. Starting with EF Core 6, the correct approach is configuring it through ConfigureConventions. If configuring through OnModelCreating, you always have to call the extension method in the end (otherwise entityType.IsOwned() will always be false). Doing so will override user-specific conversions defined for a given SmartEnum property. On the other hand, if using conventions, the user has the freedom to override any configuration he wants.

I've been using an extension for ModelConfigurationBuilder for quite some time, and I want to point out one caveat. You must call configurationBuilder.ConfigureSmartEnum() before other conventions, otherwise, the configuration will fail. For instance, if you modify your TestDbcontext2 as follows, the test will fail.

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<string>().HaveMaxLength(250);
    configurationBuilder.ConfigureSmartEnum();
}

I suspect that configurationBuilder.CreateModelBuilder(null); messes things up. Anyhow, perhaps the usage should be documented. It's a subtle bug, and it took me quite some time to figure out what was going on :)

[fiseni]

Btw, all of these extensions will work only with simple enums. For more complex scenarios with derived enums, they will fail. There are some variations defined in UnitTests project. So if we include all these types in our test models, we might get better test coverage

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

    public Weekday Weekday { get; set; }

    public TestEnum Test1 { get; set; }
    public TestDerivedEnum Test2 { get; set; }
    public TestStringEnum Test3 { get; set; }
    public TestBaseEnumWithDerivedValues Test4 { get; set; }
    public DerivedTestEnumWithValues1 Test5 { get; set; }
    public DerivedTestEnumWithValues2 Test6 { get; set; }
}

PS. Sorry for being a party breaker. You did a great job here actually. I just wanted to point out some pitfalls.

[fiseni]

This would work only if you have included the property in the configuration, otherwise FindProperty will return null. If you do the following change in TestDbContext1 (comment the line), the test will fail. It is very likely that users won't include additional configuration.

public class TestDbContext1 : Microsoft.EntityFrameworkCore.DbContext
{
    public TestDbContext1(DbContextOptions<TestDbContext1> options) : base(options)
    { }

    public TestDbContext1()
    {
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<SomeEntity>()
            .OwnsOne(
                e => e.OwnedEntity,
                owned =>
                {
                    owned.Property(o => o.Value).HasColumnName("Owned1Value");
                    //owned.Property(o => o.Weekday).HasColumnName("Owned1Weekday");
                });

        modelBuilder.ConfigureSmartEnum();
    }

    public DbSet<SomeEntity> SomeEntities { get; set; }
}

[mgraf1]

@fiseni, @ardalis

Btw, all of these extensions will work only with simple enums. For more complex scenarios with derived enums, they will fail.

It seems the complex enums which cause problems are DerivedTestEnumWithValues1 and DerivedTestEnumWithValues2. The reason being that SmartEnumConverter<TEnum, TValue> only works for SmartEnums where the first generic type parameter TEnum is the same as the type itself. DerivedTestEnumWithValues1 and DerivedTestEnumWithValues2 extend SmartEnum<TestBaseEnumWithDerivedValues>.

To handle these cases, I think there would need to be a new implementation of SmartEnumConverter which has a 3rd generic type argument for the base type. Maybe that should be outside the scope of this PR since the current version of the library does not support those enums and the goal is to fix the issue with owned entities.

This would work only if you have included the property in the configuration, otherwise FindProperty will return null. If you do the following change in TestDbContext1 (comment the line), the test will fail. It is very likely that users won't include additional configuration.

I could make a change to check if the property is null prior to calling SetValueConverter. This will prevent the code from throwing an exception, but it will not add a value converter to the property. I fear this may to be a compromise. If the user does not call add any configuration to the property, EF Core will treat it as a navigation. I cannot figure out how to use the low-level Metadata API to explicitly remove the navigation from an owned entity. For regular entities, you can just use the fluent API, but there doesn't appear to be an equivalent mechanism for owned entities. If we do decide this caveat is acceptable, I can add it to the documentation. IMO, it's a fair compromise since owned entities just flat out do not work with the current code, so requiring a little extra configuration could be acceptable. Definitely curious on your thoughts here though.

[fiseni]

Yes, you're right about the derived enums. It's out of scope here. Once this is finished, I'll create a PR and fix those issues.

As for the FindProperty, yea that might be a reasonable compromise I guess. But, instead of doing nothing if null, perhaps we should throw an exception with an explicit message guiding the consumer on what to do to fix the issue. Otherwise, it will be treated as navigation and EF will throw an exception (e.g. something like not suitable constructor..). Consumers will be confused and won't have an idea what they did wrong.

Btw, the more correct approach here would be not to use FindProperty at all, but something as follows. Give it a try, I think it should work.

if (isOwned)
{
    var ownership = entityType.FindOwnership();
    if (ownership != null)
    {
        modelBuilder.Entity(ownership.PrincipalEntityType.ClrType).OwnsOne(entityType.ClrType, ownership.GetNavigation(false).Name, x =>
        {
            x.Property(property.Name).HasConversion(converter);
        });
    }
}
else
{
    modelBuilder.Entity(entityType.Name).Property(property.Name).HasConversion(converter);
}

PS. As you can imagine, there are corner cases here too. What if the owned entity is part of another owned entity? For a full solution, we need recursion here until we get to the parent that is not Owned.

[mgraf1]

Just updated my PR based on your feedback. I started by looking at your suggestion around not using FindProperty. It's a really nice idea, however I found that it broke when dealing with OwnsMany relationships. My latest PR takes things a bit further and does some additional checking to see if the ownership is unique. If it is, the code uses OwnsOne, otherwise it uses OwnsMany.

My latest updates should also handle nested ownership relationships. I did it iteratively instead of recursively, but the end result should be the same. I added the suggested enums to the integration tests (except the problematic once we discussed above). I also added a nested owned entity as well as an owns many relationship. Let me know what you think and if you have any additional feedback. Really enjoying working on this with you! Thanks again for all your help!

[fiseni]

It looks great. You have covered all cases and it's backward compatible.
@ardalis this is ready for merging.

[mgraf1]

Hey @fiseni, thanks for the careful review! When I come back from vacation tomorrow, I'll update the PR based on your feedback.

[ardalis]

Woohoo! Nice job both of you!