Fix xml nesting bug in CustomSerializer.WriteMemberInfoCollection()#8476
Conversation
Decrementing `depth` makes no sense here and causes nested PSObject instances with more than one property to generate invalid xml
This reverts commit 56e07a0.
WriteMemberInfoCollection() incorrectly nests sibling elements at depth==0 The reason seems to be that WriteMemberInfoCollection() calls WriteEndElement() at most once, even if `WriteStartElement()` has been called more than once
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Xml.Tests.ps1
Outdated
Show resolved
Hide resolved
…-Xml.Tests.ps1 Co-Authored-By: IISResetMe <IISResetMe@users.noreply.github.com>
| @@ -691,7 +691,6 @@ private void WriteMemberInfoCollection( | |||
| { | |||
There was a problem hiding this comment.
These changes look right to me ... not sure what was originally intended. But can you also remove the depth and writeEnclosingememberSetElementTag parameters? They are not used within the method and it would help clean up the code by removing them (also please remove the xml comments for the parameters).
There was a problem hiding this comment.
Done, I renamed the first parameter to members as well
Removed unused parameters `depth` and `writeEnclosingMemberSetElementTag`, renamed `me` to `members`
…b.com/IISResetMe/PowerShell into patch/invalid-xml-CustomSerialization
|
The original code is obviously broken. |
|
@iSazonov I agree that depth limitations are not at all correctly imposed by the serializer right now, but that's a broader issue that should be addressed in a separate issue/PR. This is simply an attempt to not produce invalid XML by fixing an adjecent bug. For the broader issue of depth limitations we'll need to address the fact that depth is currently ignored by container types as well. |
|
@IISResetMe In the case I'd keep the method signature. And could you please open new issue about the wrong depth ignoring? |
|
I'll revert the last commit, but plan on tackle the depth issue as part of #7290 |
|
@iSazonov Reverted |
|
@IISResetMe Please add a empty commit with |
|
Please be aware that this issue shouldn't only concern the technical fix of #7290 but also the presumed compatibility with prior versions. Meaning: a lot of scripts in the field might break due to this change! |
|
@iRon7 In the PR we fix a bug in ConvertTo-Xml cmdlet but you reference a ConvertTo-Json - could you clarify that you mean? |
|
I am sorry, I guess my comment wasn't that clear. What I mean is, that appearently every user of the |
|
@iRon7 one approach could be:
Doesnt have to be 2 just because that's the case for the json cmdlet |
|
@IISResetMe. Yes, changing the default depth might work but could cause a performance impact for existing scripts were the default depth (of 2) did work. Get-Service | ConvertTo-Xmltakes ~700ms, where: Get-Service | ConvertTo-Xml -Depth 5takes 6.5 minutes... |
|
@IISResetMe |
|
@PaulHigin A code you reference is not new code so the change is not related the PR. I believe we get better code with fixing #7290. @IISResetMe Are ready to fix #7290?
@iRon7 Please open new performance issue so we can investigate this problem. |
|
@iSazonov, I do not have a performance issue (...yet). Just some concerns that the fix (even it is technically correct) might break a few scripts in the field and if you try to prevent that with a higher default |
|
@SteveL-MSFT We need an additional review from MSFT experts and get a conclusion about @iRon7's concerns - can we merge before PowerShell Committee review #7290? |
|
I think the scope of these changes is ok to accept without waiting on conclusion of #7290 |
|
I just found a similar issue with a $Parent = [PSCustomObject]@{
Name = "Parent"
}
$Child = [PSCustomObject]@{
Name = "Child"
}
$Parent | Add-Member Child $Child
$Child | Add-Member Parent $ParentBut the error message is completely different from #7290 PS C:\> $Parent | ConvertTo-Xml
I am not sure whether this is covered in the upcoming fix and tests |
|
@iRon7 You can download a latest night build from main page of the repo and test. |
PR Summary
Fix #8308
WriteMemberInfoCollection()incorrectly nests sibling elements at depth==0The reason seems to be that
WriteMemberInfoCollection()callsWriteEndElement()at most once, even ifWriteStartElement()has been called more than once.This PR moves the
WriteEndElement()call up immediately after the elements text value has been written.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests