Prettify Data Provider APIs

[hishamco]

AFAIK YesSql support variety of RDBMS, nothing but what I have seen here is:

• Add supported databases
• Using TryAddDataProvider multiple times, to add the supported databases
• All the database providers and their configuration in one place

so, I come up with similar approach that EF are using in ASP.NET Core nowadays to make developers life easier 😄

public void ConfigureServices(IServiceCollection services)
{
    services.AddDbProvider(options =>
        options.UseSqlServer("Server=.;Database=OrchardDb;Integrated Security=True"));
    ...
}

This approach will give us a lot of benefits:

• No more dependencies, we just rely on YesSql APIs, so no need to reference:

  using System.Data.SqlClient;
  using Microsoft.Data.Sqlite;
  using MySql.Data.MySqlClient;
  using Npgsql;

• Readability
• Simplicity
• Make the database providers pluggable, which is the important thing for us. So anyone can integrate a a new data provider in future such as Oracle .. etc
• We can implement the default database provider for Orchard CMS, and let the developers implement the rest - which is not tough -

What we need to make this happen?

• Add IDbProviderOptions or DbProviderOptions to Orchard.Data.Abstractions
• Add the default provider using the pattern UseXXX extension method to DbProviderOptions
• Add AddDbProvider extension method to IServiceCollection to register the IStore service

FYI I already made a prototype for this, it's works like a charm 😄 , I will refer to it ASAP

/cc @sebastienros

[hishamco]

FYI https://github.com/hishamco/yessql/commit/61da035bc81678562ef45a356cd7ed20fb859e1c

[Jetski5822]

Awesome work, I like it. Seb what do you think?

Sent from my iPhone

On 27 Mar 2017, at 02:19, Hisham Bin Ateya notifications@github.com wrote:

FYI hishamco/yessql@61da035

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[Jetski5822]

It might be worth doing a PR for this too, if you have time.

Sent from my iPhone

On 27 Mar 2017, at 02:19, Hisham Bin Ateya notifications@github.com wrote:

FYI hishamco/yessql@61da035

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[hishamco]

Of course I'll do @Jetski5822, but I have one question, shall we make the ASP.NET Core stuff built-in in YesSQL so anyone can use in his web application(s) or shall we do it here

Of course I don't wanna bring dependencies to YesSQL, but this worth specially if we doing web development stuff

[hishamco]

After digging into the code I notice that the ConnectionString came from ShellSettings, is there a plan to use the Configuration APIs to make the proposal readable like this:

services.AddDbProvider(options =>
        options.UseSqlServer(Configuration["Data:DefaultConnection:ConnectionString"]));

For now I can make another overload like this:

services.AddDbProvider(options => options.UseSqlServer());

to hide the heavy lifting for bringing the ConnectionString

Last point I'd like to mention is what's the default database provider that we need to ship? Shall we separate into another project Orchard.Data.[DatabaseProvider], which is the thing I prefer, so the Orchard Core will be data provider(s) independent? if that's the case, we can add the other data providers - in the future - in the same way as well

[Jetski5822]

That sounds like a good approach.

Orchard.Data.[DatabaseProvider]

+1

[hishamco]

What about my previous comment about the ConnectionString shall I add another overload for now until the ShellSettings access via Configuration APIs?

[sebastienros]

From what I understand you are suggesting something like this:

services.AddDbProvider(options =>
        options.UseSqlServer("Server=.;Database=OrchardDb;Integrated Security=True"));

But the goal of the Orchard.Data Module is to provide a list of providers, names of servers and capabilities.

I think your suggestion is good for YesSql but Orchard (the CMS part) cannot use it. It need to create the DbFactory dynamically from the tenant settings.

@hishamco I think you didn't realize how Orchard Core CMS supports multi-tenancy and Configuration is not adapted to store tenants information, as tenants are created dynamically by the end user (from UI for instance).

Right now I would close this issue as long as it's reopened on the YesSql repository where it totally makes sense.

[hishamco]

But the goal of the Orchard.Data Module is to provide a list of providers, names of servers and capabilities.

The current proposal can list all data providers if we do so, by having: Orchard.Data.SqlServer, Orchard.Data.Sqlite, Orchard.Data.MySQL and Orchard.Data.Postgres, separate these providers will be better for us, because the Orchard Core will data provider(s) independent, but when it find a XXXDbProvider it be listed as you want

I think your suggestion is good for YesSql but Orchard (the CMS part) cannot use it. It need to create the DbFactory dynamically from the tenant settings.

Of course it will be good for YesSQL, but Orchard CMS can use the benefits, furthermore no need to create DbFactroy dynamically because the YesSQL default providers will be in isolated assemblies and will be registered in the DI, my aim is not create it dynamically

[sebastienros]

Now I can see how it would allow Db extensibility. Good then. And then each provider comes from a specific module, and they are enabled by Default for the Setup. Thanks Hisham.

[hishamco]

IMHO we can simplify the process by adding such APIs in YesSQL, I already blogged about this today Prettify YesSQL Data Access APIs in ASP.NET Core as well as Orchard CMS can use the same approach, plus adding the data provider(s) module(s). No need to write the code twice for both YesSQL & Orchard, but each one will use it from the way that it fits.

So let me think about it again from different angle, I will share my suggestions with you guys ..

[sebastienros]

I don't agree here:

YesSql needs to be configured with a DbFactory instance containing a connection string, which you are showing correctly in your blog post about YesSql.

Orchard Core CMS needs to be configured with DbProvider types, and other metadata to drive the UI. So it can itself generate a DbFactory instance.

So in my opinion, changes need to be done in both repos, and they are not the same changes.

[hishamco]

Yep, exactly .. let me open another issue in YesSQL repo, so can track both as per their needs. Also I will dig into the source code here to see how the `DbProvider types and other metadata drives the UI

thanks

[hishamco]

@sebastienros after the latest changes that we have done in YesSql repo, what's the changes that you suggest to reuse some of the APIs that a;ready done there

[sebastienros]

This lambda could be simplified then: https://github.com/OrchardCMS/Orchard2/blob/master/src/OrchardCore/Orchard.Data/ServiceCollectionExtensions.cs#L36

But nothing outside of it. And it might be helped by other improvements in YesSql API for instance to add index providers. Maybe it will require an options object, who knows ;)

[hishamco]

What about those https://github.com/OrchardCMS/Orchard2/blob/master/src/OrchardCore/Orchard.Data/ServiceCollectionExtensions.cs#L29-L32

Another benefit may we have here to reduce the amount of the dependencies, so please have a look to your earlier comment

Now I can see how it would allow Db extensibility. Good then. And then each provider comes from a specific module, and they are enabled by Default for the Setup. Thanks Hisham.

[hishamco]

I think it may needs a good design to make things better ..

[sebastienros]

What about those https://github.com/OrchardCMS/Orchard2/blob/master/src/OrchardCore/Orchard.Data/ServiceCollectionExtensions.cs#L29-L32

These are the ones that provide UI metadata. This project exposes the available choices for the user. It also decides what the choices are. So yes, this project should reference the new projects you created instead of the dbproviders directly. But don't change these four lines.

[hishamco]

Sorry @sebastienros because I late to submit a PR for this, after that changes that I made in YesSql repo, but I notice that you made a PR #757 is it covered this issue?

[sebastienros]

You already made yessql pretty. I don't see anything else that could be better for Orchard.

[hishamco]

Sure 😄 , but I surprised when the issue was closed, because this https://github.com/OrchardCMS/Orchard2/blob/master/src/OrchardCore/Orchard.Data/ServiceCollectionExtensions.cs#L39 lambda still the same, I will make a PR for this soon