Open cremor opened 1 year ago
Thanks for your feature request. I agree that this can be confusing and we'll think about the possible solutions to this problem
Maybe it would even be possible to create a "merge" option? That might have a higher complexity to implement, but could also be useful. I assume that would require either a way to specify which property is compared for the equals check, or user provided method or IEqualityComparer
for the equals check.
Need this too - have a Tags property of List
Idea for collection mapping methods (tbd):
Replace
: Replace entire collection with a new collection object
Add
: Add all source items to the target collection
Update
: Map target[i] to source[i] with an existing target mapping, remove target items with i>= source.Length, add source items with i >= target.Length. For dictionaries the key should be used instead of the index.
Really looking forward to this feature! It looks like the update is index based in your suggestion?
Could this feature be expanded to also implement Collection Synchronization / Upsert mode? The main use case is handling lists of entities tracked by Entity Framework.
Define what property to use for collection equality matching, example:
[CollectionEquality(nameof(CarDto.Id), nameof(CarModel.Id))]
public static ICollection<CarModel> ToModels(this ICollection<CarDto> sourceCars);
When mapping a source collection to a target collection, the specified equality properties are used to:
@stigrune how would you write this by hand? Would you collect to a HashSet or Dictionary first? Without collecting first, the performance would be really bad, wouldn't it (O(n^2))? Not sure if this would be too much magic for an object-to-object mapper 🤔
@latonz I can understand the feeling that this is to much magic for a object-to-object mapper, and a bit out of scope. Totally understandable. The index based one thats already suggested could be the default, and this could be opt-in for those that want this behavior. I think its a common use case.
Performance-wise, the fastest approach is likely to use a HashSet or Dictionary as you suggest. If this is something you want included in Mapperly in the future, we would have to benchmark a few different implementations. Different mapping implementations performance might also vary depending on the specific type of collections used for source and target.
Here is a quick example implementation, without much performance or edge-cases thought into it. I'm sure there is a lot that can be done to optimize this, and also support IEqualityComparer
as suggested by @cremor.
static void UpsertCollection(ICollection<CarDto> source, ICollection<CarModel> target)
{
var sourceDict = source.ToDictionary(dto => dto.Id);
var targetDict = target.ToDictionary(model => model.Id);
foreach (var sourceKeyValue in sourceDict)
{
var sourceValue = sourceKeyValue.Value;
// Update
if (targetDict.TryGetValue(sourceValue.Id, out var targetValue))
{
targetValue.Make = sourceValue.Make;
targetValue.Model = sourceValue.Model;
}
else
{
// Insert
target.Add(
new CarModel
{
Id = sourceValue.Id,
Make = sourceValue.Make,
Model = sourceValue.Model
}
);
}
}
// Delete
foreach (var targetObj in target.Where(obj => !sourceDict.ContainsKey(obj.Id)).ToList())
{
target.Remove(targetObj);
}
}
We could consider this as a 4. mode. But I think in a first iteration it would be a good idea to implement the already proposed ones.
Is your feature request related to a problem? Please describe. It seems like Mapperly currently uses the target property setter visibility in an "existing target object" mapper (void method) to decide if a collection (
ICollection<T>
) is completely replaced or if the items in the collection are just added. This is confusing/dangerous because a simple visiblity change on the setter can lead to different behavior of the mapper.Describe the solution you'd like I'd like to configure if items are added to an existing collection or if a collection is replaced. One of the two possibilities should be the default for all collection properties, regardless of their setter visibility. If the currently active setting is to replace the collection, but the target property has no visible setter, then the generated code should call
Clear()
on the collection before adding the items.Describe alternatives you've considered I know that I can call
Clear()
in a "before map" method. But the main problem here is that I might not even notice that I have to do this until I get a runtime bug.Additional context In theory this also affects mappers that return the mapped object, but that would only show a difference if the constructor/object factory already adds items to the collection.