Skip to content

Commit

Permalink
[Discover] Add long numerals support (#5592) (#5798)
Browse files Browse the repository at this point in the history
* Fix the usage of `pegjs` in tasks



* [Discover] Add long numerals support

* Add long numerals support to Discover tabular view, document view, inspector, hash-url, and share
* Use a fork of `numeral-js` that supports long numerals
* Patch `rison` to correctly serialize BigInts
* Replace the uses of `withLongNumerals` with `withLongNumeralsSupport` to match the rest of the codebase and mark `withLongNumerals` for deprecation
* Add `withLongNumeralsSupport` as an option to `code/public/http/fetch`
* Add long numerals support to `UiSettingsClient`
* Add long numerals support to `core/server/http/router` response handler
* Add long numerals support to `RangeFilter` and `FilterBar`
* Add long numerals support to `kuery/ast`
* Introduce `OPENSEARCH_SEARCH_WITH_LONG_NUMERALS_STRATEGY` in `core/plugins/data/search`
* Remove `ng-non-bindable` leftovers



---------



(cherry picked from commit 10ae4ee)

Signed-off-by: Miki <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
  • Loading branch information
AMoo-Miki and ashwin-pc authored Feb 6, 2024
1 parent 769dbd4 commit 2fbe480
Show file tree
Hide file tree
Showing 53 changed files with 454 additions and 137 deletions.
3 changes: 1 addition & 2 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ target
# plugin overrides
/src/core/lib/osd_internal_native_observable
/src/legacy/plugin_discovery/plugin_pack/__tests__/fixtures/plugins/broken
/src/plugins/data/common/opensearch_query/kuery/ast/_generated_/**
/src/plugins/vis_type_timeline/public/_generated_/**
/src/plugins/**/_generated_/**

# package overrides
/packages/opensearch-eslint-config-opensearch-dashboards
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@
"@elastic/datemath": "5.0.3",
"@elastic/eui": "npm:@opensearch-project/[email protected]",
"@elastic/good": "^9.0.1-kibana3",
"@elastic/numeral": "^2.5.0",
"@elastic/numeral": "npm:@amoo-miki/[email protected].0",
"@elastic/request-crypto": "2.0.0",
"@elastic/safer-lodash-set": "0.0.0",
"@hapi/accept": "^5.0.2",
Expand Down
1 change: 1 addition & 0 deletions packages/osd-std/src/json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ export const parse = (
if (
numeralsAreNumbers &&
typeof val === 'number' &&
isFinite(val) &&
(val < Number.MAX_SAFE_INTEGER || val > Number.MAX_SAFE_INTEGER)
) {
numeralsAreNumbers = false;
Expand Down
3 changes: 2 additions & 1 deletion packages/osd-ui-shared-deps/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"dependencies": {
"@elastic/charts": "31.1.0",
"@elastic/eui": "npm:@opensearch-project/[email protected]",
"@elastic/numeral": "^2.5.0",
"@elastic/numeral": "npm:@amoo-miki/[email protected].0",
"@opensearch/datemath": "5.0.3",
"@osd/i18n": "1.0.0",
"@osd/monaco": "1.0.0",
Expand Down Expand Up @@ -52,3 +52,4 @@
"webpack": "npm:@amoo-miki/[email protected]"
}
}

13 changes: 13 additions & 0 deletions scripts/postinstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,19 @@ const run = async () => {
},
])
);
promises.push(
patchFile('node_modules/rison-node/js/rison.js', [
{
from: 'return Number(s)',
to:
'return isFinite(s) && (s > Number.MAX_SAFE_INTEGER || s < Number.MIN_SAFE_INTEGER) ? BigInt(s) : Number(s)',
},
{
from: 's = {',
to: 's = {\n bigint: x => x.toString(),',
},
])
);

await Promise.all(promises);
};
Expand Down
8 changes: 6 additions & 2 deletions src/core/public/http/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,9 @@ describe('Fetch', () => {
},
});

await expect(fetchInstance.fetch('/my/path', { withLongNumerals: true })).resolves.toEqual({
await expect(
fetchInstance.fetch('/my/path', { withLongNumeralsSupport: true })
).resolves.toEqual({
'long-max': longPositive,
'long-min': longNegative,
});
Expand All @@ -854,7 +856,9 @@ describe('Fetch', () => {
},
});

await expect(fetchInstance.fetch('/my/path', { withLongNumerals: true })).resolves.toEqual({
await expect(
fetchInstance.fetch('/my/path', { withLongNumeralsSupport: true })
).resolves.toEqual({
'long-max': longPositive,
'long-min': longNegative,
});
Expand Down
12 changes: 10 additions & 2 deletions src/core/public/http/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,20 @@ export class Fetch {
if (NDJSON_CONTENT.test(contentType)) {
body = await response.blob();
} else if (JSON_CONTENT.test(contentType)) {
body = fetchOptions.withLongNumerals ? parse(await response.text()) : await response.json();
// ToDo: [3.x] Remove withLongNumerals
body =
fetchOptions.withLongNumeralsSupport || fetchOptions.withLongNumerals
? parse(await response.text())
: await response.json();
} else {
const text = await response.text();

try {
body = fetchOptions.withLongNumerals ? parse(text) : JSON.parse(text);
// ToDo: [3.x] Remove withLongNumerals
body =
fetchOptions.withLongNumeralsSupport || fetchOptions.withLongNumerals
? parse(text)
: JSON.parse(text);
} catch (err) {
body = text;
}
Expand Down
3 changes: 3 additions & 0 deletions src/core/public/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ export interface HttpFetchOptions extends HttpRequestInit {
* When `true`, if the response has a JSON mime type, the {@link HttpResponse} will use an alternate JSON parser
* that converts long numerals to BigInts. Defaults to `false`.
*/
withLongNumeralsSupport?: boolean;

/** @deprecated use {@link withLongNumeralsSupport} instead */
withLongNumerals?: boolean;
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 14 additions & 1 deletion src/core/public/ui_settings/ui_settings_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,13 @@ import { UiSettingsClient } from './ui_settings_client';
let done$: Subject<unknown>;

function setup(options: { defaults?: any; initialSettings?: any } = {}) {
const { defaults = { dateFormat: { value: 'Browser' } }, initialSettings = {} } = options;
const {
defaults = {
dateFormat: { value: 'Browser' },
aLongNumeral: { value: `${BigInt(Number.MAX_SAFE_INTEGER) + 11n}`, type: 'number' },
},
initialSettings = {},
} = options;

const batchSet = jest.fn(() => ({
settings: {},
Expand All @@ -62,6 +68,7 @@ describe('#get', () => {
it('gives access to uiSettings values', () => {
const { client } = setup();
expect(client.get('dateFormat')).toMatchSnapshot();
expect(client.get('aLongNumeral')).toBe(BigInt(Number.MAX_SAFE_INTEGER) + 11n);
});

it('supports the default value overload', () => {
Expand All @@ -82,13 +89,19 @@ describe('#get', () => {
const { client } = setup();
// if you are hitting this error, then a test is setting this client value globally and not unsetting it!
expect(client.isDefault('dateFormat')).toBe(true);
expect(client.isDefault('aLongNumeral')).toBe(true);

const defaultDateFormat = client.get('dateFormat');
const defaultLongNumeral = client.get('aLongNumeral');

expect(client.get('dateFormat', 'xyz')).toBe('xyz');
expect(client.get('aLongNumeral', 1n)).toBe(1n);

// shouldn't change other usages
expect(client.get('dateFormat')).toBe(defaultDateFormat);
expect(client.get('dataFormat', defaultDateFormat)).toBe(defaultDateFormat);
expect(client.get('aLongNumeral')).toBe(defaultLongNumeral);
expect(client.get('aLongNumeral', defaultLongNumeral)).toBe(defaultLongNumeral);
});

it("throws on unknown properties that don't have a value yet.", () => {
Expand Down
10 changes: 5 additions & 5 deletions src/core/public/ui_settings/ui_settings_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,11 @@ You can use \`IUiSettingsClient.get("${key}", defaultValue)\`, which will just r
return JSON.parse(value);
}

if (type === 'number') {
return parseFloat(value);
}

return value;
return type === 'number' && typeof value !== 'bigint'
? isFinite(value) && (value > Number.MAX_SAFE_INTEGER || value < Number.MIN_SAFE_INTEGER)
? BigInt(value)
: parseFloat(value)
: value;
}

get$<T = any>(key: string, defaultOverride?: T) {
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/http/router/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ export interface HttpResponseOptions {
body?: HttpResponsePayload;
/** HTTP Headers with additional information about response */
headers?: ResponseHeaders;
/** Indicates if alternate serialization should be employed */
withLongNumeralsSupport?: boolean;
}

/**
Expand Down
12 changes: 11 additions & 1 deletion src/core/server/http/router/response_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
import typeDetect from 'type-detect';
import Boom from '@hapi/boom';
import * as stream from 'stream';
import { stringify } from '@osd/std';

import {
HttpResponsePayload,
Expand Down Expand Up @@ -108,9 +109,18 @@ export class HapiResponseAdapter {
opensearchDashboardsResponse: OpenSearchDashboardsResponse<HttpResponsePayload>
) {
const response = this.responseToolkit
.response(opensearchDashboardsResponse.payload)
.response(
opensearchDashboardsResponse.options.withLongNumeralsSupport
? stringify(opensearchDashboardsResponse.payload)
: opensearchDashboardsResponse.payload
)
.code(opensearchDashboardsResponse.status);
setHeaders(response, opensearchDashboardsResponse.options.headers);
if (opensearchDashboardsResponse.options.withLongNumeralsSupport) {
setHeaders(response, {
'content-type': 'application/json',
});
}
return response;
}

Expand Down
2 changes: 1 addition & 1 deletion src/plugins/console/public/lib/opensearch/opensearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export async function send(
body: data,
prependBasePath: true,
asResponse: true,
withLongNumerals: true,
withLongNumeralsSupport: true,
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export const setup = (
};

const wrap: HtmlContextTypeConvert = (value, options) => {
return `<span ng-non-bindable>${recurse(value, options)}</span>`;
return `<span>${recurse(value, options)}</span>`;
};

return wrap;
Expand Down
30 changes: 15 additions & 15 deletions src/plugins/data/common/field_formats/converters/color.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ describe('Color Format', () => {
jest.fn()
);

expect(colorer.convert(99, HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>99</span>');
expect(colorer.convert(99, HTML_CONTEXT_TYPE)).toBe('<span>99</span>');
expect(colorer.convert(100, HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">100</span></span>'
'<span><span style="color: blue;background-color: yellow;">100</span></span>'
);
expect(colorer.convert(150, HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">150</span></span>'
'<span><span style="color: blue;background-color: yellow;">150</span></span>'
);
expect(colorer.convert(151, HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>151</span>');
expect(colorer.convert(151, HTML_CONTEXT_TYPE)).toBe('<span>151</span>');
});

test('should not convert invalid ranges', () => {
Expand All @@ -73,7 +73,7 @@ describe('Color Format', () => {
jest.fn()
);

expect(colorer.convert(99, HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>99</span>');
expect(colorer.convert(99, HTML_CONTEXT_TYPE)).toBe('<span>99</span>');
});
});

Expand All @@ -94,26 +94,26 @@ describe('Color Format', () => {
);
const converter = colorer.getConverterFor(HTML_CONTEXT_TYPE) as Function;

expect(converter('B', HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>B</span>');
expect(converter('B', HTML_CONTEXT_TYPE)).toBe('<span>B</span>');
expect(converter('AAA', HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">AAA</span></span>'
'<span><span style="color: blue;background-color: yellow;">AAA</span></span>'
);
expect(converter('AB', HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">AB</span></span>'
'<span><span style="color: blue;background-color: yellow;">AB</span></span>'
);
expect(converter('a', HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>a</span>');
expect(converter('a', HTML_CONTEXT_TYPE)).toBe('<span>a</span>');

expect(converter('B', HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>B</span>');
expect(converter('B', HTML_CONTEXT_TYPE)).toBe('<span>B</span>');
expect(converter('AAA', HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">AAA</span></span>'
'<span><span style="color: blue;background-color: yellow;">AAA</span></span>'
);
expect(converter('AB', HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">AB</span></span>'
'<span><span style="color: blue;background-color: yellow;">AB</span></span>'
);
expect(converter('AB <', HTML_CONTEXT_TYPE)).toBe(
'<span ng-non-bindable><span style="color: blue;background-color: yellow;">AB &lt;</span></span>'
'<span><span style="color: blue;background-color: yellow;">AB &lt;</span></span>'
);
expect(converter('a', HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>a</span>');
expect(converter('a', HTML_CONTEXT_TYPE)).toBe('<span>a</span>');
});

test('returns original value (escaped) when regex is invalid', () => {
Expand All @@ -132,7 +132,7 @@ describe('Color Format', () => {
);
const converter = colorer.getConverterFor(HTML_CONTEXT_TYPE) as Function;

expect(converter('<', HTML_CONTEXT_TYPE)).toBe('<span ng-non-bindable>&lt;</span>');
expect(converter('<', HTML_CONTEXT_TYPE)).toBe('<span>&lt;</span>');
});
});
});
7 changes: 5 additions & 2 deletions src/plugins/data/common/field_formats/converters/numeral.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,14 @@ export abstract class NumeralFormat extends FieldFormat {
protected getConvertedValue(val: any): string {
if (val === -Infinity) return '-∞';
if (val === +Infinity) return '+∞';
if (typeof val !== 'number') {

const isBigInt = typeof val === 'bigint';

if (typeof val !== 'number' && !isBigInt) {
val = parseFloat(val);
}

if (isNaN(val)) return '';
if (!isBigInt && isNaN(val)) return '';

const previousLocale = numeral.language();
const defaultLocale =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('Source Format', () => {
};

expect(convertHtml(hit)).toBe(
'<span ng-non-bindable>{&quot;foo&quot;:&quot;bar&quot;,&quot;number&quot;:42,&quot;hello&quot;:&quot;&lt;h1&gt;World&lt;/h1&gt;&quot;,&quot;also&quot;:&quot;with \\&quot;quotes\\&quot; or &#39;single quotes&#39;&quot;}</span>'
'<span>{&quot;foo&quot;:&quot;bar&quot;,&quot;number&quot;:42,&quot;hello&quot;:&quot;&lt;h1&gt;World&lt;/h1&gt;&quot;,&quot;also&quot;:&quot;with \\&quot;quotes\\&quot; or &#39;single quotes&#39;&quot;}</span>'
);
});
});
Loading

0 comments on commit 2fbe480

Please sign in to comment.