Enhance support for deleting resource with incoming optional 1-to-1 relationship when using DeleteBehavior.ClientSetNull

[bart-degreed]

DESCRIPTION

When trying to delete a resource that has an incoming optional one-to-one relationship, the operation may fail using EF Core default mapping conventions when such a related record exists.

This is because optional relationships in EF Core have the delete behavior ClientSetNull by default, instead of SetNull. That means that instead of delegating the side effects to the database, EF Core tries to handle it by sending multiple SQL statements. That only works when all related resources of all incoming relationships are loaded in the change tracker upfront (which is expensive and not handled very well by JADNC).

The reason for this odd default is poor support in SQL Server, as explained here and here.

As demonstrated in #1205, these restrictions do not apply when using PostgreSQL. Therefore the general advice is to map optional one-to-one relationships with .OnDelete(DeleteBehavior.SetNull). This is simpler and more efficient.

STEPS TO REPRODUCE

Consider the next model:

#nullable enable

public sealed class Order : Identifiable<long>
{
    [Attr]
    public decimal Amount { get; set; }

    [HasOne]
    public Customer Customer { get; set; } = null!;

    [HasOne]
    public Order? Parent { get; set; }
}

public sealed class Customer : Identifiable<long>
{
    [Attr]
    public string Name { get; set; } = null!;

    [HasOne]
    public Order? FirstOrder { get; set; }

    [HasOne]
    public Order? LastOrder { get; set; }

    [HasMany]
    public ISet<Order> Orders { get; set; } = new HashSet<Order>();
}

public sealed class ShoppingBasket : Identifiable<long>
{
    [Attr]
    public int ProductCount { get; set; }

    [HasOne]
    public Order? CurrentOrder { get; set; }
}

This results in the next SQL to create the database:

CREATE TABLE "Customers" (
  "Id" bigint GENERATED BY DEFAULT AS IDENTITY,
  "Name" text NOT NULL,
  "FirstOrderId" bigint NULL,
  "LastOrderId" bigint NULL,
  CONSTRAINT "PK_Customers" PRIMARY KEY ("Id")
);

CREATE TABLE "Orders" (
  "Id" bigint GENERATED BY DEFAULT AS IDENTITY,
  "Amount" numeric NOT NULL,
  "CustomerId" bigint NOT NULL,
  "ParentOrderId" bigint NULL,
  CONSTRAINT "PK_Orders" PRIMARY KEY ("Id"),
  CONSTRAINT "FK_Orders_Customers_CustomerId" FOREIGN KEY ("CustomerId") REFERENCES "Customers" ("Id") ON DELETE CASCADE,
  CONSTRAINT "FK_Orders_Orders_ParentOrderId" FOREIGN KEY ("ParentOrderId") REFERENCES "Orders" ("Id")
);

CREATE TABLE "ShoppingBaskets" (
  "Id" bigint GENERATED BY DEFAULT AS IDENTITY,
  "ProductCount" integer NOT NULL,
  "CurrentOrderId" bigint NULL,
  CONSTRAINT "PK_ShoppingBaskets" PRIMARY KEY ("Id"),
  CONSTRAINT "FK_ShoppingBaskets_Orders_CurrentOrderId" FOREIGN KEY ("CurrentOrderId") REFERENCES "Orders" ("Id")
);

CREATE UNIQUE INDEX "IX_Customers_FirstOrderId" ON "Customers" ("FirstOrderId");
CREATE UNIQUE INDEX "IX_Customers_LastOrderId" ON "Customers" ("LastOrderId");
CREATE INDEX "IX_Orders_CustomerId" ON "Orders" ("CustomerId");
CREATE UNIQUE INDEX "IX_Orders_ParentOrderId" ON "Orders" ("ParentOrderId");
CREATE UNIQUE INDEX "IX_ShoppingBaskets_CurrentOrderId" ON "ShoppingBaskets" ("CurrentOrderId");
ALTER TABLE "Customers" ADD CONSTRAINT "FK_Customers_Orders_FirstOrderId" FOREIGN KEY ("FirstOrderId") REFERENCES "Orders" ("Id");
ALTER TABLE "Customers" ADD CONSTRAINT "FK_Customers_Orders_LastOrderId" FOREIGN KEY ("LastOrderId") REFERENCES "Orders" ("Id");

Then seed the database with the following data:

Order existingOrder = _fakers.Order.Generate();
existingOrder.Customer = _fakers.Customer.Generate();

dbContext.Orders.Add(existingOrder);
await dbContext.SaveChangesAsync();

