Skip to content

Fix xml nesting bug in CustomSerializer.WriteMemberInfoCollection()#8476

Merged
iSazonov merged 9 commits intoPowerShell:masterfrom
IISResetMe:patch/invalid-xml-CustomSerialization
Dec 22, 2018
Merged

Fix xml nesting bug in CustomSerializer.WriteMemberInfoCollection()#8476
iSazonov merged 9 commits intoPowerShell:masterfrom
IISResetMe:patch/invalid-xml-CustomSerialization

Conversation

@IISResetMe
Copy link
Collaborator

PR Summary

Fix #8308

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.

This PR moves the WriteEndElement() call up immediately after the elements text value has been written.

PR Checklist

Decrementing `depth` makes no sense here and causes nested PSObject instances with more than one property to generate invalid xml
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
…-Xml.Tests.ps1

Co-Authored-By: IISResetMe <IISResetMe@users.noreply.github.com>
@@ -691,7 +691,6 @@ private void WriteMemberInfoCollection(
{
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, I renamed the first parameter to members as well

Removed unused parameters `depth` and `writeEnclosingMemberSetElementTag`, renamed `me` to `members`
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.

LGTM

@iSazonov
Copy link
Collaborator

The original code is obviously broken.
As for the depth, I am afraid that this fix is not entirely correct. I assume that recursion was originally meant to serialize members.
@mklement0 What do you think?

@IISResetMe
Copy link
Collaborator Author

@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.

@iSazonov
Copy link
Collaborator

@IISResetMe In the case I'd keep the method signature. And could you please open new issue about the wrong depth ignoring?

@IISResetMe
Copy link
Collaborator Author

I'll revert the last commit, but plan on tackle the depth issue as part of #7290

@IISResetMe
Copy link
Collaborator Author

@iSazonov Reverted depth-related refactorings

@iSazonov iSazonov self-assigned this Dec 19, 2018
@iSazonov
Copy link
Collaborator

@IISResetMe Please add a empty commit with [Feature] in title before we merge..

@iRon7
Copy link

iRon7 commented Dec 20, 2018

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!
Refer to the discussion in case #8393 from @mklement0 and StackOverflow Question "Unexpected ConvertTo-Json results? Answer: it has a default -Depth of 2" concerning the perception of the default -Depth (of 2) of the similar cmdlet: ConvertTo-Json.

@iSazonov
Copy link
Collaborator

@iRon7 In the PR we fix a bug in ConvertTo-Xml cmdlet but you reference a ConvertTo-Json - could you clarify that you mean?

@iRon7
Copy link

iRon7 commented Dec 20, 2018

I am sorry, I guess my comment wasn't that clear. What I mean is, that appearently every user of the ConvertTo-Json cmdlets appears to trapped by the pitfall that the -Depthis limited by default to 2. The issues with ConvertTo-Xml are less than ConvertTo-Json which could be explained by usage but also by the concerned bug. Meaning that if the default depth now suddenly changes to 2, users might experience the same type of issues as with ConvertTo-Json. Besides, scripts that used to work might now suddenly break and have truncated property paths...

@iSazonov
Copy link
Collaborator

@iRon7 The PR doesn't change defaults. It only fix a bug. Defaults we'll change after PowerShell Committee review #7290.

@IISResetMe
Copy link
Collaborator Author

@iRon7 one approach could be:

  • Fix depth enforcement
  • Set default depth = 5

Doesnt have to be 2 just because that's the case for the json cmdlet

@iRon7
Copy link

iRon7 commented Dec 20, 2018

@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.
E.g.:

Get-Service | ConvertTo-Xml

takes ~700ms, where:

Get-Service | ConvertTo-Xml -Depth 5

takes 6.5 minutes...

@PaulHigin
Copy link
Contributor

@IISResetMe
I still feel the unused method parameters should be removed. Yes, they should eventually be implemented, but since they are not now, they should be removed. It is Ok to add a TODO comment and create an Issue to add the functionality later.

@iSazonov
Copy link
Collaborator

@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?

takes 6.5 minutes...

@iRon7 Please open new performance issue so we can investigate this problem.

@iRon7
Copy link

iRon7 commented Dec 21, 2018

@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 -Depth, you might introduce a few performance complains with existing scripts.
Anyway, it is totally up to you what to decide (any direction in this delimema has its pro's and cons).

@iSazonov
Copy link
Collaborator

@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?

@SteveL-MSFT
Copy link
Member

I think the scope of these changes is ok to accept without waiting on conclusion of #7290

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 22, 2018
@iSazonov iSazonov merged commit 0e971a4 into PowerShell:master Dec 22, 2018
@iRon7
Copy link

iRon7 commented Jan 6, 2019

I just found a similar issue with a PSCustomObject and a recurring reference:

$Parent = [PSCustomObject]@{
	Name = "Parent"
}
$Child = [PSCustomObject]@{
	Name = "Child"
}
$Parent | Add-Member Child $Child
$Child | Add-Member Parent $Parent

But the error message is completely different from #7290

PS C:\> $Parent | ConvertTo-Xml

ConvertTo-Xml : Unexpected end of file has occurred. The following elements are not closed: Objects. Line 9, position 12.
At line:1 char:11

  • $Parent | ConvertTo-Xml
  •       ~~~~~~~~~~~~~
    
    • CategoryInfo : NotSpecified: (:) [ConvertTo-Xml], XmlException
    • FullyQualifiedErrorId : System.Xml.XmlException,Microsoft.PowerShell.Commands.ConvertToXmlCommand

I am not sure whether this is covered in the upcoming fix and tests
Anyways, let me know if you like me to open a new bug report for this.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 6, 2019

@iRon7 You can download a latest night build from main page of the repo and test.
For your example I get:

$Parent | ConvertTo-Xml

xml                            Objects
---                            -------
version="1.0" encoding="utf-8" Objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConvertTo-Xml fails with nested custom objects: omits closing </Objects> tag

5 participants