Skip to content

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.

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

@DocSvartz

This comment was marked as duplicate.

@JMolenkamp JMolenkamp marked this pull request as draft March 11, 2025 09:24
@JMolenkamp
Copy link
Author

Moved to draft in favor of #781

@JMolenkamp JMolenkamp closed this Mar 18, 2025
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