Small peculiarities

[shuebner]

Disclaimer: I kind of feel bad for opening so many issues. I don't want to be annoying. My sole intention is to help improve this library because the "better" (whatever that means) this library is, the more it will help me with my project. I looked at other JSON API libraries and yours was by far the one appealing to me the most. Thus I am really getting into your library (great job btw!) and notice things of which I don't know if they are intentional, oversights or don't really matter.

IEntityRepository.UpdateAsync

• is the entity parameter really necessary? All the attributes to be updated are in the IJsonApiContext anyway and DefaultEntityRepository does not use the parameter. Is there a use case for other IEntityRepository implementations that use this? Same question up the call chain for IEntityResourceService.UpdateAsync and maybe eventually BaseJsonApiController.PathAsync. Although I can imagine that having the entity in the controller might be useful.

ContextGraph

• Entities and UsesDbContext are only set on initialization and could be set in a constructor instead of having public properties; only bool UsesDbContext {get;} is needed; this would convey the immutability of the two members and not expose more than is necessary

EntityResourceService

• ApplyPageQueryAsync declares unused local variable query; the declaration seems not to have any side effects either

IQueryableExtensions

• Last two comments in IQueryableExtensions.GetFilterExpressionLambda are wrong (copy-paste)
• System.Reflection is declared but not used; this doesn't really matter but maybe gives the wrong impression of what the extensions do

[jaredcnance]

No worries! Your issues are detailed and I appreciate the effort you're putting in. No complaints from me!

• IEntityRepository.UpdateAsync

This is an area where I think we can definitely improve upon. We also currently do a fetch prior to performing an update, which I don't think should be necessary (although it's not that simple b/c there are some scenarios we need to support - specifically authorization). One issue with the current implementation is the requirement to use nullable types to determine whether or not the attribute/relationship was actually included in the request. This creates a problem when we want to support relationship deletion since we cannot determine whether or not the relationship was included or set to null. This topic probably deserves its own issue for further discussion.

But, back to your point, I agree that we shouldn't need the entity parameter. Unfortunately, this requires a breaking change. For that reason, I want to fully evaluate the things I mentioned above before going down that road so we can break once.

Regarding the remaining items, my initial reaction is that these are all reasonable suggestions. I'll need more time to review them, but I don't see why we couldn't include them in the next release.

[shuebner]

I found something else:
EntityResourceService.ApplyPageQueryAsync does not realize the query if there is no paging.
The signature implies it does, though.

So, shouldn't it be return await entities.ToListAsync() (as long as we are EF-dependent here)?

[shuebner]

And another thing:
in EntityResourceService.GetAsync the statement

_jsonApiContext.QuerySet?.IncludedRelationships != null

is true if IncludedRelationships is empty.
If QuerySet is not null QuerySet?.IncludedRelationships will never be null, since IncludedRelationships is initialized as empty list and only set with QueryParser.ParseIncludedRelationships, which always returns a list.
As a result, even when no relationships are included, GetWithRelationshipsAsync is called instead of GetAsync.

I think the statement should rather be

_jsonApiContext.QuerySet?.IncludedRelationships.Any() ?? false

[jaredcnance]

@shuebner regarding ApplyPageQueryAsync, I'd like to address that in the PR for #211 since it touches on query execution.

Regarding the last point, I'm not sure I follow. This is the current state on master:

JsonApiDotNetCore/src/JsonApiDotNetCore/Services/EntityResourceService.cs

Line 50 in 12eafc8

if (_jsonApiContext.QuerySet?.IncludedRelationships != null && _jsonApiContext.QuerySet.IncludedRelationships.Count > 0)

This already requires the list to be non-empty.

Disregard, wrong GetAsync method. I see the issue you're referring to now

[shuebner]

Ok, I'll include the change to ApplyPageQueryAsync in my PR for #211.
I have it locally already anyway.