Closed ThomasBarnekow closed 3 years ago
Hi @ThomasBarnekow, thanks for your elaborate description. I've never tried combining an exposed relationship with [EagerLoad]
(it wasn't designed for that), so I'm not totally surprised it causes problems. EagerLoads is an EF Core-only feature. They are applied at a very late stage in the pipeline, when LINQ queries are composed (long after processing relationships and query string parameters has occurred).
I haven't debugged why exactly sorting breaks, but I advise to remove the [EagerLoad]
annotations on exposed relationships and instead implement JsonApiResourceDefinition<T>.OnApplyIncludes
(docs here) to add the relationships you want to load unconditionally to the set of includes that comes from query string.
Example:
public sealed class EngagementResourceDefinition : JsonApiResourceDefinition<Engagement, Guid>
{
public EngagementResourceDefinition(IResourceGraph resourceGraph)
: base(resourceGraph)
{
}
public override IReadOnlyCollection<IncludeElementExpression> OnApplyIncludes(IReadOnlyCollection<IncludeElementExpression> existingIncludes)
{
ResourceContext engagementContext = ResourceGraph.GetResourceContext<Engagement>();
RelationshipAttribute partiesRelationship = engagementContext.Relationships.Single(relationship => relationship.Property.Name == nameof(Engagement.Parties));
HashSet<IncludeElementExpression> newIncludes = existingIncludes.ToHashSet();
newIncludes.Add(new IncludeElementExpression(partiesRelationship));
return newIncludes;
}
}
Hi @bart-degreed, thanks for coming back so quickly.
I've tried your suggestion, testing it in the browser, but it does not work quite as expected (although I don't know what exactly to expect).
When visiting http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45
, for example, this does not produce a response that visibly includes the parties
:
{
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45"
},
"data": {
"type": "engagements",
"id": "2a65799a-12f6-4392-e88d-08d8f511af45",
"attributes": {
"name": "My Test Engagement"
},
"relationships": {
"documentTypes": {
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/relationships/documentTypes",
"related": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/documentTypes"
}
},
"parties": {
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/relationships/parties",
"related": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/parties"
}
},
"firstParties": {
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/relationships/firstParties",
"related": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/firstParties"
}
},
"secondParties": {
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/relationships/secondParties",
"related": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/secondParties"
}
}
},
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45"
}
}
}
You'd have to use http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45?include=parties
for the parties
relationship to look like this:
"parties": {
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/relationships/parties",
"related": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/parties"
},
"data": [
{
"type": "engagementParties",
"id": "9de002a8-daca-4816-81b7-129964a071bc"
},
{
"type": "engagementParties",
"id": "ba0d144b-8d04-4d22-9272-9f6c8ed68f0d"
},
{
"type": "engagementParties",
"id": "0eb00a88-1fd4-4eb0-8a8b-4cd92fd78dfa"
},
{
"type": "engagementParties",
"id": "2933f9de-a171-4146-bca9-8d8f90516412"
}
]
}
What form of output would you expect in this case?
Anyhow, other than with EagerLoad
, visiting http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/firstParties
, for example, leads to this (no first parties, while there is exactly one):
{
"links": {
"self": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/firstParties",
"first": "http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/firstParties"
},
"data": []
}
With EagerLoad
the first party would appear in the list.
The difference made by the EngagementResourceDefinition
is that it is sufficient to append ?include=firstParties
rather than ?include=parties,firstParties
to the URL to include firstParties
.
So, unfortunately, this has not yet solved my issue. Would you have expected a different behavior? Am I trying something that should not be done?
At the moment, what I would do is to simply filter the resources by using filter=equals(role,'FirstParty')
, for example.
You're right, my suggestion does not work. It should return an included
array. This is a bug (tracked at #989). Unfortunately it is not easy to fix.
Let's rewind a bit to ensure we're looking at the same. I've created a PR to repro your original scenario (without OnApplyIncludes
), which contains a fix for the sorting issue when combining [EagerLoad]
with an exposed relationship. But I'm getting an error from EF Core because of the unmapped navigation properties (see the skipped tests). Am I missing something here?
I'm not saying that the combination of [EagerLoad]
with a relationship is not possible, its just untested. So additional problems may surface.
While I am always using explicit foreign keys such as EngagementId
(which you omitted), I'd say the models look OK. However, I am using the FluentAPI to configure the model in protected override void OnModelCreating(ModelBuilder modelBuilder)
. In the simplified model, that might not be required at all, but here's the code for the simplified Engagement
and EngagementParty
entities as a checklist. You could also simplify your tests by ignoring the DocumentType
entity.
modelBuilder.Entity<Engagement>(engagement =>
{
engagement.HasKey(t => t.Id);
engagement.Property(t => t.Id).ValueGeneratedOnAdd();
});
modelBuilder.Entity<EngagementParty>(party =>
{
party.HasKey(e => e.Id);
party.Property(e => e.Id).ValueGeneratedOnAdd();
party.HasOne(ep => ep.Engagement)
.WithMany(e => e.Parties)
.HasForeignKey(ep => ep.EngagementId)
.IsRequired()
.OnDelete(DeleteBehavior.Restrict);
});
I would have said "I don't get that error", but I now remember that I did in fact get it, too, until I did what was indicated by the error message. I made EF core ignore that issue because it is none. Here's how I configured my DB Context based on the hint I got:
services.AddDbContext<MyDbContext>(builder =>
{
builder.UseSqlServer(Configuration["ConnectionStrings:SqlServerConnection"]);
builder.ConfigureWarnings(wcb => wcb.Ignore(CoreEventId.InvalidIncludePathError));
});
For simplicity I try not to write unneeded code. Except for .IsRequired().OnDelete(DeleteBehavior.Restrict)
, this all matches the defaults and produces the exact same schema. I wasn't aware the error could be suppressed. Updated PR accordingly, which makes the failing tests succeed.
In my data model, I needed enough of those definitions so that I decided I'd rather produce a complete and explicit specification of the model than a partial one that relied on some EF Core magic while specifying some aspects explicitly. The portion that is not strictly necessary is small enough so that the benefit of having everything explicit and in one place outweighs the disadvantage of writing "unneded code".
DESCRIPTION
Before describing my issue, let me first thank you for this project. It is absolutely awesome.
On to my issue. I have a resource called
Engagement
with severalHasMany
relationships. Three of those are related in the following way: The first one,Parties
, is the navigation property in an EF core relationship betweenEngagement
andEngagementParty
(where one engagement can have many engagement parties). The other two,FirstParties
andSecondParties
, areNotMapped
from an EF Core perspective and derived fromParties
by selecting a subset of the elements contained inParties
. Based on your documentation, I've added theEagerLoad
attribute to theParties
relationship. This also works, meaning that I can GET theFirstParties
andSecondParties
. However, what does not work is sorting theEngagementParty
resources by any of their attributes (e.g.,ShortName
). For example:produces the same order as
which produces the same order as
When I remove the
EagerLoad
attribute from theParties
relationship, the resources can be sorted as expected.The same restriction/issue applies to sort orders applied by resource definitions (in the
OnApplySort()
method). WithEagerLoad
, the sort order has no effect. WithoutEagerLoad
the sort order is applied correctly.STEPS TO REPRODUCE
Here are the
Engagement
andEngagementParty
entities. Note theEagerLoad
attribute on theParties
property.Here is the resource definition that establishes a default sort order:
It is registered in the
Startup
class as follows:With the
EagerLoad
attribute, sorting does not work. When removing theEagerLoad
attribute, sorting works as expected.EXPECTED BEHAVIOR
Sorting works with the
EagerLoad
attribute applied to theParties
relationship property.ACTUAL BEHAVIOR
Sorting does not work with the
EagerLoad
attribute applied to theParties
relationship property.VERSIONS USED
4.1.1
5.0
5.0
MSSQLLocalDB
(SQL Server 13.0.4001
)