Add `ConvertTo-CliXml` and `ConvertFrom-CliXml` cmdlets by ArmaanMcleod · Pull Request #21011 · PowerShell/PowerShell

Armaan Mcleod (ArmaanMcleod) · 2024-01-06T01:52:44Z

PR Summary

Add ConvertTo-CliXml and ConvertFrom-CliXml cmdlets to work with CliXml objects in memory without needing to read/write to file system.

PR Context

Parameter sets

ConvertTo-CliXml [-InputObject] <psobject> [-Depth <int>] [<CommonParameters>]

ConvertFrom-CliXml [-InputObject] <string> [<CommonParameters>]

Usage

> $cliXmlString = [pscustomobject]@{'hello' = 1} | ConvertTo-CliXML
> $cliXmlString
<Objs Version="1.1.0.1" xmlns="http://schemas.microsoft.com/powershell/2004/04">
  <Obj RefId="0">
    <TN RefId="0">
      <T>System.Management.Automation.PSCustomObject</T>
      <T>System.Object</T>
    </TN>
    <MS>
      <I32 N="hello">1</I32>
    </MS>
  </Obj>
</Objs>
> $cliXmlString | ConvertFrom-CliXml

hello
-----
    1

Alot of this was inspired from #12845, but that PR is quite old and never got merged. I just created a new PR and made some changes since it was easier.

Also included internal static string Serialize(IList<object> source, int depth, bool enumerate) in PSSerializer API. This was to be able to enumerate and serialize objects one at a time instead of writing one top level object.

PR Checklist

Ilya (iSazonov) · 2024-01-06T07:44:11Z

Did you address #12845 (comment)?

Armaan Mcleod (ArmaanMcleod) · 2024-01-06T09:57:28Z

Did you address #12845 (comment)?

I'm not sure if I agree with that comment. The existing Import-CliXml/Export-CliXml cmdlets don't even use the PSSeralizer APIs, they use InternalSerializer/InternalDeserializer classes.

It can probably be done for ConvertFrom-CliXml/ConvertTo-CliXml but there is a lot of rework since PSSerializer don't have access to ImportXmlHelper class and methods like Import(), which has all the pagination support for the Export and Import cmdlets.

Also reasoning for using PSSerializer APIs doesn't make sense to me still, they are using .NET serializer API under the hood anyways.

Ilya (iSazonov) · 2024-01-06T10:30:17Z

Also reasoning for using PSSerializer APIs doesn't make sense to me still, they are using .NET serializer API under the hood anyways.

There are security "wrappers" specific to PowerShell. We should follow that.
As for PSSerializer it was mentioned as public API. I think it is based on InternalSerializer/InternalDeserializer. So, if it is impossible use the public API you could use internal API. I suggest you directly consult with Travis Plunk (@TravisEz13) before you start the security sensitive work.

Armaan Mcleod (ArmaanMcleod) · 2024-01-06T10:48:21Z

Also reasoning for using PSSerializer APIs doesn't make sense to me still, they are using .NET serializer API under the hood anyways.

There are security "wrappers" specific to PowerShell. We should follow that. As for PSSerializer it was mentioned as public API. I think it is based on InternalSerializer/InternalDeserializer. So, if it is impossible use the public API you could use internal API. I suggest you directly consult with Travis Plunk (@TravisEz13) before you start the security sensitive work.

I think for using security wrapper PSSerializer it is possible, but it requires creating more overloads for cmdlet PagingParameters to get similar implementation of XmlHelper.Import() code. That is currently not possible in PSSerializerclass alone. If that is something that MSFT team is fine with, we can do it, otherwise we have to use internal API code already being used for import/export CLIXML cmdlets.

Armaan Mcleod (ArmaanMcleod) · 2024-01-06T11:06:32Z

Ilya (@iSazonov) Travis Plunk (@TravisEz13) I have converted code to use PSSerializer API but had to include some overloads to support PagingParameters, which is basically a copy of XmlHelper.Import() implementation, but returns a list of objects instead. This was the only way to address security concern and use PSSerializer API.

If introducing these overloads is not acceptable, I can go back to previous approach with internal API. I can also not make the access modifiers not public and maybe do internal as well if that is better.

Armaan Mcleod (ArmaanMcleod) · 2024-01-08T21:19:06Z

Ilya (@iSazonov) Travis Plunk (@TravisEz13) I have converted code to use PSSerializer API but had to include some overloads to support PagingParameters, which is basically a copy of XmlHelper.Import() implementation, but returns a list of objects instead. This was the only way to address security concern and use PSSerializer API.

If introducing these overloads is not acceptable, I can go back to previous approach with internal API. I can also not make the access modifiers not public and maybe do internal as well if that is better.

Reverted this as we don't see a need for SupportsPaging capability in this cmdlet, since -IncludeTotalCount is not helpful and -First/-Skip can be achieved with Select-Object. It also reduces code complexity with having to get this to work with PSSerializer API since it doesn't support this.

Armaan Mcleod (ArmaanMcleod) · 2024-01-09T11:15:53Z

Thanks Michael Klement (@mklement0) and Ilya (@iSazonov) for comprehensive and very helpful reviews 🙂.