existingOrder.Customer.FirstOrder = existingOrder;
existingOrder.Customer.LastOrder = existingOrder;

List<ShoppingBasket> existingBaskets = _fakers.ShoppingBasket.Generate(3);
existingBaskets[0].CurrentOrder = existingOrder;
existingBaskets[1].CurrentOrder = existingOrder;

dbContext.ShoppingBaskets.AddRange(existingBaskets);
await dbContext.SaveChangesAsync();

Now when trying to delete the order:

DELETE /orders/1 HTTP/1.1

It fails with the next exception:

DbUpdateException: (0x80004005): 23503: update or delete on table "Orders" violates foreign key constraint "FK_ShoppingBaskets_Orders_CurrentOrderId" on table "ShoppingBaskets"

EXPECTED BEHAVIOR

Order 1 is deleted successfully, which results in setting Customer.FirstOrderId, Customer.LastOrderId and ShoppingBasket.CurrentOrder to null for all affected records.

ACTUAL BEHAVIOR

JsonApiDotNetCore does not load all affected records into the EF Core change tracker upfront so that EF Core would be able to produce the right set of UPDATE statements:

UPDATE "Customers" SET "FirstOrderId" = NULL, "LastOrderId" = NULL WHERE "Id" = @p0;
UPDATE "ShoppingBaskets" SET "CurrentOrderId" = NULL WHERE "Id" = @p1;
UPDATE "ShoppingBaskets" SET "CurrentOrderId" = NULL WHERE "Id" = @p2;
DELETE FROM "Order" WHERE "Id" = @p3;

The current behavior is that JsonApiDotNetCore loops over the JSON:API relationships in Order, which only matches Customer in this case. It then concludes that it does not need to side-load the related customer because the foreign key is configured as ON DELETE CASCADE. That logic is insufficient because it fails to take records into account where Customer.FirstOrder, Customer.LastOrder and ShoppingBasket.CurrentOrder match the ID of the order to delete.

To fix this:

• JsonApiDotNetCore should look at navigations in the EF Core model, not just the subset of navigations that are modeled as JSON:API relationships. Even looking at just the inverse navigation of Order is insufficient, because that would not find Customer.FirstOrder, Customer.LastOrder and ShoppingBasket.CurrentOrder. So it needs to scan the full model and find all navigations pointing to an order.
• From the set of found navigations, it needs to determine which ones need to be loaded by looking at where the foreign key resides and its ON DELETE behavior. Note it should not abort on required relationships, because there may not be any matching records so the operation can still succeed.
• From the set of affected navigations, a SELECT query must be produced to obtain the IDs of the related records. In this example, three shopping baskets exist, but only two of them are linked to our order. So it should only fetch the IDs of those two. In a production database, these numbers are usually a lot higher, which makes the distinction important.
• Corner cases, such as self-referencing records and cycles, should be taken into account.
• This does not affect many-to-many relationships, because its join table always uses cascading deletes. Neither does it affect one-to-many, as it uses cascading deletes by default. Required relationships use cascading deletes as well. Optional one-to-one relationships are what's causing the issue.

[bart-degreed]

Related to #502, which solved the simpler cases.

[bart-degreed]

One could argue it's better to fail in this case instead of executing unexpected extra queries. I think it matches the intent of the API developer who has set delete behavior to DeleteBehavior.ClientSetNull. If this is undesired, setting DeleteBehavior.ClientNoAction results in failure, like today.

We can always add a configurable threshold to fail instead if this turns out to fetch too much data. Note however that the API consumer executes this call for a reason, so it means then the API will receive preceding requests to set all existing related keys to null before re-trying the delete. That takes longer, requires more traffic, and happens in a non-transactional way.

[bkoelman]

In response to updated issue description:

Changed this issue from bug to enhancement because changing the EF Core mapping for one-to-one optional relationships to .OnDelete(DeleteBehavior.SetNull) is a better solution, over JADNC fetching a lot more extra records to account for a corner case we've never seen happen in practice.

Leaving this open to possibly address in the future. I've pushed a branch that fixes the simple cases, but it requires a lot more testing and it is a high-risk change. Until then, we should update the documentation to warn about this inconvenient default mapping convention. Even with a fix released, this default is going to result in lots of unneeded extra queries.

Perhaps we can point users in the right direction by adding a custom convention (new in EF 7) that overrides the EF Core default convention (unless turned off via a setting). Similarly, we could possibly do that for identifying foreign keys too.