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.

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

Don't escape entities in CDATA sections #633

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

wackbyte
Copy link
Contributor

@wackbyte wackbyte commented Jan 14, 2024

Purpose / Goal

Changes the parsing of CDATA sections to not escape entities.

In other words:

  • Before: <[CDATA[&amp;]]> -> &
  • After: <[CDATA[&amp;]]> -> &amp;

I also added a new test so that this behavior can be tested on its own.

Please note that I did not attempt to avoid any breaking changes here as the issue suggested, but if it is desired, I would be happy to gate this fix behind a new option.

The contents of a CDATA section will also now be run through tagValueProcessor even when the cdataPropName option is provided. I am unsure as to if this inconsistency was intentional (the code that would have made it consistent seems to have been commented out a while ago), so if that is considered an unrelated change, I would also be happy to split it out into a separate PR.

Closes #632.

Performance tests

Before:

fxp v3 : 57564.501862728255 requests/second
fxp : 32731.178545462913 requests/second
fxp - preserve order : 35714.0221273455 requests/second
xmlbuilder2 : 15299.516192662688 requests/second
xml2js  : 18881.74875804895 requests/second

After:

fxp v3 : 57721.92080194106 requests/second
fxp : 36231.444954502214 requests/second
fxp - preserve order : 39372.370501577694 requests/second
xmlbuilder2 : 15294.661359575506 requests/second
xml2js  : 18533.786914067532 requests/second

Type

Please mention the type of PR

  • Bug Fix
  • Refactoring / Technology upgrade
  • New Feature

@amitguptagwl
Copy link
Member

Thanks for you PR. let me check

@coveralls
Copy link

Coverage Status

coverage: 98.251% (+0.004%) from 98.247%
when pulling 66384d1 on wackbyte:cdata
into 291fe73 on NaturalIntelligence:master.

@amitguptagwl amitguptagwl merged commit 92e07cb into NaturalIntelligence:master Jan 19, 2024
7 of 8 checks passed
@wackbyte wackbyte deleted the cdata branch January 19, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entities are being processed in CDATA sections
3 participants