From d585893dfcdcacb4c1652e6447af26f4397ce35d Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 17 Jun 2021 09:46:48 +0200 Subject: [PATCH 1/7] 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. --- .../Repositories/EntityFrameworkCoreRepository.cs | 2 +- .../Repositories/PlaceholderResourceCollector.cs | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs index 24f835c6df..397eea4a5b 100644 --- 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 index d28ab4a6c0..51efa96aae 100644 --- a/src/JsonApiDotNetCore/Repositories/PlaceholderResourceCollector.cs +++ b/src/JsonApiDotNetCore/Repositories/PlaceholderResourceCollector.cs @@ -70,7 +70,15 @@ private void Detach(IEnumerable 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. + } } } } From 5eae4e28f35f5d50dd5d013dd78bb7fd54f85263 Mon Sep 17 00:00:00 2001 From: maurei Date: Fri, 18 Jun 2021 13:58:00 +0200 Subject: [PATCH 2/7] 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. --- .../RequiredRelationships/DefaultBehaviorDbContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs index d8a617037b..42acd4ab74 100644 --- a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs +++ b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs @@ -27,7 +27,7 @@ protected override void OnModelCreating(ModelBuilder builder) builder.Entity() .HasOne(order => order.Shipment) .WithOne(shipment => shipment.Order) - .HasForeignKey() + .HasForeignKey("OrderId") .IsRequired(); } } From 04dac0e9d316fb0f242accc64263427837cb28ef Mon Sep 17 00:00:00 2001 From: maurei Date: Fri, 18 Jun 2021 14:20:05 +0200 Subject: [PATCH 3/7] Updated tests to cover for zero-or-one-to-one cases --- .../DefaultBehaviorTests.cs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs index fb5e465329..9609766040 100644 --- 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; @@ -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(); @@ -424,18 +425,19 @@ await _testContext.RunOnDatabaseAsync(async dbContext => (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); // Assert - httpResponse.Should().HaveStatusCode(HttpStatusCode.InternalServerError); + httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); - responseDocument.Errors.Should().HaveCount(1); + await _testContext.RunOnDatabaseAsync(async dbContext => + { + Shipment existingShipmentInDatabase = + await dbContext.Shipments.Include(shipment => shipment.Order).FirstWithIdOrDefaultAsync(orderWithShipment.Shipment.Id); - 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."); + 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(); @@ -466,14 +468,15 @@ await _testContext.RunOnDatabaseAsync(async dbContext => (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); // Assert - httpResponse.Should().HaveStatusCode(HttpStatusCode.InternalServerError); + httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); - responseDocument.Errors.Should().HaveCount(1); + await _testContext.RunOnDatabaseAsync(async dbContext => + { + Shipment existingShipmentInDatabase = + await dbContext.Shipments.Include(shipment => shipment.Order).FirstWithIdOrDefaultAsync(orderWithShipment.Shipment.Id); - 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."); + existingShipmentInDatabase.Order.Id.Should().Be(orderWithoutShipment.Id); + }); } } } From 8806a0404a9ecff92ac10a04a7dfcc6bac63e636 Mon Sep 17 00:00:00 2001 From: maurei Date: Fri, 18 Jun 2021 14:29:37 +0200 Subject: [PATCH 4/7] add comment explaining why this change was needed --- .../RequiredRelationships/DefaultBehaviorDbContext.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs index 42acd4ab74..3374381d10 100644 --- a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs +++ b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs @@ -27,6 +27,7 @@ protected override void OnModelCreating(ModelBuilder builder) builder.Entity() .HasOne(order => order.Shipment) .WithOne(shipment => shipment.Order) + // Without specifying "OrderId", the primary key will be used as a foreign key which would result in attempt to update the shipment identity when this relationship is patched. .HasForeignKey("OrderId") .IsRequired(); } From 0e9fcfcd8824dc48500f7e11da55dd4340be5ac5 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Tue, 22 Jun 2021 19:04:50 +0200 Subject: [PATCH 5/7] Clarified comment --- .../RequiredRelationships/DefaultBehaviorDbContext.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs index 3374381d10..2c0e3a88df 100644 --- a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs +++ b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorDbContext.cs @@ -24,10 +24,13 @@ 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() .HasOne(order => order.Shipment) .WithOne(shipment => shipment.Order) - // Without specifying "OrderId", the primary key will be used as a foreign key which would result in attempt to update the shipment identity when this relationship is patched. .HasForeignKey("OrderId") .IsRequired(); } From 77be4ff346daf9661f10a4a096264c7867a7d3cf Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Wed, 23 Jun 2021 00:23:08 +0200 Subject: [PATCH 6/7] 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. --- .../IntegrationTests/AtomicOperations/OperationsDbContext.cs | 2 +- .../IntegrationTests/EagerLoading/EagerLoadingDbContext.cs | 4 ++-- .../IntegrationTests/Links/LinksDbContext.cs | 2 +- .../IntegrationTests/ReadWrite/ReadWriteDbContext.cs | 2 +- .../IntegrationTests/ResourceHooks/HooksDbContext.cs | 2 +- test/UnitTests/ResourceHooks/HooksDbContext.cs | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/AtomicOperations/OperationsDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/AtomicOperations/OperationsDbContext.cs index c7ab6d943c..ad45e3bb85 100644 --- 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() .HasOne(musicTrack => musicTrack.Lyric) .WithOne(lyric => lyric.Track) - .HasForeignKey(); + .HasForeignKey("LyricId"); } } } diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/EagerLoading/EagerLoadingDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/EagerLoading/EagerLoadingDbContext.cs index efce1b3014..62dd263f79 100644 --- 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() .HasOne(building => building.PrimaryDoor) .WithOne() - .HasForeignKey("PrimaryDoorKey") + .HasForeignKey("PrimaryDoorId") .IsRequired(); builder.Entity() .HasOne(building => building.SecondaryDoor) .WithOne() - .HasForeignKey("SecondaryDoorKey") + .HasForeignKey("SecondaryDoorId") .IsRequired(false); } } diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/Links/LinksDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/Links/LinksDbContext.cs index 1e0a63bfad..bad61d1e3a 100644 --- 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() .HasOne(photo => photo.Location) .WithOne(location => location.Photo) - .HasForeignKey("PhotoLocationKey"); + .HasForeignKey("LocationId"); } } } diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ReadWrite/ReadWriteDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ReadWrite/ReadWriteDbContext.cs index da22daa4d3..c5d589cbb8 100644 --- 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() .HasOne(workItemGroup => workItemGroup.Color) .WithOne(color => color.Group) - .HasForeignKey(); + .HasForeignKey("GroupId"); builder.Entity() .HasKey(workItemTag => new diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ResourceHooks/HooksDbContext.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/ResourceHooks/HooksDbContext.cs index a20c9a4f8f..1c8dd2948d 100644 --- 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() .HasOne(passport => passport.Person) .WithOne(person => person.Passport) - .HasForeignKey("PassportKey") + .HasForeignKey("PassportId") .OnDelete(DeleteBehavior.SetNull); } } diff --git a/test/UnitTests/ResourceHooks/HooksDbContext.cs b/test/UnitTests/ResourceHooks/HooksDbContext.cs index 5b5a8e0dcc..2d06f15b33 100644 --- a/test/UnitTests/ResourceHooks/HooksDbContext.cs +++ b/test/UnitTests/ResourceHooks/HooksDbContext.cs @@ -54,13 +54,13 @@ protected override void OnModelCreating(ModelBuilder builder) builder.Entity() .HasOne(passport => passport.Person) .WithOne(person => person.Passport) - .HasForeignKey("PassportKey") + .HasForeignKey("PassportId") .OnDelete(DeleteBehavior.SetNull); builder.Entity() .HasOne(todoItem => todoItem.OneToOnePerson) .WithOne(person => person.OneToOneTodoItem) - .HasForeignKey("OneToOnePersonKey"); + .HasForeignKey("OneToOnePersonId"); } } } From c38d2237ac34e1a55f12d3be318b9b5ca892369b Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Wed, 23 Jun 2021 02:05:22 +0200 Subject: [PATCH 7/7] Fixed missing body assertions --- .../DefaultBehaviorTests.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs index 9609766040..df90cfcf93 100644 --- a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs @@ -114,12 +114,12 @@ await _testContext.RunOnDatabaseAsync(async dbContext => string route = $"/customers/{existingOrder.Customer.Id}"; // Act - (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteDeleteAsync(route); + (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecuteDeleteAsync(route); // Assert httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); - responseDocument.Should().BeNull(); + responseDocument.Should().BeEmpty(); await _testContext.RunOnDatabaseAsync(async dbContext => { @@ -148,12 +148,12 @@ await _testContext.RunOnDatabaseAsync(async dbContext => string route = $"/orders/{existingOrder.Id}"; // Act - (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteDeleteAsync(route); + (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecuteDeleteAsync(route); // Assert httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); - responseDocument.Should().BeNull(); + responseDocument.Should().BeEmpty(); await _testContext.RunOnDatabaseAsync(async dbContext => { @@ -422,11 +422,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext => string route = $"/orders/{orderWithoutShipment.Id}"; // Act - (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); + (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); // Assert httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); + responseDocument.Should().BeEmpty(); + await _testContext.RunOnDatabaseAsync(async dbContext => { Shipment existingShipmentInDatabase = @@ -465,11 +467,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext => string route = $"/orders/{orderWithoutShipment.Id}/relationships/shipment"; // Act - (HttpResponseMessage httpResponse, ErrorDocument responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); + (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); // Assert httpResponse.Should().HaveStatusCode(HttpStatusCode.NoContent); + responseDocument.Should().BeEmpty(); + await _testContext.RunOnDatabaseAsync(async dbContext => { Shipment existingShipmentInDatabase =