Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Add Naming Convention Option for Database Entity Naming #6337

Open
adamfisher opened this issue Jan 25, 2025 · 6 comments
Open

[ENH] Add Naming Convention Option for Database Entity Naming #6337

adamfisher opened this issue Jan 25, 2025 · 6 comments

Comments

@adamfisher
Copy link

adamfisher commented Jan 25, 2025

Enhancement Request

Enhancement Overview

Database table names and column names default to however they were specified in the migration scripts. On SQL Server this leads to the Quartz tables being all uppercase while Elsa specific tables are all pascal case. On MySQL provider all names are lower case without an underscore making it more difficult to read (i.e. workflowdefinitions).

Proposed Enhancement

Integrate the EFCore.NamingConventions package and expose the various naming conventions offered by the library as configuration chained options on each Elsa DbContext configuration to modify the table and column names:

UseSnakeCaseNamingConvention()
UseLowerCaseNamingConvention()
UseCamelCaseNamingConvention()
UseUpperCaseNamingConvention()
UseUpperSnakeCaseNamingConvention()

The resulting startup would look similar to this:

services.AddElsa(elsa =>
{
    var elsaConnectionString = configuration.GetConnectionString(nameof(Elsa))!;
    var dbContextOptions = new ElsaDbContextOptions() { MigrationsHistoryTableName = "__ef_migrations_history" };

    elsa.UseWorkflowManagement(management => management.UseEntityFrameworkCore(ef => ef
        .UseMySql(elsaConnectionString, dbContextOptions)
        .UseSnakeCaseNamingConvention()));

    elsa.UseWorkflowRuntime(runtime => runtime.UseEntityFrameworkCore(ef => ef
        .UseMySql(elsaConnectionString, dbContextOptions)
        .UseSnakeCaseNamingConvention()));
    
    elsa.UseLabels(labels => labels.UseEntityFrameworkCore(ef => ef
        .UseMySql(elsaConnectionString, dbContextOptions)
        .UseSnakeCaseNamingConvention()));
    
    elsa.UseAlterations(alterations => alterations.UseEntityFrameworkCore(ef => ef
        .UseMySql(elsaConnectionString, dbContextOptions)
        .UseSnakeCaseNamingConvention()));
    
    elsa.UseIdentity(identity => identity.UseEntityFrameworkCore(ef => ef
        .UseMySql(elsaConnectionString, dbContextOptions)
        .UseSnakeCaseNamingConvention()));

    elsa.UseIdentity(identity => identity.UseEntityFrameworkCore(ef => ef.UseMySql(elsaConnectionString, dbContextOptions).UseSnakeCaseNamingConvention()));

    elsa.UseQuartz(quartz => quartz.UseMySql(elsaConnectionString).UseSnakeCaseNamingConvention());
}

Use Cases

This gives users flexibility to specify the naming convention that should be used throughout their database to match naming conventions of the platform they are using. MySQL for example is snake case but the default behavior for Elsa's MySQL provider is to just make everything lower case and all run together making it more difficult to read.

Impact of Enhancement

Naming things in the standard the database ensures coding standards and improves the developer experience.

Visuals and Mockups

Here is an initial implementation that seems close. I got Quartz migration history to respect snake casing of the migration history table by extending CustomHistoryRepository and replacing it in DI.

As for the Elsa configurations, neither feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention(); or feature.Services.ConfigureDbContext<ManagementElsaDbContext>(options => options.UseSnakeCaseNamingConvention()); worked. The tables always came out in lowercase and all running together:

Image

/// <summary>
/// Provides extensions to configure EF Core to use specific naming conventions.
/// </summary>
public static class EFCoreProvidersNamingConventionExtensions
{
    /// <summary>
    /// Configures the <see cref="EFCoreIdentityPersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreIdentityPersistenceFeature UseSnakeCaseNamingConvention(this EFCoreIdentityPersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="EFCoreAlterationsPersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreAlterationsPersistenceFeature UseSnakeCaseNamingConvention(this EFCoreAlterationsPersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="EFCoreLabelPersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreLabelPersistenceFeature UseSnakeCaseNamingConvention(this EFCoreLabelPersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="EFCoreWorkflowDefinitionPersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreWorkflowDefinitionPersistenceFeature UseSnakeCaseNamingConvention(this EFCoreWorkflowDefinitionPersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="EFCoreWorkflowInstancePersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreWorkflowInstancePersistenceFeature UseSnakeCaseNamingConvention(this EFCoreWorkflowInstancePersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="WorkflowManagementPersistenceFeature"/> to use MySql.
    /// </summary>
    public static WorkflowManagementPersistenceFeature UseSnakeCaseNamingConvention(this WorkflowManagementPersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        // feature.Services.ConfigureDbContext<ManagementElsaDbContext>(options => options.UseSnakeCaseNamingConvention());
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="EFCoreWorkflowRuntimePersistenceFeature"/> to use MySql.
    /// </summary>
    public static EFCoreWorkflowRuntimePersistenceFeature UseSnakeCaseNamingConvention(this EFCoreWorkflowRuntimePersistenceFeature feature)
    {
        feature.DbContextOptionsBuilder += (_, db) => db.UseSnakeCaseNamingConvention();
        return feature;
    }

    /// <summary>
    /// Configures the <see cref="QuartzFeature"/> to use MySql.
    /// </summary>
    public static QuartzFeature UseSnakeCaseNamingConvention(this QuartzFeature feature)
    {
        feature.Services.ConfigureDbContext<MySqlQuartzDbContext>(options =>
        {
            options.UseSnakeCaseNamingConvention()
                .ReplaceService<IHistoryRepository, CustomHistoryRepository>();
        });

        return feature;
    }
}

public class CustomHistoryRepository(HistoryRepositoryDependencies dependencies) : MySqlHistoryRepository(dependencies)
{
    protected override string TableName => "__ef_migrations_history";
}
@adamfisher adamfisher changed the title Add Naming Convention Option for Database Entity Naming [ENH] Add Naming Convention Option for Database Entity Naming Jan 25, 2025
@adamfisher
Copy link
Author

Opened enhancement based on discussion.

@Suchiman
Copy link
Contributor

Suchiman commented Jan 25, 2025

The issue here is that this configuration must be set before scaffolding migrations, so its an "everyone gets snake case" or "nobody gets snake case" kind of thing. Once you do Add-Migration, the naming convention is hardwired in the generated migration as you can see here for example:

migrationBuilder.AddColumn<string>(
name: "TenantId",
schema: _schema.Schema,
table: "WorkflowInboxMessages",
type: "longtext",
nullable: true)
.Annotation("MySql:CharSet", "utf8mb4");

This is why your attempts at changing the naming convention after the fact had no effect. So i don't think this is really actionable.

It might work if you copy this file and change this line

.MigrationsAssembly(options.GetMigrationsAssemblyName(migrationsAssembly))

to point at your own assembly and then generate your own set of migrations that include the changed naming conventions.

@adamfisher
Copy link
Author

I did see how those migrations were coded but I thought there was a way to get in between that process with the naming convention library. Anytime you attach new rules to the model binding of a DbContext I always viewed it as the "new lens" through which the database tables where created/read from. I was expecting it to behave similar to how when you generate identity tables in .NET core or just using the naming conventions on an already existing database.

The EFCore.NamingConventions library does mention this as well:

If you have an existing database, adding this naming convention will cause a migration to be produced, renaming everything.

But I guess maybe the difference here is that Elsa is doing code-first which is why we can't get in between that process? I was also experimenting with IModelCustomizer but I was hoping there was a clean way to either hijack the process or expose these options perhaps as part of the initial migration script if they are specified? That second approach is sort of what this issue was calling for.

@Suchiman
Copy link
Contributor

Suchiman commented Jan 25, 2025

But I guess maybe the difference here is that Elsa is doing code-first which is why we can't get in between that process?

No, that distinction isn't meaningful in EFC anymore, there's only code first. (Scaffold-DbContext, which generates a DbContext from a database for example, is not database first, it merely generates a code first model).

Anytime you attach new rules to the model binding of a DbContext I always viewed it as the "new lens" through which the database tables where created/read from.

Not quite. The way it works is, that when a DbContext is first accessed, EFC builds a Model. That involves discovering all entity types and their data annotations. This then runs all configured (built-in's as well as user defined) model conventions which make changes to the model that was discovered so far, e.g. one convention is to create an index for every navigation property. And then lastly it runs your fluent configuration. At this point, there's a "current model" in memory now that describes how the database should look like that EFC will use to formulate the SQL queries against the database.

The optional "code first migrations" feature, once you do the first ever Add-Migration, will create a snapshot file, e.g. https://github.com/elsa-workflows/elsa-core/blob/main/src/modules/Elsa.EntityFrameworkCore.MySql/Migrations/Runtime/RuntimeElsaDbContextModelSnapshot.cs
that contains a serialized model of the "current model" as EFC was told the database should look like. It then generates a migration snapshot that contains all the necessary instructions to generate that version of a database. Henceforth it is immutable. When you type Add-Migration a second time, the existing ModelSnapshot.cs is loaded and then compared to the now "current model". A new immutable migration snapshot is then generated that contains only the necessary instructions to move from then ModelSnapshot.cs to then "current model". The ModelSnapshot.cs is then updated to reflect "current model" again.

If you now add the naming convention package, that sits at the same stage of the pipeline as "create an index for every navigation property". It will affect the "current model" and if you were inclined to do Add-Migration it would notice from comparing ModelSnapshot.cs with "current model" that it needs to rename everything but it never changes migrations retroactively.

Changing migrations retroactively is unlikely to work in all but the simplest cases, e.g. let's say you have:
Migration 1: AddColumn("HelloWorld");
Migration 2: RenameColumn("HelloWorld", "ByeWorld");
Let's say user applies Migration 1, then enables EFCore.NamingConventions and then tries to apply Migration 2. If EFCore.NamingConventions now were to change RenameColumn("HelloWorld", "ByeWorld"); to RenameColumn("hello_world", "bye_world"); then this would not work as the column is currently HelloWorld and not hello_world

@Suchiman
Copy link
Contributor

Suchiman commented Jan 25, 2025

Elsa's approach is to ship its own DbContext with its own Migrations, all fully self contained.
For contrast, in Openiddict for example, you have to provide a DbContext to Openiddict which then adds their entities to your DbContext. This affords you the flexibility to change their configuration but you're also responsible for managing the migrations.

@adamfisher
Copy link
Author

I guess I'll just learn to live with the bad naming convention unless it's possible we could just add a migration script to always force postgres and my sql to use snake case while SQL server uses Pascal case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants