From 21ce99674da6c8764a3836958452e9f5da43aea2 Mon Sep 17 00:00:00 2001 From: igalklebanov Date: Wed, 17 Apr 2024 02:44:50 +0300 Subject: [PATCH 1/3] add option to not mutate result objects/arrays @ ParseJSONResultsPlugin. .. --- .../parse-json-results-plugin.ts | 64 +++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/src/plugin/parse-json-results/parse-json-results-plugin.ts b/src/plugin/parse-json-results/parse-json-results-plugin.ts index 899068d2a..71c5f0321 100644 --- a/src/plugin/parse-json-results/parse-json-results-plugin.ts +++ b/src/plugin/parse-json-results/parse-json-results-plugin.ts @@ -8,6 +8,27 @@ import { PluginTransformResultArgs, } from '../kysely-plugin.js' +export interface ParseJSONResultsPluginOptions { + /** + * When `'in-place'`, arrays' and objects' values are parsed in-place. This is + * the most time and space efficient option. + * + * This can result in runtime errors if some objects/arrays are readonly. + * + * When `'spread'`, arrays and objects are spread (`...`) into new arrays and + * objects. This is least efficient time-wise, but safe from readonly runtime + * errors. + * + * When `'create'`, new arrays and objects are created. This is least efficient + * space-wise, but safe from readonly runtime errors. + * + * Defaults to `'in-place'`. + */ + objectStrategy?: ObjectStrategy +} + +type ObjectStrategy = 'in-place' | 'spread' | 'create' + /** * Parses JSON strings in query results into JSON objects. * @@ -22,6 +43,12 @@ import { * ``` */ export class ParseJSONResultsPlugin implements KyselyPlugin { + readonly #objectStrategy: ObjectStrategy + + constructor(readonly opt: ParseJSONResultsPluginOptions = {}) { + this.#objectStrategy = opt.objectStrategy || 'in-place' + } + // noop transformQuery(args: PluginTransformQueryArgs): RootOperationNode { return args.node @@ -32,30 +59,36 @@ export class ParseJSONResultsPlugin implements KyselyPlugin { ): Promise> { return { ...args.result, - rows: parseArray(args.result.rows), + rows: parseArray(args.result.rows, this.#objectStrategy), } } } -function parseArray(arr: T[]): T[] { +function parseArray(arr: T[], objectStrategy: ObjectStrategy): T[] { + if (objectStrategy === 'spread') { + arr = [...arr] + } + + const target = objectStrategy === 'create' ? [] : arr + for (let i = 0; i < arr.length; ++i) { - arr[i] = parse(arr[i]) as T + target[i] = parse(arr[i], objectStrategy) as T } - return arr + return target } -function parse(obj: unknown): unknown { +function parse(obj: unknown, objectStrategy: ObjectStrategy): unknown { if (isString(obj)) { return parseString(obj) } if (Array.isArray(obj)) { - return parseArray(obj) + return parseArray(obj, objectStrategy) } if (isPlainObject(obj)) { - return parseObject(obj) + return parseObject(obj, objectStrategy) } return obj @@ -64,7 +97,7 @@ function parse(obj: unknown): unknown { function parseString(str: string): unknown { if (maybeJson(str)) { try { - return parse(JSON.parse(str)) + return parse(JSON.parse(str), 'in-place') } catch (err) { // this catch block is intentionally empty. } @@ -77,10 +110,19 @@ function maybeJson(value: string): boolean { return value.match(/^[\[\{]/) != null } -function parseObject(obj: Record): Record { +function parseObject( + obj: Record, + objectStrategy: ObjectStrategy, +): Record { + if (objectStrategy === 'spread') { + obj = { ...obj } + } + + const target = objectStrategy === 'create' ? {} : obj + for (const key in obj) { - obj[key] = parse(obj[key]) + target[key] = parse(obj[key], objectStrategy) } - return obj + return target } From 4069b5388a3f0aef1d469d4b15275a5a301295b9 Mon Sep 17 00:00:00 2001 From: igalklebanov Date: Wed, 17 Apr 2024 23:48:52 +0300 Subject: [PATCH 2/3] unit test. --- .../parse-json-results-plugin.ts | 17 ++---------- .../src/parse-json-results-plugin.test.ts | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 15 deletions(-) create mode 100644 test/node/src/parse-json-results-plugin.test.ts diff --git a/src/plugin/parse-json-results/parse-json-results-plugin.ts b/src/plugin/parse-json-results/parse-json-results-plugin.ts index 71c5f0321..202167fb3 100644 --- a/src/plugin/parse-json-results/parse-json-results-plugin.ts +++ b/src/plugin/parse-json-results/parse-json-results-plugin.ts @@ -15,19 +15,14 @@ export interface ParseJSONResultsPluginOptions { * * This can result in runtime errors if some objects/arrays are readonly. * - * When `'spread'`, arrays and objects are spread (`...`) into new arrays and - * objects. This is least efficient time-wise, but safe from readonly runtime - * errors. - * - * When `'create'`, new arrays and objects are created. This is least efficient - * space-wise, but safe from readonly runtime errors. + * When `'create'`, new arrays and objects are created to avoid such errors. * * Defaults to `'in-place'`. */ objectStrategy?: ObjectStrategy } -type ObjectStrategy = 'in-place' | 'spread' | 'create' +type ObjectStrategy = 'in-place' | 'create' /** * Parses JSON strings in query results into JSON objects. @@ -65,10 +60,6 @@ export class ParseJSONResultsPlugin implements KyselyPlugin { } function parseArray(arr: T[], objectStrategy: ObjectStrategy): T[] { - if (objectStrategy === 'spread') { - arr = [...arr] - } - const target = objectStrategy === 'create' ? [] : arr for (let i = 0; i < arr.length; ++i) { @@ -114,10 +105,6 @@ function parseObject( obj: Record, objectStrategy: ObjectStrategy, ): Record { - if (objectStrategy === 'spread') { - obj = { ...obj } - } - const target = objectStrategy === 'create' ? {} : obj for (const key in obj) { diff --git a/test/node/src/parse-json-results-plugin.test.ts b/test/node/src/parse-json-results-plugin.test.ts new file mode 100644 index 000000000..0de14f1ee --- /dev/null +++ b/test/node/src/parse-json-results-plugin.test.ts @@ -0,0 +1,27 @@ +import { ParseJSONResultsPlugin } from '../../..' +import { createQueryId } from '../../../dist/cjs/util/query-id.js' + +describe.only('ParseJSONResultsPlugin', () => { + describe("when `objectStrategy` is 'create'", () => { + let plugin: ParseJSONResultsPlugin + + beforeEach(() => { + plugin = new ParseJSONResultsPlugin({ objectStrategy: 'create' }) + }) + + it('should parse JSON results that contain readonly arrays/objects', async () => { + await plugin.transformResult({ + queryId: createQueryId(), + result: { + rows: [ + Object.freeze({ + id: 1, + carIds: Object.freeze([1, 2, 3]), + metadata: JSON.stringify({ foo: 'bar' }), + }), + ], + }, + }) + }) + }) +}) From f9dbd05fe8fbd6368222f98207e8802854aae49a Mon Sep 17 00:00:00 2001 From: igalklebanov Date: Sat, 20 Apr 2024 21:22:58 +0300 Subject: [PATCH 3/3] pr comments. --- package-lock.json | 4 ++-- src/plugin/parse-json-results/parse-json-results-plugin.ts | 2 +- test/node/src/parse-json-results-plugin.test.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index a83c3fe4d..965f7e86f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "kysely", - "version": "0.27.2", + "version": "0.27.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "kysely", - "version": "0.27.2", + "version": "0.27.3", "license": "MIT", "devDependencies": { "@types/better-sqlite3": "^7.6.8", diff --git a/src/plugin/parse-json-results/parse-json-results-plugin.ts b/src/plugin/parse-json-results/parse-json-results-plugin.ts index 202167fb3..59c612d29 100644 --- a/src/plugin/parse-json-results/parse-json-results-plugin.ts +++ b/src/plugin/parse-json-results/parse-json-results-plugin.ts @@ -60,7 +60,7 @@ export class ParseJSONResultsPlugin implements KyselyPlugin { } function parseArray(arr: T[], objectStrategy: ObjectStrategy): T[] { - const target = objectStrategy === 'create' ? [] : arr + const target = objectStrategy === 'create' ? new Array(arr.length) : arr for (let i = 0; i < arr.length; ++i) { target[i] = parse(arr[i], objectStrategy) as T diff --git a/test/node/src/parse-json-results-plugin.test.ts b/test/node/src/parse-json-results-plugin.test.ts index 0de14f1ee..c529657c7 100644 --- a/test/node/src/parse-json-results-plugin.test.ts +++ b/test/node/src/parse-json-results-plugin.test.ts @@ -1,7 +1,7 @@ import { ParseJSONResultsPlugin } from '../../..' import { createQueryId } from '../../../dist/cjs/util/query-id.js' -describe.only('ParseJSONResultsPlugin', () => { +describe('ParseJSONResultsPlugin', () => { describe("when `objectStrategy` is 'create'", () => { let plugin: ParseJSONResultsPlugin