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

feat: add support for nested properties #587

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/Riok.Mapperly.Abstractions/MapperNestedPropertiesAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
namespace Riok.Mapperly.Abstractions;

/// <summary>
/// Maps all properties from a nested path to the root destination.
/// </summary>
[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
public class MapperNestedPropertiesAttribute : Attribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename according to the issue and other mapping related attributes to MapNestedPropertiesAttribute.

{
private const string PropertyAccessSeparatorStr = ".";
private const char PropertyAccessSeparator = '.';

/// <summary>
/// Maps a specified source property to the specified target property.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't correct, is it? The same for the other ctor.

/// </summary>
/// <param name="source">The name of the source property. The use of `nameof()` is encouraged. A path can be specified by joining property names with a '.'.</param>
public MapperNestedPropertiesAttribute(string source)
: this(source.Split(PropertyAccessSeparator)) { }

/// <summary>
/// Maps a specified source property to the specified target property.
/// </summary>
/// <param name="source">The path of the source property. The use of `nameof()` is encouraged.</param>
public MapperNestedPropertiesAttribute(string[] source)
{
Source = source;
}

/// <summary>
/// Gets the name of the source property.
/// </summary>
public IReadOnlyCollection<string> Source { get; }

/// <summary>
/// Gets the full name of the source property path.
/// </summary>
public string SourceFullName => string.Join(PropertyAccessSeparatorStr, Source);
}
5 changes: 5 additions & 0 deletions src/Riok.Mapperly.Abstractions/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,8 @@ Riok.Mapperly.Abstractions.MapperIgnoreTargetValueAttribute
Riok.Mapperly.Abstractions.MapperIgnoreTargetValueAttribute.MapperIgnoreTargetValueAttribute(object! target) -> void
Riok.Mapperly.Abstractions.MapperIgnoreSourceValueAttribute.SourceValue.get -> System.Enum?
Riok.Mapperly.Abstractions.MapperIgnoreTargetValueAttribute.TargetValue.get -> System.Enum?
Riok.Mapperly.Abstractions.MapperNestedPropertiesAttribute
Riok.Mapperly.Abstractions.MapperNestedPropertiesAttribute.MapperNestedPropertiesAttribute(string! source) -> void
Riok.Mapperly.Abstractions.MapperNestedPropertiesAttribute.MapperNestedPropertiesAttribute(string![]! source) -> void
Riok.Mapperly.Abstractions.MapperNestedPropertiesAttribute.Source.get -> System.Collections.Generic.IReadOnlyCollection<string!>!
Riok.Mapperly.Abstractions.MapperNestedPropertiesAttribute.SourceFullName.get -> string!
11 changes: 9 additions & 2 deletions src/Riok.Mapperly/Configuration/MapperConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public MapperConfiguration(SymbolAccessor symbolAccessor, ISymbol mapperSymbol)
Array.Empty<string>(),
Array.Empty<string>(),
Array.Empty<PropertyMappingConfiguration>(),
Array.Empty<NestedPropertyMappingConfiguration>(),
Mapper.IgnoreObsoleteMembersStrategy
),
Array.Empty<DerivedTypeMappingConfiguration>()
Expand Down Expand Up @@ -74,8 +75,14 @@ private PropertiesMappingConfiguration BuildPropertiesConfig(IMethodSymbol metho
var ignoreObsolete = _dataAccessor.Access<MapperIgnoreObsoleteMembersAttribute>(method).FirstOrDefault() is not { } methodIgnore
? _defaultConfiguration.Properties.IgnoreObsoleteMembersStrategy
: methodIgnore.IgnoreObsoleteStrategy;

return new PropertiesMappingConfiguration(ignoredSourceProperties, ignoredTargetProperties, explicitMappings, ignoreObsolete);
var nestedMappings = _dataAccessor.Access<MapperNestedPropertiesAttribute, NestedPropertyMappingConfiguration>(method).ToList();
return new PropertiesMappingConfiguration(
ignoredSourceProperties,
ignoredTargetProperties,
explicitMappings,
nestedMappings,
ignoreObsolete
);
}

private EnumMappingConfiguration BuildEnumConfig(MappingConfigurationReference configRef)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
namespace Riok.Mapperly.Configuration;

