c# entity framework: correct use of DBContext class inside your repository class

I used to implement my repository classes as you can see below

public Class MyRepository
{
      private MyDbContext _context; 

      public MyRepository(MyDbContext context)
      {
          _context = context;
      }

      public Entity GetEntity(Guid id)
      {
          return _context.Entities.Find(id);
      }
}

However I recently read this article which says that's a bad practice to have data context as a private member in your repository: http://devproconnections.com/development/solving-net-scalability-problem

Now, theoretically the article is right: since DbContext implements IDisposable the most correct implementation would be the following.

public Class MyRepository
{
      public Entity  GetEntity(Guid id)
      {
          using (MyDbContext context = new MyDBContext())
          {
              return context.Entities.Find(id);
          }
      }
}

However, according to this other article disposing DbContext would be not essential: http://blog.jongallant.com/2012/10/do-i-have-to-call-dispose-on-dbcontext.html

Which of the two articles is right? I'm quite confused. Having DbContext as private member in your repository class can really cause "scalability problems" as the first article suggests?

I think you should not follow the first article, and I will tell you why.

Following the second approach you're loosing pretty much every feature that Entity Framework provides via the DbContext, including its 1st-level cache, its identity map, its unit-of-work, and its change tracking and lazy-loading abilities. That's because in the scenario above, a new DbContext instance is created for every database query and disposed immediately afterwards, hence preventing the DbContext instance from being able to track the state of your data objects across the entire business transaction.

Having a DbContext as a private property in your repository class also has its problems. I believe the better approach is having a CustomDbContextScope. This approach is very well explained by this guy: Mehdi El Gueddari

This article http://mehdi.me/ambient-dbcontext-in-ef6/ one of the best articles about EntityFramework I've seen. You should read it entirely, and I believe it will answer all your questions.

Let's assume, you have more than one repository and you need to update 2 records from different repositories. And you need to do it transactional (if one fails - both updates rollbacks):

var repositoryA = GetRepository<ClassA>();
var repositoryB = GetRepository<ClassB>();

repository.Update(entityA);
repository.Update(entityB);

So, if you have own DbContext for each repository (case 2), you need to use TransactionScope to achieve this.

Better way - have one shared DbContext for one operation (for one call, for one unit of work). So, DbContext can manage transaction. EF is quite for that. You can create only one DbContext, make all changes in many repositories, call SaveChanges once, dispose it after all operations and work is done.

Here is example of UnitOfWork pattern implementation.

Your second way can be good for read-only operations.

The root rule is : your DbContext lifetime should be limited to the transaction you are running.

Here, "transaction" may refer to a read-only query or a write query. And as you may already know, a transaction should be as short as possible.

That said, I would say that you should favor the "using" way in most cases and not use a private member.

The only one case I can see to use a private member is for a CQRS pattern (CQRS : A Cross Examination Of How It Works).

By the way, the Diego Vega response in the Jon Gallant's post also gives some wise advice :

> There are two main reasons our sample code tends to always use “using” > or dispose the context in some other way: > > 1. The default automatic open/close behavior is relatively easy to override: you can assume control of when the connection is opened and > closed by manually opening the connection. Once you start doing this > in some part of your code, then forgetting to dipose the context > becomes harmful, because you might be leaking open connections. > > 2. DbContext implements IDiposable following the recommended pattern, which includes exposing a virtual protected Dispose method that > derived types can override if for example the need to aggregate other > unmanaged resources into the lifetime of the context.

HTH

Which approach to use depends on the responsibility of the repository.

