From 199fbc66ab140554b1d772d2d87842bbc782564d Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 1 Jul 2022 16:29:27 +0200 Subject: [PATCH 1/5] Validate the ZoneId of the DateTime with ZoneId The validation of the DateTime was only being done in the new patched driver while unpacking the struct. This changes force any new DateTime with ZoneID to have a valid ZoneId. This changes also treats struct unpacking errors and deffers the occurred to the moment the object is manipulate. For instance, a DateTime with invalid ZoneId returned in a Record will not break the records consumption until any code try to interacts with the broken DateTime. --- .../src/packstream/packstream-v1.js | 25 +++++++++------ .../test/packstream/packstream-v2.test.js | 14 ++++---- packages/core/src/internal/temporal-util.ts | 10 ++++++ packages/core/src/internal/util.ts | 32 ++++++++++++++++++- packages/core/src/temporal-types.ts | 1 + .../neo4j-driver/test/temporal-types.test.js | 7 ++++ .../src/skipped-tests/common.js | 5 +-- .../testkit-backend/src/skipped-tests/skip.js | 25 ++++++++++++--- 8 files changed, 97 insertions(+), 22 deletions(-) diff --git a/packages/bolt-connection/src/packstream/packstream-v1.js b/packages/bolt-connection/src/packstream/packstream-v1.js index b1361cac5..20073231e 100644 --- a/packages/bolt-connection/src/packstream/packstream-v1.js +++ b/packages/bolt-connection/src/packstream/packstream-v1.js @@ -28,9 +28,11 @@ import { Path, PathSegment, Relationship, - UnboundRelationship + UnboundRelationship, + internal } from 'neo4j-driver-core' +const { util } = internal const { PROTOCOL_ERROR } = error const TINY_STRING = 0x80 @@ -562,15 +564,20 @@ class Unpacker { } _unpackStruct (marker, markerHigh, markerLow, buffer) { - if (markerHigh === TINY_STRUCT) { - return this._unpackStructWithSize(markerLow, buffer) - } else if (marker === STRUCT_8) { - return this._unpackStructWithSize(buffer.readUInt8(), buffer) - } else if (marker === STRUCT_16) { - return this._unpackStructWithSize(buffer.readUInt16(), buffer) - } else { - return null + try { + if (markerHigh === TINY_STRUCT) { + return this._unpackStructWithSize(markerLow, buffer) + } else if (marker === STRUCT_8) { + return this._unpackStructWithSize(buffer.readUInt8(), buffer) + } else if (marker === STRUCT_16) { + return this._unpackStructWithSize(buffer.readUInt16(), buffer) + } else { + return null + } + } catch (error) { + return util.createBrokenObject(error) } + } _unpackStructWithSize (structSize, buffer) { diff --git a/packages/bolt-connection/test/packstream/packstream-v2.test.js b/packages/bolt-connection/test/packstream/packstream-v2.test.js index 0be8d8c78..28e600646 100644 --- a/packages/bolt-connection/test/packstream/packstream-v2.test.js +++ b/packages/bolt-connection/test/packstream/packstream-v2.test.js @@ -315,7 +315,9 @@ describe('#unit PackStreamV2', () => { new Structure(0x69, [1, 2, 'America/Sao Paulo', 'Brasil']) ] ])('should not unpack with wrong size (%s)', (_, struct) => { - expect(() => packAndUnpack(struct, { useUtc: true })).toThrowErrorMatchingSnapshot() + const result = packAndUnpack(struct, { useUtc: true }) + // Errors are postponed for when the data is accessed. + expect(() => result instanceof DateTime).toThrowErrorMatchingSnapshot() }) it.each([ @@ -352,7 +354,7 @@ describe('#unit PackStreamV2', () => { ], [ 'DateTimeWithZoneId/0x66', - new Structure(0x66, [1, 2, 'America/Sao Paulo']) + new Structure(0x66, [1, 2, 'America/Sao_Paulo']) ] ])('should unpack deprecated temporal types as unknown structs (%s)', (_, struct) => { const unpacked = packAndUnpack(struct, { disableLosslessIntegers: true, useUtc: true}) @@ -368,7 +370,7 @@ describe('#unit PackStreamV2', () => { ], [ 'DateTimeWithZoneId/0x69', - new Structure(0x69, [1, 2, 'America/Sao Paulo']) + new Structure(0x69, [1, 2, 'America/Sao_Paulo']) ] ])('should unpack utc temporal types as unknown structs (%s)', (_, struct) => { const unpacked = packAndUnpack(struct, { disableLosslessIntegers: true }) @@ -383,8 +385,8 @@ describe('#unit PackStreamV2', () => { ], [ 'DateTimeWithZoneId', - new Structure(0x66, [int(1), int(2), 'America/Sao Paulo']), - new DateTime(1970, 1, 1, 0, 0, 1, 2, undefined, 'America/Sao Paulo') + new Structure(0x66, [int(1), int(2), 'America/Sao_Paulo']), + new DateTime(1970, 1, 1, 0, 0, 1, 2, undefined, 'America/Sao_Paulo') ] ])('should unpack temporal types without utc fix (%s)', (_, struct, object) => { const unpacked = packAndUnpack(struct, { disableLosslessIntegers: true }) @@ -392,7 +394,7 @@ describe('#unit PackStreamV2', () => { }) it.each([ - ['DateTimeWithZoneId', new DateTime(1, 1, 1, 1, 1, 1, 1, undefined, 'America/Sao Paulo')], + ['DateTimeWithZoneId', new DateTime(1, 1, 1, 1, 1, 1, 1, undefined, 'America/Sao_Paulo')], ['DateTime', new DateTime(1, 1, 1, 1, 1, 1, 1, 1)] ])('should pack temporal types (no utc) (%s)', (_, object) => { const unpacked = packAndUnpack(object, { disableLosslessIntegers: true }) diff --git a/packages/core/src/internal/temporal-util.ts b/packages/core/src/internal/temporal-util.ts index e243f9418..57155ec51 100644 --- a/packages/core/src/internal/temporal-util.ts +++ b/packages/core/src/internal/temporal-util.ts @@ -407,6 +407,16 @@ export function assertValidNanosecond ( ) } +export function assertValidZoneId (fieldName: string, zoneId: string) { + try { + Intl.DateTimeFormat(undefined, { timeZone: zoneId }) + } catch (e) { + throw newError( + `${fieldName} is expected to be a valid ZoneId but was: "${zoneId}"` + ) + } +} + /** * Check if the given value is of expected type and is in the expected range. * @param {Integer|number} value the value to check. diff --git a/packages/core/src/internal/util.ts b/packages/core/src/internal/util.ts index 509b2027c..95f2ab7b6 100644 --- a/packages/core/src/internal/util.ts +++ b/packages/core/src/internal/util.ts @@ -223,6 +223,35 @@ function isString(str: any): str is string { return Object.prototype.toString.call(str) === '[object String]' } +/** + * Creates a object which all method call will thrown the given error + * + * @param {Error} error The error + * @param {any} object The object. Default: {} + * @returns {any} A broken object + */ +function createBrokenObject (error: Error, object: any = {}): T { + const thrown = () => { + throw error + } + + return new Proxy(object, { + get: thrown, + set: thrown, + apply: thrown, + construct: thrown, + defineProperty: thrown, + deleteProperty: thrown, + getOwnPropertyDescriptor: thrown, + getPrototypeOf: thrown, + has: thrown, + isExtensible: thrown, + ownKeys: thrown, + preventExtensions: thrown, + setPrototypeOf: thrown, + }) +} + export { isEmptyObjectOrNull, isObject, @@ -234,5 +263,6 @@ export { assertValidDate, validateQueryAndParameters, ENCRYPTION_ON, - ENCRYPTION_OFF + ENCRYPTION_OFF, + createBrokenObject } diff --git a/packages/core/src/temporal-types.ts b/packages/core/src/temporal-types.ts index d687f250a..4e5d6113e 100644 --- a/packages/core/src/temporal-types.ts +++ b/packages/core/src/temporal-types.ts @@ -722,6 +722,7 @@ function verifyTimeZoneArguments( if (idDefined) { assertString(timeZoneId, 'Time zone ID') + util.assertValidZoneId('Time zone ID', timeZoneId) result[1] = timeZoneId } diff --git a/packages/neo4j-driver/test/temporal-types.test.js b/packages/neo4j-driver/test/temporal-types.test.js index 5398e4121..ca795dbd1 100644 --- a/packages/neo4j-driver/test/temporal-types.test.js +++ b/packages/neo4j-driver/test/temporal-types.test.js @@ -1401,6 +1401,13 @@ describe('#integration temporal-types', () => { verifyTimeZoneOffset(neo4jDateTime5, -1 * 150 * 60, '-02:30') }, 60000) + + it('should not create DateTime with invalid ZoneId', () => { + expect(() => dateTimeWithZoneId(1999, 10, 1, 10, 15, 0, 0, 'Europe/Neo4j')).toThrowError( + 'Time zone ID is expected to be a valid ZoneId but was "Europe/Neo4j"' + ) + }) + function testSendAndReceiveRandomTemporalValues (valueGenerator) { const asyncFunction = (index, callback) => { testSendReceiveTemporalValue(valueGenerator()) diff --git a/packages/testkit-backend/src/skipped-tests/common.js b/packages/testkit-backend/src/skipped-tests/common.js index 9efe41d7e..063c5bd84 100644 --- a/packages/testkit-backend/src/skipped-tests/common.js +++ b/packages/testkit-backend/src/skipped-tests/common.js @@ -1,8 +1,9 @@ -import skip, { ifEquals, ifEndsWith } from './skip' +import skip, { ifEquals, ifEndsWith, endsWith, ifStartsWith } from './skip' const skippedTests = [ skip( - 'Driver does not return offset for old DateTime implementations', + 'Driver does not return offset for old DateTime implementations', + ifStartsWith('stub.types.test_temporal_types.TestTemporalTypes').and(endsWith('test_zoned_date_time')), ifEquals('neo4j.datatypes.test_temporal_types.TestDataTypes.test_nested_datetime'), ifEquals('neo4j.datatypes.test_temporal_types.TestDataTypes.test_should_echo_all_timezone_ids'), ifEquals('neo4j.datatypes.test_temporal_types.TestDataTypes.test_cypher_created_datetime') diff --git a/packages/testkit-backend/src/skipped-tests/skip.js b/packages/testkit-backend/src/skipped-tests/skip.js index adb997a97..69d4638a5 100644 --- a/packages/testkit-backend/src/skipped-tests/skip.js +++ b/packages/testkit-backend/src/skipped-tests/skip.js @@ -1,21 +1,38 @@ + +function asComposablePredicate (predicate) { + return new Proxy(predicate, { + get: (target, p) => { + if (p === 'and') { + return otherPredicate => asComposablePredicate(testName => target(testName) && otherPredicate(testName)) + } else if (p === 'or') { + return otherPredicate => asComposablePredicate(testName => target(testName) || otherPredicate(testName)) + } + return target[p] + } + }) +} + export function ifEndsWith (suffix) { - return testName => testName.endsWith(suffix) + return asComposablePredicate(testName => testName.endsWith(suffix)) } export function ifStartsWith (prefix) { - return testName => testName.startsWith(prefix) + return asComposablePredicate(testName => testName.startsWith(prefix)) } export function ifEquals (expectedName) { - return testName => testName === expectedName + return asComposablePredicate(testName => testName === expectedName) } export function or () { - return testName => [...arguments].find(predicate => predicate(testName)) + return asComposablePredicate(testName => [...arguments].find(predicate => predicate(testName))) } export function skip (reason, ...predicate) { return { reason, predicate: or(...predicate) } } +export const endsWith = ifEndsWith +export const startsWith = ifStartsWith + export default skip From 1a1f2380739333e1f4c4ab900c1645d0c2362e92 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 4 Jul 2022 12:59:16 +0200 Subject: [PATCH 2/5] Fix test setup --- packages/neo4j-driver/test/temporal-types.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/neo4j-driver/test/temporal-types.test.js b/packages/neo4j-driver/test/temporal-types.test.js index ca795dbd1..dc46ba044 100644 --- a/packages/neo4j-driver/test/temporal-types.test.js +++ b/packages/neo4j-driver/test/temporal-types.test.js @@ -44,8 +44,8 @@ const MIN_YEAR = -MAX_YEAR const MAX_TIME_ZONE_OFFSET = 64800 const MIN_TIME_ZONE_OFFSET = -MAX_TIME_ZONE_OFFSET const SECONDS_PER_MINUTE = 60 -const MIN_ZONE_ID = 'Etc/GMT+12' -const MAX_ZONE_ID = 'Etc/GMT-14' +const MIN_ZONE_ID = 'Pacific/Samoa' +const MAX_ZONE_ID = 'Pacific/Kiritimati' const ZONE_IDS = ['Europe/Zaporozhye', 'Europe/London', 'UTC', 'Africa/Cairo'] describe('#integration temporal-types', () => { From aafd95ac32943b1b2cd0217cf5089b63fd7377fb Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 5 Jul 2022 13:06:56 +0200 Subject: [PATCH 3/5] Adjusting tests --- packages/neo4j-driver/test/temporal-types.test.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/neo4j-driver/test/temporal-types.test.js b/packages/neo4j-driver/test/temporal-types.test.js index dc46ba044..cc74708d1 100644 --- a/packages/neo4j-driver/test/temporal-types.test.js +++ b/packages/neo4j-driver/test/temporal-types.test.js @@ -644,9 +644,6 @@ describe('#integration temporal-types', () => { 'Asia/Yangon' ).toString() ).toEqual('-30455-05-05T12:24:10.000000123[Asia/Yangon]') - expect( - dateTimeWithZoneId(248, 12, 30, 23, 59, 59, 3, 'CET').toString() - ).toEqual('0248-12-30T23:59:59.000000003[CET]') }, 60000) it('should expose local time components in time', () => { @@ -1404,7 +1401,7 @@ describe('#integration temporal-types', () => { it('should not create DateTime with invalid ZoneId', () => { expect(() => dateTimeWithZoneId(1999, 10, 1, 10, 15, 0, 0, 'Europe/Neo4j')).toThrowError( - 'Time zone ID is expected to be a valid ZoneId but was "Europe/Neo4j"' + 'Time zone ID is expected to be a valid ZoneId but was: "Europe/Neo4j"' ) }) From 5d6e634902375b5b1dcfcd862594745f9660f6c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Tue, 5 Jul 2022 17:13:58 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Robsdedude --- packages/core/src/internal/util.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/core/src/internal/util.ts b/packages/core/src/internal/util.ts index 95f2ab7b6..ecad93e7e 100644 --- a/packages/core/src/internal/util.ts +++ b/packages/core/src/internal/util.ts @@ -224,31 +224,31 @@ function isString(str: any): str is string { } /** - * Creates a object which all method call will thrown the given error + * Creates a object which all method call will throw the given error * * @param {Error} error The error * @param {any} object The object. Default: {} * @returns {any} A broken object */ function createBrokenObject (error: Error, object: any = {}): T { - const thrown = () => { + const fail = () => { throw error } return new Proxy(object, { - get: thrown, - set: thrown, - apply: thrown, - construct: thrown, - defineProperty: thrown, - deleteProperty: thrown, - getOwnPropertyDescriptor: thrown, - getPrototypeOf: thrown, - has: thrown, - isExtensible: thrown, - ownKeys: thrown, - preventExtensions: thrown, - setPrototypeOf: thrown, + get: fail, + set: fail, + apply: fail, + construct: fail, + defineProperty: fail, + deleteProperty: fail, + getOwnPropertyDescriptor: fail, + getPrototypeOf: fail, + has: fail, + isExtensible: fail, + ownKeys: fail, + preventExtensions: fail, + setPrototypeOf: fail, }) } From f24d8d46f50298898d97213543eef5209559d2d5 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 6 Jul 2022 12:27:47 +0200 Subject: [PATCH 5/5] Add feature flag related to the broken record --- packages/testkit-backend/src/request-handlers.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 8f5264bf4..882664643 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -343,6 +343,7 @@ export function GetFeatures (_context, _params, wire) { 'Feature:Bolt:4.3', 'Feature:Bolt:4.4', 'Feature:API:Result.List', + 'Detail:ResultStreamWorksAfterBrokenRecord', ...SUPPORTED_TLS ] })