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

Support alternate date formats in StateMachine.fromJson(). #3095

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcvayc
Copy link

@mcvayc mcvayc commented Feb 15, 2024

Issue: #2130

Issue Being Resolved

Currently, this code:

StateMachine.fromJson("""
  {
    "StartAt": "TestTime",
    "States": {
      "TestTime": {
        "Type": "Wait",
        "Timestamp": "2016-03-14T01:59:00Z",
        "End": true
      }
    }
  }
  """);

fails with the error message java.lang.IllegalArgumentException: Invalid format: "2016-03-14T01:59:00Z" is malformed at "Z".

However, this code:

StateMachine.fromJson("""
  {
    "StartAt": "TestTime",
    "States": {
      "TestTime": {
        "Type": "Wait",
        "Timestamp": "2016-03-14T01:59:00.000Z",
        "End": true
      }
    }
  }
  """);

works.

This is because we currently use:

org.joda.time.format.ISODateTimeFormat.dateTime().parseDateTime()

to parse dates found in JSON.

According to the AWS Step Functions documentation for the wait state:

Timestamps must conform to the RFC3339 profile of ISO 8601, 
with the further restrictions that an uppercase T must separate the date and time portions, 
and an uppercase Z must denote that a numeric time zone offset is not present, 
for example, 2024-08-18T17:33:00Z.

The RFC3339 profile of ISO 8601 allows fractional seconds to be optional and the example given in the AWS documentation does not include any partial seconds. Therefore, we should support dates without fractional seconds.

Proposed Solution
This PR changes the date parsing logic to use com.amazonaws.util.DateUtils.parseISO8601Date(...) to parse dates during JSON deserialization. This method supports both the date format with fractional seconds and without fractional seconds.

This PR does not change the JSON serialization format of dates.

Test Cases
I have added test cases to demonstrate that the solution works with both date formats, however, these test cases may have limited value since I have not changed any date parsing logic used during JSON deserialization. I am happy to remove them upon request.

Confirmation
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mcvayc
Copy link
Author

mcvayc commented Feb 16, 2024

Hi team,

This is my first open-source PR so I apologize in advance if it's missing something.

I tried running mvn clean install but I was unable to get all of the unit tests to pass even on the master branch. It looks like this is a known issue (#1981) so I verified that in IntelliJ all of the unit tests for the child module I touched passed and that mvn clean install -Dgpg.skip=true -DskipTests succeeds.

This commit fixes issue aws#2130.

Currently, `StateMachine.fromJson(...)` can parse JSON documents with timestamps in the format `2016-03-14T01:59:00.000Z` but it fails to parse JSON documents with timestamps in the format `2016-03-14T01:59:00Z`.

This is because we currently use `org.joda.time.format.ISODateTimeFormat.dateTime().parseDateTime(...)`, which requires fractional seconds in timestamps, for JSON date deserialization.

According to the documentation and examples in the AWS StepFunction documentation, fractional seconds in timestamps are optional.

This PR changes the date parsing logic to use `com.amazonaws.util.DateUtils.parseISO8601Date(...)` to parse dates during JSON deserialization. This method supports both the date format with fractional seconds and without fractional seconds.

This PR does not change the JSON serialization format of dates.
@mcvayc
Copy link
Author

mcvayc commented Apr 23, 2024

Hi team, just want to check in and see if there is anything else I can do to help get this merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant