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

Add extension methods to enable simpler adding and configuration of EF Core providers #25192

Closed
DamianEdwards opened this issue Jul 2, 2021 · 22 comments
Labels
area-dbcontext closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-enhancement
Milestone

Comments

@DamianEdwards
Copy link
Member

Today, adding and configuring Entity Framework Core in a project that uses Microsoft.Extensions.Hosting (e.g. an ASP.NET Core project) requires calling multiple layers of extension methods to first add EF Core, and then add and configure the desired provider, e.g. SQLite. In simple cases where the project has no need for the utility provided by DbContextOptionsBuilder or in tutorials this can result in extra concepts being forced upon the user that aren't required at this stage.

I propose we add higher-level extension methods to enable simpler setup of EF Core with a specific provider. In each provider package a new set of IServiceCollection extension methods would be declared in the Microsoft.Extensions.DependencyInjection namespace, e.g.:

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.Extensions.DependencyInjection
{
    public static class ServiceCollectionExtensions
    {
        public static IServiceCollection AddSqlite<TDbContext>(this IServiceCollection services, string connectionString)
            where TDbContext : DbContext
        {
            services.AddDbContext<TDbContext>(options => options.UseSqlite(connectionString));
            return services;
        }

        public static IServiceCollection AddSqlite<TDbContext>(this IServiceCollection services, Action<SqliteDbContextOptionsBuilder> sqliteOptionsAction)
            where TDbContext : DbContext
        {
            services.AddDbContext<TDbContext>(options => options.UseSqlite(sqliteOptionsAction));
            return services;
        }

        public static IServiceCollection AddSqlite<TDbContext>(this IServiceCollection services, string connectionString, Action<SqliteDbContextOptionsBuilder> sqliteOptionsAction)
            where TDbContext : DbContext
        {
            services.AddDbContext<TDbContext>(options => options.UseSqlite(connectionString, sqliteOptionsAction));
            return services;
        }
    }
}

We can compare the common case today, vs. what would be possible with such extension methods:
Before

using Microsoft.EntityFrameworkCore;

var connectionString = builder.Configuration.GetConnectionString("DefaultConnection");
builder.Services.AddDbContext<ApplicationDbContext>(options =>
    options.UseSqlite(connectionString));

After

var connectionString = builder.Configuration.GetConnectionString("DefaultConnection");
builder.Services.AddSqlite<ApplicationDbContext>(connectionString);

Potential benefits:

  • The methods elevate the the provider name, e.g. "Sqlite", to be the primary subject, rather than the DbContext. This can be useful in learning scenarios where the arc isn't ready to explain the concepts related to EF Core and its provider model (e.g. ORMs, providers, option delegates, etc.)
  • The methods remove the need for importing the Microsoft.EntityFrameworkCore namespace to get to the "can read/write to/from a database" stage

@davidfowl @LadyNaggaga

@bricelam
Copy link
Contributor

bricelam commented Jul 8, 2021

You could also avoid code inside Program.cs by just configuring it in the context:

class ApplicationDbContext : DbContext
{
    // ...

    protected override void OnConfiguring(DbContextOptionsBuilder options)
        // NB: This is a ✨magic✨ connection string that reads from IConfiguration
        => options.UseSqlite("Name=DefaultConnection");
}
builder.Services.AddDbContext<ApplicationDbContext>();

@davidfowl
Copy link
Member

davidfowl commented Jul 8, 2021

But connection strings........ OH WAIT that works.

@JeremyLikness
Copy link
Member

I propose we add higher-level extension methods to enable simpler setup of EF Core with a specific provider. In each provider package a new set of IServiceCollection extension methods would be declared in the

Potential benefits:

  • The methods elevate the the provider name, e.g. "Sqlite", to be the primary subject, rather than the DbContext. This can be useful in learning scenarios where the arc isn't ready to explain the concepts related to EF Core and its provider model (e.g. ORMs, providers, option delegates, etc.)
  • The methods remove the need for importing the Microsoft.EntityFrameworkCore namespace to get to the "can read/write to/from a database" stage

A few thoughts/considerations after discussing this with the team:

  • If the user has to create a DbContext won't this approach be confusing, i.e. if they just configure a SQLite provider, why did they have to also make a context and how are they related?
  • What about cases like Blazor Server apps that the AddDbContextFactory is needed instead?
  • Users already struggle with options; won't this hide them even more?
  • There is already a decision point that can add confusion around when to AddDbContext vs AddDbContextFactory, now we would have a third option: AddSqlite
  • This completely hides that EF is being configured. Is that the goal? If so, back to a previous point, how do we explain the DbContext? Does it make sense to name it AddEFSqlite or AddSqliteContext to make it more clear?

