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

Replace JSON11-produced hex escape codes with unicode escape codes #8355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wjordan
Copy link

@wjordan wjordan commented Sep 26, 2024

Description

JSON11.stringify produces JSON5 documents with hex escape codes (\x1b), which aren't standard JSON and cause JSON.parse to error. When using JSON11, replace all \xXX escape codes with the JSON-compatible equivalent Unicode escape codes (\u00XX).

Issues Resolved

Fixes Partially addresses #7367.

Screenshot

Testing the changes

Included a unit test. Without the fix in this PR, the test fails:

    ✕ can handle BigInt values and ANSI escape sequences (2 ms)

  ● json › can handle BigInt values and ANSI escape sequences

    SyntaxError: Unexpected token x in JSON at position 19
        at JSON.parse (<anonymous>)

       96 |    * still need it in its string form to find a suitable marker.
       97 |    */
    >  98 |   obj = JSON.parse(text, checkForLargeNumerals);

With this fix, the test passes.

Changelog

  • fix: Replace JSON11 hex escape codes with unicode escape codes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

@wjordan
Copy link
Author

wjordan commented Sep 26, 2024

This may only be a partial fix for the overall issue. I found another JSON5-serialization PR at opensearch-project/opensearch-js#784 that will also need a similar patch.

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

JSON11.stringify produces JSON5 documents with hex escape codes (`\x1b`),
which aren't standard JSON and cause `JSON.parse` to error.
When using JSON11, replace all `\xXX` escape codes with the JSON-compatible
equivalent Unicode escape codes (`\u00XX`).

Partially addresses opensearch-project#7367.

Signed-off-by: Will Jordan <[email protected]>
@wjordan wjordan force-pushed the json-stringify-hex-escape-fix branch from ac0eae9 to 1bb3782 Compare September 27, 2024 17:17
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.92%. Comparing base (1db585d) to head (1bb3782).
Report is 167 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8355      +/-   ##
==========================================
- Coverage   64.14%   60.92%   -3.23%     
==========================================
  Files        3743     3749       +6     
  Lines       88849    89022     +173     
  Branches    13856    13898      +42     
==========================================
- Hits        56993    54237    -2756     
- Misses      31241    31423     +182     
- Partials      615     3362    +2747     
Flag Coverage Δ
Linux_1 ?
Linux_2 56.35% <50.00%> (-2.52%) ⬇️
Linux_3 37.75% <100.00%> (-2.61%) ⬇️
Linux_4 29.94% <50.00%> (-1.64%) ⬇️
Windows_1 28.87% <50.00%> (-1.22%) ⬇️
Windows_2 56.30% <50.00%> (-2.52%) ⬇️
Windows_3 37.76% <100.00%> (-2.61%) ⬇️
Windows_4 29.94% <50.00%> (-1.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Sep 27, 2024

JSON11 v2.0.0 was published with a change in behavior.

@wjordan can u try bumping JSON11 to 2, without the changes in this PR to see if it solves it for you?

@AMoo-Miki AMoo-Miki self-assigned this Sep 27, 2024
@wjordan
Copy link
Author

wjordan commented Oct 4, 2024

I haven't managed to verify the updated package in my deployment, this patch fixed my issue for now so further followup has been a lower priority.

I gave the updated json11 npm module source a quick look and the added option to disable the hex-escape code should work fine as an alternative to the changes here. It seems to be missing a corresponding commit in the GitHub repo though.

Feel free to use the unit test in this PR to verify the module change works equally well to fix the issue.

@ananzh ananzh added the v2.19.0 label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants