Skip to content

Validate the ZoneId of the DateTime with ZoneId #959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions packages/bolt-connection/src/packstream/packstream-v1.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
14 changes: 8 additions & 6 deletions packages/bolt-connection/test/packstream/packstream-v2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down Expand Up @@ -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})
Expand All @@ -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 })
Expand All @@ -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 })
Expand Down
10 changes: 10 additions & 0 deletions packages/core/src/internal/temporal-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
32 changes: 31 additions & 1 deletion packages/core/src/internal/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -234,5 +263,6 @@ export {
assertValidDate,
validateQueryAndParameters,
ENCRYPTION_ON,
ENCRYPTION_OFF
ENCRYPTION_OFF,
createBrokenObject
}
1 change: 1 addition & 0 deletions packages/core/src/temporal-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,7 @@ function verifyTimeZoneArguments(

if (idDefined) {
assertString(timeZoneId, 'Time zone ID')
util.assertValidZoneId('Time zone ID', timeZoneId)
result[1] = timeZoneId
}

Expand Down
14 changes: 9 additions & 5 deletions packages/neo4j-driver/test/temporal-types.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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())
Expand Down
1 change: 1 addition & 0 deletions packages/testkit-backend/src/request-handlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
]
})
Expand Down
5 changes: 3 additions & 2 deletions packages/testkit-backend/src/skipped-tests/common.js
Original file line number Diff line number Diff line change
@@ -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')
Expand Down
25 changes: 21 additions & 4 deletions packages/testkit-backend/src/skipped-tests/skip.js
Original file line number Diff line number Diff line change
@@ -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