Fix #4010: update repositories to using db context factory.

[zyhfish]

No description provided.

[sbwalker]

@zyhfish this PR does not follow the pattern recommended by Microsoft for Blazor (https://learn.microsoft.com/en-us/aspnet/core/blazor/blazor-ef-core?view=aspnetcore-8.0#new-dbcontext-instances). The recommended pattern is to use "short lived" DbContexts - which means that the DbContentFactory should be created within each method where it will be used. The pattern in this PR creates the DbContentFactory in the constructor - which means that it is not "short-lived". I merged the earlier PR for PageModuleRepository because it used the "short-lived" approach - not creation via constructor. Also note that in the example code from Microsoft it is possible to call:

using var context = DbFactory.CreateDbContext();

at the top of the method, without having to wrap the method logic in enclosing { ... }. This was the pattern I used in SiteRepository and HtmlTextRepository and it would be good if we followed a consistent pattern in all Repositories.

[zyhfish]

Hi @sbwalker , the exception is for the GetList method, as the method will return lazy load collection, if the db context is disposed in the method, then query the data from the collection will throw out exception.

if we don't want to add this exception, then we need to load the collection into memory every time calling to the GetList method, this will cause performance issue.

the using pattern is followed in all other methods.

[sbwalker]

@zyhfish I understand what you are saying... but doesn't it sort of defeat the purpose of using DbContextFactory ie. the "short-lived" approach. Essentially it means that the DbContext is still long-lived for these Get methods - which means they behave exactly the same as the standard DbContext which we were trying to replace (because they cause issues with Blazor's process model)? I am curious what Microsoft recommends in this scenario.

[sbwalker]

@zyhfish Microsoft announced some new CRUD templates for Blazor last week for scaffolding code... and they did not use DbContextFactory so someone logged an issue:

dotnet/aspnetcore#54539

I also noticed in the aspnetcore repo that the Identity classes do not use DbContextFactory.

dotnet/aspnetcore#42260

It seems Microsoft does not even follow its own advice :(

[zyhfish]

Hi @sbwalker , IMO we have to keep this exception for the GetList methods. but maybe we shouldn't use DbFactory.CreateDbContext to create the context but use DI to inject the exception context instance, so that it can be managed by the DI and will be disposed when the repository class is disposed.

[sbwalker]

@zyhfish Oqtane is not relying on lazy loading in its repositories. Generally, a Controller GET method will call a Repository Get method and will expect to eager load the data so that it can return it from the API. When a Repository method needs to reference other entities it uses .Include() within the repository classes to eager load the related data. Can you please share an example where a Repository is relying on lazy loading and will throw an exception by using DbContextFactory?

[zyhfish]

Hi @sbwalker , you can reproduce the issue with below steps:

1. based on this PR, please make below changes in Oqtane.Server/Repository/SiteRepository.cs line 110 to follow the using pattern:

using var ctx = _factory.CreateDbContext();
return ctx.Site.OrderBy(item => item.Name);

2. perform a new installation with the code. you will see "disposed instance exception" during the install process.

[sbwalker]

Thank you for the example. I assume this could be resolved by adding a .ToList() (ie. return ctx.Site.OrderBy(item => item.Name).ToList() ) which essentially would change it to eager loading. And this would probably have no performance impact because the caller is going to trigger an eager load and iterate the full collection anyways.

[zyhfish]

Hi @sbwalker , yes, use ToList will resolve the issue as I already tried it, but one problem is that the UserRepository will return all users data in the List method:
return _db.User;

I'm sure this will cause performance issue when the user list becomes large.

[sbwalker]

I am researching this further because Get methods are generally the most common methods called in a repository - they are often called 98% of the time with Add/Update/Delete being called 2% of the time. So based on this perspective it is far more important to resolve the DbContext issues for Get methods than it is for Add/Update/Delete methods. By continuing to use the problematic DbContext approach for GetList methods, it means that the framework will continue to experience these strange EF Core exceptions due to Blazor's process model. So this does not seem like the optimal solution. It would be best to use DbContextFactory for ALL methods - especially the Get methods - to ensure this problem is fully resolved. If this means that we need to evaluate/change the GetList methods to avoid exceptions, then we should do that.

[sbwalker]

Although GetUsers() is implemented in the UserRepository there is nothing which calls it - so it could be removed.

[zyhfish]

Thanks @sbwalker , I have updated the code to remove the exception context, we can check the performance issue later when it's happened.

[sbwalker]

When following an eager loading approach, we just need to ensure that filtering, sorting, and includes are implemented within the Repository layer to ensure the volume of data returned to callers is optimized.

[sbwalker]

@zyhfish when the topic of DbContextFactory first came up it was because the Html/Text module was throwing exceptions when using Static Server-Side rendering - correct? And transitioning to DbContextFactory resolved the issue. I just want to confirm that this is a problem in Static Server-Side rendering.

[zyhfish]

Hi @sbwalker , yes I think so, thx.