EFCore.BulkExtensions icon indicating copy to clipboard operation
EFCore.BulkExtensions copied to clipboard

Per Provider Packaging

Open HClausing opened this issue 2 years ago • 2 comments

As described in #941 , this PR show how the ideia could be implemented.

image

I would be honored if this PR were reviewed.

Work did:

  • Added separated projects for SqlServer, MySql, PostgreSql and SQLite;
  • Moved all members from SqlAdapters folder for target provider project;
  • Reviewed all dependencies by EfCore provider;
  • Created IDbServer interface to be implemented by each EfCore provider, supplying instances for Adapter, Dialect, and some other features;
  • Added SqlAdaptersMapping.DbServer static property for setting up the desired provider to be used;
  • Refactored code that depends to call members from providers on core project.

About the tests:

I didn't figured yet how to get the tests working. I'm getting some connection errors. Should the databases exists on real instances on my computer?

Thanks.

HClausing avatar Sep 06 '22 18:09 HClausing

Will take a look 👍

borisdj avatar Sep 07 '22 07:09 borisdj

I forgot to mention on the first topic: There's a breaking change in this PR: It' important to set the DbServer at some app's point, like Startup, etc:

// For SqlServer:
EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer = new EFCore.BulkExtensions.SqlAdapters.SQLServer.SqlDbServer();

// For MySql:
EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer = new EFCore.BulkExtensions.SqlAdapters.MySql.MySqlDbServer();

// For PostgreSql:
EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer = new EFCore.BulkExtensions.SqlAdapters.PostgreSql.PostgreSqlDbServer();

// For SQLite:
EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer = new EFCore.BulkExtensions.SqlAdapters.SQLite.SqlLiteDbServer();

HClausing avatar Sep 08 '22 11:09 HClausing

I forgot to mention on the first topic: There's a breaking change in this PR: It' important to set the DbServer at some app's point, like Startup, etc:

@HClausing can't this be done using Dependency Injection instead? You could register the type (probably interface) of EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer as a singleton, and then inject it, right? To make it nice for the user you could add Extension methods for registering the right server.

marklagendijk avatar Nov 11 '22 07:11 marklagendijk

I forgot to mention on the first topic: There's a breaking change in this PR: It' important to set the DbServer at some app's point, like Startup, etc:

@HClausing can't this be done using Dependency Injection instead? You could register the type (probably interface) of EFCore.BulkExtensions.SqlAdapters.SqlAdaptersMapping.DbServer as a singleton, and then inject it, right? To make it nice for the user you could add Extension methods for registering the right server.

I really like this ideia, thanks.

But, thinking on desktop apps, like WPF, Winforms, etc, I think we can keep the current way for compatibility, and create the DI extension for web apps. What do you think ?

HClausing avatar Nov 11 '22 11:11 HClausing

Those also support dependency injection setup via the IServiceCollection don't they?

The extensions could look like something this:

    public static class ServiceCollectionExtensions
    {
        public static IServiceCollection AddBulkExtensionsMySqlAdapter(this IServiceCollection services)
        {
            if (services == null)
            {
                throw new ArgumentNullException(nameof(services));
            }

            services.TryAddSingleton<EFCore.BulkExtensions.SqlAdapters.IDbServer, EFCore.BulkExtensions.SqlAdapters.MySql.MySqlDbServer>();
            return services;
        }
    }

If it is a common use case to not use DI, it makes sense to keep the current approach for those use cases.

marklagendijk avatar Nov 11 '22 11:11 marklagendijk

Those also support dependency injection setup via the IServiceCollection don't they?

The extensions could look like something this:

    public static class ServiceCollectionExtensions
    {
        public static IServiceCollection AddBulkExtensionsMySqlAdapter(this IServiceCollection services)
        {
            if (services == null)
            {
                throw new ArgumentNullException(nameof(services));
            }

            services.TryAddSingleton<EFCore.BulkExtensions.SqlAdapters.IDbServer, EFCore.BulkExtensions.SqlAdapters.MySql.MySqlDbServer>();
            return services;
        }
    }

