Make service layer independent of persistence framework

[shuebner]

I am currently writing an IEntityRepository implementation for MongoDB, for which there is no EF support.
The code for Filter and Sort is the same and PageAsync only differs in its query realization in that instead of ToListAsync() it is ToAsyncEnumerable().ToList().

To avoid code duplication for those methods I suggest adding an abstract class that handles Filter, Sort and PageAsync for all DB drivers that can provide an IQueryable<>, with an abstract method for DB-specific query realization.
That abstract class would only depend on IJsonApiContext.

Alternatively the query realization could be in its own interface for DI.

What do you think?

[shuebner]

On second thought, an additional abstract class might not even be necessary.
Filter, Sort and Page could be extensions methods on IQueryable<>.

Sort

Sort could be an extension method since it has no dependencies.
Then the sorting would all be in the same place and not split up between IEntityRepository and IQueryableExtensions.

Page

Page could be an extension method as well, if it wasn't for the ordering by Id on negative pageNumber.
Why does Page with negative pageNumber order by Id again after query realization?
JSON API does not make any recommendations like this.
Without that behavior for negative pageNumbers the additional dependency on Models in IQueryableExtensions would not be necessary and Page could be an extension method no strings attached.

Filter

That leaves Filter, which could -- without any additional dependencies in IQueryableExtensions -- also be an extension method if RelatedAttrFilterQuery.Ctor and AttrFilterQuery.Ctor would take the ContextEntity and the IContextGraph instead of the whole IJsonApiContext.

Query Realization

I am not sure why Paging is permanently linked to query realization in IEntityRepository.
However, an IEntityRepository implementation could delegate paging to IQueryableExtensions and then DB-specifically realize the query. DefaultEntityRepository could even keep its behavior on negative pageNumber.

[jaredcnance]

I still need time to think on this a bit more. I'm not sure we need an additional layer. Maybe query realization should simply be the responsibility of the service layer?

Another thing I would like to see is the separation of different DB provider implementations into separate packages, e.g.:

• JsonApiDotNetCore.EntityFramework
• JsonApiDotNetCore.MongoDb
• JsonApiDotNetCore.Dapper

The purpose behind the in-memory sort on negative paging is so you can get the last page in the same order you would have received it with forward paging. Consider the following:

// page 1
0
1
// page 2
2
3

If I simply OrderByDescending(t => t.Id).Take(2), then I will get:

3
2

The in-memory sort lets me get back:

2
3

Although, if we made it a rule that query realization occurs outside the repository layer, then I would be fine pushing that out as well.

[shuebner]

I do not want to add an additional layer anymore, but suggested that Sort, Filter and Page be extension methods on IQueryable.
I agree on the separate packages for different DB providers.

I do not think the service layer should concern itself with query realization.
At the moment it is completely persistence-provider-agnostic, does not depend on IQueryable or anything else.
And that is its purpose: to be an access point for the domain / the read models.
I would not introduce a dependency on a persistence provider.

I know why the IEnumerable is sorted backwards by Id again, but I don't understand why the IQueryable is sorted by Id in the first place, ignoring the order that was already established.
That is not getting "the last page in the same order you would have received it with forward paging", because any previous order is overridden.
Additionally, sorting by Id is almost the same as no sorting at all, since the Ids are not guaranteed to be in any order, nor do they carry any meaning; even more so in the current age of UUIDs.
Could this be an artifact of the special case <TEntity, int> base classes and a hidden assumption that not only can Ids be ordered at all, but are sequentially assigned correlating to the row's creation time?

If there are no restrictions on TId, those assumptions should not be made.
I, for example, use the library exclusively with Guid as TId.

[jaredcnance]

Those are good points and really I suppose we should just be reversing the list.

Regarding extension methods, I'm fine moving the current logic into extension methods as long as we keep the method on the repository as a proxy to the extension method. The reason for this is we have users who need to supplement/alter the behavior the sort and filter methods and we need to ensure they can override the default behavior.

These are the following methods that we would need to pull out to allow custom query realization by itself. These could be exposed on the DefaultEntityRepository

protected int CountAsync(IQueryable<T> query);
protected TEntity FirstOrDefaultAsync(IQueryable<T> query);
protected List<TEntity> ToListAsync(IQueryable<T> query);

Your MongoDbRepository would then simply override those methods.

I agree the service layer interface needs to be persistence agnostic. However, there are currently some cases where the implementation is responsible for actual query realization:

JsonApiDotNetCore/src/JsonApiDotNetCore/Services/EntityResourceService.cs

Line 54 in eb2c309

_jsonApiContext.PageManager.TotalRecords = await entities.CountAsync();

JsonApiDotNetCore/src/JsonApiDotNetCore/Services/EntityResourceService.cs

Line 78 in eb2c309

return await query.FirstOrDefaultAsync(e => e.Id.Equals(id));

I'm not sure we need to remove all dependency on IQueryable from the service layer. But, I do think it is worth further consideration.

[shuebner]

I am all for leaving the methods on the repository and relaying them to the extension methods.

Yes, I meant the interface of the ResourceService, sorry for the ambiguity.
An implementation can do whatever, no one cares ;-).
You could even call the default implementation QueryableResourceService to convey that it can be used with any IQueryableRepository (which could be the name of the current IEntityRepository).

I would rather your proposed methods be part of IEntityRepository, since they are used by the resource service.
It doesn't help a non-EF implementation if the methods are part of DefaultEntityRepository, because a subclass still has to call the base constructor which expects a DbContext, which does not exist.
(I assume it is a typo that their results are not wrapped in Task<>. Also, I suggest using IOrderedEnumerable<> or IReadOnlyList<>).

[shuebner]

I added three query realization methods to IReadEntityRepository in my fork https://github.com/shuebner/JsonApiDotNetCore/tree/feat/%23211 and removed the dependency on EntityFramework from the EntityResourceService.

I prepared that as a baseline for the final decisions on this issue.
Please have a look at it and tell me what you think.

[jaredcnance]

Looks good to me. Feel free to open a PR back to master once you're satisfied and we can do a review there.

[jaredcnance]

@shuebner I've just published this in the 2.2.0-beta5 release. Would you be interested in open-sourcing the work you're doing on MongoDB? Something I've been wanting to do is separate the implementations into separate git repos and packages to make using different ORMs and stores easier. I can take on setting up the repos if you wouldn't mind contributing your work? I'm thinking in the 3.0.0 release I'd like to complete the separation into the following packages/repositories:

• JsonApiDotNetCore
• JsonApiDotNetCore.EntityFramework
• JsonApiDotNetCore.MongoDb
• JsonApiDotNetCore.Dapper (maybe)

Let me know if this is something you're interested in and we can chat elsewhere about the details. Thanks again for your work on this.