fix: Handle JSON-mapped complex properties. Fixes #336

[stigrune]

Resolved an issue where EFCore.NamingConventions incorrectly assigned a column name to a ComplexProperty stored as JSON, which caused EF Core to throw an exception.

When an entity has complex properties that are mapped to JSON, the code will detect them and null the Column Name set by RewriteColumnName.

I initially attempted to implement the fix earlier in the conventions pipeline, specifically within RewriteColumnName, trying to prevent the property from being set in the first place. This approach was not successful because I couldn't reliably determine if the complex property was stored as JSON or not as this stage. Further details on the first attempt can be found in the issue.

[ltbyun]

@stigrune maybe you could do all the things in ProcessModelFinalizing.
Maybe you don’t need to make such complex judgments at all. Whether a property needs a column name or not, let EF Core decide for itself. You only need to replace the column name when it exists. Just remove all the interfaces from NameRewritingConvention except IModelFinalizingConvention, and do as below. it works well in my project, very simple.

        public void ProcessModelFinalizing(IConventionModelBuilder modelBuilder,
            IConventionContext<IConventionModelBuilder> context)
        {
            foreach (var entityType in modelBuilder.Metadata.GetEntityTypes())
            {
                var tableName = entityType.GetTableName();
                if (!string.IsNullOrEmpty(tableName))
                {
                    entityType.SetTableName(tableName.ToSnakeCase());
                }

                var viewName = entityType.GetViewName();
                if (!string.IsNullOrEmpty(viewName))
                {
                    entityType.SetViewName(viewName.ToSnakeCase());
                }

                foreach (var property in entityType.GetProperties())
                {
                    var columnName = property.GetColumnName();
                    if (!string.IsNullOrEmpty(columnName))
                    {
                        property.SetColumnName(columnName.ToSnakeCase());
                    }
                }

                foreach (var key in entityType.GetKeys())
                {
                    var keyName = key.GetName();
                    if (!string.IsNullOrWhiteSpace(keyName))
                    {
                        key.SetName(keyName.ToSnakeCase());
                    }
                }

                foreach (var foreignKey in entityType.GetForeignKeys())
                {
                    var fkName = foreignKey.GetConstraintName();
                    if (!string.IsNullOrWhiteSpace(fkName))
                    {
                        foreignKey.SetConstraintName(fkName.ToSnakeCase());
                    }
                }

                foreach (var index in entityType.GetIndexes())
                {
                    var indexName = index.GetDatabaseName();
                    if (!string.IsNullOrEmpty(indexName))
                    {
                        index.SetDatabaseName(indexName.ToSnakeCase());
                    }
                }
            }
        }

[roji]

Everyone, it will take me a bit more time to get around to EFCore.NamingConventions (there's a lot going on around the EF / Npgsql 10 release at the moment). But in a week or two I should be able to take a look at the issues.

[stigrune]

Thanks for the comments from both of you! This might not be the ideal solution, but it was the best I could come up with given my limited understanding of how EF Core works internally. If this can be fixed in a more elegant way, please feel free to close this PR without merging 😊

[Ta1sty]

Hello everyone,

I also want to chime in here. I ran into the same problem while migrating my previously owned json columns to complex properties mapped to json.

@ltbyun While this approach would probably work, I think Roji followed the approach that the convention should act as soon as possible. Such that consumers which would act upon column renames (suppose someone wrote a convention which automatically creates triggers (it needs column name for that)) would get properly notified.

@stigrune This will fix the problem for single-nested complex properties. Did you test it with a chain like this?

entity.ComplexPropertyMappedToJson.ComplexProperty.ComplexProperty.Property

or for that matter (I am unsure if this is even supported):

entity.OwnedEntity.ComplexPropertyMappedToJson.ComplexProperty.Property

@roji

I addition I also want to add that this convention also currently does not set:

RelationalAnnotationNames.ContainerColumnName

on the root complex type which is mapped to the json column (i.e. the container column). Or at least it didn't for me.

---

For everyone looking for a quick fix:

Add the following code in your OnModelCreating to temporarily fix the issue until someone fixes this:

!PLEASE note that this may override other explicitly set ContainerColumnNames!
You will either need to adjust this code or explicitly set them again after calling the loop.

foreach (var complexProperty in modelBuilder.Model.GetEntityTypes().SelectMany(x => x.GetComplexProperties()))
{
    ConfigureComplexJsonProperties(complexProperty);
}

// TODO remove this once EfCore.NamingConventions fixed this:
// https://github.com/efcore/EFCore.NamingConventions/pull/338
// pick the name rewriter of your choice
private readonly SnakeCaseNameRewriter _nameRewriter = new(CultureInfo.CurrentCulture);
private void ConfigureComplexJsonProperties(IMutableComplexProperty property)
    {
        if (!property.ComplexType.IsMappedToJson())
        {
            return;
        }

        if (property.DeclaringType is IEntityType)
        {
            // Trying to avoid overwriting explicitly set column names, this will probably work most of the time.
            if (property.ComplexType.GetContainerColumnName() == property.Name)
            {
                property.ComplexType.SetContainerColumnName(_nameRewriter.RewriteName(property.Name));
            }
        }

        foreach (var child in property.ComplexType.GetProperties())
        {
            child.SetColumnName(null);
        }

        foreach (var child in property.ComplexType.GetComplexProperties())
        {
            ConfigureComplexJsonProperties(child);
        }
    }

[Ta1sty]

@roji

Another weird behaviour is that the ConfigurationSource for the ContainerColumnName seems to always be 'Explicit' which makes writing a convention kind of pointless, since we can't distinguish if we should act or not.

The only action to configure the complex property I used was:

entity.ComplexProperty(x => x.Organizer).ToJson();

IMO this should not count as explicitly setting the ContainerColumnName. Is this a bug within EF?

Let me know if you need a repo of this and/or i should open an issue on EF.

[stigrune]

@Ta1sty Thnx! I updated the code to support nested complex properties now.

[roji]

@stigrune I finally found some time to work on EFCore.NamingConventions - sorry for the wait.

I took a look at this, and managed to implement it in the preferred way in #345. As you noticed above, calling ToJson() indeed sets the container column name; since this is a regular annotation, it gets annotated via IComplexTypeAnnotationChangedConvention. I implemented that, both rewriting the container column name and resetting the previously-rewritten column name on all properties nested within.

Everything seems to work as expected, so I'll go head and merge this and release 10.0.0. But let me know if I've missed anything - I can always fix and release a 10.0.1.

In any case, thanks for spending time on this - the effort is appreciated!