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

Fix handling of readonly private fields #1267

Merged
merged 1 commit into from Jun 21, 2021

Conversation

Alxandr
Copy link
Contributor

@Alxandr Alxandr commented Jun 14, 2021

Disallow writing to readonly private fields even when using a resolver
that allows using private fields.

Fixes: #1218

Disallow writing to readonly private fields even when using a resolver
that allows using private fields.

Fixes: MessagePack-CSharp#1218
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 good. Thanks.

@neuecc Any concerns?

@AArnott AArnott changed the base branch from master to develop June 14, 2021 16:35
@AArnott AArnott added this to the v2.3 milestone Jun 14, 2021
Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@AArnott AArnott merged commit 615a0b5 into MessagePack-CSharp:develop Jun 21, 2021
@AArnott
Copy link
Collaborator

AArnott commented Aug 21, 2021

This regressed #1308. I'm looking for a mutually acceptable solution.

@RobotGizmo
Copy link

RobotGizmo commented Sep 20, 2021

This change unfortunately broke deserializing for many of our objects. Is there any way to restore the ability to set the values of readonly fields without having to create custom constructors for each object? For example, this is what an object might look like:

[Key(0)] public readonly string Code = "";

[SerializationConstructor]
private MyObject() {}

Before MessagePack 2.3.75 this worked, but after 2.3.75 the Code field is always empty after deserializing. What is the best way to get this working again? Thanks!

@Alxandr
Copy link
Contributor Author

Alxandr commented Sep 20, 2021

IMHO; this shouldn't work. I expect serializer libraries to maintain the "readonly" constraints set by the compiler. The fact that message-pack was writing to readonly fields came as a big surprise to me.

Also, @RobotGizmo, how on earth is that MyObject usable? If the field is readonly, it will always only have the value ""?

@RobotGizmo
Copy link

I think our issue is that we view the serializer as something that dumps the data from an object then puts it back in blindly. If there is a field with data it should be filled in again unless it's marked with [IgnoreMember]. Otherwise the only solution I can see is to create private SerializationConstructor constructors that take all the fields as parameters. This is much harder to maintain as you have to make sure to keep it in sync with the fields in the class. It would be fantastic to have a resolver (or however we can solve this) like StandardResolverAllowPrivateIncludingReadonly or have a "WriteReadonlyFields" property as an option.

The example above was just a part of the code as a sample, we do have a public constructor as well, like this:

[Key(0)] public readonly string Code = "";

[SerializationConstructor]
private MyObject() {}

public MyObject(string code)
{
    this.Code = code;
}

@AArnott
Copy link
Collaborator

AArnott commented Sep 20, 2021

@RobotGizmo #1314 already corrected the regression. Can you confirm?

@RobotGizmo
Copy link

@AArnott Thanks, it looks like it's working as it did before with v2.3.85. :)

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.

StandardResolverAllowPrivate overwrites readonly fields set in constructor
4 participants