Saving entity duplicates navigational property in database-CodePudding

When adding an imageURL to my database and passing a foreign key in my builder the foreign key gets duplicated in my database. My program adds an Artwork and then tries to add images of said artwork to a seperate table in SQL.

When adding the image it duplicates the entirety of the artwork it's using as a foreign key. I've tried a few things such as adding [ForeignKey("Artwork")] & [ForeignKey("ArtworkId")] as well as fiddling with my builder.

My models:

    public partial class ArtworkImage
    {
    [Key]
    public int ArtworkImageId { get; set; }
    public string ImageURL { get; set; }
    public string ImageSize { get; set; }
    [ForeignKey("ArtworkId")]
    public virtual Artwork Artwork { get; set; }
    public int ArtworkId { get; set; }
    }

    public partial class Artwork
    {
    [Key]
    public int ArtworkId { get; set; }
    public string Identifier { get; set; }
    public string Category { get; set; }
    public ICollection<ArtworkImage> ArtworkImage { get; set; }
    }

My builder looks like:

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Artwork>()
                .HasKey(x => x.ArtworkId)
                .HasMany(p => p.ArtworkImage)
                .WithRequired(p => p.Artwork);


            modelBuilder.Entity<ArtworkImage>()
                .HasKey(b => b.ArtworkImageId)
                .HasRequired(p => p.Artwork)
                .WithMany(d => d.ArtworkImage)
                .HasForeignKey(p => p.ArtworkId);


            Database.SetInitializer<Artworkcontext>(null);
            base.OnModelCreating(modelBuilder);
        }

My migration:

    public override void Up()
        {
            CreateTable(
                "dbo.ArtworkImages",
                c => new
                    {
                        ArtworkImageId = c.Int(nullable: false, identity: true),
                        ImageURL = c.String(),
                        ImageSize = c.String(),
                        ArtworkId = c.Int(nullable: false),
                    })
                .PrimaryKey(t => t.ArtworkImageId)
                .ForeignKey("dbo.Artworks", t => t.ArtworkId, cascadeDelete: true)
                .Index(t => t.ArtworkId);
            
            CreateTable(
                "dbo.Artworks",
                c => new
                    {
                        ArtworkId = c.Int(nullable: false, identity: true),
                        Identifier = c.String(),
                        Category = c.String(),
                    })
                .PrimaryKey(t => t.ArtworkId);
            
        }

Code to add artwork and then add artworkimage:

    Artwork newArtwork = new Artwork();
    ArtworkImage newArtworkImage = new ArtworkImage();
    newArtwork.Identifier = identifierCode;
    artworkRepository.Add(newArtwork);
    newArtworkImage.ImageURL = fileName;
    newArtworkImage.Artwork = newArtwork;
    artworkImageRepository.Add(newArtworkImage);

EFArtworkRepository: (add)

    public void Add(Artwork artwork)
    {
         context.Artworks.Add(artwork);
         context.SaveChanges();
    }

EFArtworkImageRepository: (add)

    public void Add(ArtworkImage artworkImage)
    {
          context.ArtworkImages.Add(artworkImage);
          context.SaveChanges();
    }

Some things I've looked at and implemented but didn't work, maybe implemented wrong?:

CodePudding user response：

The issue here is not the configuration, you can see from the migration code that the relationship has been correctly defined, so now we move on to your operational logic.

It may not be obvious, but your code is explicitly adding the newArtwork instance to two different underlying DbContext instances, as far as your code is concerned, it might as well be two different databases! Judging from your observations and common implementation patterns both EFArtworkRepository and EFArtworkImageRepository will have their own instance of DbContext.

We will cover the fundamental reasons behind the duplication of data, but in Repo patterns like this you will experience less issues by setting the FK references instead of the objects, especially if you are going to be passing the same object between multiple repositories. The following is a common workaround:

Artwork newArtwork = new Artwork();
newArtwork.Identifier = identifierCode;
artworkRepository.Add(newArtwork);

ArtworkImage newArtworkImage = new ArtworkImage();
newArtworkImage.ImageURL = fileName;
newArtworkImage.ArtworkId = newArtwork.ArtworkId;
artworkImageRepository.Add(newArtworkImage);

The underlying DbContext in EF is actually expecting to manage the assignment of Ids and associated FKs for you when new entities are created. To achieve this, it tracks objects internally using object references, it can't use the key columns because for inserts those keys do not have a value yet, so normally they are all zero.

If you are using Object based relationships and you are creating the Principal and the Dependent entities, then DbContext will support these objects being processed as a single operation:

Artwork newArtwork = new Artwork();
newArtwork.Identifier = identifierCode;
ArtworkImage newArtworkImage = new ArtworkImage();
newArtworkImage.ImageURL = fileName;
newArtwork.ArtworkImage = new HashSet<ArtworkImage>();
newArtwork.ArtworkImage.Add(newArtworkImage);
artworkRepository.Add(newArtwork);

NOTE: We have only used artworkRepository here and have achieved the expected result

When you are inserting a Dependent entity, similar logic can create the Principal entity on demand as well. This is actually the code that is executed from the point of view of the artworkImageRepository in your logic:

Artwork newArtwork = new Artwork();
ArtworkImage newArtworkImage = new ArtworkImage();
newArtwork.Identifier = identifierCode;
//artworkRepository.Add(newArtwork);
newArtworkImage.ImageURL = fileName;
newArtworkImage.Artwork = newArtwork;
artworkImageRepository.Add(newArtworkImage);

Which is identical to this fluent initialization:

Artwork newArtwork = new Artwork();
ArtworkImage newArtworkImage = new ArtworkImage
{
    ImageUrl = fileName,
    Artwork = new {
       ArtworkId = 12,
       Identifier = identifierCode 
    }
};

