Skip to content

Updating constructor matching for int keys#1277

Merged
AArnott merged 3 commits intoMessagePack-CSharp:masterfrom
seriousbox:fix-int-key-constructor-matching
Jul 14, 2021
Merged

Updating constructor matching for int keys#1277
AArnott merged 3 commits intoMessagePack-CSharp:masterfrom
seriousbox:fix-int-key-constructor-matching

Conversation

@seriousbox
Copy link
Contributor

The following example works fine for mutable objects.

[MessagePackObject]
public class Example
{
    [Key(0)]
    public int IntProperty { get; set; }

    [Key(2)]
    public string StringProperty { get; set; }
}

Unfortunately, it is impossible to repeat the same thing for immutable objects. Serializing the following example throws an exception: MessagePack.MessagePackSerializationException {"Failed to serialize Example value."}. InnerException: MessagePack.Internal.MessagePackDynamicObjectResolverException {"can't find matched constructor. type:Example "}.

[MessagePackObject]
public class Example
{
    [Key(0)]
    public int IntProperty { get; }

    [Key(2)]
    public string StringProperty { get; }

    public Example(int intProperty, string stringProperty)
    {
        IntProperty = intProperty;
        StringProperty = stringProperty;
    }
}

The current implementation searches for the matched constructor according to the following logic: for each constructor parameter, there must be int key with value of the index of this parameter. So for immutable types, keys must start with 0 and have no gaps. It becomes more difficult to support changes for such types.

I propose a new implementation without breaking backward compatibility. We determine the indexes of the constructor parameters corresponding to the int keys in ascending order of their values.

This implementation does not break the current serialization logic and supports missing keys in the previous example.

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

It's a peculiar use case, but I guess I can imagine how folks may land there. Thinking about it, I can't see how this change would present any problem.
But we do want a test for it. Can you add one?
@neuecc how do you feel about it?

@seriousbox
Copy link
Contributor Author

But we do want a test for it. Can you add one?

Yes, I will add it today.

@seriousbox seriousbox requested a review from AArnott July 2, 2021 08:12
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Looks great. Let's give a couple more days for @neuecc to have a peak at it before merging.

@AArnott AArnott added this to the v2.2 milestone Jul 14, 2021
@AArnott AArnott merged commit 1551f93 into MessagePack-CSharp:master Jul 14, 2021
@neuecc
Copy link
Member

neuecc commented Jul 15, 2021

Sorry, I missed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants