Cannot sort resource having a relationship with EagerLoadAttribute

[ThomasBarnekow]

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 several HasMany relationships. Three of those are related in the following way: The first one, Parties, is the navigation property in an EF core relationship between Engagement and EngagementParty (where one engagement can have many engagement parties). The other two, FirstParties and SecondParties, are NotMapped from an EF Core perspective and derived from Parties by selecting a subset of the elements contained in Parties. Based on your documentation, I've added the EagerLoad attribute to the Parties relationship. This also works, meaning that I can GET the FirstParties and SecondParties. However, what does not work is sorting the EngagementParty resources by any of their attributes (e.g., ShortName). For example:

GET http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/parties

produces the same order as

GET http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/parties?sort=shortName

which produces the same order as

GET http://localhost:5000/engagements/2a65799a-12f6-4392-e88d-08d8f511af45/parties?sort=-shortName

When I remove the EagerLoad attribute from the Parties relationship, the resources can be sorted as expected.

The same restriction/issue applies to sort orders applied by resource definitions (in the OnApplySort() method). With EagerLoad, the sort order has no effect. Without EagerLoadthe sort order is applied correctly.

STEPS TO REPRODUCE

Here are the Engagement and EngagementParty entities. Note the EagerLoad attribute on the Parties property.

    public class Engagement : EntityBase<Guid>
    {
        // Data

        [Attr]
        public string Name { get; set; }

        // Navigation Properties

        [HasMany]
        public ICollection<DocumentType> DocumentTypes { get; set; } = new List<DocumentType>();

        [HasMany]
        [EagerLoad]
        public ICollection<EngagementParty> Parties { get; set; } = new List<EngagementParty>();

        [HasMany]
        [NotMapped]
        public ICollection<EngagementParty> FirstParties =>
            Parties.Where(p => p.Role == ModelConstants.FirstParty).OrderBy(p => p.ShortName).ToList();

        [HasMany]
        [NotMapped]
        public ICollection<EngagementParty> SecondParties =>
            Parties.Where(p => p.Role == ModelConstants.SecondParty).OrderBy(p => p.ShortName).ToList();
    }

    public class EngagementParty : EntityBase<Guid>
    {
        // Foreign Keys (simplified)

        public Guid EngagementId { get; set; }

        [HasOne]
        public Engagement Engagement { get; set; }

        // Data (simplified)

        [Attr]
        public string Role { get; set; }

        [Attr]
        public string ShortName { get; set; }
    }

    public abstract class EntityBase<TId> : Identifiable<TId>
    {
        public DateTimeOffset? DateCreated { get; set; }

        public DateTimeOffset? DateModified { get; set; }

        [Timestamp]
        public byte[] RowVersion { get; set; }
    }

Here is the resource definition that establishes a default sort order:

    public class EngagementPartyResourceDefinition : JsonApiResourceDefinition<EngagementParty, Guid>
    {
        /// <inheritdoc />
        public EngagementPartyResourceDefinition(IResourceGraph resourceGraph) : base(resourceGraph)
        {
        }

        /// <inheritdoc />
        public override SortExpression OnApplySort(SortExpression? existingSort)
        {
            if (existingSort != null)
            {
                return existingSort;
            }

            return CreateSortExpressionFromLambda(new PropertySortOrder
            {
                (ep => ep.Role, ListSortDirection.Ascending),
                (ep => ep.ShortName, ListSortDirection.Ascending)
            });
        }
    }

It is registered in the Startup class as follows:

    services.AddScoped<IResourceDefinition<EngagementParty, Guid>, EngagementPartyResourceDefinition>();

With the EagerLoad attribute, sorting does not work. When removing the EagerLoad attribute, sorting works as expected.

EXPECTED BEHAVIOR

Sorting works with the EagerLoad attribute applied to the Parties relationship property.

ACTUAL BEHAVIOR

Sorting does not work with the EagerLoad attribute applied to the Parties relationship property.

VERSIONS USED

• JsonApiDotNetCore version: 4.1.1
• ASP.NET Core version: 5.0
• Entity Framework Core version: 5.0
• Database provider: MSSQLLocalDB (SQL Server 13.0.4001)

[bart-degreed]

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;
    }
}

[ThomasBarnekow]

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.

[bart-degreed]

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.

[ThomasBarnekow]

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));
            });

[bart-degreed]

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.

[ThomasBarnekow]

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".