Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supply additional parameters to the mapping method #103

Open
cyril265 opened this issue Jul 18, 2022 · 45 comments
Open

Supply additional parameters to the mapping method #103

cyril265 opened this issue Jul 18, 2022 · 45 comments
Labels
enhancement New feature or request

Comments

@cyril265
Copy link

cyril265 commented Jul 18, 2022

Sometimes you want to provide an external value to the mapping method. For example your DTO doesn't have a tenant id. You are getting the tenant id from the currently signed in user and it's the same for every entity inside the current request.

[Mapper]
public partial class CarMapper
{
    public partial Car Map(CarDto dto, string tenantId);
}

public record Car(string Name, string TenantId);

public record CarDto(string Name);

Example usage:

    public void UseMapper(CarMapper mapper, IEnumerable<CarDto> dtos)
    {
        string tenantId = ""; //GetFromDb;
        var entities = dtos.Select(dto => mapper.Map(dto, tenantId));
    }

You could accomplish this by moving TenantId to a property and using the after map feature. But I think this solution is cleaner. It doesn't force you to change your domain code for a mapper, it shows the intent more clearly and it's simpler to add.

@latonz
Copy link
Contributor

latonz commented Jul 19, 2022

I like this idea. However, it is not that easy to implement. It may take some time until Mapperly supports this.

@latonz latonz added the enhancement New feature or request label Jul 19, 2022
@Fube
Copy link

Fube commented Nov 18, 2022

@latonz
Do you have an architectural approach in mind for this?

Would you rework TypeMapping itself to include more than one ITypeSymbol for the source or would you build another type on top of it, something like a MultiSourceTypeMapping which would hold multiple TypeMappings that point to the same target type.

I would love to contribute, but don't want to go against the vision you might have for this 🙂

@latonz
Copy link
Contributor

latonz commented Dec 12, 2022

@Fube Sorry, I missed the notification of your comment.
I didn't have the time to think about a concrete architecture yet. Are all arguments considered as mapping sources? Or are the additional arguments only sources for properties with the same name? On a first glance I think if all arguments are considered as mapping sources, the TypeMapping needs to be refactored. Otherwise it seems to be limited to the PropertyMapping and it's related classes. Tbd is also how to handle it, if another mapping depends on a user defined mapping with additional arguments.

Feel free to submit an architectural proposal to this issue.

@DerMave
Copy link
Contributor

DerMave commented Dec 25, 2022

I would like to work on this feature as I have an usecase for it. Have you begun working on this? @latonz @Fube

@Fube
Copy link

Fube commented Dec 25, 2022

@DerMave
Unfortunately not, but feel free to takeover.
I investigated the issue, but did not have time to come up with a solution.

As far as I can tell, the TypeMapping has to be changed to account for more than one source.

@latonz
Copy link
Contributor

latonz commented Jan 2, 2023

@DerMave I didn't have time yet to work on this. Let me know if you start working on this, then I'd assign the issue to you.
However, before you start with an actual implementation, I think it would be useful to discuss a design proposal on how this feature should work exactly and which parts of Mapperly need adjustments.

@akoken
Copy link

akoken commented Jan 7, 2023

+1 for this feature. I think this is very common use case. As @cyril265 mentioned above, we also obtain tenantId, userId etc. from HttpContext.

@DerMave
Copy link
Contributor

DerMave commented Jan 10, 2023

@latonz I haven't really gotten around to it yet. I made some unit tests and looked at the relevant bits and pieces. It seems I have underestimated the scope of this change. I am still interested in going forward with it, but I have to find some free time.

@Fube
Copy link

Fube commented Mar 18, 2023

Maybe we can use the flattening / unflattening logic to implement this
We could make an intermediary type to nest the arguments and then flatten it into the target type

Though it is a bit hacky, if it is possible, it would be the simplest way to implement this

Otherwise, we're looking at a change in IMapping which would propagate throughout the rest of the code

Not sure if my suggestion makes sense though, @latonz is it plausible?

@latonz
Copy link
Contributor

latonz commented Mar 29, 2023

@Fube I don't think flattening/unflattening would be the right approach for this, it could work as a workaround though.
IMO the way to go would either be providing parameters directly to the mapping methods which are then mapped by name to properties of the target object or to add a secondary source objects which are also considered as mapping sources.

