Open two methods of UmbracoDatabaseFactory for External Packages (closes #21606)

[idseefeld]

This PR changes two private methods of UmbracoDatabaseFactory to make them accessible and overridable by packages.

protected DbProviderFactory? DbProviderFactory { get {...} }

protected virtual UmbracoDatabase? CreateDatabaseInstance() {...}

[github-actions]

Hi there @idseefeld, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Copilot]

Pull request overview

This PR exposes previously private extension points in UmbracoDatabaseFactory to enable plugin authors to access the resolved DbProviderFactory and override database instance creation.

Changes:

• Changed DbProviderFactory from private to protected.
• Changed CreateDatabaseInstance() from private to protected virtual.

Comments suppressed due to low confidence (1)

src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs:113

• DbProviderFactory was changed from private to protected, but it is still non-virtual. If the goal is to make it overridable by plugins (as per the PR description), derived factories cannot override this member (hiding with new won’t affect calls inside the base class). Consider making it protected virtual, or adjust the PR description to state that it’s only being made accessible (not overridable).

    protected DbProviderFactory? DbProviderFactory
    {
        get
        {
            if (_dbProviderFactory == null)
            {
                _dbProviderFactory = string.IsNullOrWhiteSpace(ProviderName)
                    ? null
                    : _dbProviderFactoryCreator.CreateFactory(ProviderName);
            }

            return _dbProviderFactory;
        }
    }

[AndyButland]

Can you expand on why you need these changes please @idseefeld? Maybe share a sample of your code where you show why you need to be able to access this property or override this method.

[idseefeld]

@AndyButland I use a method override for CreateDatabaseInstance to create a my own class PostgreSqlDatabase derived from UmbracoDatabase (source code).
This happens only when ProviderName == "Npgsql2" (the PostgreSQL provider name).
The PostgreSqlDatabase override some methods of UmbracoDatabase.

This is the whole factory class source:

public class PostgreSqlDatabaseFactory : UmbracoDatabaseFactory
{
    private readonly Dictionary<string, long> _lastInsertIds = new Dictionary<string, long>();

    private readonly DatabaseSchemaCreatorFactory _databaseSchemaCreatorFactory;
    private readonly ILogger<PostgreSqlDatabaseFactory> _logger;
    private readonly ILoggerFactory _loggerFactory;

    private NPoco.MapperCollection? _pocoMappers;

    /// <summary>
    ///     Initializes a new instance of the <see cref="PostgreSqlDatabaseFactory" />.
    /// </summary>
    /// <remarks>Used by the other ctor and in tests.</remarks>
    public PostgreSqlDatabaseFactory(
        ILogger<PostgreSqlDatabaseFactory> logger,
        ILoggerFactory loggerFactory,
        IOptions<GlobalSettings> globalSettings,
        IOptionsMonitor<ConnectionStrings> connectionStrings,
        IMapperCollection mappers,
        IDbProviderFactoryCreator dbProviderFactoryCreator,
        DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory,
        NPocoMapperCollection npocoMappers)
        : base(logger, loggerFactory, globalSettings, connectionStrings, mappers, dbProviderFactoryCreator, databaseSchemaCreatorFactory, npocoMappers)
    {
        _databaseSchemaCreatorFactory = databaseSchemaCreatorFactory ??
                                        throw new ArgumentNullException(nameof(databaseSchemaCreatorFactory));
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
        _loggerFactory = loggerFactory;

        ConnectionStrings umbracoConnectionString = connectionStrings.CurrentValue;
        if (umbracoConnectionString.ProviderName.IsNullOrWhiteSpace())
        {
            umbracoConnectionString.ProviderName = Constants.ProviderName;
        }

        if (!umbracoConnectionString.IsConnectionStringConfigured())
        {
            if (_logger.IsEnabled(LogLevel.Debug))
            {
                logger.LogDebug("Missing connection string, defer configuration.");
            }
            return; // not configured
        }

        Configure(umbracoConnectionString);
    }

    protected override UmbracoDatabase? CreateDatabaseInstance()
    {
        if (ProviderName != Constants.ProviderName)
        {
            return base.CreateDatabaseInstance();
        }

        if (ConnectionString is null || SqlContext is null || DbProviderFactory is null)
        {
            return null;
        }

        return new PostgreSqlDatabase(
            ConnectionString,
            SqlContext,
            DbProviderFactory,
            _loggerFactory.CreateLogger<PostgreSqlDatabase>(),
            BulkSqlInsertProvider,
            _databaseSchemaCreatorFactory,
            _pocoMappers);
    }
}

This all is related to PR #21590 because PostgreSqlDatabase.Insert<T>, PostgreSqlDatabase.InsertAsync<T> and PostgreSqlDatabase.CreateCommand methods fix some SQL issues.

But I figured out this approach some time ago and will have a look into it again. I'll check whether this PR and PS #21590 can be avoided.

[AndyButland]

Yes, I was hoping you'd mostly be able to follow the same pattern as the existing projects in core for SQL Server (Umbraco.Cms.Persistence.SqlServer )and SQLite (Umbraco.Cms.Persistence.Sqlite). Of course, as we've been doing, we need to fix any hardcoded incompatible SQL we have, and extend the SQL syntax abstraction where necessary to go beyond what we need for the core supported databases. But after that, I would hope you would be able to follow a similar approach as these projects, and you wouldn't have to override classes that these don't need to.

[idseefeld]

Sorry, but I could not find another solution. The NPoco project is not well maintained and PRs get not handled in time. The tests are mostly broken and the PostgreSQL handling is not compatible with Umbraco.
There seems to be an option to extend the PrimaryKey attribute like [PrimaryKey(PrimaryKeyColumnName, SequenceName = $"{TableName}_{PrimaryKeyColumnName}_seq")], but even than it does not work