PIMS-1284, PIMS-1227 Resolving AutoMapper issues #96
Conversation
Fosol
requested review from
asanchezr,
devinleighsmith and
jdtoombs
as code owners
| @@ -140,7 +140,7 @@ public IActionResult AddUser([FromBody] Model.UserModel model) | |||
|
|
|||
| var user = _mapper.Map<Model.UserModel>(addedEntity); | |||
|
|
|||
| return new CreatedAtActionResult(nameof(GetUser), nameof(UserController), new { id = user.Id }, user); | |||
| return CreatedAtAction(nameof(GetUser), new { id = user.Id }, user); | |||
| @@ -105,7 +105,7 @@ public IActionResult AddRole([FromBody] Model.RoleModel model) | |||
| _pimsAdminService.Role.Add(entity); | |||
| var role = _mapper.Map<Model.RoleModel>(entity); | |||
|
|
|||
| return new CreatedAtActionResult(nameof(GetRole), nameof(RoleController), new { id = role.Id }, role); | |||
| return CreatedAtAction(nameof(GetRole), new { id = role.Id }, role); | |||
| @@ -1,4 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8" ?> | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Organizing the solution for Visual Studio.
I've given up on vscode to work on the API. Way too many issues with it.
| @@ -3,25 +3,25 @@ | |||
| "windowsAuthentication": false, | |||
| "anonymousAuthentication": true, | |||
| "iisExpress": { | |||
| "applicationUrl": "http://localhost:53591/", | |||
| "applicationUrl": "http://localhost:5000/", | |||
There was a problem hiding this comment.
Launch in Visual Studio locally.
| @@ -13,6 +12,9 @@ public class AgencyProfile : Profile | |||
| #region Constructors | |||
| public AgencyProfile() | |||
| { | |||
| CreateMap<Entity.Agency, Entity.Agency>() | |||
There was a problem hiding this comment.
^ See? that's what happens when I have to review 200+ files. After the 20th my attention span drops exponentially LOL
Good catch Devin
There was a problem hiding this comment.
I agree, but I was testing how we could copy values from one entity to another with AutoMapper. I think I ended up not doing it. Probably can remove this.
There was a problem hiding this comment.
This was required for the Keycloak User management.
There was a problem hiding this comment.
Phew! this is a huge PR (200+ files to review)...
I'd like to see if we could try to keep changes to more manageable chunks to facilitate review. I also understand a lot of the changes were moving files around so I'll approve is as-is.
About giving up on VS Code and changing the config to rely on VS proper; I understand there might be some challenges but would have preferred that those changes were not bundled with the other 200+ files. If we ever need to revert that configuration then we have to manually "undo" it or revert the whole PR and it's associated 200+ files.
Also, if now VS proper is a hard requirements is there a documentation somewhere stating it or are we still able to use VS Code if we want to?
| namespace Pims.Api.Areas.Keycloak.Profiles.User.Resolvers | ||
| { | ||
| /// <summary> | ||
| /// UpdateRoleToEntityResolver class, provides automapper configuration to convert entiy agencies to model agencies. |
There was a problem hiding this comment.
should be converting entity roles to model roles I believe.
There was a problem hiding this comment.
This copies a Role Model into an Entity.
|
The majority of the effort (files) were models or profiles. Other than creating 20 PRs it didn't make a lot of sense to break them up. |
| } | ||
|
|
||
| /// <summary> | ||
| /// Detach the entity from the context. |
There was a problem hiding this comment.
might be worth noting that this only detaches the top-level entity.
| @@ -46,7 +34,9 @@ public virtual ET Find(params object[] keyValues) | |||
| { | |||
| this.User.ThrowIfNotAuthorized(Permissions.SystemAdmin, Permissions.AgencyAdmin); | |||
|
|
|||
| return this.Context.Set<ET>().Find(keyValues); | |||
| var result = this.Context.Set<ET>().Find(keyValues); | |||
| this.Context.Entry(result).State = Microsoft.EntityFrameworkCore.EntityState.Detached; // Force detach so that outside the DAL the DB cannot be manipulated. | |||
There was a problem hiding this comment.
I think this only detaches the top level entity.
There was a problem hiding this comment.
Good point. I should update the comment.
There was a problem hiding this comment.
I also experimented with manually parsing all entities in context and removing them one-by-one. This threw a number of weird errors for me, which I believe is because removing entities from context in this manner is not a supported workflow:
dotnet/efcore#18406 (comment)
Officially the way to remove all tracking is to dispose of the active dbcontext. This won't always work for us because we get our dbcontext injected into the controller action, so I'm not sure what the best way forward is.
There was a problem hiding this comment.
The best way would probably be not passing Entities outside of the DAL. But I don't want to do that atm.
In an effort to resolve AutoMapper challenges along side EF Core Concurrency issues, I have forced us to use unique models for each controller. The only shared models will be the
BaseModelandCodeModelfor now.I have tested a number of scenarios, but still need to do more testing to confirm nothing is broken. Please let me know if you discover anything?
This means all Models and AutoMapper Profiles will be found in their own namespace that matches the Controller.
All Controllers now only should return untracked entities. I still have to update a few scenarios.
Don't use the EF.Update(entity) or EF.Add(entity) methods unless you are extremely careful with what you are doing. Using these will result in either ignored concurrency, or they will edit tables they should not be touching.