Fix exception propagation (#1021)

Bart Koelman · maurei · web-flow · commit e42a7a474073 · 2021-06-23T11:06:26.000+02:00
* The exception thrown and wrapped on SaveChanges failure was superseeded while disposing PlaceholderResourceCollector that threw too, resulting in a 500 error response. And it produced noisy logs in cibuild, coming from ExceptionHandler. This commit swallows the second exception so the original one can propagate through the pipeline as intended.

* Specified a FK in scenario of a required one-to-one relationship. Note that not specifying it results in EF Core configuring this relationship using an identifying FK on the dependent side (Shipment), which is a type of modeling that does not make sense in the scope of JSON:API.

* Updated tests to cover for zero-or-one-to-one cases

* add comment explaining why this change was needed

* Clarified comment

* Verified existing one-to-one relationships in our codebase and updated to use an explicit foreign key accordingly. It turns out that not in all cases this makes a difference. I wonder under which circumstances EF Core chooses to use an identifying foreign key and when it does not.

* Fixed missing body assertions

Co-authored-by: maurei &lt;maurits.moeys@gmail.com&gt;
diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs
@@ -528,7 +528,7 @@ protected virtual async Task SaveChangesAsync(CancellationToken cancellationToke
             {
                 await _dbContext.SaveChangesAsync(cancellationToken);
             }
-            catch (DbUpdateException exception)
+            catch (Exception exception) when (exception is DbUpdateException || exception is InvalidOperationException)
             {
                 if (_dbContext.Database.CurrentTransaction != null)
                 {
diff --git a/src/JsonApiDotNetCore/Repositories/PlaceholderResourceCollector.cs b/src/JsonApiDotNetCore/Repositories/PlaceholderResourceCollector.cs
@@ -70,7 +70,15 @@ private void Detach(IEnumerable<object> resources)
         {
             foreach (object resource in resources)
             {
-                _dbContext.Entry(resource).State = EntityState.Detached;
+                try
+                {
+                    _dbContext.Entry(resource).State = EntityState.Detached;
+                }
+                catch (InvalidOperationException)
+                {
+                    // If SaveChanges() threw due to a foreign key constraint violation, its exception is rethrown here.
+                    // We swallow this exception, to allow the originating error to propagate.
+                }
             }
         }
     }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/AtomicOperations/OperationsDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/AtomicOperations/OperationsDbContext.cs
@@ -33,7 +33,7 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<MusicTrack>()
                 .HasOne(musicTrack => musicTrack.Lyric)
                 .WithOne(lyric => lyric.Track)
-                .HasForeignKey<MusicTrack>();
+                .HasForeignKey<MusicTrack>("LyricId");
         }
     }
 }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/EagerLoading/EagerLoadingDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/EagerLoading/EagerLoadingDbContext.cs
@@ -22,13 +22,13 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<Building>()
                 .HasOne(building => building.PrimaryDoor)
                 .WithOne()
-                .HasForeignKey<Building>("PrimaryDoorKey")
+                .HasForeignKey<Building>("PrimaryDoorId")
                 .IsRequired();
 
             builder.Entity<Building>()
                 .HasOne(building => building.SecondaryDoor)
                 .WithOne()
-                .HasForeignKey<Building>("SecondaryDoorKey")
+                .HasForeignKey<Building>("SecondaryDoorId")
                 .IsRequired(false);
         }
     }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/Links/LinksDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/Links/LinksDbContext.cs
@@ -22,7 +22,7 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<Photo>()
                 .HasOne(photo => photo.Location)
                 .WithOne(location => location.Photo)
-                .HasForeignKey<Photo>("PhotoLocationKey");
+                .HasForeignKey<Photo>("LocationId");
         }
     }
 }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ReadWrite/ReadWriteDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ReadWrite/ReadWriteDbContext.cs
@@ -33,7 +33,7 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<WorkItemGroup>()
                 .HasOne(workItemGroup => workItemGroup.Color)
                 .WithOne(color => color.Group)
-                .HasForeignKey<RgbColor>();
+                .HasForeignKey<RgbColor>("GroupId");
 
             builder.Entity<WorkItemTag>()
                 .HasKey(workItemTag => new
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs
@@ -24,10 +24,14 @@ protected override void OnModelCreating(ModelBuilder builder)
                 .WithOne(order => order.Customer)
                 .IsRequired();
 
+            // By default, EF Core generates an identifying foreign key for a required 1-to-1 relationship.
+            // This means no foreign key column is generated, instead the primary keys point to each other directly.
+            // That mechanism does not make sense for JSON:API, because patching a relationship would result in
+            // also changing the identity of a resource. Naming the key explicitly forces to create a foreign key column.
             builder.Entity<Order>()
                 .HasOne(order => order.Shipment)
                 .WithOne(shipment => shipment.Order)
-                .HasForeignKey<Shipment>()
+                .HasForeignKey<Shipment>("OrderId")
                 .IsRequired();
         }
     }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs
