Add() method adding duplicate rows for linked models in Code-First Entity Framework

Question

Following is the action that is adding a Loan request to the database:

[HttpPost]
public ActionResult Add(Models.ViewModels.Loans.LoanEditorViewModel loanEditorViewModel)
{
    if (!ModelState.IsValid)
        return View(loanEditorViewModel);

    var loanViewModel = loanEditorViewModel.LoanViewModel;

    loanViewModel.LoanProduct = LoanProductService.GetLoanProductById(loanViewModel.LoanProductId); // <-- don't want to add to this table in database
    loanViewModel.Borrower = BorrowerService.GetBorrowerById(loanViewModel.BorrowerId); //<-- don't want to add to this table in database

    Models.Loans.Loan loan = AutoMapper.Mapper.Map<Models.Loans.Loan>(loanEditorViewModel.LoanViewModel);
    loanService.AddNewLoan(loan);
    return RedirectToAction("Index");
}

Following is the AddNewLoan() method:

public int AddNewLoan(Models.Loans.Loan loan)
{
    loan.LoanStatus = Models.Loans.LoanStatus.PENDING;
    _LoanService.Insert(loan);

    return 0;
}

And here is the code for Insert()

public virtual void Insert(TEntity entity)
{
    if (entity == null)
        throw new ArgumentNullException(nameof(entity));

    try
    {
        entity.DateCreated = entity.DateUpdated = DateTime.Now;
        entity.CreatedBy = entity.UpdatedBy = GetCurrentUser();

        Entities.Add(entity);
        context.SaveChanges();
    }
    catch (DbUpdateException exception)
    {
        throw new Exception(GetFullErrorTextAndRollbackEntityChanges(exception), exception);
    }
}

It is adding one row successfully in Loans table but it is also adding rows to LoanProduct and Borrower table as I showed in first code comments.

I checked the possibility of multiple calls to this action and Insert method but they are called once.

UPDATE

I am facing similar problem but opposite in functioning problem here: Entity not updating using Code-First approach

I think these two have same reason of Change Tracking. But one is adding other is not updating.

Answer 1

The following code seems a bit odd:

var loanViewModel = loanEditorViewModel.LoanViewModel;

loanViewModel.LoanProduct = LoanProductService.GetLoanProductById(loanViewModel.LoanProductId); // <-- don't want to add to this table in database
loanViewModel.Borrower = BorrowerService.GetBorrowerById(loanViewModel.BorrowerId); //<-- don't want to add to this table in database

Models.Loans.Loan loan = AutoMapper.Mapper.Map<Models.Loans.Loan>(loanEditorViewModel.LoanViewModel);

You are setting entity references on the view model, then calling automapper. ViewModels should not hold entity references, and automapper should effectively be ignoring any referenced entities and only map the entity structure being created. Automapper will be creating new instances based on the data being passed in.

Instead, something like this should work as expected:

// Assuming these will throw if not found? Otherwise assert that these were returned.
var loanProduct = LoanProductService.GetLoanProductById(loanViewModel.LoanProductId);
var borrower = BorrowerService.GetBorrowerById(loanViewModel.BorrowerId);

Models.Loans.Loan loan = AutoMapper.Mapper.Map<Models.Loans.Loan>(loanEditorViewModel.LoanViewModel);
loan.LoanProduct = loanProduct;
loan.Borrower = borrower;

Edit:

The next thing to check is that your Services are using the exact same DbContext reference. Are you using Dependency Injection with an IoC container such as Autofac or Unity? If so, make sure that the DbContext is set registered as Instance Per Request or similar lifetime scope. If the Services effectively new up a new DbContext then the LoanService DbContext will not know about the instances of the Product and Borrower that were fetched by another service's DbContext.

If you are not using a DI library, then you should consider adding one. Otherwise you will need to update your services to accept a single DbContext with each call or leverage a Unit of Work pattern such as Mehdime's DbContextScope to facilitate the services resolving their DbContext from the Unit of Work.

For example to ensure the same DbContext:

using (var context = new MyDbContext())
{
    var loanProduct = LoanProductService.GetLoanProductById(context, loanViewModel.LoanProductId);
    var borrower = BorrowerService.GetBorrowerById(context, loanViewModel.BorrowerId);

    Models.Loans.Loan loan = AutoMapper.Mapper.Map<Models.Loans.Loan>(loanEditorViewModel.LoanViewModel);
    loan.LoanProduct = loanProduct;
    loan.Borrower = borrower;

    LoanService.AddNewLoan(context, loan);
}    