If it is a common use case to not use DI, it makes sense to keep the current approach for those use cases.

Yes it's supported also, but I guess it's not commonly used on desktop scenario. I can't say it with certain, but I've seen many desktop applications (in most of cases legacy apps) that didn't use DI.

About the DI sample, I liked and will push a new commit with this.

Thanks a lot!

HClausing avatar Nov 11 '22 12:11 HClausing

Only now did I catch time to look more into this. Project split is fine. However I would like to keep the main Project/Nuget common and to additionally have specific ones for just single provider. I figured this can be achieved by renamed current Project from EFCore.BulkExtensions to EFCore.BulkExtensionsBase for example, and then adding one more project with original name EFCore.BulkExtensions that would include all projects for specific ones. Another thing is to keep it without break changes, so the DbServer maybe could be created dynamically with reflection after reading providerType from context.

borisdj avatar Nov 30 '22 16:11 borisdj

Only now did I catch time to look more into this. Project split is fine. However I would like to keep the main Project/Nuget common for and to additionally have specific ones for just single provider. I figured this can be achieved by renamed current Project from EFCore.BulkExtensions to EFCore.BulkExtensionsBase for example, and then adding one more project with original name EFCore.BulkExtensions that would include all projects for specific ones. Another thing is to keep it without break changes, so the DbServer maybe could be created dynamically after reading providerType from context.

Thanks for the review.

So, If I understand correctly, will be a single project named EFCore.BulkExtensions, how it is today: Untouched and without any dependencies. And another project called EFCore.BulkExtensionsBase (why not EFCore.BulkExtensionsCore ? ) for basic implementations (and working to remove that breaking change) to be referencied by anothers projects (per provider).

HClausing avatar Nov 30 '22 17:11 HClausing

UPDATE: The following is new structure, projects: [1]. EFCore.BulkExtensionsCore - shared core project - No Nuget [2]. EFCore.BulkExtensions.SQLServer (references [1], includes SQLServer provider) - new Nuget [3]. EFCore.BulkExtensions.PostgreSQL (references [1], includes PostgreSql provider) - new Nuget [4]. EFCore.BulkExtensions.MySQL (references [1], includes MySql provider) - new Nuget [5]. EFCore.BulkExtensions.SQLite (references [1], includes Sqlite provider) - new Nuget [6]. EFCore.BulkExtensions.Common (new project that references [2],[3],[4],[5]) - existing Nuget with base name: 'EFCore.BulkExtensions' that keeps all providers with full compatibility

borisdj avatar Nov 30 '22 22:11 borisdj

@HClausing Thx for contrib. PR was merge, restructure made, previous comment updated according to new structure. Nuget 6.6.0 published

borisdj avatar Dec 07 '22 11:12 borisdj

@HClausing Thx for contrib. PR was merge, restructure made, previous comment updated according to new structure. Nuget 6.6.0 published

Thank you! I'm glad to help.

HClausing avatar Dec 07 '22 12:12 HClausing

@borisdj it seems that something went wrong (or is delayed) in publishing the new packages. I'm trying to use the new .SqlServer package:

<PackageReference Include="EFCore.BulkExtensions.SqlServer" Version="6.6.0"/>

I'm getting the following error:

[NU1101] Unable to find package EFCore.BulkExtensionsCore. No packages exist with this id in source(s): nuget.org

If I look on Nuget, I also don't see that package: https://www.nuget.org/profiles/borisdj

One of the problems seems to be that someone already published a .SQLite package: https://www.nuget.org/packages/EFCore.BulkExtensions.Sqlite

marklagendijk avatar Dec 07 '22 12:12 marklagendijk

Delay, now it should available. I know for the Lite, more info in issue https://github.com/borisdj/EFCore.BulkExtensions/issues/828

borisdj avatar Dec 07 '22 12:12 borisdj

Solved for Lite as well.

borisdj avatar Dec 07 '22 13:12 borisdj