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

Tweak RazorConfigurationFormatter to account for serialization format change #10157

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

DustinCampbell
Copy link
Member

The recent FUSE changes updated the RazorConfigurationFormatter to write serialize/deserialize a new boolean. However, SerializationFormat.Version for MessagePack wasn't updated, so it just fails during deserialization when older project.razor.bin files are initially read off disk. This tweaks the deserializer to be more forgiving and handle the case that the boolean isn't present.

A similar change is not needed in the JSON serialization because that deserializer was written to be a bit more forgiving and the JSON SerializationFormat.Version was updated.

I ran into this when opening an old clone of OrchardCore that I'd opened previously, and started hitting exceptions during message pack deserialization.

@DustinCampbell DustinCampbell requested a review from a team as a code owner March 25, 2024 16:53
… change

The recent FUSE changes updated the RazorConfigurationFormatter to write serialize/deserialize a new boolean. However, `SerializationFormat.Version` for MessagePack wasn't updated, so it just fails during deserialization when older project.razor.bin files are initially read off disk. This tweaks the deserializer to be more forgiving and handle the case that the boolean isn't present.

A similar change is not needed in the JSON serialization because that deserializer was written to be a bit more forgiving and the JSON `SerializationFormat.Version` was updated.
@DustinCampbell DustinCampbell force-pushed the fix-msgpack-formatter branch from b626b86 to d45f5a4 Compare March 25, 2024 17:38
@ryzngard
Copy link
Contributor

Do we need to bump the version number? Thanks for fixing this, sorry I missed it! I didn't notice I only bumped the json version not the messagepack.

Also, is there a way we can catch this in the future?

@DustinCampbell
Copy link
Member Author

Do we need to bump the version number?

I don't think so. The idea was to write this in such a way that the version number shouldn't need bumping. Bumping the version number has the consequence of needing to insert a new Razor for VS Code, so I try to avoid doing that unless absolutely necessary. Looking at the JSON serializer change, that actually probably didn't need a version number change, since the change used ReadBooleanOrFalse to returns false if the requested property doesn't exist. However, since the JSON serialization isn't actually used in product code to serialize RazorProjectInfo, the bump shouldn't really have an effect anyway.

Also, is there a way we can catch this in the future?

Honestly, it's kind of hard to catch. Nothing simple comes to mind. 😦

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Sorry, I missed that the version bump was in the wrong spot.

I don't love having the two versions be different, and the two deserialization classes be different, but at the same time its probably a little mean to make Andrew do another dual insertion after he already did one, and it turned out to not be necessary 😁

@DustinCampbell DustinCampbell merged commit c8523e4 into dotnet:main Mar 25, 2024
12 checks passed
@DustinCampbell DustinCampbell deleted the fix-msgpack-formatter branch March 25, 2024 23:18
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 25, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024
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.

4 participants