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

Feature: Allow for mapping properties containing periods #777

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

JMolenkamp
Copy link

@JMolenkamp JMolenkamp commented Feb 25, 2025

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.

…el to allow for mapping properties that contain periods
@DocSvartz
Copy link

Hello @JMolenkamp Excellent work!

  1. Your test only checks that the configuration compiles successfully. If possible, add tests that check that the data is transfered successfully. (_sourse.Adapt() and _sourse.Adapt(_destination));

  2. Have you checked the functionality of your solution when these Properties are used as constructor parameters?

@JMolenkamp
Copy link
Author

Hi @DocSvartz,

  1. Compilation is where the exception arose in my situation, which is why this test ends here.
    I will add some further testing.

  2. I do not think so. At this point, I only ensured that the existing tests still succeeded and that no exception was thrown on compilation (my use case).
    Also, I am not quite sure about what you mean with using these properties as constructor parameters.

@DocSvartz
Copy link

DocSvartz commented Feb 26, 2025

@JMolenkamp I meant that if the constructor parameters in your cases can have names with dots, then you should pay special attention to this.

public class Destination
{
  public Destination (string "Some.Property.With.Periods")
}
 

var destination = new ( source."Some.Property.With.Periods");

They may not mapping, even though the Properties with the same names will be mapping.

@JMolenkamp
Copy link
Author

@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.

@JMolenkamp
Copy link
Author

@DocSvartz
Added some tests:

  • Mapping to and from a property containing a period does work
  • Mapping to a constructor parameter containing a period does work
  • Configuring a mapping using a property path string does not work, as it is still split on the periods
  • Mapping to a dictionary key containing periods does not work, also didn't work originally. This one is probably not really a part of this PR, but I wanted to take a look at it anyway.

The latter two gave me an idea to try a different approach and keep the period joined string on the InvokerModel.
Instead of only using the parts after splitting as member names, it might be possible to try out combinations of consecutive parts when members for singular parts are not found. Not really sure what this would involve, but I might take a shot.

}

[TestMethod]
public void Using_Property_Path_String_Is_Supported()

Choose a reason for hiding this comment

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

@JMolenkamp

Configuring a mapping using a property path string does not work, as it is still split on the periods

Do you mean this test?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

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.

@JMolenkamp
Copy link
Author

JMolenkamp commented Feb 27, 2025

@DocSvartz

I tried the alternative approach which turns out to be less impactful and might be preferred.
The test mentioned above does also succeed with this change.
See this branch

Some ambiguity might be possible though, what should "A.B.C" reference:

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);
Copy link

@DocSvartz DocSvartz Feb 27, 2025

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

Copy link
Author

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

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').

Copy link
Author

@JMolenkamp JMolenkamp Feb 27, 2025

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.

Copy link

@DocSvartz DocSvartz Feb 28, 2025

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.

@DocSvartz

This comment was marked as duplicate.

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

Successfully merging this pull request may close these issues.

2 participants