How to handle encoded and decoded version of the same c# class

Question

Scenario

I am working on updating my .NET API to encode all database key fields so that the sequential key is not exposed to the end user. I'm using hashids.org for this and have built helper methods to quickly decode/encode properties in my automapper mapping. However, there are multiple versions of the API and only the most current version should be updated with this functionality, which means that I can't simply overwrite my existing classes. I've implemented a few solutions that work, but they all have a bad code smell that I'm hoping to clear up.

Solutions

I am currently performing the encoding at the controller layer. I can see the merits of doing this at the data access layer as well, but feel there is more risk of leaks/missed conversions at that layer, especially since the API has many different data sources. Plus, hiding keys is an issue with the outside world, for which the controller is the gatekeeper, so it feels appropriate there.

The application currently has the following model pattern, which cannot be changed: Model (model that exists in DB) > ValueObject (service model, VO) > DTO (API model).

(1) Initial attempt

Below is an example of a class that needs to support an encoded and decoded state, where Utils.Encode() and Utils.Decode() are helper methods that will convert the field between int and string using Hashids.

//EquipmentDTO.cs
public class EquipmentDTO //encoded class
{
  public string Id {get; set;}
  public string Name {get; set;}
}

public class EquipmentUnencodedDTO //decoded class
{
  public int Id {get; set;}
  public string Name {get; set;}
}

//Automapper.cs
CreateMap<EquipmentUnencodedDTO, EquipmentDTO>()
  .ForMember(dst => dst.Id, opt => opt.MapFrom(src => Utils.Encode(src.Id)));

CreateMap<EquipmentDTO, EquipmentUnencodedDTO>()
  .ForMember(dst => dst.Id, opt => opt.MapFrom(src => Utils.Decode(src.Id)));

CreateMap<EquipmentVO, EquipmentDTO>() //mapping from service model to controller model
  .ForMember(dst => dst.Id, opt => opt.MapFrom(src => Utils.Encode(src.Id)));
CreateMap<EquipmentDTO, EquipmentVO>()
  .ForMember(dst => dst.Id, opt => opt.MapFrom(src => Utils.Decode(src.Id)));

CreateMap<Equipment, EquipmentVO>() //mapping from DB model to service model
  .ForMember(dst => dst.Id, opt => opt.MapFrom(src => src.Id));

