Skip to content

Fixed support for using multiple DbContexts #836

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

Merged
merged 2 commits into from
Sep 29, 2020
Merged

Fixed support for using multiple DbContexts #836

merged 2 commits into from
Sep 29, 2020

Conversation

bart-degreed
Copy link
Contributor

Fixes #739.

Added example project with tests, as this is tricky to get right (and not accidentally break in the future).

In the past, this was accomplished by calling ResourceGraphBuilder.AddDbContext() from services.AddJsonApi(). I've changed that to instead accept a collection of DbContext types in the non-generic overload of services.AddJsonApi(), so that we can also register matching resolvers.

This also fixes InverseNavigation to loop over all DbContexts instead of a single one.

And for this to work, I found another bug where ResourceService<TResource> was depending on injected IResourceRepository<TResource,TId> instead of IResourceRepository<TResource>, so it would pick the wrong repository.

Finally, I simplified caching of scanned assemblies that was spread over multiple layers doing duplicate work.

@bart-degreed bart-degreed requested a review from maurei September 22, 2020 16:37
Copy link
Member

@maurei maurei left a comment

Choose a reason for hiding this comment

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

I am aware this PR only fixes support for multiple DbContexts rather than introducing it as a new feature or improving it as a feature. But regardless, perhaps we can consider improving the follow.

I think it is unnecessarily verbose having to both pass along DbContextA, DbContextB to AddJsonApi() call and also having to register a IDbContextResolver implementation for each.

We could register a generic resolver service DbContextResolver<TDbContext> like this

public class DbContextResolver<TDbContext> : IDbContextResolver where TDbContext : DbContext
{
    private readonly DbContext _context;

    public DbContextAResolver(TDbContext context)
    {
        _context = context;
    }

    public DbContext GetContext()
    {
        return _context;
    }
}

Then a dev can just inject DbContextResolver<DbContextA> in his custom repository.

Alternatively, if we do not want to offer this solution out-of-the-box but want to leave the responsibility with the developer, I would propose to change the example using this rather approach than two separate DbContextResolver services. It is cleaner and as such easier to follow.

Other than this I have nothing to comment on the PR. Looking good 👍

@bart-degreed
Copy link
Contributor Author

I am aware this PR only fixes support for multiple DbContexts rather than introducing it as a new feature or improving it as a feature. But regardless, perhaps we can consider improving the follow.

Great suggestion! I've pushed another commit to address this.

@bart-degreed bart-degreed requested a review from maurei September 29, 2020 09:17
@maurei maurei merged commit 262d0f2 into json-api-dotnet:master Sep 29, 2020
@bart-degreed bart-degreed deleted the fix-multiple-dbcontexts branch September 29, 2020 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Injecting multiple DbContexts does not work
2 participants