-
Notifications
You must be signed in to change notification settings - Fork 913
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
[Console] Add support for JSON with long numerals #4562
Conversation
93810f3
to
0728d74
Compare
Codecov Report
@@ Coverage Diff @@
## main #4562 +/- ##
==========================================
+ Coverage 66.39% 66.43% +0.04%
==========================================
Files 3397 3398 +1
Lines 64804 64904 +100
Branches 10360 10385 +25
==========================================
+ Hits 43026 43120 +94
- Misses 19218 19226 +8
+ Partials 2560 2558 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
0728d74
to
986acb1
Compare
Nice addition, can you add documentation for the change in the package @AMoo-Miki ? |
986acb1
to
92f46b5
Compare
re-running failed CIs |
@AMoo-Miki Hi Miki, Would you check the CI failures? Is it because we haven't release the js client with the fix? |
Thanks. This needs to be bumped to use the 2.3.1 client. |
js client 2.3.1 is out. Would you update the PR? Thx! @AMoo-Miki |
I am stuck with making a decision for how this should be implemented as we don't want to add it all over OSD. Will continue thinking. |
@AMoo-Miki should i remove the 2.10 tag for this then? |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-4562-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0709333c641f1e2bcaf116eb7d5504b33afcd71d
# Push it to GitHub
git push --set-upstream origin backport/backport-4562-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one concern about how we are exposing the custom client. The rest are just minor nits or opinions, feel free to use them if you think it makes sense :)
public readonly asCurrentUser: OpenSearchClient, | ||
public readonly asInternalUserWithLongNumeralsSupport: OpenSearchClient, | ||
public readonly asCurrentUserWithLongNumeralsSupport: OpenSearchClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is verbose, cant we make it more generic? This is the first customization that we are introducing but it doubt it will be the last. And since this client will become a part of the API it will be annoying to change this in the future.
interface ExtendedClient extends OpenSearchClient {
_custom : {
[key: string]: OpenSearchClient,
}
}
public readonly asCurrentUser: OpenSearchClient, | |
public readonly asInternalUserWithLongNumeralsSupport: OpenSearchClient, | |
public readonly asCurrentUserWithLongNumeralsSupport: OpenSearchClient | |
public readonly asCurrentUser: ExtendedClient, |
And add the custom long numeral client like:
internalClient.customClient = {
longNumeral: client,
}
const maxIntAsString = String(Number.MAX_SAFE_INTEGER); | ||
const maxIntLength = maxIntAsString.length; | ||
// Sub-patterns for each digit | ||
const longNumeralMatcherTokens = [`\\d{${maxIntAsString.length + 1},}`]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Cant we make this a lot simpler by just converting anything greater than a generic high number? e.g. 9000000000000000. What you are doing should work, but this reduces the matchers from 12 to 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explained why not in the comments in the code. Before parsing the long string, I wouldn't know what's in it. Assuming 8192 is max int, if i take everything greater than 8000 and turn them into BigInt, that would be unexpected to everyone who don't use numbers bigger than 8192. Hence, doing this one time activity of making a marginally more complex regex.
? BigInt(val.substring(length)) | ||
: val; | ||
|
||
/* For better performance, instead of testing for existence of `reviver` on each value, two almost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt understand this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code could be
loop {
condition ? doA : doB
}
but then condition is evaluated repeatedly. I decided to do
condition ? loop { doA } : loop { doB }
this results in a little less maintainable code.
/* Experiments with different combinations of various lengths, until one is found to not be in | ||
* the input string. | ||
*/ | ||
const getMarker = (text: string): { marker: string; length: number } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: cant we use UUID here? That way instead of a marker system, we can keep a dictionary of uuid's and values that can be substituted when parsing fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a value uses UUIDs? also, the longer the UUID, the more expensive my regex will be. Some more details here.
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]> Signed-off-by: Kawika Avilla <[email protected]>
Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]> Signed-off-by: Kawika Avilla <[email protected]> Co-authored-by: Miki <[email protected]>
Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]> Signed-off-by: Kawika Avilla <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit e69d38d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` (cherry picked from commit e69d38d) Signed-off-by: Miki <[email protected]> Signed-off-by: Kawika Avilla <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Miki <[email protected]>
…-project#4562) (opensearch-project#4967) (opensearch-project#4973)" This reverts commit e3cebe7. Signed-off-by: Kawika Avilla <[email protected]>
@AMoo-Miki I have reverted this on 2.10 and tagged with 2.11. The reason being I'm not positive if the JS client cherry pick commit opensearch-project/opensearch-js#550 is intended to make the client 2.x version to be non-breaking for major version bump. So that we can utilize the declaration of |
) (#4973)" (#4977) This reverts commit e3cebe7. Signed-off-by: Kawika Avilla <[email protected]>
) (#4973)" (#4977) This reverts commit e3cebe7. Signed-off-by: Kawika Avilla <[email protected]> (cherry picked from commit b75edfa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
) (#4973)" (#4977) (#4978) This reverts commit e3cebe7. (cherry picked from commit b75edfa) Signed-off-by: Kawika Avilla <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@AMoo-Miki will address the backport |
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` Signed-off-by: Miki <[email protected]>
…#4562)(opensearch-project#5130) Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` * Improve testing of clients with long numerals support Signed-off-by: Miki <[email protected]>
Also: * Add support for parsing and stringifying JSON with long numerals into `@osd/std` * Upgrade to `@opensearch/[email protected]` which supports long numerals * Add support for long numerals to `http/fetch` * Improve testing of clients with long numerals support Signed-off-by: Miki <[email protected]>
Add support for JSON with long numerals in Dev Tools.
Also:
@osd/std
@opensearch/[email protected]
which supports long numeralshttp/fetch
Description
Issues Resolved
Fixes #4536
Screenshot
Before
before-long-numerals-fix.mp4
After
after-long-numerals-fix.mp4
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr