fix(datastore): store time zone info in Temporal.DateTime #3393
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3393 +/- ##
==========================================
- Coverage 68.17% 68.07% -0.10%
==========================================
Files 1077 1078 +1
Lines 35973 36066 +93
==========================================
+ Hits 24523 24551 +28
- Misses 11450 11515 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Amplify/Categories/DataStore/Model/Temporal/TimeZone+Extension.swift
Outdated
Show resolved
Hide resolved
Amplify/Categories/DataStore/Model/Temporal/TimeZone+Extension.swift
Outdated
Show resolved
Hide resolved
| import Foundation | ||
|
|
||
| extension TimeZone { | ||
| private static let iso8601TimeZoneHHColonMMColonSSRegex = try? NSRegularExpression(pattern: "^[+-]\\d{2}:\\d{2}:\\d{2}$") |
There was a problem hiding this comment.
In addition to HH:MM:SS, does AWSDateTime also support HHMMSS? HH:MMSS? HHMM:SS ? I think we should verify this and try to support it fully if it does not impact the performance of the app during parsing the input string
I suspect that HHMMSS is supported since the docs say "can optionally include a time zone offset" linking to the ISO 8601 wiki, while only providing "hh:mm:ss" as an example
There was a problem hiding this comment.
From current amplify-js implementation, it only supports HH:MM:SS format.
There was a problem hiding this comment.
that's surprising, I ran a few live tests
HH:MM:SS works as expected
mutation MyMutationHHColonMMColonSS {
createTest(input: {userID: "123", dt: "2023-11-30T11:04:03+08:00:30"}) {
dt
}
}
{
"data": {
"createTest": {
"dt": "2023-11-30T11:04:03+08:00:30"
}
}
}
HHMM does not work
mutation MyMutationHHMM {
createTest(input: {userID: "123", dt: "2023-11-30T11:04:03+0800"}) {
dt
}
}
{
"data": null,
"errors": [
{
"path": null,
"locations": [
{
"line": 2,
"column": 14,
"sourceName": null
}
],
"message": "Validation error of type WrongType: argument 'input.dt' with value 'StringValue{value='2023-11-30T11:04:03+0800'}' is not a valid 'AWSDateTime' @ 'createTest'"
}
]
}
So expectedly, HHMMSS also does not work..
mutation MyMutationHHMMSS {
createTest(input: {userID: "123", dt: "2023-11-30T11:04:03+080030"}) {
dt
}
}
{
"data": null,
"errors": [
{
"path": null,
"locations": [
{
"line": 2,
"column": 14,
"sourceName": null
}
],
"message": "Validation error of type WrongType: argument 'input.dt' with value 'StringValue{value='2023-11-30T11:04:03+080030'}' is not a valid 'AWSDateTime' @ 'createTest'"
}
]
}
HH:MM:SS works as expected
mutation MyMutationHHColonMMColonSS {
createTest(input: {userID: "123", dt: "2023-11-30T11:04:03+08:00:30"}) {
dt
}
}
{
"data": {
"createTest": {
"dt": "2023-11-30T11:04:03+08:00:30"
}
}
}
There was a problem hiding this comment.
So what we have now looks good to me.
-
Data that is AWSDateTime with HH:MM:SS will be decoded properly.
-
Data would never be stored in the format HHMM since AppSync doesn't allow it, so even though we support decoding HHMM, that code path won't ever be used. This means it may pose the inverse problem: AppSync type is more restrictive than the Amplify Swift type. We allow storing date times with offset containing HHMM but the mutation with AppSync will fail. I believe this is still the correct thing to do since it conforms to the iSO 8601 spec and we can have varying types of data sources, ie. SQLite / local-only use cases.
-
HHMMSS is not supported by AppSync AWSDateTime and not part of the ISO8601 spec.
There was a problem hiding this comment.
Thanks for verifying this with AppSync.
We won't send date with string format HHMM or HH:MM:SS. We always send date to AppSync with string formatHH:MM code.
The format HHMM or HH:MM:SS are only the acceptable inputs. The code here is ensure we are able to decode these ISO8601 date formats to correct Tempral.DateTime instance. When encoding them back to string and submit to AppSync. They will always be in HH:MM format.
Issue #
#1175
Description
Temporal.DateTimetoISO8601date string.General Checklist
Given When Theninline code documentation and are named accordinglytestThing_condition_expectation()By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.