Skip to content
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

fix(XMLSerializer): Preserve whitespace character references #310

Merged
merged 12 commits into from
Sep 14, 2021
Merged

fix(XMLSerializer): Preserve whitespace character references #310

merged 12 commits into from
Sep 14, 2021

Conversation

karfau
Copy link
Member

@karfau karfau commented Aug 29, 2021

BREAKING CHANGE: If you relied on the not spec compliant preservation of \t, \n or \r in attribute values. To preserve those you will have to create XML that instead contains the correct numerical (or hexadecimal) equivalent (	, 
, 
).

@karfau karfau added this to the 0.8.0 milestone Aug 29, 2021
@karfau karfau added the breaking change Some thing that requires a version bump due to breaking changes label Aug 29, 2021
@karfau karfau mentioned this pull request Aug 29, 2021
Closed
@karfau karfau requested a review from brodybits August 29, 2021 07:33
@brodybits
Copy link
Member

I have a couple of questions:

I am also wondering if xmltest can eventually be considered a total overlap with the tests we have added and fixed, or if we will just keep it as an extra test in the long term.

brodybits
brodybits approved these changes Aug 29, 2021
View reviewed changes
Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

It would be nice to get my questions answered but I would consider them to be non-blocking.

@karfau
Copy link
Member Author

karfau commented Aug 29, 2021

I am also wondering if xmltest can eventually be considered a total overlap with the tests we have added and fixed, or if we will just keep it as an extra test in the long term.

My perspective on xmltest is that it provides an orientation regarding going into the "right"/expected direction (actual getting closer to or same as expected) and I would prefer to keep this, even if it's treated as a separate test run (e.g. only after the other tests passed).
The drawback is that they are not very good at communicating why something is expected.

The more I work with all the tests, the clearer the redundancies get, despite all the chaos. I think at some point not to far in the future we will be able to unclutter this without loosing any coverage/mutation score.

Regarding comments in lib, I usually put them to make things more obvious.
The git history always provides more information on details. But I'll check.

karfau
karfau commented Aug 29, 2021
View reviewed changes
Copy link
Member Author

@karfau karfau left a comment

Choose a reason for hiding this comment

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

Your suggestions for additional comments make total sense

lib/dom.js Outdated Show resolved Hide resolved
test/parse/parse-element.test.js Show resolved Hide resolved
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch from 5217b37 to 6840849 Compare August 29, 2021 19:56
brodybits
brodybits approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

LGTM with a couple more nits. Yeah your comments make total sense.

lib/dom.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch from ff7210e to 63538b3 Compare September 2, 2021 06:19
@karfau karfau changed the base branch from master to 49-more-linebreaks September 2, 2021 06:19
Base automatically changed from 49-more-linebreaks to master September 9, 2021 06:09
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch 2 times, most recently from f204e98 to 84ef8c7 Compare September 9, 2021 14:12
karfau and others added 10 commits September 9, 2021 16:12
@karfau
fix: Normalize all line endings
6478b29 
according to XML1.1 spec. Fixes #49

Since #307 only implemented XML 1.0 spec

https://www.w3.org/TR/xml/#sec-line-ends
https://www.w3.org/TR/xml11/#sec-line-ends
@karfau
fix(XMLSerializer): Preserve whitespace character references
3f8a96b
@karfau
fix(sax): Normalize whitespace in input to spaces
54db4f6 
as specified in https://www.w3.org/TR/xml/#AVNormalize
and document the parts that are not implemented
@karfau
style: Apply suggested formatting in lib
bb2b10d 
Co-authored-by: Chris Brody <git.brodybits@gmail.com>
@karfau
style: Apply suggested blank lines in tests
acac9e6 
Co-authored-by: Chris Brody <git.brodybits@gmail.com>
@karfau
test: Adapt tests after combining fixes
f1eaa21
@karfau
Update lib/dom.js
6d55338
@karfau
Update test/parse/parse-element.test.js
bb6c23b
@karfau @brodybits
style(dom): Correct whitespace (pun intended)
e81984f 
Co-authored-by: Chris Brody <chris.brody+brodybits@gmail.com>
@karfau
docs(dom): Elaborate comment for addSerializedAttribute
84ef8c7 
and update links to XML 1.1
brodybits
brodybits reviewed Sep 14, 2021
View reviewed changes
Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Still LGTM with a minor comment / question

lib/dom-parser.js Outdated Show resolved Hide resolved
lib/dom.js
Comment on lines +1140 to +1141
* > The replacement text of any entity referred to directly or indirectly
* > in an attribute value must not contain a <.
Copy link
Member

Choose a reason for hiding this comment

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

+1 for starting quoted lines with > here 👍

@karfau karfau merged commit 40745c6 into master Sep 14, 2021
@karfau karfau deleted the 284-serialize-whitespace-literals-to-character-references branch September 14, 2021 05:58
This was referenced Dec 25, 2021
Open
Closed
Merged
Closed
Merged
Closed
Merged
Merged
Closed
Merged
Merged
gkwang added a commit to auth0/node-xml-encryption that referenced this pull request Oct 18, 2022
@gkwang
release 3.0.0
a3ff9cd 
add semgrep support
uodate xmldom
breaking changes in xmldom:
xmldom/xmldom#310
xmldom/xmldom#314
xmldom/xmldom#307
@gkwang gkwang mentioned this pull request Oct 18, 2022
Merged
4 tasks
gkwang added a commit to auth0/node-xml-encryption that referenced this pull request Oct 18, 2022
@gkwang
release 3.0.0 (#104)
6ae0fcd 
add semgrep support
uodate xmldom
breaking changes in xmldom:
xmldom/xmldom#310
xmldom/xmldom#314
xmldom/xmldom#307
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brodybits brodybits brodybits approved these changes

Assignees
No one assigned
Labels
breaking change Some thing that requires a version bump due to breaking changes
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

Incorrect serialization of XML attributes containing white space
2 participants
@karfau @brodybits