@@ -5,6 +5,7 @@
 using JsonApiDotNetCore.Configuration;
 using JsonApiDotNetCore.Serialization.Objects;
 using JsonApiDotNetCoreExampleTests.Startups;
+using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.DependencyInjection;
 using TestBuildingBlocks;
 using Xunit;
@@ -113,12 +114,12 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
             string route = $"/customers/{existingOrder.Customer.Id}";
 
             // Act
-            (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteDeleteAsync<Document>(route);
+            (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecuteDeleteAsync<string>(route);
 
             // Assert
             httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent);
 
-            responseDocument.Should().BeNull();
+            responseDocument.Should().BeEmpty();
 
             await _testContext.RunOnDatabaseAsync(async dbContext =>
             {
@@ -147,12 +148,12 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
             string route = $"/orders/{existingOrder.Id}";
 
             // Act
-            (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteDeleteAsync<Document>(route);
+            (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecuteDeleteAsync<string>(route);
 
             // Assert
             httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent);
 
-            responseDocument.Should().BeNull();
+            responseDocument.Should().BeEmpty();
 
             await _testContext.RunOnDatabaseAsync(async dbContext =>
             {
@@ -382,7 +383,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
         }
 
         [Fact]
-        public async Task Cannot_reassign_dependent_side_of_OneToOne_relationship_with_identifying_foreign_key_through_primary_endpoint()
+        public async Task Can_reassign_dependent_side_of_ZeroOrOneToOne_relationship_through_primary_endpoint()
         {
             // Arrange
             Order orderWithShipment = _fakers.Orders.Generate();
@@ -421,21 +422,24 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
             string route = $"/orders/{orderWithoutShipment.Id}";
 
             // Act
-            (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync<ErrorDocument>(route, requestBody);
+            (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecutePatchAsync<string>(route, requestBody);
 
             // Assert
-            httpResponse.Should().HaveStatusCode(HttpStatusCode.InternalServerError);
+            httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent);
 
-            responseDocument.Errors.Should().HaveCount(1);
+            responseDocument.Should().BeEmpty();
 
-            Error error = responseDocument.Errors[0];
-            error.StatusCode.Should().Be(HttpStatusCode.InternalServerError);
-            error.Title.Should().Be("An unhandled error occurred while processing this request.");
-            error.Detail.Should().StartWith("The property 'Id' on entity type 'Shipment' is part of a key and so cannot be modified or marked as modified.");
+            await _testContext.RunOnDatabaseAsync(async dbContext =>
+            {
+                Shipment existingShipmentInDatabase =
+                    await dbContext.Shipments.Include(shipment => shipment.Order).FirstWithIdOrDefaultAsync(orderWithShipment.Shipment.Id);
+
+                existingShipmentInDatabase.Order.Id.Should().Be(orderWithoutShipment.Id);
+            });
         }
 
         [Fact]
-        public async Task Cannot_reassign_dependent_side_of_OneToOne_relationship_with_identifying_foreign_key_through_relationship_endpoint()
+        public async Task Can_reassign_dependent_side_of_ZeroOrOneToOne_relationship_through_relationship_endpoint()
         {
             // Arrange
             Order orderWithShipment = _fakers.Orders.Generate();
@@ -463,17 +467,20 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
             string route = $"/orders/{orderWithoutShipment.Id}/relationships/shipment";
 
             // Act
-            (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync<ErrorDocument>(route, requestBody);
+            (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecutePatchAsync<string>(route, requestBody);
 
             // Assert
-            httpResponse.Should().HaveStatusCode(HttpStatusCode.InternalServerError);
+            httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent);
 
-            responseDocument.Errors.Should().HaveCount(1);
+            responseDocument.Should().BeEmpty();
 
-            Error error = responseDocument.Errors[0];
-            error.StatusCode.Should().Be(HttpStatusCode.InternalServerError);
-            error.Title.Should().Be("An unhandled error occurred while processing this request.");
-            error.Detail.Should().StartWith("The property 'Id' on entity type 'Shipment' is part of a key and so cannot be modified or marked as modified.");
+            await _testContext.RunOnDatabaseAsync(async dbContext =>
+            {
+                Shipment existingShipmentInDatabase =
+                    await dbContext.Shipments.Include(shipment => shipment.Order).FirstWithIdOrDefaultAsync(orderWithShipment.Shipment.Id);
+
+                existingShipmentInDatabase.Order.Id.Should().Be(orderWithoutShipment.Id);
+            });
         }
     }
 }
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ResourceHooks/HooksDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ResourceHooks/HooksDbContext.cs
@@ -37,7 +37,7 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<Passport>()
                 .HasOne(passport => passport.Person)
                 .WithOne(person => person.Passport)
-                .HasForeignKey<Person>("PassportKey")
+                .HasForeignKey<Person>("PassportId")
                 .OnDelete(DeleteBehavior.SetNull);
         }
     }
diff --git a/test/UnitTests/ResourceHooks/HooksDbContext.cs b/test/UnitTests/ResourceHooks/HooksDbContext.cs
@@ -54,13 +54,13 @@ protected override void OnModelCreating(ModelBuilder builder)
             builder.Entity<Passport>()
                 .HasOne(passport => passport.Person)
                 .WithOne(person => person.Passport)
-                .HasForeignKey<Person>("PassportKey")
+                .HasForeignKey<Person>("PassportId")
                 .OnDelete(DeleteBehavior.SetNull);
 
             builder.Entity<TodoItem>()
                 .HasOne(todoItem => todoItem.OneToOnePerson)
                 .WithOne(person => person.OneToOneTodoItem)
-                .HasForeignKey<TodoItem>("OneToOnePersonKey");
+                .HasForeignKey<TodoItem>("OneToOnePersonId");
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -528,7 +528,7 @@ protected virtual async Task SaveChangesAsync(CancellationToken cancellationToke`
`528`	`528`	`{`
`529`	`529`	`await _dbContext.SaveChangesAsync(cancellationToken);`
`530`	`530`	`}`
`531`		`- catch (DbUpdateException exception)`
	`531`	`+ catch (Exception exception) when (exception is DbUpdateException \|\| exception is InvalidOperationException)`
`532`	`532`	`{`
`533`	`533`	`if (_dbContext.Database.CurrentTransaction != null)`
`534`	`534`	`{`
Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,15 @@ private void Detach(IEnumerable<object> resources)`
`70`	`70`	`{`
`71`	`71`	`foreach (object resource in resources)`
`72`	`72`	`{`
`73`		`- _dbContext.Entry(resource).State = EntityState.Detached;`
	`73`	`+ try`
	`74`	`+ {`
	`75`	`+ _dbContext.Entry(resource).State = EntityState.Detached;`
	`76`	`+ }`
	`77`	`+ catch (InvalidOperationException)`
	`78`	`+ {`
	`79`	`+ // If SaveChanges() threw due to a foreign key constraint violation, its exception is rethrown here.`
	`80`	`+ // We swallow this exception, to allow the originating error to propagate.`
	`81`	`+ }`
`74`	`82`	`}`
`75`	`83`	`}`
`76`	`84`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ protected override void OnModelCreating(ModelBuilder builder)`
`33`	`33`	`builder.Entity<MusicTrack>()`
`34`	`34`	`.HasOne(musicTrack => musicTrack.Lyric)`
`35`	`35`	`.WithOne(lyric => lyric.Track)`
`36`		`- .HasForeignKey<MusicTrack>();`
	`36`	`+ .HasForeignKey<MusicTrack>("LyricId");`
`37`	`37`	`}`
`38`	`38`	`}`
`39`	`39`	`}`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ protected override void OnModelCreating(ModelBuilder builder)`
`22`	`22`	`builder.Entity<Photo>()`
`23`	`23`	`.HasOne(photo => photo.Location)`
`24`	`24`	`.WithOne(location => location.Photo)`
`25`		`- .HasForeignKey<Photo>("PhotoLocationKey");`
	`25`	`+ .HasForeignKey<Photo>("LocationId");`
`26`	`26`	`}`
`27`	`27`	`}`
`28`	`28`	`}`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ protected override void OnModelCreating(ModelBuilder builder)`
`37`	`37`	`builder.Entity<Passport>()`
`38`	`38`	`.HasOne(passport => passport.Person)`
`39`	`39`	`.WithOne(person => person.Passport)`
`40`		`- .HasForeignKey<Person>("PassportKey")`
	`40`	`+ .HasForeignKey<Person>("PassportId")`
`41`	`41`	`.OnDelete(DeleteBehavior.SetNull);`
`42`	`42`	`}`
`43`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -54,13 +54,13 @@ protected override void OnModelCreating(ModelBuilder builder)`
`54`	`54`	`builder.Entity<Passport>()`
`55`	`55`	`.HasOne(passport => passport.Person)`
`56`	`56`	`.WithOne(person => person.Passport)`
`57`		`- .HasForeignKey<Person>("PassportKey")`
	`57`	`+ .HasForeignKey<Person>("PassportId")`
`58`	`58`	`.OnDelete(DeleteBehavior.SetNull);`
`59`	`59`
`60`	`60`	`builder.Entity<TodoItem>()`
`61`	`61`	`.HasOne(todoItem => todoItem.OneToOnePerson)`
`62`	`62`	`.WithOne(person => person.OneToOneTodoItem)`
`63`		`- .HasForeignKey<TodoItem>("OneToOnePersonKey");`
	`63`	`+ .HasForeignKey<TodoItem>("OneToOnePersonId");`
`64`	`64`	`}`
`65`	`65`	`}`
`66`	`66`	`}`