fix: date/time ajv validation #441
Conversation
index.js
Outdated
| ajvInstance.addKeyword({ | ||
| keyword: 'fjs_date_type', | ||
| validate: (schema, date) => { | ||
| if (date && typeof date.toISOString === 'function') { |
There was a problem hiding this comment.
This check is not sufficient:
❯ node
Welcome to Node.js v16.15.0.
Type ".help" for more information.
> var d = new Date({})
undefined
> typeof d.toISOString === 'function'
true
>
A proper way to validate if something is a Date looks like:
❯ node
Welcome to Node.js v16.15.0.
Type ".help" for more information.
> var d = new Date({})
undefined
> typeof d.toISOString === 'function'
true
> isNaN(d.valueOf()) === false
false
>
There was a problem hiding this comment.
IMHO if someone passes the wrong date object, it should still be validated as a date object. It will fail during serialization. But we can discuss it.
There was a problem hiding this comment.
As I understand, fjs not specifies that date should be the Date type object. It says that it should be an object with toISOString or format method.
There was a problem hiding this comment.
No, it says it should be a string or a Date object, where in the case of the Date object it will be serialized through the .toISOString method -- https://github.com/fastify/fast-json-stringify#specific-use-cases
Simply because an object has a .toISOString method does not mean it is a valid Date object. I showed this above. So if there is going to be code to verify that an object is a valid Date object instance, it should be more complete.
There was a problem hiding this comment.
Line 130 in 074a120
3383f4a to
9f212b6
Compare
9f212b6 to
36e277c
Compare
❗ Depends on BREAKING CHANGE PR #442
Fixed #325