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 two methods for consuming repositories in scenarios where repositories could be longer lived (e.g. Blazor component Injections) #289

Merged
merged 4 commits into from
Apr 11, 2023

Conversation

jasonsummers
Copy link
Contributor

@jasonsummers jasonsummers commented Aug 20, 2022

Provides two solutions for #280

  • BREAKING CHANGE - requires support for netstandard2.0 to be dropped from Ardalis.Specification.EntityFrameworkCore.csproj in order to make use of IDbContextFactory
  • Add IRepositoryFactory interface and EFRepositoryFactory concrete implementation to encapsulate the 'Unit of Work' principle at the repository level, consuming DbContextFactories from DI containers such as those added using the .AddDbContextFactory method, following blazor best practices for managing DbContext lifetimes
  • Add ContextFactoryRepositoryBaseOfT.cs abstract implementation of IRepositoryBase which again consumes DbContextFactories from DI containers such as those added using the .AddDbContextFactory method but creates a new instance of the DbContext for every method call in the repository. This breaks Entity Framework change tracking so Update and Delete methods will have to be overloaded in concrete implementations using the TrackGraph method on the context.

…ories could be longer lived (e.g. Blazor component Injections)

- BREAKING CHANGE - requires support for netstandard2.0 to be dropped from Ardalis.Specification.EntityFrameworkCore.csproj in order to make use of IDbContextFactory
- Add IRepositoryFactory interface and EFRepositoryFactory concrete implementation to encapsulate the 'Unit of Work' principle at the repository level, consuming DbContextFactories from DI containers such as those added using the .AddDbContextFactory method, following blazor best practices for managing DbContext lifetimes
- Add ContextFactoryRepositoryBaseOfT.cs abstract implementation of IRepositoryBase<T> which again consumes DbContextFactories from DI containers such as those added using the .AddDbContextFactory method but creates a new instance of the DbContext for every method call in the repository. This breaks Entity Framework change tracking so Update and Delete methods will have to be overloaded in concrete implementations using the TrackChanges method on the context.
/// <inheritdoc/>
public async Task<TResult?> FirstOrDefaultAsync<TResult>(ISpecification<TEntity, TResult> specification, CancellationToken cancellationToken = default)
{
await using var dbContext = this.dbContextFactory.CreateDbContext();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is using using won't it make change tracking and later updates of the fetched entity impossible? The dbContext that originally is tracking the entity will be disposed by the end of this method, and so when a savechanges is called there will be an error saying "entity is already tracked by another dbcontext" or something equivalent, right? If not, can you write another few tests demonstrating that your solution works for fetch-change-save operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies Steve, I didn't give that fact much attention in my initial request.

Yes, I would expect such an error with this approach. I'll add a virtual protected method which invokes the TrackGraph method inside the using to solve this. The original intention was to allow for maximum flexibility but on reflection a default approach to managing the change tracker is definitely appropriate.

I just need to work through getting the error we're expecting to present itself in tests to ensure it's being handled appropriately.

I'll post another update here when the above is completed.

Copy link

@mwasson74 mwasson74 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a virtual protected method which invokes the TrackGraph method inside the using to solve this.

@jasonsummers, was this done? If so, where? Either way, can you explain this for me please?

Thanks,

Matt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwasson74, this wasn't completed in the end, mainly because the only solutions I could come up with added too much opinion into the library.

Essentially, due to the prolonged lifecycle of Blazor (and WPF/UWP/MAUI etc) apps, the new ContextFactoryRepositoryBaseOfT class instantiates a new instance of the DBContext every time a method is invoked. This means that all of the Entity Framework Change Tracking goodness is lost.

From a DDD perspective, this actually makes sense, whether or not an Entity has changed is a subject for the Entity to manage itself and not be reliant on a 3rd party to deduce.

What this means is that you need to implement change tracking manually within your solution. I think the only method you'll have to overload is the UpdateAsync method since that's the only one which really needs to know what's changed.

Sorry this is so vague, it's been a while since I've looked at this. Reply back here if you need more info. I'll try and carve out some time to write a proper doc for this as well.

@ardalis
Copy link
Owner

ardalis commented Oct 5, 2022

Let me know when you want me to review again @jasonsummers . Are you thinking it's good to go now?

@jasonsummers
Copy link
Contributor Author

Hi @ardalis, I think this is good to go. Unit tests are have been added to cover all of the permutations I could think of which would cause the entity is already tracked by another dbcontext error.

@fiseni
Copy link
Collaborator

fiseni commented Apr 11, 2023

Hi,

We've been holding back this PR for a while. It has to do with the breaking change, we have to drop the support for netstandard2.0. If we do that, then EF Core 3 users won't be able to consume the library anymore. We have been using multi-targeting to offer support for the following consumers:

  • EF Core 3 - they will consume the netstandard2.0 TFM
  • EF Core 5 - they will consume the netstandard2.1 TFM
  • EF Core 6 (and above) - they will consume the net6.0 TFM

@ardalis what are your thoughts here? Should we drop everything prior EF Core 6?

@ardalis
Copy link
Owner

ardalis commented Apr 11, 2023

I think we should support Blazor Server (and its wonky lifetime requirements). If that means we need to drop forward compatibility with netstandard 2.0 I think that's probably fine. We can document which version is the latest one to support that TFM in the README.

What do you think about our version relative to .NET's version, @fiseni ?

On the one hand I kind of like(d) the idea of tracking with .NET, so when .NET 8 ships we'd aim to have a Specification 8.0 to go with it. But on the other hand we currently have folks who are confused about which version of EF Core our library supports because of the version number differences. Maybe it would be better if our version number were, say, 17 or something so it was clear that it wasn't meant to be anywhere near .NET's or EF's version.

I mention the version number only because this breaking change would require us to increment our majorversion, and if we do it soon-ish, it will obviously be well before .NET 8 ships in Nov 2023.

@fiseni
Copy link
Collaborator

fiseni commented Apr 11, 2023

Yes, let's do that. The 3.1 and 5 are EOL anyway, I see no reason why we should keep supporting them.

As for versioning, as long as there are no features that we want to utilize, we should not force newer dependency versions. We should depend on the minimum viable version, EF Core 6 as of now. Also, we should not necessarily be coupled to dotnet and EF releases. That said, the idea of just assigning some arbitrary version number (e.g. 17 as you said) would prevent any association with EF versions. I like that. Then, the minimum required dependency versions are clearly listed.
image

Conclusion: Let's merge this PR. Then, I'll also drop the netstandard2.1. We'll be targeting a single version net6.0.

@ardalis ardalis merged commit da63643 into ardalis:main Apr 11, 2023
@wrakocy
Copy link

wrakocy commented Apr 28, 2023

Hi @fiseni. Wondering, can you provide a general time line for releasing these changes? Thanks so much!

@fiseni
Copy link
Collaborator

fiseni commented May 2, 2023

Yes, in the next two weeks, around 15th May.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants