Specify format: 'date-time' for date properties

[duncanmak]

Addresses #147.

According to https://json-schema.org/understanding-json-schema/reference/string.html#dates-and-times, there's already a predefined format called date-time for the ISO-8601 format in JSON Schema.

The concrete definition is here:

"date-time": fmtDef(
    /^\d\d\d\d-[0-1]\d-[0-3]\dt(?:[0-2]\d:[0-5]\d:[0-5]\d|23:59:60)(?:\.\d+)?(?:z|[+-]\d\d(?::?\d\d)?)$/i,
    compareDateTime
  ),

These formats are part of the ajv-formats package, so I've added it via npm. If we want to avoid installing an entire package, we could also add a single format using the addFormat method in our own compile-validators.js.

While` I'm at it, I also bumped the ajv package version, I could back that out if we don't want to bundle it in.

Having reviewed #150, I hope that by using a regex defined elsewhere, that this is a better approach that what was in #150.

If this PR is indeed correct, I can work out some test cases and add it to the test suite.

cc @thehenrytsai @diehuxx

[duncanmak]

Looks like we might have hit this issue ajv-validator/ajv-formats#73

[thehenrytsai]

I am not a regex guru by any means and I understand if you don't want to write a full test suite before confirming approach is valid, but a couple of sanity test cases to begin would help.

Even the happiest path 2022-10-14T10:20:30.405060Z doesn't appear to pass the regex.

I have no problem at all (and actually probably prefer it) for us to come up with our own tailor-made regex since we have specific requirements on the format.

[dcrousso]

Even the happiest path 2022-10-14T10:20:30.405060Z doesn't appear to pass the regex.

where are you seeing this? im pretty sure it works just fine

/^\d\d\d\d-[0-1]\d-[0-3]\dt(?:[0-2]\d:[0-5]\d:[0-5]\d|23:59:60)(?:\.\d+)?(?:z|[+-]\d\d(?::?\d\d)?)$/i.test('2022-10-14T10:20:30.405060Z') // true

the only issue i potentially could see is that we have extra precision compared to browser environments, so if they changed the \.\d+ to be \.\d{,3} then we may suddenly start having problems

[thehenrytsai]

@dcrousso, you are right, I sanity tested a few tests and saw the wrong result, it's 2022-10-14T10:20:30.405Z that is unexpectedly passing.

[dcrousso]

it's 2022-10-14T10:20:30.405Z that is unexpectedly passing.

can you elaborate why that is unexpected? is the issue that there's not enough precision?

[duncanmak]

I'm happy that @thehenrytsai gave me permission to just add our own regex for the date-time format, which lets me sidestep the whole issue with using ajv-formats and dealing with ESM vs CJS. What a relief!

Once I made the change, I can see that some tests started to fail immediately, and they were easy to patch up.

We started with 12 errors; I got it down to 2 errors with 59cc6ba.

The remaining errors I get from running npm run test:node are:

  2 failing

  1) RecordsQueryHandler.handle()
       functional tests
         should be able to range query by `dateCreated`:
     Error: /descriptor/dateCreated: must match format "date-time"
   
  2) RecordsQueryHandler.handle()
       functional tests
         should be able use range and exact match queries at the same time:
     Error: /descriptor/dateCreated: must match format "date-time"

It looks like the Temporal library is not writing the trailing 'Z' in its toString() method.

// scenario: 3 records authored by alice, created on first of 2021, 2022, and 2023 respectively, only the first 2 records share the same schema
const firstDayOf2021 = Temporal.PlainDateTime.from({ year: 2021, month: 1, day: 1 }).toString({ smallestUnit: 'microseconds' });
const firstDayOf2022 = Temporal.PlainDateTime.from({ year: 2022, month: 1, day: 1 }).toString({ smallestUnit: 'microseconds' });
const firstDayOf2023 = Temporal.PlainDateTime.from({ year: 2023, month: 1, day: 1 }).toString({ smallestUnit: 'microseconds' });

firstDayOf2021: '2021-01-01T00:00:00.000000'
firstDayOf2022: '2022-01-01T00:00:00.000000'
firstDayOf2023: '2023-01-01T00:00:00.000000'

I don't quite understand the nuance of the ISO 8601 format, currently I'm assuming that a trailing Z is required, which is what @thehenrytsai wrote in #113.

These might be relevant:
js-temporal/temporal-polyfill#133
js-temporal/temporal-polyfill#101

Having read this comment from 133 above, I think PlainDateTime is not the right class because it explicitly doesn't support time zones. Most likely, we should switch to using Instant instead.

[duncanmak]

In 2189fd3 I just decided to inline the date strings and that made all the tests pass. Let me know if we want to keep that.

[duncanmak]

I figured out how to construct Temporal.Instants, so I took out the hardcoded strings.

Now the code looks more like how it was before, but we're not using Temporal.PlainDateTime anymore, and we now have the Z marker to designate UTC at the end of the string.

I hope this patch can now be called completed.

If we want to ensure that there are always 6 digits before the Z, the following regex can be used:

> const r = /^\d\d\d\d-[0-1]\d-[0-3]\dt(?:[0-2]\d:[0-5]\d:[0-5]\d|23:59:60)(?:\.\d{6})?(?:z|[+-]\d\d(?::?\d\d)?)$/i;
>
> r.test('2022-10-14T10:20:30.405060Z')
true
> r.test('2022-10-14T10:20:30.405Z')
false

[thehenrytsai]

If we want to ensure that there are always 6 digits before the Z, the following regex can be used:

Yes, please enforce 6 digits, and please add tests to sanity verify the 3 cases outlined in #113 as you referenced.

[thehenrytsai]

Thanks @duncanmak for the contribution! 🙏 🚀 🥇