@DamianEdwards
Copy link
Member Author

  • Fair point on the need to create a DbContext but in working through the learning arc for this before, it typically follows that you create the model to represent your entity first (e.g. Todo) and build some API methods that take and return that type, before then looking at storage/persistence whereby the focus is "where are we going to store this" not "what framework are we going to use to manage storing this". At that point, adding the NuGet package for EFCore.Sqlite or SqlServer before creating the DbContext and adding properties for the entity type
  • The decision points and other cases raised are similar in notion to choosing between Host and the new WebApplication. Before that we had WebHost. WebApplication doesn't invalidate Host, it's simply more direct, opinionated, and reduces ceremony for the very common cases. Starting with WebApplication is simpler, and once understood it's easier to learn about the underlying infrastructure provided by IHost and the cases in which that might be needed. I think the same applies here.
  • I may not be following the point about options completely but isn't that a case of trying to ensure progressive disclosure such that options aren't required until they are?
  • I straight up admit I completely forgot which method to call when building an app the other day. I had created my DbContext and went looking for AddEntityFramework and AddSqlServer on IServiceCollection before admitting defeat and looking at the code in our workshop and then totally kicking myself for not being able to remember AddDbContext
  • @bricelam's suggestion is reasonable, although I'd point out that in the context of the kind of tutorial/scenarios we're talking about, concepts such as method overloading and access modifiers simply don't appear anywhere else. Requiring these in the code before the user can even store or retrieve something from the storage mechanism just adds to the cognitive load the learning arc is imposing.
  • Adding EF to the name of the methods is certainly worth discussing

For further context, the app being built in the kind of tutorial we're talking about would look like the following at the point it supports creating and retrieving todos (even the absence of using statements). As you can see, even with the proposed method added, the DbContext is arguably the thing in the file with the most concepts (inheritance, constructors, generics, overloads):

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddSqlite<TodoDb>("Data Source=todos.db");

var app = builder.Build();

app.MapGet("/", () => "Hello World");
app.MapGet("/todos", async (TodoDb db) => await db.Todos.ToListAsync());
app.MapPost("/todos", async (Todo todo, TodoDb db) =>
{
    db.Todos.Add(todo);
    await db.SaveChangesAsync();
    return CreatedAt($"/todos/{todo.Id}", todo);
});

app.Run();

class Todo
{
    public int Id { get; set; }
    public string? Title { get; set; }
    public bool IsComplete { get; set; }
}

class TodoDb : DbContext
{
    public TodoDb(DbContextOptions<TodoDb> options)
        : base(options) { }

    public DbSet<Todo> Todos => Set<Todo>();
}

@AndriySvyryd
Copy link
Member

using Microsoft.EntityFrameworkCore; is needed for deriving from DbContext anyway, so this proposal would just be saving 43 characters and increasing the number of APIs a new user would need to learn.

@davidfowl
Copy link
Member

I don't think it's about the number of characters, its the concept count and the happy path through the learning arc. You want to the code to look intuitive before you begin to learn more deeply about what it does.

@AndriySvyryd
Copy link
Member

The user still needs to learn about DbContext, so the concept count remains the same.
And using a method that allows additional options to be specified when necessary provides a smoother learning curve beyond the basic "Hello World"

@DamianEdwards
Copy link
Member Author

I agree being exposed to DbContext is unfortunate, especially given that brings constructors and type inheritance with it, along with options types, generics in parameters and properties, etc.

However one might imagine the class definition for the context can be smoothed over by simply instructing folks to paste that block at the end of their file where it can be discussed in more depth later.

Dealing with services however isn't so clean as it's located amongst the rest of the app code. So in that context, we're comparing these two blocks:

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddSqlite<TodoDb>("Data Source=todos.db");
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddDbContext<TodoDb>(options =>
    options.UseSqlite("Data Source=todos.db"));

I can see that it might seem like a trivial difference, but I'd argue the first is far simpler to explain and absorb, even to folks not familiar with C#. We call a generic method named for the place we want to store our data and pass a string, as opposed to calling a generic method named for the complex code we just pasted that we're not yet going to explain, then pass it a delegate as a lambda that will be called to configure the complex thing in which we call a method named for where we want to store our data and pass a string.

@bricelam
Copy link
Contributor

bricelam commented Jul 9, 2021

Time to start working on an active record library for .NET? :trollface:

@bricelam
Copy link
Contributor

bricelam commented Jul 9, 2021

I'd argue the first is far simpler to explain and absorb.