public record NestedPropertyMappingConfiguration(StringMemberPath Source);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename to MapNestedPropertiesMappingConfiguration to match other namings and attribute name.

Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ public record PropertiesMappingConfiguration(
IReadOnlyCollection<string> IgnoredSources,
IReadOnlyCollection<string> IgnoredTargets,
IReadOnlyCollection<PropertyMappingConfiguration> ExplicitMappings,
IReadOnlyCollection<NestedPropertyMappingConfiguration> NestedMappings,
IgnoreObsoleteMembersStrategy IgnoreObsoleteMembersStrategy
);
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ protected MembersMappingBuilderContext(MappingBuilderContext builderContext, T m

_unmappedSourceMemberNames.ExceptWith(IgnoredSourceMemberNames);

MemberConfigsByRootTargetName = GetMemberConfigurations();

// remove explicitly mapped ignored targets from ignoredTargetMemberNames
// then remove all ignored targets from TargetMembers, leaving unignored and explicitly mapped ignored members
ignoredTargetMemberNames.ExceptWith(MemberConfigsByRootTargetName.Keys);
Expand Down Expand Up @@ -117,9 +115,25 @@ private Dictionary<string, IMappableMember> GetTargetMembers()

private Dictionary<string, List<PropertyMappingConfiguration>> GetMemberConfigurations()
{
return BuilderContext.Configuration.Properties.ExplicitMappings
.GroupBy(x => x.Target.Path.First())
.ToDictionary(x => x.Key, x => x.ToList());
var simpleMappings = BuilderContext.Configuration.Properties.ExplicitMappings.GroupBy(x => x.Target.Path.First());

var wildcardMappings = BuilderContext.Configuration.Properties.NestedMappings
.SelectMany(x =>
{
return BuilderContext.SymbolAccessor
.GetAllAccessibleMappableMembers(Mapping.SourceType)
.Where(i => i.Name == x.Source.Path.Last())
.SelectMany(i => BuilderContext.SymbolAccessor.GetAllAccessibleMappableMembers(i.Type))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The members included via MapNestedProperties should also be included in _unmappedSourceMemberNames and should also emit a diagnostic if not mapped. This should also be tested.

.Select(i =>
{
var list = x.Source.Path.ToList();
list.Add(i.Name);
return new PropertyMappingConfiguration(new StringMemberPath(list), new StringMemberPath(new[] { i.Name }));
});
Comment on lines +123 to +132
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mixing these with the manually configured mappings won't fully work...
IMO this needs to be handled separately. If no matching source member could be found, the member should also be looked up in these MapNestedProperties.

})
.GroupBy(x => x.Target.Path.Last());

return simpleMappings.Union(wildcardMappings).ToDictionary(x => x.Key, x => x.ToList());
}

private void AddUnmatchedIgnoredTargetMembersDiagnostics()
Expand Down
42 changes: 42 additions & 0 deletions test/Riok.Mapperly.Tests/Mapping/ObjectPropertyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,19 @@ public Task WithPropertyNameMappingStrategyCaseSensitive()
return TestHelper.VerifyGenerator(source);
}

[Fact]
public Task WithPropertyNameMappingStrategyCaseSensitiveAndManualMappedProperty()
{
var source = TestSourceBuilder.MapperWithBodyAndTypes(
"[MapProperty(nameof(A.stringvalue), nameof(B.StringValue2)] partial B Map(A source);",
new TestSourceBuilderOptions { PropertyNameMappingStrategy = PropertyNameMappingStrategy.CaseSensitive },
"class A { public string stringvalue { get; set; } }",
"class B { public string StringValue2 { get; set; } }"
);

return TestHelper.VerifyGenerator(source);
}

[Fact]
public Task WithManualMappedNotFoundTargetPropertyShouldDiagnostic()
{
Expand Down Expand Up @@ -443,4 +456,33 @@ public void ShouldIgnoreStaticConstructorAndDiagnostic()
.HaveDiagnostic(DiagnosticDescriptors.CouldNotCreateMapping)
.HaveAssertedAllDiagnostics();
}

[Fact]
public void ShouldFlattedNestedToTargetRootMapping()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move these tests to ObjectPropertyFlatteningTest as they fit better there as this always flattens the values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a complete new feature, an integration test should be added to ensure this works with all supported Roslyn versions.

{
var mapperBody = """
[MapperNestedProperties(nameof(A.NestedValue)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test for deep nested properties. Eg. [MapNestedProperties(new[] { nameof(A.Nested), nameof(A.Nested.Value) })]? I'm not sure, but I think these won't work yet.
And probably also add a test for q required init property, a constructor parameter and a queryable projection mapping.

public partial C Map(A source);
""";

var source = TestSourceBuilder.MapperWithBodyAndTypes(
mapperBody,
"class A { public B NestedValue { get; set; } public string DummyValue { get; set; } }",
"class B { public string GoodProp1 { get; set; } public string GoodProp2 { get; set; } }",
"class C { public string GoodProp1 { get; set; } public string GoodProp2 { get; set; } public string DummyValue { get; set; } }"
);

TestHelper
.GenerateMapper(source)
.Should()
.HaveMapMethodBody(
"""
var target = new global::C();
target.GoodProp1 = source.NestedValue.GoodProp1;
target.GoodProp2 = source.NestedValue.GoodProp2;
target.DummyValue = source.DummyValue;
return target;
"""
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//HintName: Mapper.g.cs
// <auto-generated />
#nullable enable
public partial class Mapper
{
private partial global::B Map(global::A source)
{
var target = new global::B();
target.StringValue2 = source.stringvalue;
return target;
}
}
Loading