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

[osd/std] Add additional recovery from false-positives in handling of long numerals #5956

Conversation

AMoo-Miki
Copy link
Collaborator

@AMoo-Miki AMoo-Miki commented Feb 26, 2024

Description

The parsing of JSON strings with long numerals in @osd/std handled two false-positive scenarios. A new one was discovered where mixed escaped quotes inside a string value resulted in a different exception in browsers that the one already handled for Node.

Issues Resolved

Fixes #5923

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

…ng numerals

Signed-off-by: Miki <miki@amazon.com>
@AMoo-Miki AMoo-Miki force-pushed the add-extra-recovery-for-long-numerals branch from c44ce2f to e77adaf Compare February 26, 2024 22:42
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 67.03%. Comparing base (6d5560e) to head (2d8d65f).
Report is 1 commits behind head on main.

Files Patch % Lines
packages/osd-std/src/json.ts 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5956      +/-   ##
==========================================
+ Coverage   67.01%   67.03%   +0.01%     
==========================================
  Files        3310     3310              
  Lines       63647    63651       +4     
  Branches    10165    10165              
==========================================
+ Hits        42656    42668      +12     
+ Misses      18522    18514       -8     
  Partials     2469     2469              
Flag Coverage Δ
Linux_1 35.21% <0.00%> (-0.01%) ⬇️
Linux_2 55.08% <16.66%> (-0.01%) ⬇️
Linux_3 44.38% <0.00%> (+<0.01%) ⬆️
Linux_4 35.19% <0.00%> (-0.01%) ⬇️
Windows_1 35.26% <0.00%> (+0.02%) ⬆️
Windows_2 55.05% <16.66%> (-0.01%) ⬇️
Windows_3 44.39% <0.00%> (-0.01%) ⬇️
Windows_4 35.19% <0.00%> (-0.01%) ⬇️

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.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Can we add tests to the corresponding test file for this change? looks like there is already a json.test.ts file

@AMoo-Miki
Copy link
Collaborator Author

The particular exception is only thrown in browsers. I have created an issue to add functional test to handle the new exception. The existing unit tests cover this case already.

ashwin-pc
ashwin-pc previously approved these changes Feb 27, 2024
@ashwin-pc
Copy link
Member

@AMoo-Miki can you link that issue here?

@AMoo-Miki
Copy link
Collaborator Author

@AMoo-Miki can you link that issue here?

#5957

@AMoo-Miki AMoo-Miki merged commit a7ab795 into opensearch-project:main Feb 27, 2024
67 of 68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 27, 2024
…ng numerals (#5956)

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit a7ab795)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 27, 2024
…ng numerals (#5956)

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit a7ab795)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
gaobinlong pushed a commit to gaobinlong/OpenSearch-Dashboards that referenced this pull request Feb 28, 2024
abbyhu2000 added a commit that referenced this pull request Mar 1, 2024
…ng numerals (#5956) (#5972)

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit a7ab795)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
abbyhu2000 pushed a commit that referenced this pull request Mar 1, 2024
…ng numerals (#5956) (#5973)

Signed-off-by: Miki <miki@amazon.com>
(cherry picked from commit a7ab795)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Mar 4, 2024
@atreyd
Copy link

atreyd commented Mar 20, 2024

@AMoo-Miki Could you please confirm if this is going to be released to 2.12.0 as it's already labeled as backport with 2.12.0 ?

@AMoo-Miki
Copy link
Collaborator Author

@atreyd the 2.12 branch is not going to receive a new release as 2.13.0 is just around the corner.

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.

[BUG] JSON parsing issue in Discover since upgrade to v2.12
5 participants