@Herdo
Copy link

Herdo commented May 19, 2023

This is currently the blocking issue for me to switch from AutoMapper to Mapperly.

I'm receiving requests that are mapped to an internal command and require additional properties from the HttpContext.

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Jun 22, 2023

I have a working POC for this feature, I've opted to change IMapping and add MethodParameter[] Parameters { get; } (probably shouldn't be using MethodParameter)

I have some questions/concerns about breaking future features.

  • How should method resolving work?

    • For void method mappings should the target member always be the second non type/IRefernceHandler parameter
    • Derived type mapping uses Type as a discriminator, unlike IReferenceHandler it doesn't use an attribute label. Could this cause issues while parsing the method?
    • Might cause future issues with Add support for destination value in user implemented mapping methods #276
  • Where are additional parameter mappings allowed?

    • Stay in the first method scope
    • Pass all the parameters to all child methods
    • Used in methods when needed (harder but possible)
  • Can IQueryable have parameters?

  • Should multi param user implemented mapping be allowed. How should the resolution be handled? ie match by type and name.

    • When trying to use a user mapping should Mapperly attempt to resolve the parameters by finding matching members? ie ToDogDto(src.Dog, src.DogOwner, id)
  • I'm currently using the name of the variable to determine if it can map to a member. This feels natural but is a bit of an anti pattern, plus Mapperly doesn't do this for the source type.

@Herdo
Copy link

Herdo commented Jun 23, 2023

@TimothyMakkison I think most of the listed questions derive from the fact that you approach it in an implicit way.

Wouldn't most of these questions vanish if we followed an explicit approach by introducing an additional attribute:

[MapAdditionalValueToProperty("id", "ChipId")]
public partial DogDto MapDogToDto(Dog dog, Guid id);

Attribute name could be whatever, just some thoughts.

  • MapAddition
  • MapExtra
  • MapExternal
  • ...

@TimothyMakkison
Copy link
Collaborator

@Herdo you're right most of my problems are self inflicted 😄
I almost mentioned an explicit attribute but decided to try implementing implicit parameters first. It feels much more idiomatic.

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Jul 8, 2023

I'm torn between two ideas for this feature and unsure which one would be better:

Top Level

  • Additional parameters are only used on the immediate mapping, generated NewInstanceObjectMemberMapping will not pass parameters down.
// this works
public partial DogDto ToDogDto(Dog src, int id)
{
    return new DogDto(src.Name, id);
}

// will not pass parameter down
public partial CatOwnerDto ToCatOwnerDto(CatOwner src, int id)
{
    var target = new CatOwnerDto()
    {
        // id will not be passed
        Cat = MapToCatDto(src.Cat);
    }
    return target;
}
  • If a user defined or user implemented method is found with similar parameters it will be used.
// User defined mapping (can also be user implemented)
public partial CatDto ToCatDto(Cat src, int id) ...
// public CatDto ToCatDto(Cat src, int id) ...

public partial CatOwnerDto ToCatOwnerDto(CatOwner src, int id)
{
    var target = new CatOwnerDto()
    {
        // id will be passed
        Cat = ToCatDto(src.Cat, id);
    }
    return target;
}

Pros

  • Faster to generate
  • Simple
  • Top level is similar to other mapperly features

