-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
Comments
I like this idea. However, it is not that easy to implement. It may take some time until Mapperly supports this. |
@latonz Would you rework I would love to contribute, but don't want to go against the vision you might have for this 🙂 |
@Fube Sorry, I missed the notification of your comment. Feel free to submit an architectural proposal to this issue. |
@DerMave As far as I can tell, the |
@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. |
+1 for this feature. I think this is very common use case. As @cyril265 mentioned above, we also obtain tenantId, userId etc. from HttpContext. |
@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. |
Maybe we can use the flattening / unflattening logic to implement this 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 Not sure if my suggestion makes sense though, @latonz is it plausible? |
@Fube I don't think flattening/unflattening would be the right approach for this, it could work as a workaround though. |
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. |
I have a working POC for this feature, I've opted to change I have some questions/concerns about breaking future features.
|
@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.
|
@Herdo you're right most of my problems are self inflicted 😄 |
I'm torn between two ideas for this feature and unsure which one would be better: Top Level
// 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;
}
// 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
Cons
Nested
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
Cons
I have a working POC for the nested logic mapper see #550. It can be adapted to do top level mappings |
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 😊 |
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); |
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. |
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. [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; }
} |
little late to the discussion, it would be nice to have this feature, |
No updates, feel free to contribute. I think before an implementation, several things need to be thought of and decided:
I think handling these mappings the same as the generic mappings (they are never called by Mapperly generated code) with an explicitly mapped parameter ( |
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;
} |
You could simply provide the name of the ctor parameter to MapTo. |
doesn't that defeat the 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? |
Nameof: yes it does |
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 |
@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? |
I'm not sure I follow the question. 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 |
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 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. |
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);
} |
I think that's where matching by name would come in. If it were me, I think I'd go something like this
Therefor, three would cover your scenario |
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? |
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. |
@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. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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 All the same, thank you for this approach, it could be a viable workaround. |
The shown code wouldn't compile if the |
aaaah, good point. That didn't even dawn on me. |
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";
} |
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.
Example usage:
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.The text was updated successfully, but these errors were encountered: