JoshClose / CsvHelper

Library to help reading and writing CSV files
http://joshclose.github.io/CsvHelper/
Other
4.63k stars 1.05k forks source link

Unregister ClassMap does not clear the previous maps if the same class is used #2243

Open NicolaeSn opened 3 months ago

NicolaeSn commented 3 months ago

Description: When having a class map with different mappings depending on some property when the mapping changes registering the class again with the new mapping does not take affect even though in the object the mapping is reinitialized correctly.

Code example:

public class UserMapping  :  ClassMap<UserDto> {
 public UserMapping(bool isAdmin)  {
 if(isAdmin) {
 Map(u => u.Permission);
}
else {
Map(u => u.Permissopn).Constant(Permissions.Normal);
}
 }
}

//This part here is how it is used
csv.Context.UnregisterClassMap();
var isAdmin = csv.GetField<string>(nameof(UserDto.IsAdmin));
var classMap = new UserMapping(isAdmin); //here if isAdmin is true for first user then mapping is done correctly, but if the second row is not admin the mapping is initialized correctly but internally the first Map(u => u.Permission); is still used.
csv.Context.RegisterClassMap(classMap);
var record = csv.GetRecord<UserDto>();

Expected: Mapping is unregistered correctly.

What I've found to work as workaround: I thought initially that splitting the UserMapping in two and then deciding which map to use would fix the problem keep in mind that both class maps where for the same UserDto class but it was not the case, somehow it seems that once map is registered for a class it is reused all over again.

What worked was to have basically a new UserDTO2 which inherits UserDTO (basically an empty class) and use that for the second class map and it worked.

Note: Other then this problem which I think could improve the usage a lot, the library is great and worked for a pretty complex validation scenario.

JoshClose commented 3 months ago

This is because the delegate to create the objects is cached with the settings for the original map. I may need to clear those when a mapping for a type is removed, or even added a second time (updated).

For now you can have base class that does everything, and the 2 classes that don't do anything accept pass in their isAdmin true or false value.

public class BaseMapping : ClassMap<UserDto>
{
    public BaseMapping(bool isAdmin)
    {
        if (isAdmin)
        {
            Map(u => u.Permission);
        }
        else
        {
            Map(u => u.Permission).Constant(Permissions.Normal);
        }
    }
}

public class AdminMapping : BaseMapping
{
    public AdminMapping() : base(true) {}
}

public class UserMapping : BaseMapping
{
    public UserMapping() : base(false) {}
}
NicolaeSn commented 3 months ago

I've just tested this and it s not working, I think because behind the mapping is cached for the UserDto class which is still the same even though the registered class map is different, I've tried it with inheritance and without, basically just having two different classes like:

public class AdminMapping : ClassMap<UserDto>
{
    public AdminMapping() { //some mapping here }
}

public class UserMapping : ClassMap<UserDto>
{
    public UserMapping() {// different mapping here}
}

I guess for now the only solution that works is this:

public class AdminMapping : ClassMap<UserDto>
{
    public AdminMapping() { //some mapping here }
}

public class UserMapping : ClassMap<UserDtoFake>
{
    public UserMapping() {// different mapping here}
}

public class UserDtoFake : UserDto { }

But regarding of what workaround we can use, I think the cleanest way will be to clear the cache from behind.

This is not something impactful on our side even though we use this in production we can live with the way it works now so when you have time if it's not something big to do (from the way you've described it looks like it's not) a fix for this will be a great value.