Dropdown Value Xml Escaping and unescaping Exception

[QilongTang]

Please Note:

1. Before submitting the PR, please review How to Contribute to Dynamo
2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
3. PRs will be reviewed from oldest to newest
4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

DYN-1195
Dropdown nodes can fail to maintain previous selection in Dynamo Player.

This PR fixes an edge case that when parsing selection which contains special characters in XML doc like &, <, >, etc, our code failed catch the exception causing unescaping to fail silently.

For example, Revit Fill Pattern selection contains which contains invalid XML string when parsing. Internally, we escape this string to be

"0:<Solid fill>"

After unescaping, the string will went back to

"0:<Solid Fill>"

Behavior after bug fixing

TODO:

• Unit Tests

Declarations

Check these if you believe they are true

• The code base is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @aparajit-pratap

FYIs

@BogdanZavu @dobriai

[mjkkirschner]

can you give some more background on this? Do we xml escape strings that we save as json? Why does invalid xml break json deserialization?

[QilongTang]

@mjkkirschner This is actually not part of Json deserialization, this expose a problem underneath that the undo, redo, update value commands are still using XML format. So executing these command still going through the XML code. I am not sure why we escape charters in this case, need to check with @sm6srw why he added this guard in the first place.

[mjkkirschner]

okay gotcha, it's a large refactor to remove this code, but it should be done eventually as of now we have to maintain double the serialization logic.

[QilongTang]

@mjkkirschner Yes, if we removed the XML serialization/ deserialization code and finished the refactor on time, we should not need this fix at all. But it is what it is.

[QilongTang]

Update to use native .Net functions instead of XML reader/writer for string with Xml special char escaping and unescaping. Unit tests added and passing.

Self CI passed in https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/176/

[QilongTang]

Ping

[QilongTang]

@mjkkirschner @aparajit-pratap Comments addressed which does not affect newly added unit tests

[mjkkirschner]

LGTM - do we need any updated tests in d4r @ZiyunShang @QilongTang ?

[QilongTang]

@mjkkirschner With this implementation, D4R code and tests can stay as its current state. I would like to run D4R self CI though @ZiyunShang @AndyDu1985 😉

[mjkkirschner]

@QilongTang do you want to wait to merge this until the tests are run on the revit side?

[QilongTang]

@mjkkirschner I think the unit tests in DynamoCore are sufficient. I did not have time to make a final testing on Revit side before merging this.

[QilongTang]

@mjkkirschner @aparajit-pratap @smangarole Added a gif for local testing results, merging