Your code has created an instance of newArtwork that has been saved to a database that assigned it a value for the key, in this example I have stated it was assigned a value of 12.

From the point of view of the context inside artworkImageRepository this is an external reference, and to accurately re-construct this unique entity graph in this database, we need to also create a new instance of the principal Artwork.

In EF6 object references will win over Key and FK values.

Do not assume that there is some magic that will try to first lookup the database to identify if there is already a matching record in the principal table before generating and executing the associated SQL statements.

To do so before every DbSet.Add() operation would be very inefficient.

The magic in EF is that we can use Object references in a model that doesn't even expose the FKs at all if you really want to. But this magic only works if you use the same DbContext instances for your logic.

Don't get me wrong, I'm an FK kind of guy and having FKs in your model can really simplify EF data operations and logic, but we don't technically need them in our models for EF to work

For EF to Not insert the duplicate Artwork instance in the artworkImageRepository with your current code (using an object reference), you would first need to Attach the reference of the newArtwork to the context. In this way internal object reference could potentially be identified and not be marked as an Inserted entity change state. This code example is not good advice, not by a long shot, so do not try to replicate it, but I hope it help explain this phenomenon:

public void Add(ArtworkImage artworkImage)
{
    context.Artworks.Attach(artworkImage.Artwork);
    context.ArtworkImages.Add(artworkImage);
    context.SaveChanges();
}

What your code demonstrates is a conflict between the Unit of Work pattern and Domain Repository Pattern. Your code represents a single Unit of work (UOW) but it is accessing multiple Domains individually. When you do this the different Domain Repositories do not even know about each other, so when we pass data between them each Repository cannot assume that you have made specific calls on the other repositories to ensure the data is in a specific state, the only reasonable state that can be assumed is that objects coming in to an Add() method, do not yet exist.

When we wrap EF's DbContext in a repository, we take it upon ourselves to manage object lifetimes and references. The repository pattern works really well if you need a state-less abstraction layer, or if your Repository represents a specific isolated unit of work. But as you have identified it introduces logical code practices that can prevent EF from doing what you might be expecting it to do.

The DbContext in EF represents a UOW itself, your original logic, executed against the DbContext directly would not have exhibited the behaviour that you have observed. It is important to understand some of these fundamental concepts to get the most out of your chosen architectural design patterns. This code against EF itself may have worked:

Artwork newArtwork = new Artwork();
ArtworkImage newArtworkImage = new ArtworkImage();
newArtwork.Identifier = identifierCode;
context.Artworks.Add(newArtwork);
newArtworkImage.ImageURL = fileName;
newArtworkImage.Artwork = newArtwork;
context.ArtworkImages.Add(newArtworkImage);
context.SaveChanges();

I would prefer this code to fail due to an ambiguous reference, I wouldn't add use this line at all context.ArtworkImages.Add(newArtworkImage); and instead rely on the object reference that is already established.

I say may have, because I haven't tested this explicitly and do not expect you to do so either, not unless you are ready to drop your Repository Pattern ;)

Some secondary points related to observations from your code:

In the Principal entity it is helpful to pre-initialize the collection for Dependent entities, this simplifies the code related to both entity graph initialization and deserialization. It is also a standard convention to use a pluralized name for the property that represents a collection an not a single reference:

public partial class Artwork
{
    [Key]
    public int ArtworkId { get; set; }
    public string Identifier { get; set; }
    public string Category { get; set; }
    public ICollection<ArtworkImage> ArtworkImages { get; set; }
        = new HashSet<ArtworkImage>();
}

This allows for simpler initialization syntax:

Artwork newArtwork = new Artwork();
newArtwork.Identifier = identifierCode;
newArtwork.ArtworkImages.Add(new ArtworkImage { ImageURL = fileName });
newArtwork.ArtworkImages.Add(new ArtworkImage { ImageURL = fileName2 });

You could also implement a less redundant naming convention and drop the Artwork from the navigation property name. Focus on the name of the relationship or the verb that describes the relationship instead of the type to make you code more natural to read:

Artwork newArtwork = new Artwork();
newArtwork.Identifier = identifierCode;
newArtwork.Images.Add(new ArtworkImage { ImageURL = fileName });
newArtwork.Images.Add(new ArtworkImage { ImageURL = fileName2 });

In Fluent Configuration you do not need to specify both ends of a relationship, doing so is supported but can lead to configuration issues if one end conflicts with it's reciprocal configuration. A strategy that can help is to have a convention that you follow for all configuration like only defining relationships from the dependent entity, not the principal or vice versa.
If you are using Attribute Notation (Code First Data Annotations) then it is not necessary to specify the relationship mapping in OnModelCreating at all. Use attribute notation to reduce the fluent configuration that you need to maintain.

I've tried a few things such as adding [ForeignKey("Artwork")] & [ForeignKey("ArtworkId")] as well as fiddling with my builder.

The key to attribute notation with ForeignKeyAttribute is that if you annotate the Foreign Key field, then the attribute is used to describe the Navigation Property. If it is placed on the Navigation Property, then it describes the Foreign Key. Either of these two configurations are valid:
```
[ForeignKey(nameof(ArtworkId))]
public virtual Artwork Artwork { get; set; }
public int ArtworkId { get; set; }
```
or this:
```
public virtual Artwork Artwork { get; set; }
[ForeignKey(nameof(Artwork))]
public int ArtworkId { get; set; }
```
Either will result in the same EF configuration and migration that you have displayed and mean your builder is kept clean:
```
protected override void OnModelCreating(DbModelBuilder modelBuilder)
{
    Database.SetInitializer<Artworkcontext>(null);
    base.OnModelCreating(modelBuilder);
}
```