Skip to content

Fix serialization for records which have no properties of their own #8289

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

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Jan 27, 2023

  • As the GenerateSerializerAttribute.IncludePrimaryConstructorParameters doc comment says, it defaults to true for record types - but it should only default to true for record types. That is fixed now. Also note that non-record types don't (yet) support primary constructors and adding one to an existing type would be a breaking change.
  • If a type opts in (or not out, in the case of records) to including primary constructor parameters in serialization, then wire serialization should treat the type as potentially having primary constructor parameters even if it currently does not. Anything else would make adding a parameter a breaking change, which it is not supposed to be (version tolerance is important)

Fixes #8286 and #8275

cc @christiansparre & @ahydrax

Microsoft Reviewers: Open in CodeFlow

@ReubenBond ReubenBond force-pushed the fix/record-serialization branch from de631d1 to 9774b08 Compare January 27, 2023 23:35
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Cryptography.X509Certificates;
Copy link
Contributor

@ahydrax ahydrax Jan 28, 2023

Choose a reason for hiding this comment

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

I suppose this was imported accidentally

@christiansparre
Copy link
Contributor

👍 I have verified the fix using our end to end tests and everything looks good.

@ReubenBond ReubenBond merged commit 62b0b27 into dotnet:main Jan 30, 2023
@ReubenBond ReubenBond deleted the fix/record-serialization branch January 30, 2023 17:58
@jkonecki
Copy link
Contributor

@ReubenBond thank you for the fix! What are the plans for getting it out?

@ReubenBond
Copy link
Member Author

Building and testing a release at the moment. Hopefully it goes well. I am working through some CI issues

@jkonecki
Copy link
Contributor

jkonecki commented Jan 31, 2023 via email

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

Successfully merging this pull request may close these issues.

The serializer works incorrectly with records
4 participants