Cons

  • Cumbersome
    • Using an extra parameter for a nested property forces you to add a user defined mappings for each step
      • Could use [MapProperty] (won't work with init/required)

Nested

  • Additional parameters can be used for all levels of the mapping.
    • The mapper is smart enough to determine if a new method needs an extra parameter, even if a child of the new method uses it. (Its also recursion safe)
  • Methods created by the mapping that use extra parameters are scoped to the current user defined mapping to avoid resolving/config issues.
  • Can use other user implemented or user defined methods.
public partial CatOwnerDto ToCatOwnerDto(CatOwner src, int id)
{
    var target = new CatOwnerDto()
    {
        // below method was created by Mapperly, no user defined method required
        // if the method doesn't need `id`, then it won't be passed
        Cat = MapToCatDto(src.Cat, id);
    }
    return target;
}

Pros

  • Easier to use
  • In the future logic can be adapted to support nested ignores and nested init/required mapping and more.
  • Works with nested init/required

Cons

  • Slower generation
  • Potentially confusing
  • Will likely require nested mapping and nested ignores to be added

I have a working POC for the nested logic mapper see #550. It can be adapted to do top level mappings

@latonz
Copy link
Contributor

latonz commented Jul 12, 2023

I'm busy right now and I'm afraid I won't find time in the upcoming days to look into this 😞, but I'll look into it in the upcoming weeks. In the meantime I'd like to focus on getting v2.9.0 released 😊

@MisinformedDNA
Copy link

Ideally, this would work without any additional attributes or modifications:

public record Person(string FirstName, string LastName);
public record Employee(string FirstName, string LastName, string EmployeeId);

public static partial Employee ToEmployee(this Person person, string EmployeeId);

Workaround

public record Employee(string FirstName, string LastName, string EmployeeId)
{
    public string EmployeeId { get; init; } = default!;
}

public static Employee ToEmployee(this Person person, string employeeId)
{
    return person.ToEmployee() with { EmployeeId = EmployeeId };
}

[MapperIgnoreSource(nameof(Employee.EmployeeId))]
private static partial Employee ToEmployee(this Person person);

@Tvde1
Copy link

Tvde1 commented Sep 7, 2023

This sounds like a terrible idea to do using AutoMapper or Mapperly. Writing a few manual mappings for complex situations seems like more maintainable for the codebase and for this library.

@MisinformedDNA
Copy link

This sounds like a terrible idea to do using AutoMapper or Mapperly. Writing a few manual mappings for complex situations seems like more maintainable for the codebase and for this library.

That's the same argument against Mapperly and AutoMapper in general. Whether it is terrible is subjective and developers can choose to use it or not.

@darlov
Copy link

darlov commented Nov 14, 2023

From my perspective every parameters should have the attribute to identify the destination property for assign to. It's will avoid mistakes when properties will renamed and it's add ability to rename all mappings without manual work for rename every parameters.
Look's like this:

[Mapper]
public partial class CarMapper
{
    public partial Car Map(CarDto dto, [MapTo(nameof(Car.TenantId))] string tenantId);
}

[AttributeUsage(AttributeTargets.Parameter)]
public sealed class MapTo : Attribute
{
  public MapTo(string name)
  {
    Name = name;
  }
  
  public string Name { get; }
}

@fizmhd
Copy link

fizmhd commented Mar 14, 2024

little late to the discussion, it would be nice to have this feature,
any update on this one ?

@latonz
Copy link
Contributor

latonz commented Mar 15, 2024

No updates, feel free to contribute.

I think before an implementation, several things need to be thought of and decided:

  • Are mapping methods with additional parameters still resolved by Mapperly in transitive mappings?
  • Are only explicitly mapped parameters allowed (MapTo (Supply additional parameters to the mapping method #103 (comment)) approach)?
  • If not only explicitly mapped parameters are allowed, are the parameters handled the same as the source object (eg. only properties of the additional parameters are considered) or are they handled the same as a source object property?

I think handling these mappings the same as the generic mappings (they are never called by Mapperly generated code) with an explicitly mapped parameter (MapTo) approach would be the best for now. The added complexity is manageable while still addressing most of the use-cases. This would only allow additional parameters to be mapped to the target object directly.

@ChaseFlorell
Copy link

ChaseFlorell commented Mar 25, 2024

As far as (2) is concerned, what happens if the target object only has a ctor parameter and no public facing property? I feel as though we can have the opinion within our codebase without forcing the opinion globally. I for one only use ctor mapping and forcing the MapTo would give me heartburn.

I need the ability to do something akin to this.

[Mapper]
public partial class CarMapper
{
    public partial Car ToCar(CarDto car, ICarContext carContext);
}

public record CarDto(int Wheels);

public class Car
{
    public Car(int wheels, ICarContext carContext)
    {
        Wheels = wheels;
        _carContext = carContext;
    }
    public int Wheels { get; }
    private readonly ICarContext _carContext;
}

@latonz
Copy link
Contributor

latonz commented Mar 25, 2024

You could simply provide the name of the ctor parameter to MapTo.

@ChaseFlorell
Copy link

ChaseFlorell commented Mar 25, 2024

You could simply provide the name of the ctor parameter to MapTo.

doesn't that defeat the nameof and the rationale given by @darlov around renaming properties?

I guess I'm curious as to whether or not there is a technical reason that this would be required, or if it is simply an opinion?

@latonz
Copy link
Contributor

latonz commented Mar 25, 2024

Nameof: yes it does
There is no real technical reason in Mapperly to require this, it just clearifies things on the user side. Otherwise the question would arise whether a parameter object is considered the same as a property on the source object or as an other source object itself.

@ChaseFlorell
Copy link

I think the thing I love about this library is that we get compile time errors when mappings fail. It makes it crystal clear to the developer why a mapping is no bueno. Don't get me wrong, I love the option to add a [MapTo] attribute for additional flexibility, I'm only cautioning against forcing it.

@latonz
Copy link
Contributor

latonz commented Mar 25, 2024

@ChaseFlorell which option would you prefer and why? Matching the the additional parameter itself to a target property or matching the additional parameter's properties to target properties?

@ChaseFlorell
Copy link

ChaseFlorell commented Mar 25, 2024

I'm not sure I follow the question.
I want to match additional mapping parameters to target constructor parameters because there are no target properties to match. I understand that there are scenarios where there are properties, but my use case doesn't have it.

I am new to the world of source generation so I'm ignorant to some of it. Can you match to types and/or case insensitive name?

public partial Car ToCar(CarDto car, ICarContext carContext);
// matches
public Car ( /*car properties*/, ICarContext carContext) // matches by name

or

public partial Car ToCar(CarDto car, ICarContext carContext);
// matches
public Car ( /*car properties*/, ICarContext ctx) // matches by type

@ChaseFlorell
Copy link

ChaseFlorell commented Mar 26, 2024

@ChaseFlorell which option would you prefer and why? Matching the the additional parameter itself to a target property or matching the additional parameter's properties to target properties?

Oh wait, I think I understand the question now.

I think the first parameter should be the mapped object source => target. This is where any mapping logic lives and functions exactly the way it does today.

I think any additional parameters should always match the type on the target. In other words, I don't think additional parameters in a mapping method should even consider additional mapping, it should always be a 1:1 type with the target. I say this because if any additional items need mapping, they should be done as follows

a) as a part of the source => target mapping OR
b) can be done before the mapping call and an already mapped object can be passed in as a second parameter

public class Car
{
  public Car(int wheels, Driver driver)
  {
      Wheels = wheels;
      Driver = driver;
  }
  public int Wheels { get; }
  public Driver Driver { get; }
}

// use //

Driver driver = Mapper.MapTo(driverDto);
Car car = Mapper.MapTo(carDto, driver);

Overall, I think any additional parameters will almost always be contextual data that falls outside the scope of mapping.

@cremor
Copy link

cremor commented Mar 26, 2024

What if matching by type is not enough? For example, would the following work?

public class Car
{
    // Not shown: Properties which are available in CarDto

    // Two additional properties:
    public int SomeValue { get; set; }
    public int SomeOtherValue { get; set; }
}

public static partial class CarMapper
{
    public static partial Car Map(CarDto dto, int someValue, int someOtherValue);
}

@ChaseFlorell
Copy link

ChaseFlorell commented Mar 26, 2024

What if matching by type is not enough? For example, would the following work?

I think that's where matching by name would come in. If it were me, I think I'd go something like this

  1. Try matching by [MapTo]
  2. Try matching by type (faster than name because we don't have to worry about case)
  3. Try matching by name
  4. throw

Therefor, three would cover your scenario

@latonz
Copy link
Contributor

latonz commented Mar 27, 2024

Matching by type would again be a completely different approach which Mapperly currently doesn‘t have any support for at all.

@ChaseFlorell
Copy link

Matching by type would again be a completely different approach which Mapperly currently doesn‘t have any support for at all.

Is it something Mapperly could support? Is there an openness for this?

@latonz
Copy link
Contributor

latonz commented Mar 28, 2024

Mapperly could support it, but I don‘t see why it would be useful. A common use-case I see for this feature is if you have an entity you want to map with additional data (for example from headers/metadata/…). Often the data from the headers is not in the same format / classes as the data on your business layer.

@robinabganpat
Copy link

Mapperly could support it, but I don‘t see why it would be useful. A common use-case I see for this feature is if you have an entity you want to map with additional data (for example from headers/metadata/…). Often the data from the headers is not in the same format / classes as the data on your business layer.

You mention you don't see why it would be useful, but then mention a use case where it would be useful... 😂

Another use case I would see this fit, is when you want to map from a parent-child business layer model to a data layer model. In the business layer you'd probably need only need certain properties (like an ID) at the top level model and not in any child models, but in the data layer, you'd need those properties for all your child models to be able to save the relation between the parent and the child model.

@latonz
Copy link
Contributor

latonz commented Apr 12, 2024

@robinabganpat sorry if this was unclear, english is not my primary language. The first part of my comment is about support for additional parameters to properties/constructor parameters matching by type.
The example I gave afterwards shows a use-case which I think additional parameters are useful for, but doesn‘t work by matching the type.

@DDaveid

This comment was marked as resolved.

@latonz

This comment was marked as resolved.

@DDaveid

This comment was marked as resolved.

@bberka
Copy link

bberka commented May 23, 2024

This was something i had a use case for simply converting some entities to Log version of that entity and i found this workaround for it. Might not be the best thing but simplest way to do it is this for now.

[Mapper]
public static partial class MapperProductInformation
{
  [MapperIgnoreSource(nameof(ProductInformation.Id))]
  private static partial ProductInformationLog ToLogEntityInternal(this ProductInformation product);
  public static ProductInformationLog ToLogEntity(this ProductInformation product, ProductInformationLogType type) {
    var entity = ToLogEntityInternal(product);
    entity.Id = Guid.NewGuid(); //could remove this and let EF handle it
    entity.Type = type;
    return entity;
  }
}

This way i make sure ProductInformationLogType is always passed when mapping to log entity.

@ChaseFlorell
Copy link

This was something i had a use case for simply converting some entities to Log version of that entity and i found this workaround for it. Might not be the best thing but simplest way to do it is this for now.

[Mapper]
public static partial class MapperProductInformation
{
  [MapperIgnoreSource(nameof(ProductInformation.Id))]
  private static partial ProductInformationLog ToLogEntityInternal(this ProductInformation product);
  public static ProductInformationLog ToLogEntity(this ProductInformation product, ProductInformationLogType type) {
    var entity = ToLogEntityInternal(product);
    entity.Id = Guid.NewGuid(); //could remove this and let EF handle it
    entity.Type = type;
    return entity;
  }
}

This way i make sure ProductInformationLogType is always passed when mapping to log entity.

This is a decent workaround to the issue. The only disadvantage I see is that type is not constructor injected and therefore might not be able to go through the same validation rules as everything else. I like that newer .NET languages allow for the init keyword in order to make it immutable, but my personal opinion is that constructor injection is superior.

All the same, thank you for this approach, it could be a viable workaround.

@cremor
Copy link

cremor commented May 23, 2024

I like that newer .NET languages allow for the init keyword in order to make it immutable

The shown code wouldn't compile if the Type property had an init setter. A real solution with Mapperly support is needed.

@ChaseFlorell
Copy link

I like that newer .NET languages allow for the init keyword in order to make it immutable

The shown code wouldn't compile if the Type property had an init setter. A real solution with Mapperly support is needed.

aaaah, good point. That didn't even dawn on me.

@bberka
Copy link

bberka commented May 23, 2024

I like that newer .NET languages allow for the init keyword in order to make it immutable

The shown code wouldn't compile if the Type property had an init setter. A real solution with Mapperly support is needed.

Yes only downside would be you have to set setter public in order to use this. All tho you can make the mapper method take created instance but then you either need to choose between model properties or the parameters for the constructor to take.

I agree with you tho library should implement something like this.

I would also like to see custom mapper methods for the properties to take the original parameter best way would be using attributes to specify it is from parameters or properties etc. Something like this:

  public static partial ProductInformationLog ToLogEntity(this ProductInformation e,int errorLevel);

  private static string MapLevel([Parameter] int level) {
    if(level == 0)
       return "success";
    return "fail";
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.