• I chose to make the existing EquipmentDTO the encoded version because I want this to become the new standard, which would eventually lead to the deprecation and removal of EquipmentUnencodedDTO as the old controllers eventually get updated.
• I chose to not copy CreateMap<EquipmentVO, EquipmentDTO> for CreateMap<EquipmentVO, EquipmentUnencodedDTO> (and the reverse) because it would lead to a lot of duplication in the AutoMapper file, which is already huge (though maybe this isn't a real problem?)
• I do not like this solution because in my old controllers, the mapping is now confusing. In a POST, for example, the unencoded input DTO has to be converted to the service model via: Mapper.Map<EquipmentVO>(Mapper.Map<EquipmentDTO>(unencodedEquipmentInput)) which is super ugly.
  • That being said, this is supposedly a temporary problem, so is this a real problem?
  • This problem would go away if I created CreateMap<EquipmentVO, EquipmentUnencodedDTO>
• I do not like this solution because my classes have a lot of duplicated fields that are not changing between the encoded and decoded versions

(2) Second Attempt

The two bullet points above led me to refactor to this:

public class EquipmentDTO
{
  public string Id {get; set;}
  public string Name {get; set;}
  public Decoded Decode(){
    return Mapper.Map<Decoded>(this);
  }
  public class Decoded: EquipmentDTO {
    public new int Id {get; set;}
    public EquipmentDTO Encode(){
      return Mapper.Map<EquipmentDTO>(this);
    }
  }
}

// Automappers are the same, except EquipmentUnencodedDTO is now EquipmentDTO.Decoded 

• I like how simple it is to switch between encoded and decoded states now, reducing my double mapping above to: Mapper.Map<EquipmentVO>(unencodedEquipmentInput.Encode());
• I like the nested class because it codifies the relationship between the two classes and also does a better job at identifying which fields get encoded/decoded
• I think this smells a lot worse

(3) Next Attempt

My next attempt was to add in the missing mappings for the decoded class to the service model and to undo the changes from attempt #2. This created a ton of duplicated mapping code, I'm still stuck with duplicated properties in both classes without a clear indication to which fields get decoded/encoded, and it all feels much more cumbersome than necessary.

Thanks for any advice!

Answer 1

After a lot of playing around, I ended up going the route of not using automapper and only having a single DTO for both the encoded/unencoded states by using custom getters/setters to control what value would be returned based on a readonly property isEncoded.

My problem with automapper and having multiple DTOs was that there was too much duplication and way too much code to write to add a new decodable DTO. Also, there were too many ways to break the relationship between encodedDTO and unencodedDTO, especially since there are other developers on the team (not to mention future hires) who could forget to create the encoded DTO or to create a mapping to properly encode or decode the ID values.

While I still have separate util methods to perform the encoding of a value, I moved all of the automapper "logic" into a base class EncodableDTO, which would allow a user to run Decode() or Encode() on a DTO to toggle its encoded state, including the encoded state for all of its encodable properties via reflection. Having a DTO inherit EncodableDTO also serves as a clear indicator to developers to what's going on, while custom getters/setters clearly indicate what I'm trying to do for specific fields.

Here's a sample:

public class EquipmentDTO: EncodableDTO
{
  private int id;
  public string Id {
    get
    {
      return GetIdValue(id);
    }
    set
    {
      id = SetIdValue(value);
    }
  }

  public List<PartDTO> Parts {get; set;}

  public string Name {get; set;}
}

public class PartDTO: EncodableDTO
{
  private int id;
  public string Id {
    get
    {
      return GetIdValue(id);
    }
    set
    {
      id = SetIdValue(value);
    }
  }

  public string Name {get; set;}
}

public class EncodableDTO
{
    public EncodableDTO()
    {
        // encode models by default
        isEncoded = true;
    }

    public bool isEncoded { get; private set; }

    public void Decode()
    {
        isEncoded = false;
        RunEncodableMethodOnProperties(MethodBase.GetCurrentMethod().Name);
    }

    public void Encode()
    {
        isEncoded = true;
        RunEncodableMethodOnProperties(MethodBase.GetCurrentMethod().Name);
    }

    protected string GetIdValue(int id)
    {
        return isEncoded ? Utils.EncodeParam(id) : id.ToString();
    }

    // TryParseInt() is a custom string extension method that does an int.TryParse and outputs the parameter if the string is not an int
    protected int SetIdValue(string id)
    {
        // check to see if the input is an encoded value, otherwise try to parse it.
        // the added logic to test if the 'id' is an encoded value allows the inheriting DTO to be received both in
        // unencoded and encoded forms (unencoded/encoded http request) and still populate the correct numerical value for the ID
        return id.TryParseInt(-1) == -1 ? Utils.DecodeParam(id) : id.TryParseInt(-1);
    }

    private void RunEncodableMethodOnProperties(string methodName)
    {
        var self = this;
        var selfType = self.GetType();
        // Loop through properties and check to see if any of them should be encoded/decoded
        foreach (PropertyInfo property in selfType.GetProperties())
        {
            var test = property;
            // if the property is a list, check the children to see if they are decodable
            if (property is IList || (
                    property.PropertyType.IsGenericType
                    && (property.PropertyType.GetGenericTypeDefinition() == typeof(List<>)
                    || property.PropertyType.GetGenericTypeDefinition() == typeof(IList<>))
                    )
                )
            {
                var propertyInstance = (IList)property.GetValue(self);
                if (propertyInstance == null || propertyInstance.Count == 0)
                {
                    continue;
                }
                foreach (object childInstance in propertyInstance)
                {
                    CheckIfObjectEncodable(childInstance, methodName);
                }
                continue;
            }

            CheckIfObjectEncodable(property.GetValue(self), methodName);
        }
    }

    private void CheckIfObjectEncodable(object instance, string methodName)
    {
        if (instance != null && instance.GetType().BaseType == typeof(EncodableDTO))
        {
            // child instance is encodable. Run the same decode/encode method we're running now on the child
            var method = instance.GetType().GetMethod(methodName);
            method.Invoke(instance, new object[] { });
        }
    }
}

An alternative to RunEncodableMethodOnProperties() was the explicitly decode/encode child properties in the inheriting class:

public class EquipmentDTO: EncodableDTO
{
  private int id;
  public string Id {
    get
    {
      return GetIdValue(id);
    }
    set
    {
      id = SetIdValue(value);
    }
  }

  public List<PartDTO> Parts {get; set;}

  public string Name {get; set;}

  public new void Decode() {
    base.Decode();
    // explicitly decode child properties
    Parts.ForEach(p => p.Decode());
  }
}

I chose not to do the above because it created more work for DTO creators to have to remember to explicitly add (1) the override method, and (2) any new decodable properties to the override method. That being said, I'm sure I'm taking some sort of a performance hit by looping through every class of my class' properties and its children, so in time I may have to migrate towards this solution instead.

Regardless of the method I chose to decode/encode properties, here was the end result in the controllers:

// Sample controller method that does not support encoded output
[HttpPost]
public async Task<IHttpActionResult> AddEquipment([FromBody] EquipmentDTO equipment)
{
    // EquipmentDTO is 'isEncoded=true' by default
    equipment.Decode();
    // send automapper the interger IDs (stored in a string)
    var serviceModel = Mapper.Map<EquipmentVO>(equipment);
    var addedServiceModel = myService.AddEquipment(serviceModel);
    var resultValue = Mapper.Map<EquipmentDTO>(addedServiceModel);
    resultValue.Decode();
    return Created("", resultValue);
}


// automapper
CreateMap<EquipmentVO, EquipmentDTO>().ReverseMap();
CreateMap<Equipment, EquipmentVO>();

While I don't think its the cleanest solution, it hides a lot of the necessary logic to make encoding/decoding work with the least amount of work for future developers