-
Notifications
You must be signed in to change notification settings - Fork 338
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
Feature: Allow for mapping properties containing periods #777
base: development
Are you sure you want to change the base?
Conversation
…el to allow for mapping properties that contain periods
Hello @JMolenkamp Excellent work!
|
Hi @DocSvartz,
|
@JMolenkamp I meant that if the constructor parameters in your cases can have names with dots, then you should pay special attention to this.
They may not mapping, even though the Properties with the same names will be mapping. |
@DocSvartz Got it. It won't be an issue in my case, the dynamically created types have a default constructor and each property both has a getter and a setter. I might take a look though. |
@DocSvartz
The latter two gave me an idea to try a different approach and keep the period joined string on the InvokerModel. |
} | ||
|
||
[TestMethod] | ||
public void Using_Property_Path_String_Is_Supported() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuring a mapping using a property path string does not work, as it is still split on the periods
Do you mean this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JMolenkamp
I managed to get this test to work.
The only thing that doesn't work is the first mapping.
I tried the alternative approach which turns out to be less impactful and might be preferred. Some ambiguity might be possible though, what should class Source
{
public Child A.B { get; set; }
public int A.B.C { get; set; }
}
class Child
{
public int C { get; set; }
} A risk of building dynamic types, I guess? |
|
||
// Execute the mapping both ways | ||
Source source = new() { Value = 551 }; | ||
object target = source.Adapt(typeof(Source), targetType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this not working.
551 not transmitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, that was what I was trying to tell here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this fix
public static Expression PropertyOrFieldPath(Expression expr, string[] path)
{
if(expr.Type.IsAutoClass && path.Length > 1)
{
return Expression.PropertyOrField(expr, string.Join(".", path));
}
return path.Aggregate(expr, PropertyOrField);
}
i get this error
Mapster.CompileException: Error while compiling
source=Types.Target
destination=Mapster.Tests.WhenMappingMemberNameContainingPeriod+Source
type=Map ---> System.ArgumentException: 'Some' is not a member of type 'Types.Target' (Parameter 'propertyOrFieldName').
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know (about the error that is), it gave me an idea for another approach, see this branch or see my post above where I mentioned it.
All my issues are fixed by only changing the method you mention here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand need to separate
"Toplevel" (root) Member Access from "Insider" (nested) Member Access.
this "Some.Property.With.Periods" - is top level Member
target."Some.Property.With.Periods"
not nested
target.Some.Property.With.Periods
target
{
Some {get;set}
}
where
Some
{
Property {get; set;}
}
where
Property
{
With {get; set;}
}
. .... and so on
here actually requires analysis of mapping string
But for Toplevel, This kind of processing will be quite sufficient.
public static TSetter Map<TSetter>(
this TSetter setter, string destinationMemberName, string sourceMemberName) where TSetter : TypeAdapterSetter
{
setter.CheckCompiled();
var x = setter.Config.RuleMap.First().Key;
var isTopLevelMemberSourceName = x.Source.GetFieldsAndProperties().Any(x => x.Name == sourceMemberName);
var isTopLevelMemberDestName = x.Destination.GetFieldsAndProperties().Any(x => x.Name == destinationMemberName);
setter.Settings.Resolvers.Add(new InvokerModel
{
DestinationMemberPath = isTopLevelMemberDestName ? new[] { destinationMemberName } : destinationMemberName.Split('.'),
SourceMemberPath = isTopLevelMemberSourceName ? new[] { sourceMemberName } : sourceMemberName.Split('.'),
Condition = null
});
return setter;
}
I'll think about it some more.
The
InvokerModel
stores the property path as a period joined string. When building the expression for a property path, this string is split on the periods, which makes it impossible to have a property name that contains periods.Obviously, this is kinda obscure, but I actually ran into this issue. It is possible to have a property name with a period when dynamically building types where pretty much all naming restrictions go out of the window, demonstrated in this test.
With this PR, instead of storing a period joined string, the path is stored as a string array. This allows for storing a property name with a period as it is not split on usage. The tests still succeed, except for
No_Errors_Thrown_With_Default_Configuration_On_Unmapped_Primitive
, which failed to begin with.If this change is considered, a skeptic review is required, even though the tests succeed. For now, this is just my initial attempt.
For some background:
Our applications frequently have to communicate with industrial devices, for which the OPC UA protocol is often used. When reading or writing some structured data, an application type requires mapping to some node hierarchy provided by the OPC UA server. The configuration for this mapping is external, such that changes in property names/types or even structure do not require a rebuild of the application. Based on the configuration, types are built dynamically and Mapster is configured to map between the application type and the dynamic type. I am not really up to date with the naming restrictions on OPC UA nodes, but usage of periods is allowed, at least in some places.