Expand Record Object Mapping to allow Parameter Mapping#1362
Expand Record Object Mapping to allow Parameter Mapping#1362
Conversation
robsdedude
left a comment
There was a problem hiding this comment.
Review part 1. I'll continue next week with the review. I just want to submit my comments to make sure they don't get lost.
packages/core/src/temporal-types.ts
Outdated
| * @param {string} str The string to convert | ||
| * @returns {LocalDateTime<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): LocalDateTime<NumberOrInteger> { |
There was a problem hiding this comment.
Here's a list of strings that probably should fail, but don't.
'+2026-01-05T15:36:42',
'-2026-01-05T15:36:42',
'20260-01-05T15:36:42',
'2026-001-05T15:36:42',
There was a problem hiding this comment.
The latter two are still being accepted.
|
Consider making the |
robsdedude
left a comment
There was a problem hiding this comment.
Here goes a first round of comments. Submitting them now so they don't get lost until tomorrow when I shall continue.
| * @param {string} str The string to convert | ||
| * @returns {Date<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): Date<Integer> { |
There was a problem hiding this comment.
As far as I can tell, neo4j.Date.fromString("2026-02-31") works just fine and yields a data that doesn't exist: Feb. 31st 2026. Granted, node JS also parses this date fine but wraps the days around into March
> new Date("2026-02-31")
2026-03-03T00:00:00.000ZI tested round tripping the invalid neo4j.Date via Bolt and it ends up wrapping like nodeJS does locally, too. I'm fairly certain that's because of bolt's encoding of dates using ordinals.
Note that this behavior will likely cause issues when adding/productizing HTTP Query API support because the server will likely not accept "2026-02-31" as a date string.
I realize that the neo4j.Date constructor was not added in this PR, so there not much to be done here besides raising awareness of the quirk and maybe putting a plan in place to fix it in the next major version.
| */ | ||
| function validateQueryAndParameters ( | ||
| query: string | String | { text: string, parameters?: any }, | ||
| query: string | String | { text: string, parameters?: any, parameterRules?: Rules }, |
There was a problem hiding this comment.
parameterRules missing in doc comment
| query: string | String | { text: string, parameters?: any, parameterRules?: Rules }, | ||
| parameters?: any, | ||
| opt?: { skipAsserts: boolean } | ||
| opt?: { skipAsserts?: boolean, parameterRules?: Rules } |
| * @returns {Time<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): Time<Integer> { | ||
| const values = String(str.replace(/,/g, '.')).match(/^[T|t]?(\d{2}):?(\d{2})?:?(\d{2})?(\.\d+)?(Z|\+|-)(\d{0,2}):?(\d{0,2}):?(\d{0,2})$/) |
There was a problem hiding this comment.
I find the timezone offset matching too lenient. Here's a suggested exhaustive list of formats (placeholders with lower case letters - each letter exactly one digit 0-9, other characters are literals):
- Z
- +hh
- +hhmm
- +hh:mm
- +hh:mm:ss (note: not ISO conform, but the HTTP Query API server as well as the driver itself might still produce it 😬)
- -hh
- -hhmm
- -hh:mm
- -hh:mm:ss (note: not ISO conform)
In contrast, the current regex will, among other formats, accept "Z0100", "+123", "+::7", and "-0102:2"
| * @returns {LocalTime<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): LocalTime<Integer> { | ||
| const values = String(str.replace(/,/g, '.')).match(/^T?(\d{2}):?(\d{2})?:?(\d{2})?(\.\d+)?$/) |
There was a problem hiding this comment.
If you decide to keep that letters in other Regexes are case-insensitive, this leading T should be too for consistency.
| * @returns {Date<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): Date<Integer> { | ||
| const values = String(str.replace(/,/g, '.')).match(/^(\d+)-(\d+)-(\d+)$/) |
There was a problem hiding this comment.
Does not accept negative years.
| * @returns {Date<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): Date<Integer> { | ||
| const values = String(str.replace(/,/g, '.')).match(/^(\d+)-(\d+)-(\d+)$/) |
There was a problem hiding this comment.
Dependent on how strict you want this to be:
https://en.wikipedia.org/wiki/ISO_8601#Years
For positive years, this is regex is more lenient that ISO which requires either exactly four digits and no sign XOR more than four digits and a sign.
packages/core/src/temporal-types.ts
Outdated
| * @param {string} str The string to convert | ||
| * @returns {LocalDateTime<NumberOrInteger>} | ||
| */ | ||
| static fromString (str: string): LocalDateTime<NumberOrInteger> { |
There was a problem hiding this comment.
The latter two are still being accepted.
| } | ||
| } | ||
|
|
||
| function parseTemporalFloat (str: string, field: string, maxLength?: number): number { |
There was a problem hiding this comment.
| function parseTemporalFloat (str: string, field: string, maxLength?: number): number { | |
| function parseTemporalFloat (str: string, field: string): number { |
unused parameter (inside function and at all call sites), or should it be used but isn't?
| } | ||
|
|
||
| function parseTemporalFloat (str: string, field: string, maxLength?: number): number { | ||
| if (str === undefined || str.length === 0) { |
There was a problem hiding this comment.
If str can be undefined, shouldn't the signature read str?: string?
| } | ||
|
|
||
| function parseTemporalInt (str: string, field: string, maxLength?: number): Integer { | ||
| if (str === undefined || str.length === 0 || str === 'undefined') { |
There was a problem hiding this comment.
If str can be undefined, shouldn't the signature read str?: string?
| return result | ||
| } | ||
|
|
||
| function handleTimeDecimals (hourString: string, minuteString: string, secondString: string, decimalString: string): [Integer, Integer, Integer, Integer] { |
There was a problem hiding this comment.
| function handleTimeDecimals (hourString: string, minuteString: string, secondString: string, decimalString: string): [Integer, Integer, Integer, Integer] { | |
| function handleTimeDecimals (hourString?: string, minuteString?: string, secondString?: string, decimalString?: string): [Integer, Integer, Integer, Integer] { |
Either this or get rid of the undefined checks inside the function, me thinks.
| let minutes | ||
| let seconds | ||
| let nanoseconds | ||
| if (minuteString === undefined || secondString === '') { |
There was a problem hiding this comment.
| if (minuteString === undefined || secondString === '') { | |
| if (minuteString === undefined || minuteString === '') { |
maybe?
| let nanoseconds | ||
| if (minuteString === undefined || secondString === '') { | ||
| hours = parseTemporalInt(hourString, 'hours') | ||
| minutes = int(decimalString !== undefined ? Math.round(parseFloat('0' + decimalString) * 60) : 0) |
There was a problem hiding this comment.
How about once at the top:
decimalInt = decimalString !== undefined ? parseFloat('0' + decimalString) : 0and then saving a whole bunch of undefined checks and string parsing calls.
| let minutes | ||
| let seconds | ||
| let nanoseconds | ||
| if (minuteString === undefined || secondString === '') { |
There was a problem hiding this comment.
The way the regexs are written rn, minuteString can be undefined while secondString isn't, I believe. If so, that'd lead to secondString being ignored erroneously.
They also allow for T01::.2. They probably shouldn't, but if they do, at least the .2 should mean 0.2 seconds, not 0.2 hours.
closes DRIVERS-107
This PR provides support for typechecking and automatic conversion of parameters, as well as using the object mapping registry to register mapping strategies for classes. This lets users directly pass objects even with problematic properties that can not be sent over bolt (like functions), by automatically converting them or by omitting them from the mapping strategy.
This is the 2nd half of the Record Object Mapping feature, which is also marked stabilized and taken out of preview in this PR.
Examples