I have included an PSSerializer overload for the time-being internal static string Serialize(object[] source, int depth, bool enumerate), and seems to work well and matches up behaviour with Import/Export cmldets. Happy to change this but I think this is an easy way to solve problem. I've kept it internal since don't want to expose this API unless its ready to go public, since I think we will go through more iterations in this PR and when we refactor the other CliXML cmdlets to use public API. I think there are many ways to do the API but main thing is we need the enumerate logic for serialization to avoid writing one whole top level object each time.

Please review if you get a chance and let me know if I've missed anything.

Ilya (iSazonov)

Armaan Mcleod (@ArmaanMcleod) Please replace tabs in tests if you see them.

And if you don't plan new commits I suggest squash commits (to many commits, too many comments) and open new PR.

pull-request-quantifier-deprecated · 2024-01-12T15:16:11Z

This PR has 326 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +316 -10
Percentile : 72.6%

Total files changed: 8

Change summary by file extension:
.cs : +83 -0
.psd1 : +3 -2
.ps1 : +230 -8

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Armaan Mcleod (ArmaanMcleod) · 2024-01-13T07:16:20Z

Armaan Mcleod (@ArmaanMcleod) Please replace tabs in tests if you see them.

And if you don't plan new commits I suggest squash commits (to many commits, too many comments) and open new PR.

Thanks Ilya (@iSazonov)

I have opened up a new PR #21063. Closing this one in favour of that one.

pull-request-quantifier-deprecated Bot added the Large label Jan 6, 2024

Ryan Yates (kilasuit) added WG-Cmdlets-Utility WG-NeedsReview Needs a review by the labeled Working Group labels Jan 6, 2024

Armaan Mcleod (ArmaanMcleod) mentioned this pull request Jan 6, 2024

Document ConvertTo-CliXml and ConvertFrom-CliXml cmdlets MicrosoftDocs/PowerShell-Docs#10773

Closed

2 tasks

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024

View reviewed changes

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/clixml.tests.ps1 Outdated

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 6, 2024

Ilya (iSazonov) reviewed Jan 6, 2024

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs Outdated

Ilya (iSazonov) reviewed Jan 6, 2024

View reviewed changes

Comment thread test/powershell/engine/Basic/DefaultCommands.Tests.ps1 Outdated

Armaan Mcleod (ArmaanMcleod) force-pushed the convert-clixml-cmdlets branch from 10e6ea8 to 9428101 Compare January 7, 2024 04:48

Ilya (iSazonov) reviewed Jan 7, 2024

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs Outdated

Ilya (iSazonov) reviewed Jan 7, 2024

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs Outdated

Ilya (iSazonov) reviewed Jan 7, 2024

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs Outdated

pull-request-quantifier-deprecated Bot added Extra Large and removed Large labels Jan 9, 2024

Ilya (iSazonov) reviewed Jan 9, 2024

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/XmlCommands.cs Outdated

Comment thread src/System.Management.Automation/engine/serialization.cs Outdated

Armaan Mcleod (ArmaanMcleod) commented Jan 9, 2024

View reviewed changes

Comment thread src/System.Management.Automation/engine/serialization.cs Outdated

pull-request-quantifier-deprecated Bot added Large and removed Extra Large labels Jan 9, 2024

Ilya (iSazonov) reviewed Jan 10, 2024

View reviewed changes

Comment thread src/System.Management.Automation/engine/serialization.cs Outdated

Ilya (iSazonov) reviewed Jan 11, 2024

View reviewed changes

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1 Outdated

Ilya (iSazonov) reviewed Jan 11, 2024

View reviewed changes

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1 Outdated

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1 Outdated

Comment thread src/System.Management.Automation/engine/serialization.cs Outdated

Armaan Mcleod (ArmaanMcleod) force-pushed the convert-clixml-cmdlets branch from d9068e2 to 28fbf0b Compare January 12, 2024 11:12

Add ConvertFrom-CliXml & ConvertTo-CliXml cmdlets

d11aec7

Add tests

10708e4

Armaan Mcleod (ArmaanMcleod) force-pushed the convert-clixml-cmdlets branch from 471dda7 to 10708e4 Compare January 12, 2024 11:45

Armaan Mcleod (ArmaanMcleod) added 2 commits January 13, 2024 02:04

Fix new overload XML doc summary

51d3480

Grouping cmdlets under context and simplifying test titles

c331bb4

Armaan Mcleod (ArmaanMcleod) closed this Jan 13, 2024

Armaan Mcleod (ArmaanMcleod) deleted the convert-clixml-cmdlets branch January 13, 2024 07:17

Ryan Yates (kilasuit) added WG-Cmdlets general cmdlet issues and removed WG-Cmdlets-Utility labels Jul 17, 2026

Uh oh!

Conversation

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

Parameter sets

Usage

PR Checklist

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jan 6, 2024

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jan 6, 2024

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pull-request-quantifier-deprecated Bot commented Jan 12, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Armaan Mcleod (ArmaanMcleod) commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 •

edited

Loading

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 •

edited

Loading

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 •

edited

Loading

Armaan Mcleod (ArmaanMcleod) commented Jan 6, 2024 •

edited

Loading

Armaan Mcleod (ArmaanMcleod) commented Jan 8, 2024 •

edited

Loading

Armaan Mcleod (ArmaanMcleod) commented Jan 13, 2024 •

edited

Loading