Is the responsibility of the repository to run full transactions? i.e. to make changes and then save changes to the database by calling `SaveChanges? Or is it only a part of a bigger transaction and therefore it will only make changes without saving them?

Case #1) The repository will run full transactions (it will make changes and save them):

In this case, the second approach is better (the approach of your second code sample).

I will only modify this approach slightly by introducing a factory like this:

public interface IFactory<T>
{
    T Create();
}

public class Repository : IRepository
{
    private IFactory<MyContext> m_Factory;

    public Repository(IFactory<MyContext> factory)
    {
        m_Factory = factory;
    }

    public void AddCustomer(Customer customer)
    {
        using (var context = m_Factory.Create())
        {
            context.Customers.Add(customer);
            context.SaveChanges();
        }            
    }
}

I am doing this slight change to enable Dependency Injection. This enables us to later change the way we create the context.

I don't want the repository to have the responsibility of creating the context it self. The factory which implements IFactory<MyContext> will have the responsibility of creating the context.

Notice how the repository is managing the lifetime of the context, it creates the context, does some changes, save the changes, and then disposes of the context. The repository has a longer lifetime than the context in this case.

Case #2) The repository is part of a bigger transaction (it will do some changes, other repositories will make other changes, and then someone else is going to commit the transaction by invoking SaveChanges):

In this case, the first approach (that you describe first in your question) is better.

Imagine this going on to understand how the repository can be a part of a bigger transaction:

using(MyContext context = new MyContext ())
{
    repository1 = new Repository1(context);
    repository1.DoSomething(); //Modify something without saving changes
    repository2 = new Repository2(context);
    repository2.DoSomething(); //Modify something without saving changes
    
    context.SaveChanges();
}

Please note that a new instance of the repository is used for each transaction. This means that the lifetime of the repository is very short.

Please note that I am new-ing up the repository in my code (which is a violation of dependency injection). I am just showing this as an example. In real code, we can use factories to solve this.

Now, one enhancement that we can do to this approach is to hide the context behind an interface so that the repository no longer has access to SaveChanges (take a look at the Interface Segregation Principle).

You can have something like this:

public interface IDatabaseContext
{
    IDbSet<Customer> Customers { get; }
}

public class MyContext : DbContext, IDatabaseContext
{
    public IDbSet<Customer> Customers { get; set; }
}


public class Repository : IRepository
{
    private IDatabaseContext m_Context;

    public Repository(IDatabaseContext context)
    {
        m_Context = context;
    }

    public void AddCustomer(Customer customer)
    {
        m_Context.Customers.Add(customer);      
    }
}

You can add other methods that you need to the interface if you want.

Please note also that this interface does not inherit from IDisposable. Which means that the Repository class is not responsible for managing the lifetime of the context. The context in this case has a larger lifetime than the repository. Someone else will be managing the lifetime of the context.

Notes on the first article:

The first article is suggesting that you shouldn't use the first approach you describe in your question (inject the context into the repository).

The article is not clear on how the repository is used. Is it used as a part of a single transaction? Or does it span multiple transactions?

I am guessing (I am not sure), that in the approach that the article is describing (negatively), the repository is used as a long-running service that will span a lot of transactions. In this case I agree with the article.

But what I am suggesting here is different, I am suggesting that this approach is only used in the case where the a new instance of the repository is created every time a transaction is needed.

Notes on the second article:

I think that what the second article is talking about is irrelevant to which approach you should use.

The second article is discussing whether it is necessary to dispose of the context in any case (irrelevant to the design of the repository).

Please note that in the two design approaches, we are disposing of the context. The only difference is who is responsible for such disposal.

The article is saying that the DbContext seem to clean up resources without the need of disposing the context explicitly.

The first article you linked forgot to precise one important thing: what is the lifetime of the so-called NonScalableUserRepostory instances (it also forgot to make NonScalableUserRepostory implements IDisposable too, in order to properly dispose the DbContext instance).

Imagine the following case:

public string SomeMethod()
{
	using (var myRepository = new NonScalableUserRepostory(someConfigInstance))
	{
		return myRepository.GetMyString();
	}
}

Well... there would still be some private DbContext field inside the NonScalableUserRepostory class, but the context will only be used once. So it's exactly the same as what the article describes as the best practice.

So the question is not "should I use a private member vs a using statement ?", it's more "what should be the lifetime of my context ?".

The answer would then be: try to shorten it as much as possible. There's the notion of Unit Of Work, which represents a business operation. Basically you should have a new DbContext for each unit of work.

How a unit of work is defined and how it's implemented will depend on the nature of your application ; for instance, for an ASP.Net MVC application, the lifetime of the DbContext is generally the lifetime of the HttpRequest, i.e. one new context is created each time the user generates a new web request.

---

EDIT :

To answer your comment:

One solution would be to inject via constructor a factory method. Here's some basic example:

public class MyService : IService
{
	private readonly IRepositoryFactory repositoryFactory;

	public MyService(IRepositoryFactory repositoryFactory)
	{
		this.repositoryFactory = repositoryFactory;
	}
	
	public void BusinessMethod()
	{
		using (var repo = this.repositoryFactory.Create())
		{
			// ...
		}
	}
}

public interface IRepositoryFactory
{
	IRepository Create();
}

public interface IRepository : IDisposable
{
    //methods
}

The 1st code doesn't have relation to the scability problem, the reason it is bad is because he create new context for every repository which is bad, which one of commenters comment but he failed to even reply. In web it was 1 request 1 dbContext, if you plan to use repository pattern then it translate into 1 request > many repository > 1 dbContext. this easy to achieve with IoC, but not necessary. this is how you do it without IoC:

var dbContext = new DBContext(); 
var repository = new UserRepository(dbContext); 
var repository2 = new ProductRepository(dbContext);
// do something with repo

As for disposing or not, I usually dispose it but if the lead itself said this then probably no reason to do it. I just like to dispose if it has IDisposable.

Basically DbContext class is nothing but a wrapper which handles all the database related stuffs like:

1. Create connection
2. Execute Query. Now if we do the above mentioned stuff using normal ado.net then we need to explicitly close the connection properly either by writing the code in using statement or by calling the close() method on connection class object.

Now, as the context class implements the IDisposable inteface internally, it is good practise to write the dbcontext in using statement so that we need not care about closing the connection.

Thanks.

I use the first way (injecting the dbContext) Of course it should be an IMyDbContext and your dependency injection engine is managing the lifecycle of the context so it is only live whilst it is required.

This lets you mock out the context for testing, the second way makes it impossible to examine the entities without a database for the context to use.

Second approach (using) is better as it surely holds the connection only for the minimum needed time and is easier to make thread-safe.

I think that fist approach is better, you never have to create a dbcontext for each repository, even if you dispose it. However in the first case you can use a databaseFactory to instantiate only one dbcontext:

 public class DatabaseFactory : Disposable, IDatabaseFactory {
    private XXDbContext dataContext;

    public ISefeViewerDbContext Get() {
        return dataContext ?? (dataContext = new XXDbContext());
    }

    protected override void DisposeCore() {
        if (dataContext != null) {
            dataContext.Dispose();
        }
    }
}

And use this instance in Repository:

public class Repository<TEntity> : IRepository<TEntity> where TEntity : class
{
    private IXXDbContext dataContext;

    private readonly DbSet<TEntity> dbset;

    public Repository(IDatabaseFactory databaseFactory) {
        if (databaseFactory == null) {
            throw new ArgumentNullException("databaseFactory", "argument is null");
        }
        DatabaseFactory = databaseFactory;
        dbset = DataContext.Set<TEntity>();
    }

    public ISefeViewerDbContext DataContext {
        get { return (dataContext ?? (dataContext = DatabaseFactory.Get()));
    }
    
    public virtual TEntity GetById(Guid id){
        return dbset.Find(id);
    }
....