Refactor AttachRelationships: prevent reattachment, support implicit remove

[maurei]

Problem

Consider the following scenario

• Person one-to-one Passport
• initial condition of the database:
  • two Persons items: person1, person2.
  • only one Passport item: passport
  • foreign key PassportId on person1 equals id of passport

We now do a PATCH /people/2 and attempt to update the relationship of person2 by relating it to passport. What I would expect it to do is to perform an implicit remove of the old relationship: implicit in the sense that the relationship with person1 is updated even though it was never explicitly mentioned in the original request.

Right now, it results in a 500 because a foreign key constraint is violated. This is because person1 is already related to passport and the implicit remove is not taken care of, resulting in a FK constraint error.

Update bug reproduced in this test

Issue: a 500 is not acceptable.

Solution

Two options

1. Don't actually do the implicit remove, but return a 400 Bad Request with a body stating that the relation between person1 and passport should be removed first, thus more or less reflecting the foreign key constraint error message that is generated by the database~
2. handle these situations intelligently in the repository layer. The repository layer would take care of breaking the relationship between person1 and passport before relating person2 to passport. EF Core can handle this trivially if we just attach person1 to the dbContext before saving the changes (by just loading the Person relationship of passport). Right now that doesn't happen.

I prefer the second option.

[EDIT]

This goes wrong for CREATE, UPDATE for both one-to-one and one-to-many. All goes in a similar way: the entity on the inverse navigation property needs to be attached. This means that the fix is not specific to one particular "pipeline", but the AttachRelationship method which is called in all involved pipelines needs an upgrade:

• for many-to-many and one-to-many: needs to attach relationships of the to-be-manipulated entity so that complete replacement actually works (see Update one-to-many through PATCH on resource not working #492 and Update many-to-many through PATCH on resource not working #494)
• for one-to-one and one-to-many: It needs to do attachment of inverse relationships to prevent the error as mentioned above from occurring. See Support implicit remove when creating/updating one-to-one relationship #503: it has a proof of concept for the specific example above
• needs to take care we're never re-attaching entities that were loaded into dbContext elsewhere in the application, see DefaultEntityRepository.PreventReattachment (private method) in Update many-to-many through PATCH on resource not working #494.
• Needs to be refactored such that AttachRelationships is consistent with respect to assigning the actual new relationship values. In the current implementation, for one-to-many and one-to-one they are not assigned in this method, but for many-to-many they are. This is inconsistent, which makes it a lot harder to prevent reattachment in a simple way.

[bart-degreed]

For reference, the next picture shows uniqueness constraint violations for one-to-one relationships.