From 7c46ba195f92763d3f76fb714b7fd4a53ffc1deb Mon Sep 17 00:00:00 2001 From: Matt Robenolt Date: Wed, 9 Nov 2022 16:57:41 -0800 Subject: [PATCH 1/3] Better support for NULLs In general, both bugs fixed here are the result of protojson encoding and fields being omitted from the JSON response: In the case of a NULL column, the NULL type is omitted from the types enum becasue it's numeric value is "0", and protojson omits this due to it being a default value. In the case of `select null`, we get another edge case, where the `values` itself is entirely omitted due to it being an empty seuqence of bytes. Fixes #72 --- __tests__/index.test.ts | 75 ++++++++++++++++++++++++++++++++++++----- src/index.ts | 13 +++++-- 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index eba7e7b..9d925e6 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -121,18 +121,24 @@ describe('execute', () => { const mockResponse = { session: mockSession, result: { - fields: [{ name: ':vtg1', type: 'INT32' }], - rows: [{ lengths: ['1'], values: 'MQ==' }] + fields: [ + { name: ':vtg1', type: 'INT32' }, + { name: 'null' }, + ], + rows: [{ lengths: ['1', '-1'], values: 'MQ==' }] } } const want: ExecutedQuery = { - headers: [':vtg1'], - types: { ':vtg1': 'INT32' }, - fields: [{ name: ':vtg1', type: 'INT32' }], - rows: [{ ':vtg1': 1 }], + headers: [':vtg1', 'null'], + types: { ':vtg1': 'INT32', 'null': 'NULL' }, + fields: [ + { name: ':vtg1', type: 'INT32' }, + { name: 'null', type: 'NULL' }, + ], + rows: [{ ':vtg1': 1, 'null': null }], size: 1, - statement: 'SELECT 1 from dual;', + statement: 'SELECT 1, null from dual;', time: 1, rowsAffected: null, insertId: null @@ -146,7 +152,58 @@ describe('execute', () => { }) const connection = connect(config) - const got = await connection.execute('SELECT 1 from dual;') + const got = await connection.execute('SELECT 1, null from dual;') + got.time = 1 + + expect(got).toEqual(want) + + mockPool.intercept({ path: EXECUTE_PATH, method: 'POST' }).reply(200, (opts) => { + expect(opts.headers['authorization']).toMatch(/Basic /) + const bodyObj = JSON.parse(opts.body.toString()) + expect(bodyObj.session).toEqual(mockSession) + return mockResponse + }) + + const got2 = await connection.execute('SELECT 1, null from dual;') + got2.time = 1 + + expect(got2).toEqual(want) + }) + + test('it properly returns and decodes a select query (select null)', async () => { + const mockResponse = { + session: mockSession, + result: { + fields: [ + { name: 'null' }, + ], + rows: [{ lengths: ['-1']}] + } + } + + const want: ExecutedQuery = { + headers: ['null'], + types: { 'null': 'NULL' }, + fields: [ + { name: 'null', type: 'NULL' }, + ], + rows: [{ 'null': null }], + size: 1, + statement: 'SELECT null', + time: 1, + rowsAffected: null, + insertId: null + } + + mockPool.intercept({ path: EXECUTE_PATH, method: 'POST' }).reply(200, (opts) => { + expect(opts.headers['authorization']).toMatch(/Basic /) + const bodyObj = JSON.parse(opts.body.toString()) + expect(bodyObj.session).toEqual(null) + return mockResponse + }) + + const connection = connect(config) + const got = await connection.execute('SELECT null') got.time = 1 expect(got).toEqual(want) @@ -158,7 +215,7 @@ describe('execute', () => { return mockResponse }) - const got2 = await connection.execute('SELECT 1 from dual;') + const got2 = await connection.execute('SELECT null') got2.time = 1 expect(got2).toEqual(want) diff --git a/src/index.ts b/src/index.ts index 125b3bb..1d5e6da 100644 --- a/src/index.ts +++ b/src/index.ts @@ -64,12 +64,12 @@ export interface Config { interface QueryResultRow { lengths: string[] - values: string + values?: string } export interface Field { name: string - type: string + type?: string table?: string // Only populated for included fields @@ -217,6 +217,13 @@ export class Connection { this.session = session const fields = result?.fields ?? [] + // ensure each field has a type assigned, + // the only case it would be omitted is in the case of + // NULL due to the protojson spec. NULL in our enum + // is 0, and empty fields are omitted from the JSON response, + // so we should backfill an expected type. + fields.forEach((f) => { f.type = f.type || 'NULL' }) + const rows = result ? parse(result, this.config.cast || cast, options.as || 'object') : [] const headers = fields.map((f) => f.name) @@ -304,7 +311,7 @@ function parse(result: QueryResult, cast: Cast, returnAs: ExecuteAs): Row[] { } function decodeRow(row: QueryResultRow): Array { - const values = atob(row.values) + const values = row.values ? atob(row.values) : '' let offset = 0 return row.lengths.map((size) => { const width = parseInt(size, 10) From 7f7ce9c4dfd5379e4927b790c01dd173ec297845 Mon Sep 17 00:00:00 2001 From: mattrobenolt Date: Thu, 10 Nov 2022 01:01:40 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20linting=20with=20autofix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- __tests__/index.test.ts | 25 +++++++++---------------- src/index.ts | 4 +++- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index 9d925e6..9300d7b 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -121,22 +121,19 @@ describe('execute', () => { const mockResponse = { session: mockSession, result: { - fields: [ - { name: ':vtg1', type: 'INT32' }, - { name: 'null' }, - ], + fields: [{ name: ':vtg1', type: 'INT32' }, { name: 'null' }], rows: [{ lengths: ['1', '-1'], values: 'MQ==' }] } } const want: ExecutedQuery = { headers: [':vtg1', 'null'], - types: { ':vtg1': 'INT32', 'null': 'NULL' }, + types: { ':vtg1': 'INT32', null: 'NULL' }, fields: [ { name: ':vtg1', type: 'INT32' }, - { name: 'null', type: 'NULL' }, + { name: 'null', type: 'NULL' } ], - rows: [{ ':vtg1': 1, 'null': null }], + rows: [{ ':vtg1': 1, null: null }], size: 1, statement: 'SELECT 1, null from dual;', time: 1, @@ -174,20 +171,16 @@ describe('execute', () => { const mockResponse = { session: mockSession, result: { - fields: [ - { name: 'null' }, - ], - rows: [{ lengths: ['-1']}] + fields: [{ name: 'null' }], + rows: [{ lengths: ['-1'] }] } } const want: ExecutedQuery = { headers: ['null'], - types: { 'null': 'NULL' }, - fields: [ - { name: 'null', type: 'NULL' }, - ], - rows: [{ 'null': null }], + types: { null: 'NULL' }, + fields: [{ name: 'null', type: 'NULL' }], + rows: [{ null: null }], size: 1, statement: 'SELECT null', time: 1, diff --git a/src/index.ts b/src/index.ts index 1d5e6da..1be5587 100644 --- a/src/index.ts +++ b/src/index.ts @@ -222,7 +222,9 @@ export class Connection { // NULL due to the protojson spec. NULL in our enum // is 0, and empty fields are omitted from the JSON response, // so we should backfill an expected type. - fields.forEach((f) => { f.type = f.type || 'NULL' }) + fields.forEach((f) => { + f.type = f.type || 'NULL' + }) const rows = result ? parse(result, this.config.cast || cast, options.as || 'object') : [] const headers = fields.map((f) => f.name) From b8b5c2c0544e46a6bf787e7ce510df9a5ab86003 Mon Sep 17 00:00:00 2001 From: Iheanyi Ekechukwu Date: Fri, 18 Nov 2022 12:05:09 -0600 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: David Graham --- src/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 1be5587..99caf71 100644 --- a/src/index.ts +++ b/src/index.ts @@ -69,7 +69,7 @@ interface QueryResultRow { export interface Field { name: string - type?: string + type: string table?: string // Only populated for included fields @@ -222,9 +222,9 @@ export class Connection { // NULL due to the protojson spec. NULL in our enum // is 0, and empty fields are omitted from the JSON response, // so we should backfill an expected type. - fields.forEach((f) => { - f.type = f.type || 'NULL' - }) + for (const field of fields) { + field.type ||= 'NULL' + } const rows = result ? parse(result, this.config.cast || cast, options.as || 'object') : [] const headers = fields.map((f) => f.name)