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

Updating constructor matching for int keys #1277

Merged
merged 3 commits into from Jul 14, 2021
Merged

Updating constructor matching for int keys #1277

merged 3 commits into from Jul 14, 2021

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.

None yet

3 participants