If you are sure that the services are all provided the same DbContext instance, then there may be something odd happening in your Entities.Add() method. Honestly your solution looks to have far too much abstraction around something as simple as a CRUD create and association operation. This looks like a case of premature code optimization for DRY without starting with the simplest solution. The code can more simply just scope a DbContext, fetch the applicable entities, create the new instance, associate, add to the DbSet, and SaveChanges. There's no benefit to abstracting out calls for rudimentary operations such as fetching a reference by ID.

public ActionResult Add(Models.ViewModels.Loans.LoanEditorViewModel loanEditorViewModel)
{
    if (!ModelState.IsValid)
        return View(loanEditorViewModel);

    var loanViewModel = loanEditorViewModel.LoanViewModel;
    using (var context = new AppContext())
    {
       var loanProduct = context.LoanProducts.Single(x => x.LoanProductId == 
loanViewModel.LoanProductId);
       var borrower = context.Borrowers.Single(x => x.BorrowerId == loanViewModel.BorrowerId);
       var loan = AutoMapper.Mapper.Map<Loan>(loanEditorViewModel.LoanViewModel);
       loan.LoanProduct = loanProduct;
       loan.Borrower = borrower;
       context.SaveChanges();
    }
    return RedirectToAction("Index");
}

Sprinkle with some exception handling and it's done and dusted. No layered service abstractions. From there you can aim to make the action test-able by using an IoC container like Autofac to manage the Context and/or introducing a repository/service layer /w UoW pattern. The above would serve as a minimum viable solution for the action. Any abstraction etc. should be applied afterwards. Sketch out with pencil before cracking out the oils. :)

Using Mehdime's DbContextScope it would look like:

public ActionResult Add(Models.ViewModels.Loans.LoanEditorViewModel loanEditorViewModel)
{
    if (!ModelState.IsValid)
        return View(loanEditorViewModel);

    var loanViewModel = loanEditorViewModel.LoanViewModel;
    using (var contextScope = ContextScopeFactory.Create())
    {
       var loanProduct = LoanRepository.GetLoanProductById( loanViewModel.LoanProductId).Single();
       var borrower = LoanRepository.GetBorrowerById(loanViewModel.BorrowerId);
       var loan = LoanRepository.CreateLoan(loanViewModel, loanProduct, borrower).Single();
       contextScope.SaveChanges();
    }
    return RedirectToAction("Index");
}

In my case I leverage a repository pattern that uses the DbContextScopeLocator to resolve it's ContextScope to get a DbContext. The Repo manages fetching data and ensuring that the creation of entities are given all required data necessary to create a complete and valid entity. I opt for a repository-per-controller rather than something like a generic pattern or repository/service per entity because IMO this better manages the Single Responsibility Principle given the code only has one reason to change (It serves the controller, not shared between many controllers with potentially different concerns). Unit tests can mock out the repository to serve expected data state. Repo get methods return IQueryable so that the consumer logic can determine how it wants to consume the data.

Answer 2

Finally with the help of the link shared by @GertArnold Duplicate DataType is being created on every Product Creation

Since all my models inherit a BaseModel class, I modified my Insert method like this:

public virtual void Insert(TEntity entity, params BaseModel[] unchangedModels)
{
    if (entity == null)
        throw new ArgumentNullException(nameof(entity));

    try
    {
        entity.DateCreated = entity.DateUpdated = DateTime.Now;
        entity.CreatedBy = entity.UpdatedBy = GetCurrentUser();

        Entities.Add(entity);

        if (unchangedModels != null)
        {
            foreach (var model in unchangedModels)
            {
                _context.Entry(model).State = EntityState.Unchanged;
            }
        }

        _context.SaveChanges();
    }
    catch (DbUpdateException exception)
    {
        throw new Exception(GetFullErrorTextAndRollbackEntityChanges(exception), exception);
    }
}

And called it like this:

_LoanService.Insert(loan, loan.LoanProduct, loan.Borrower);

Answer 3

By far the simplest way to tackle this is to add the two primitive foreign key properties to the Loan class, i.e. LoanProductId and BorrowerId. For example like this (I obviously have to guess the types of LoanProduct and Borrower):

public int LoanProductId { get; set; }
[ForeignKey("LoanProductId")]
public Product LoanProduct { get; set; }

public int BorrowerId { get; set; }
[ForeignKey("BorrowerId")]
public User Borrower { get; set; }

Without the primitive FK properties you have so-called independent associations that can only be set by assigning objects of which the state must be managed carefully. Adding the FK properties turns it into foreign key associations that are must easier to set. AutoMapper will simply set these properties when the names match and you're done.