RFC: De-Couple data access from EF

[jaredcnance]

Problem

Currently, the controller leans on IEntityRepository directly which is tightly coupled to IQueryable. However, this doesn't make much sense for ORMs, like Dapper, that do not use IQueryable. Also, it is entirely possible that resources may be retrieved from non SQL storage solutions, such as another HTTP API or RPC.

Tightly Coupled Areas:

• JsonApiController
• ContextGraph
• ContextGraphBuilder
• IServiceCollectionExtensions
• Internal.Generics

Proposed Solution

1. Extract controller logic into IResourceService. Reduces controller methods to something like the following. Would allow any intermediate service to perform the data access.

public class JsonApiController<T, TId>
    : JsonApiControllerMixin where T : class, IIdentifiable<TId>
{
    private readonly IResourceService<T, TId> _service;

    public JsonApiController(IResourceService<T, TId> resourceService)
    {
        _service = resourceService;
    }

    [HttpGet]
    public virtual async Task<IActionResult> GetAsync()
    {
        var entities = await _service.GetAsync();
        return Ok(entities);
    }
}

2. Refactor the context graph:

• Rename ContextGraphBuilder to EntityContextGraphBuilder, since it uses the EF DbContext.
• Can ContextGraph be used without the DbContext generic? If not, perform rename Entity...
• Define API to either inject custom IContextGraph implementations or an API to build a generalized context graph

3. Refactor service extensions or add methods for using the new APIs defined by (2)

Considerations

• Need to take a closer look at json:api operations and how this would fit in.
  • Support for multiple operations in a single request json-api/json-api#795
• Since this may introduce a variety of breaking changes, perhaps we should consider further de-coupling from the MVC middleware in this solution as well (see discussion in Hard dependency to MVC setup #71)?

[donniefitz2]

Been getting familiar with the project and that's one of the first things that came to mind. Can I use this with a custom business/data layer? That would be really nice.

[jaredcnance]

@donniefitz2 currently, there are 2 approaches for implementing custom business logic:

• overriding controller methods
• injecting resource specific services that implement IEntityRepository
  In both cases, you would want to still call into the base implementations when possible.

However, this may be a little convoluted for non-EF DALs. The hope with the proposed changes is to lean on a IResourceService<T> for the first level of data access. This will let you create a custom implementation that might look like:

public class TodoItemService : IResourceService<TodoItem> {

  private readonly IMyTodoItemRepository _todoItemRepository;

  public TodoItemService(IMyTodoItemRepository todoItemRepository) {
    _todoItemRepository = todoItemRepository;
  }
  public async Task<List<TodoItem>> GetAsync() {
    return await _todoItemRepository.GetTodoItems();
  }

  public async Task<TodoItem> GetAsync(int id) {
    return await _todoItemRepository.GetTodoItemById(id);
  }

  // ...
}

The new default layer scheme would be:

controller -> IResourceService --> IEntityRepository

But you could, of course, change the dependency graph however you need to, beyond the IResourceService

controller -> IResourceService
controller -> IResourceService --> IMyEntityRepository
controller -> IResourceService --> IMyEntityDAL

// ...

[donniefitz2]

I like the idea of the resource interface. That keeps the controllers clean and allows the BL/DAL stuff to exist in a separate layer.

[JanMattner]

@jaredcnance I really like that idea! Actually I am currently exactly at a point where I want to expose data that is in the backend fetched from another REST service. IQueryable is here a real show stopper. That needs to be reworked first. I like the idea of the IResourceService.
After that, further decoupling of the ContextGraph and ContextGraphBuilder can be done. However, I hope that this can be done without breaking changes, so decoupling of MVC should be considered in a separate action.

Maybe I'll try a first draft in my example project next week.

[jaredcnance]

@JanMattner thanks for the feedback! It is greatly appreciated.

I'm not sure how to implement in a non-breaking way without adding significant surface area to the code, such as a new JsonApiServiceBasedController (or something) or adding a constructor overload to JsonApiController that takes the service and performs conditionals in every method. We could go that route first and deprecate the existing constructor overload, removing it in a later release? What do you think?

The problem with the ContextGraph is it looks at the DbContext to construct a map of all the entities. If an entity is not on the DbContext it won't have any knowledge of it. So, I believe, this must be addressed in the same PR. I think this can be addressed in a non-breaking way. The thought is to add an API to identify resources not included in a DbContext. Something like:

services.AddJsonApi<AppDbContext>(opt => {
  opt.BuildContextGraph(builder => {
     builder.WithResource<MyResource>("my-resources");
  });
});

[JanMattner]

Sounds good. In order to decouple it completely, we must get rid of the DbContext generic type. So rather something like:

services.AddJsonApi(opt => {
  opt.BuildContextGraph(builder => {
     builder.WithDbContext<AppDbContext>();
     builder.WithResource<MyResource>("my-resources");
  });
});

=> Everything inside the DbContext can be added and further more custom ressources not bound to any DbContext. For backwards compatibility, we can still leave the currently public methods as is and call this builder internally.

[jaredcnance]

Yeah, I like that and agree that we shouldn't require a DbContext