Validate the ZoneId of the DateTime with ZoneId (#959)

The validation of the DateTime was only being done in the new patched protocol 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 defers 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.

Co-authored-by: Robsdedude <[email protected]>

Author: bigmontz

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) {

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,16 +385,16 @@ 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 })
       expect(unpacked).toEqual(object)
     })
 
     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 })

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.

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 throw the given error
+ * 
+ * @param {Error} error The error
+ * @param {any} object The object. Default: {}
+ * @returns {any} A broken object
+ */
+function createBrokenObject<T extends object> (error: Error, object: any = {}): T {
+  const fail = () => {
+    throw error
+  }
+
+  return new Proxy(object, {
+    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,
+  })
+}
+
 export {
   isEmptyObjectOrNull,
   isObject,
@@ -234,5 +263,6 @@ export {
   assertValidDate,
   validateQueryAndParameters,
   ENCRYPTION_ON,
-  ENCRYPTION_OFF
+  ENCRYPTION_OFF,
+  createBrokenObject
 }

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
   }
 

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', () => {
@@ -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', () => {
@@ -1401,6 +1398,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())

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
     ]
   })

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')

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