I'm having problem saving a value of a one-to-many relationship to database. It keeps saying that GroupId is being tracked twice but I even have no tracking in the repository.
If you can help optimized the code in anyway I would greatly appreciate it but if not or can't that's fine too.
DepartmentController:
[HttpGet]
public async Task<IActionResult> Details (string id)
{
var department = await _repository.GetByIdAsync(id);
if (department != null)
{
var deparment = new EditDepartmentViewModel()
{
DepartmentId = department.DepartmentId,
DepartmentName = department.DepartmentName,
PeopleInDepartment = _repository.GetNamesByDepartment(id),
};
return View(deparment);
}
return RedirectToAction("Index");
}
[HttpPost]
public async Task<IActionResult> Details(EditDepartmentViewModel model)
{
model.DepartmentId = await _repository.GetIdByNameAsync(model.DepartmentName);
model.Persons = _repository.PersonsInDep(model.DepartmentId);
var userId = _contextAccessor.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
var user = await _userRepository.GetByIdAsync(userId);
var department = await _repository.GetByIdAsync(model.DepartmentId);
if (department != null)
{
department.DepartmentName = model.DepartmentName;
department.Persons = model.Persons;
if (!string.IsNullOrWhiteSpace(model.PeopleInDepartment))
{
var names = model.PeopleInDepartment.Split(new[] { ", " }, StringSplitOptions.RemoveEmptyEntries);
var alreadyin = _repository.GetNamesByDepartment(model.DepartmentId);
var alinasList = alreadyin.Split(new[] { ", " }, StringSplitOptions.RemoveEmptyEntries);
if (names.Count() >= alinasList.Count())
{
foreach (var name in names)
{
if (alreadyin.Contains(name) == false)
{
var person = new Person
{
PersonId = Guid.NewGuid().ToString(),
Name = name.Trim(),
DepartmentId = department.DepartmentId,
GroupPerDay = new List<PersonInGroupPerDay>(),
};
var dg = new DayGroup()
{
Id = Guid.NewGuid().ToString(),
Group = await _groupRepository.GetByDepartment(department),
};
var p = new PersonInGroupPerDay()
{
Id = Guid.NewGuid().ToString(),
Person = person,
DayGroup = dg,
Groups = new List<Group>(),
};
p.Groups.Add(dg.Group);
person.GroupPerDay.Add(p);
department.Persons.Add(person);
}
_repository.Update(department);
}
}
else
{
foreach(var name in alinasList)
{
if (model.PeopleInDepartment.Contains(name) == false)
{
var person = await _repository.GetByNameAsync(name);
if (person != null)
{
var ps = await _repository.GetByPerson(person);
foreach (var p in ps)
{
_repository.DeletePIGPD(p);
}
_repository.DeletePerson(person);
}
}
_repository.Update(department);
}
}
}
return RedirectToAction("Index");
}
return RedirectToAction("Index");
}
DepartmentRepository:
public bool Save()
{
var saved = _context.SaveChanges();
return saved > 0;
}
public bool Update(Department department)
{
_context.Update(department);
return Save();
}
public async Task<Department> GetByIdAsync(string id)
{
var dep = new Department();
dep.DepartmentId = id;
dep.DepartmentName = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.DepartmentName).FirstOrDefaultAsync();
dep.Persons = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(p => p.Persons).FirstOrDefaultAsync();
dep.Users = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Users).FirstOrDefaultAsync();
dep.DayDepartments = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.DayDepartments).FirstOrDefaultAsync();
dep.Group = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Group).FirstOrDefaultAsync();
dep.Days = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Days).FirstOrDefaultAsync();
dep.Months = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Months).FirstOrDefaultAsync();
return dep;
}
Department details:
@model QLAnCaRedo.Web.ViewModels.EditDepartmentViewModel
<div class="container mb-3">
<div class="row pt-4">
<div class="col-6">
<h2 class="text-primaary">
Edit phòng ban
</h2>
</div>
</div>
</div>
<form method="post" action="Details">
<div class="mb-3">
<label for="" class="form-label">Tên phòng ban</label>
<input type="text" class="form-control" asp-for="DepartmentName" />
</div>
<div class="mb-3">
<label for="" class="form-label">Người trong phòng ban</label>
<input type="text" class="form-control" asp-for="@Model.PeopleInDepartment" />
</div>
<button type="submit" class="btn btn-primary">Submit</button>
<a type="button" class="btn btn-danger" asp-controller="Department" asp-action="Delete" asp-route-id="@Model.DepartmentId">
Delete
</a>
Models
public class Department
{
[Key]
public string DepartmentId { get; set; }
public string DepartmentName { get; set; }
public List<Person> Persons { get; set; }
public Group Group { get; set; }
}
public class Group
{
[Key]
public string GroupId { get; set; }
[Required]
public AppUser AppUser { get; set; }
public Department Department { get; set; }
[Required]
public List<DayGroup> DayGroups { get; set; }
}
public class DayGroup
{
public string Id { get; set; }
public DateTime SubcriptionDate { get; set; }
public Group Group { get; set; }
public List<PersonInGroupPerDay> PersonInGroups { get; set; }
}
public class PersonInGroupPerDay
{
public string Id { get; set; }
public DayGroup DayGroup { get; set; }
public Person Person { get; set; }
[DefaultValue(0)]
public int Set1 { get; set; }
[DefaultValue(0)]
public int Set2 { get; set; }
[DefaultValue(0)]
public int Set3 { get; set; }
public List<Group> Groups { get; set; }
}
public class EditDepartmentViewModel
{
public string DepartmentId { get; set; }
public string DepartmentName { get; set; }
public string PeopleInDepartment { get; set; }
public List<Person> Persons { get; set; }
}
Ok, aside from the standard "Do not write a repository, EF's DbSet is already a repository" disclaimer, you are doing things very, very wrong and I am genuinely curious where you've come across anything like an example like this??
For a start, this code needs to go:
public async Task<Department> GetByIdAsync(string id)
{
var dep = new Department();
dep.DepartmentId = id;
dep.DepartmentName = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.DepartmentName).FirstOrDefaultAsync();
dep.Persons = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(p => p.Persons).FirstOrDefaultAsync();
dep.Users = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Users).FirstOrDefaultAsync();
dep.DayDepartments = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.DayDepartments).FirstOrDefaultAsync();
dep.Group = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Group).FirstOrDefaultAsync();
dep.Days = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Days).FirstOrDefaultAsync();
dep.Months = await _context.Departments.AsNoTracking().Where(d => d.DepartmentId == id).Select(d => d.Months).FirstOrDefaultAsync();
return dep;
}
This has many things wrong. Firstly it is making 7 calls to the database to populate 1 department. Secondly it will return a department with the provided ID and nothing else in the event that a department is not found.
This is what you should be returning:
public async Task<Department> GetByIdAsync(string id)
{
var department = await _context.Departments
.AsNoTracking()
.Single(d => d.DepartmentId == id);
return department;
}
If there are associated entities to include then eager load those by adding .Include(x => x.{Insert Related Navigation Property}). This fetches a detached entity, but if you are getting an entity with the intention to update it, then instead I would remove the .AsNoTracking() so the returned entity can be modified and the change tracker will build suitable SQL when SaveChanges is called on the DbContext. A LOT of confusion and complications will arise from trying to wrap the EF DbContext/DbSet functionality within a Repository class, so I would strongly recommend removing the repository and first get things working using the DbContext and Departments DbSet before trying to abstract anything.
If for any reason you want to exclude fields etc. when reading entities to be shuttled around then you can use a single .Select() to populate a new model to pass back. I do not recommend using the Department entity for this, if you have a Department DTO/ViewModel to populate:
public async Task<DepartmentViewModel> GetByIdAsync(string id)
{
var departmentVM = await _context.Departments
.Where(d => d.DepartmentId == id)
.Select(d => new DepartmentViewModel
{
DepartmentId = d.DepartmentId,
// .. populate remaining fields...
}).Single();
return departmentVM;
}
Do not partially populate a Department entity to serve as a view model or data transport DTO result. The issue with doing something like that is that methods expecting a Department entity should always expect a proper, tracked entity not risk getting some un-tracked, partially filled in placeholder.
To the crux of the error: EF is designed to work with references. When you create an entity that is related to other entities, there are two broad types of references. Associations and Children. Associations are links between this entity and other existing records in the database. Children are items that will essentially live or die based on their Parent. Something like "Groups" would most likely be an Associative relationship which means when you go to add or update a Department and associate it with a group, you need to ensure EF is working with a single reference to what it knows to be that existing record, not something it is going to treat as a new group.
So for instance, if you want to create a Department associated a Group record with an ID of 10, you do not do something like:
department.Group = new Group { GroupId = 10; }
or even:
var group = _context.Groups.AsNoTracking().Single(g => g.GroupId == 10);
department.Group = group;
Instead you use:
var group = _context.Groups.Single(g => g.GroupId == 10);
department.Group = group;
The key difference is that last example fetches a tracked reference to the group where AsNoTracking() would return an untracked new instance. EF will treat that as a "new" group if added to the Department, not an association to an existing row.
If you already have a group reference that may, or may not be tracked/attached then you need to check and replace that reference if the DbContext happens to be tracking that group already:
var trackedGroup = _context.Groups.Local.FirstOrDefault(g => g.GroupId = group.GroupId);
if (trackedGroup == null)
{
_context.Attach(group);
trackedGroup = group;
}
department.Group = trackedGroup;
This checks .Local which searches just the local tracking cache, it does not hit the database. If we do not find a tracked copy we will track the detached copy (group). This way the department is associated to a tracked reference.
Other items to fix would be to remove any code setting collection navigation properties. For instance:
public List<Person> Persons { get; set; }
Use:
public List<Person> Persons { get; protected set; } = new [];
Protect the setter, removing any code that tries to set it. The list of Persons will be initialized and ready to go an any new Department you create. Code should never call a setter on collection navigation properties as this will break change tracking in EF. If you want to replace a collection in an entity, it's not as simple as dumping the collection and recreating it, you need to figure out which items to remove vs. add so that the change tracker knows to delete rows that are to be removed and insert rows which are to be added. Setters should only be available on single object references such as public Group Group { get; set; }
Hopefully that helps get you started on tracking down issues with properly associating entities when inserting and updating data. There are probably a few more areas that will be causing problems, but give those changes a go and scale back the amount of code you are trying to test and test incrementally to make it easier to locate problems sooner.