Skip to content
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

Temporal.DateTime parity with Android SDK #1175

Closed
valzevul opened this issue Apr 21, 2021 · 15 comments
Closed

Temporal.DateTime parity with Android SDK #1175

valzevul opened this issue Apr 21, 2021 · 15 comments
Labels
bug Something isn't working datastore Issues related to the DataStore category

Comments

@valzevul
Copy link

As per AWS docs,

The AWSDate, AWSTime, and AWSDateTime scalars can optionally include a time zone offset. For example, the values 1970-01-01Z, 1970-01-01-07:00, and 1970-01-01+05:30 are all valid for AWSDate. The time zone offset must be either Z (UTC) or an offset in hours and minutes (and, optionally, seconds). For example, ±hh:mm:ss. The seconds field in the time zone offset is considered valid even though it's not part of the ISO 8601 standard.

While I am aware of #1048, seems like there is a different issue regarding feature parity with Android SDK: while amplify-ios supports parsing 1970-01-01+05:30 into a valid Temporal.DateTime object, it doesn't support formatting Foundation.Date or Temporal.DateTime into an ISO 8601 string with a time zone offset.

This causes a mismatch between different platforms if one wants to store dates in DynamoDB offset by users' local timezones.

To Reproduce
Steps to reproduce the behavior:

  1. Created a Temporal.DateTime object from a String with a shifted timezone:
let formatter = DateFormatter()
formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSSZZZZZ"
formatter.calendar = Calendar(identifier: .iso8601)
formatter.timeZone = TimeZone.current
let dateTime = Temporal.DateTime(iso8601String: formatter.string(from: Date()))
  1. Upload it to DynamoDB as a part of AppSync / GraphQL model
  2. The dateTime object is represented in DynamoDB as 2021-04-21T11:44:18.505Z

Expected behavior
Given a local timezone, I'd expected to have a way to specify the formatting option, so dateTime ends up in DynamoDB as 2021-04-21T11:44:18.505-05:00 instead.

Additional context
I am happy to make a PR addressing the issue if that's something on the roadmap anyway / my solution fits into the existing architecture.

Currently TemporalSpec forces the encoder to rely on iso8601String:

extension TemporalSpec where Self: Codable {

    public init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        let value = try container.decode(String.self)
        try self.init(iso8601String: value)
    }

    public func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        try container.encode(iso8601String)
    }

}

which is defined with a hardcoded time zone:

    /// The ISO8601 representation of the scalar using `.full` as the format and `.utc` as `TimeZone`.
    /// - seealso: iso8601FormattedString(TemporalFormat, TimeZone)
    public var iso8601String: String {
        iso8601FormattedString(format: .full)
    }

If instead an approach akin to Android SDK's format() was used:

        /**
         * Formats the current {@link Temporal.Date}
         * into an extended ISO-8601 Date string, with an optional timezone offset.
         *
         * @return An extended ISO-8601 Date string, with an optional timezone offset
         */
        public String format() {
            if (zoneOffset != null) {
                OffsetDateTime odt = OffsetDateTime.of(localDate, LocalTime.MIDNIGHT, zoneOffset);
                return getOffsetDateTimeFormatter().format(odt);
            } else {
                return DateTimeFormatter.ISO_LOCAL_DATE.format(this.localDate);
            }
        }

It'd give enough flexibility to specify what should be used for formatting.

@dsaliberti
Copy link

oh I definitely need that too
thanks for raising it @valzevul

@palpatim palpatim added datastore Issues related to the DataStore category enhancement pending-triage Issue is pending triage labels Apr 22, 2021
@valzevul
Copy link
Author

Hey folks, any updates on this issue?

