Working with detached entities are a nuisance. Working with detached entity graphs are a complete pain. There are several issues with your approach, namely when working with detached entities, the database state should always be treated as the point of truth for which you apply changes from your detached state only after you validate that they are still relevant. (I.e. someone else hasn't modified the entity data since your current copy had been taken.
First off,
await _context.Events.CountAsync() > 0
&& await _context.Events.FindAsync(newEvent.Id) is not null
is completely unnecessary. Why would you tell the DbContext to execute a count AND load the entity just to determine if the entity exists? If you want to know if an entity exists:
var doesExist = _context.Events.Any(x => x.Id == newEvent.Id);
if (doesExist)
return BadRequest(new ConstraintException("Event already exists"));
This executes an IF EXISTS SELECT against the database which is much faster.
Not every operation on the DbContext needs to be async. Asynchronous calls are useful for operations that will take some time to run. They come with an small extra performance cost so any operation that can be done quickly such as fetching individual entities or reasonable entity graphs can just be done synchronously.
Next, when dealing with detached entities you generally do not want to detach existing entities, and especially not references, to be overwritten by the detached entities coming in. In short you should never trust data coming into the domain to be current or safe from unexpected tampering, either by bugs or malicious consumers. For example if you have a web application where the server sends an detached entity to be rendered, then the client presents fields to be changed then the Form or Ajax POST (Javascript) serializes data into an Entity class to send back to the server, it is very easy to miss values resulting in #nulls, and malicious users can use browser debugging tools to intercept the POST, view the entity data and make changes which code like the above could unwittingly overwrite data. What gets passed in may look like an entity, but it is often not.
Instead, without changing the fact that a detached entity graph is being passed in, treat it like a DTO. The data in Event will serve as a new entity, but everything related to it you will need to decide whether those represent new entities or references to existing ones. So for instance if the relationship between an Event and a DoorPrize is one to many, where a DoorPrize entity would be created with the new event, and only ever associated with that entity, then it stands that it should be allowed to be inserted with that entity. If instead the DoorPrize is its own entity and merely associated with this Event (and others) then it needs to be re-associated with the data state.
The difference between the two: 1-to-many (Event owns DoorPrizes) in the database would have an EventId on the DoorPrize table. Many-to-many (Event is associated with DoorPrizes) There would be an EventDoorPrize linking table containing the EventId & DoorPrizeId.
In the first case, if the event is considered as New, the door prizes should all be new. However, the relationship between DoorPrize and Sponsor is most likely a many-to-many association where one sponsor will likely be associated with many different door prizes across different events.
With ownership, if a client consumer is generating new IDs for entities (not recommended, it's better to leverage things like Identity columns and let the database manage that) then you might need to check that new DoorPrize records are not in the DB. The point here wouldn't be to replace existing Door Prizes if found, but to throw a data exception since we expect to be adding these new children:
Example if DoorPrizes are "owned" by Events (1-to-many relationship) but DoorPrizeIds are set by the consumer such as using a meaningful key or Guid.New()
var doorPrizeIds = newEvent.SelectMany(e => e.DoorPrize.Id).ToList();
var doorPrizeExists = _context.DoorPrizes.Any(dp => doorPrizeIds.Contains(dp.Id));
if (doorPrizeExists)
return BadRequest(new ConstraintException("One or more door prizes already exists"))
Dealing with associations requires a bit more attention. If DoorPrizes are expected to exist and are associated with a new Event then we need to locate those. If this request needs to handle that new DoorPrize entities might be created as part of this request, then that would need to be handled as well. As a general rule it is better to handle things more atomically where creating an Event that associates with door prizes would be responsible for just that. If there was an operation to create a new Door Prize then that would be handled by a separate call.
Example if DoorPrizes are "associated" to Events (many-to-many relationship)
var doorPrizeIds = newEvent.SelectMany(e => e.DoorPrize.Id)
.ToList();
var existingDoorPrizes = await _context.DoorPrizes
.Where(dp => doorPrizeIds.Contains(dp.Id))
.ToListAsync();
var existingDoorPrizeIds = existingDoorPrizes.Select(dp => dp.Id).ToList();
var doorPrizesToExclude = newEvent.SelectMany(e => e.DoorPrize)
.Where(dp => existingDoorPrizeIds.Contains(dp.Id))
.ToList();
foreach(var doorPrize in doorPrizesToExclude)
newEvent.DoorPrizes.Remove(doorPrize);
foreach(var doorPrize in existingDoorPrizes)
newEvent.DoorPrizes.Add(doorPrize);
What this gives us is a list of matching real Door Prize entities to associate. We will want to associate these in place of the data that came in with the new event. Any door prizes that might be new would be added when the event is added. The final step here will apply to both scenarios which will be to associate the sponsors to any new DoorPrize. In the one-to-many scenario that would be every door prize, in the many-to-many that would just be the non-existing ones that might be added:
1-to-many example:
var sponsorIds = newEvent
.SelectMany(e => e.DoorPrizes.Select(dp => dp.Sponsor.Id))
.Distinct();
var sponsors = await _context.Sponsors
.Where(s => sponsorIds.Contains(s.Id))
.ToListAsync();
foreach(var doorPrize in newEvent.DoorPrizes)
{
var sponsor = sponsors.SingleOrDefault(s => s.Id == doorPrize.Sponsor.Id);
if(sponsor == null)
return BadRequest(new ConstraintException("One or more door prizes was invalid."))
doorPrize.Sponsor = sponsor;
}
Many-to-many example:
1-to-many example:
var newDoorPrizes = newEvent.DoorPrizes.Where(dp => !existingDoorPrizeIds.Contains(dp.Id)).ToList();
if(newDoorPrizes.Any())
{
var sponsorIds = newDoorPrizes.Select(dp => dp.Sponsor.Id))
.Distinct();
var sponsors = await _context.Sponsors
.Where(s => sponsorIds.Contains(s.Id))
.ToListAsync();
foreach(var doorPrize in newEvent.DoorPrizes)
{
var sponsor = sponsors.SingleOrDefault(s => s.Id == doorPrize.Sponsor.Id);
if(sponsor == null)
return BadRequest(new ConstraintException("One or more door prizes was invalid."))
doorPrize.Sponsor = sponsor;
}
}
A similar operation to deal with associations for the Sponsor, the difference just being if the DoorPrizes are associations, we only want to do the substitution for Sponsors on newly added door prizes. The door prizes we re-associated from context tracked entities will already have valid sponsors.
Later, when you perform updates, it is a similar process, except you would expect to fetch the existing entity, but also pre-fetch the associated details with eager loading:
var existingEntry = _context.Entries
.Include(e => e.DoorPrizes)
.Single(e => e.Id == entryId);
This will throw if the entry isn't found which you can catch, or call .SingleOrDefault and check for #null to return your BadRequest if you prefer doing it inline. From there it is much the same process by where you can inspect the details coming un with the existingEntry to determine if DoorPrizes need to be updated, added, or removed. Again, for adding DoorPrizes the same process to re-associate Sponsors with actual tracked instances.
The important thing when updating entity graphs (parent-child relationships or associations) is to differentiate between whether the higher level entity "owns" the relationship, or if it is an association between entities that may already exist in the database. You will want to avoid code that detaches tracked entities and then does things like setting a passed in entity state to Modified to be saved. This will lead to all manners of problems where you overwrite data you don't intend to change, or exceptions when EF/SQL get told to do something invalid.