Skip to content

Remove SemanticVersion from list of KnownTypes for serialization #7016

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.

Sign up for GitHub

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

Jump to bottom
Merged
daxian-dbw merged 1 commit into from
Jun 8, 2018
Merged

Remove SemanticVersion from list of KnownTypes for serialization #7016

daxian-dbw merged 1 commit into from
Jun 8, 2018

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jun 7, 2018 •
edited
Loading

PR Summary

PowerShell serialization has a list of known PowerShell types. When an object is deserialized containing a known type it tries to deserialize the object to that specific type and throws exception if it can't succeed. SemanticVersion only exists in PSCore6 so when remoting from Windows PowerShell or importing clixml generated from PSCore6 that contains SemanticVersion, it fails.

Fix #6448
Fix #1819

PR Checklist

@SteveL-MSFT SteveL-MSFT requested a review from PaulHigin June 7, 2018 05:22
@iSazonov
Copy link
Collaborator

iSazonov commented Jun 7, 2018

It seems to be not in line with the PowerShell Committee's conclusion in #6448.

@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jun 7, 2018
@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jun 7, 2018

@iSazonov you are correct, that committee decision was based on an incorrect hypothesis.

daxian-dbw
daxian-dbw approved these changes Jun 7, 2018
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM. This is probably the cleanest way to deal with this issue 🎉

@daxian-dbw
Copy link
Member

daxian-dbw commented Jun 7, 2018 •
edited
Loading

One drawback: when PowerShell Core talks to PowerShell Core, the client side won't have a full-fledged SemanticVersion object deserialized back. So potentially this is a breaking change.

Strike my previous comment. It will be deserailzied to a PSObject with all properties from the SemanticVersion object populated. Problem solved, perfectly!

@HemantMahawar
Copy link
Contributor

HemantMahawar commented Jun 7, 2018

Given it will come back as Deseriealized.System.Management.Automation.SemanticVersion (PSObject) in both Windows PowerShell and PowerShell Core, feels like the right fix.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jun 7, 2018
@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Jun 7, 2018

@PowerShell/powershell-committee reviewed this and agree this is approach

PaulHigin
PaulHigin approved these changes Jun 7, 2018
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

I agree that this is the right change.

@SteveL-MSFT SteveL-MSFT mentioned this pull request Jun 8, 2018
Closed
@daxian-dbw daxian-dbw merged commit 50d0611 into PowerShell:master Jun 8, 2018
@SteveL-MSFT SteveL-MSFT deleted the serialization branch June 8, 2018 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

5 participants

@SteveL-MSFT @iSazonov @daxian-dbw @HemantMahawar @PaulHigin