I can see that it got marked as an "enhancement", while to me it feels more like a bug or some unexpected behaviour:
If I save Temporal.DateTime(iso8601String: "2021-04-21T11:44:18.505+02:00"), I expect it to come to DynamoDB exactly like that, not like "2021-04-21T09:44:18.505Z" (even though it is technically the same timestamp, it lacks the valuable information about the user's local timezone).

It works as expected on Android.

@valzevul
Copy link
Author

Hey @palpatim, sorry, is there any update on the issue or at least a confirmation that it is not on the roadmap (in which case my team would commit to supporting a private fork for the time being)?

@kjones
Copy link

kjones commented May 21, 2021

I need this also. Backend is already defined as AWSDateTime. Current work around is to modify the schema.graphql after an amplify pull and change the field to String. Then do the conversion myself with the correct format that preserves the user's timezone.

@lawmicha lawmicha added feature-request Request a new feature and removed enhancement labels Jul 8, 2021
@lawmicha lawmicha removed the pending-triage Issue is pending triage label Apr 22, 2022
@lawmicha lawmicha added the p2 label Jun 8, 2022
@freddie1129
Copy link

Really need it even do not take Android behaviour into account.

@valzevul
Copy link
Author

@lawmicha sorry, are there any updates to the issue? I've tested with the recent 1.x and 2.x SDKs but the problem persists for more than a year now.

@dsaliberti
Copy link

@lawmicha Any good news on this one please? 🙏

@atierian atierian added follow up Requires follow up from maintainers bug Something isn't working feature-parity requires attention Follow up needed for more than 10 days and removed feature-request Request a new feature follow up Requires follow up from maintainers labels Jun 28, 2023
@harsh62
Copy link
Member

harsh62 commented Oct 3, 2023

Apologies for the delayed response. Our team will look into the issue and provide an update.

@atierian atierian removed the p2 label Oct 16, 2023
@diogoepsy
Copy link

@harsh62 Any update on this?

@5d
Copy link
Member

5d commented Dec 1, 2023

@diogoepsy , I've made a fix on branch 5d/date-tz and tested with a very simple datastore App. Could you help me confirming whether it functions correctly with your application?

@diogoepsy
Copy link

Thank you @5d I've tried your branch and every variation that I could of creating a Temporal.DateTime:

  • Using Temporal.DateTime(iso8601String:) and passing a iso8601 String, for instance, "2023-12-03T16:06:51+0000" which creates the following object
foundationDate : 2023-12-03 16:06:51 +0000
   - timeIntervalSinceReferenceDate : 723312411.0
 ▿ timeZone : Optional<TimeZone>
   ▿ some : GMT (fixed)
     - identifier : "GMT"
     - kind : "fixed"
     ▿ abbreviation : Optional<String>
       - some : "GMT"
     - secondsFromGMT : 0
     - isDaylightSavingTime : false

But the iso8601String representation is still "2023-12-03T16:06:51.000Z". I've also tried passing along the Timezone instead but I get the same result. We are essentially trying to maintain feature parity with Android and save the dates on the format we are specifying, which in this case is including the timezone offset like 2021-04-21T11:44:18.505-05:00.

Perhaps there is some Initializer I'm not using properly on this PR? I've made a fork and added the funcionality in there as well if you wanna look into it :) https://github.com/diogoepsy/amplify-ios/commit/a7f1efc86d5b09cf0b6c1d3af43b6be7b28c5ef7

@5d
Copy link
Member

5d commented Dec 4, 2023

Hi @diogoepsy,

But the iso8601String representation is still "2023-12-03T16:06:51.000Z". I've also tried passing along the Timezone instead but I get the same result.

Did you tried initialize the Temporal.DateTime object with timezone? is iso8601String still give the GMT date?
I've tested both on real device and unit test that timezone info is represented in the iso8601String. The unit test shows how to use the Temporal.DateTime with iso8601 date format with timezone info. It encodes/decodes the AWSDateTime to/from JSON object with server side.

Can you double check you are on the correct branch?

@diogoepsy
Copy link

Hi @diogoepsy,

But the iso8601String representation is still "2023-12-03T16:06:51.000Z". I've also tried passing along the Timezone instead but I get the same result.

Did you tried initialize the Temporal.DateTime object with timezone? is iso8601String still give the GMT date? I've tested both on real device and unit test that timezone info is represented in the iso8601String. The unit test shows how to use the Temporal.DateTime with iso8601 date format with timezone info. It encodes/decodes the AWSDateTime to/from JSON object with server side.

Can you double check you are on the correct branch?

Hey Di Wu, you are absolutely right :) It is working properly and I am able to save the date using the iso8601String representation. Thank you very much!

@atierian atierian removed requires attention Follow up needed for more than 10 days feature-parity labels Dec 8, 2023
@atierian atierian added the pending-release Code has been merged but pending release Code has been merged but pending release label Dec 8, 2023
@5d 5d added the closing soon This issue will be closed in 7 days unless further comments are made. label Dec 12, 2023
@atierian
Copy link
Member

This fix was released in 2.25.1. Thanks all for reporting this and for your patience!

@github-actions github-actions bot removed closing soon This issue will be closed in 7 days unless further comments are made. pending-release Code has been merged but pending release Code has been merged but pending release labels Dec 14, 2023
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datastore Issues related to the DataStore category
Projects
None yet
Development

No branches or pull requests

10 participants