While I don’t hate the proposed sugar, I still fail to see any weight behind this statement.

You use lambdas to define HTTP requests, why is using them to configure services so hard to explain?

@davidfowl
Copy link
Member

We basically did the same with the new host. It's not that lambdas are themselves complex but all lambda usage isn't equivalent so I think it's not the right argument to make. It's more about the 80/20 rule, the main case is pick a provider for your DbContext and connection string. Turns out EF understood this and also supports the name=connection string name idea so this isn't too far off.

@bricelam
Copy link
Contributor

bricelam commented Jul 9, 2021

Something (possibly unrelated) that I keep thinking about: Generally, we try to have good, sensible defaults so you don't have to think about things unless you want to change them. In EF 4.1, we decided that SQL Server Express was a sensible default--you didn't even have to figure out a connection string! In EF Core, we decided that this was a terrible idea. MySQL, PostgreSQL, SQL Server, SQLite, (even DB2 and Oracle if you're into that sorta thing), are all very sensible defaults. We intentionally chose to make it a decision point for the user.

Is this just a case where we've been able to hide everything else in the default templates because there are sensible defaults, so we're just struggling with how to present the configuration?

What does the code look like if I want to configure Logging to log to a file? Shouldn't configuring the database provider use the same pattern for configuring a service?

@davidfowl
Copy link
Member

Here's what the Todo app looks like in preview7 https://github.com/davidfowl/CommunityStandUpMinimalAPI/tree/davidfowl/preview7

@DamianEdwards also has an app where he's added these extensions:
https://github.com/DamianEdwards/MinimalApiPlayground/blob/main/MinimalApiPlayground/Program.cs

What does the code look like if I want to configure Logging to log to a file? Shouldn't configuring the database provider use the same pattern for configuring a service?

I think this is a stretch. I don't expect we'll try to make everything simple. As we get more experience with what the "main" things people configure we'll make more changes. There's absolutely a cliff of complexity, but EF is in the mainline (for good reasons) and we want to make sure the experience is as smooth as possible on the learning arc.

@ajcvickers
Copy link
Member

Hey all. Great discussion! I basically agree with everything Brice and Andriy have said. However, I don't feel strongly that we shouldn't do this. In other words, I think it's slightly worse than what we currently have, but not massively worse. So if Damian and David feel this is overall better for the high-level experience, then I'm okay with this going in. However, we must do it for all providers that we own, not just SQLite, and we should notify other providers (Pomelo, Oracle/MySQL, IBM, PostgreSQL, Firebird) so that they are also consistent. (As Brice said, there is nothing special about SQLite other than it's currently the getting-started flavor of choice. That has changed in the past, and may change again in the future, especially depending what happens with SQL Server.)

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 11, 2021

Imagine if there was a developer Azure SQL SKU!

@davidfowl
Copy link
Member

@ajcvickers that's good to hear, the PR draft is here #25220. Take a look and let us know what you think.

@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 22, 2021
@AndriySvyryd AndriySvyryd added this to the 6.0.0 milestone Jul 22, 2021
JunTaoLuo pushed a commit that referenced this issue Jul 22, 2021
…ds (#25220)

* Add IServiceCollection.UseSqlite<DbContext> extension methods

Contributes to #25192

* Add IServiceCollection.UseSqlServer<DbContext> extension methods

Contributes to #25192

Co-authored-by: John Luo <johluo@microsoft.com>
Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
@JunTaoLuo
Copy link
Contributor

The PR has been merged. Do we need to keep this issue open to track notifying other providers?

@AndriySvyryd
Copy link
Member

I've already pinged Pomelo MySQL and Npgsql maintainers from the PR. Only Oracle/MySQL, IBM and Firebird left.

@JunTaoLuo
Copy link
Contributor

I'll reassign this to you to follow up with the providers then. Thanks @AndriySvyryd for helping me get the change in!

@roji
Copy link
Member

roji commented Jul 30, 2021

Npgsql issue: npgsql/efcore.pg#1934

@mguinness
Copy link

@lauxjpn Do we need to open an issue for Pomelo?

@lauxjpn
Copy link
Contributor

lauxjpn commented Oct 7, 2021

@mguinness Thanks for the ping. We could, but it's also alright if we don't. I have this on my internal list already after recently looking through the commits that @roji pushed to Npgsql and then remembered seeing the discussion between the ASP.NET and the EF Core guys about it somewhere (or maybe reading this issue or something; don't remember).
Anyway, I'll add it to our RC1 release.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dbcontext closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-enhancement
Projects
None yet
Development

No branches or pull requests