-
Notifications
You must be signed in to change notification settings - Fork 578
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
feat(smithy-client): support strict timestamp parsing #2737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look very deep into all the date RFCs, only a few minor points to the syntax. You can leave the function in date-util
. Otherwise, most other function in the smithy-client
are parser-util
.
6aae1ae
to
d77009a
Compare
Codecov Report
@@ Coverage Diff @@
## main #2737 +/- ##
=======================================
Coverage ? 61.48%
=======================================
Files ? 540
Lines ? 27617
Branches ? 6745
=======================================
Hits ? 16981
Misses ? 10636
Partials ? 0 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this in a few spots, but you're gonna have to account for leading zeroes being valid all over the place, where the strictParseX
methods explicitly reject them. It'd be great if you could just use the built in parseInt
but that'll probably take a single leading zero as an octal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, meant to do request changes
d77009a
to
be3bf57
Compare
From context, we know what the format of a timestamp should be, and we can restrict the input to match that format rather than relying on the unspecified behavior of new Date(string).
be3bf57
to
7cf40f7
Compare
In release 3.30.0 (aws/aws-sdk-js-v3#2737) AWS-SDK put stricter timestamp parsing. Our stub data for cloudwatch metrics was outputing unix timestamp strings, so aws-sdk parser was throwing `TypeError: Invalid RFC-3339 date-time` In 7b1edc2 we updated the `getGappyRandomData` function to return unix timestamp to address Prometheus stub data formats. We don't want to change the output of the `getGappyRandomData` function, so for cloudwatch stub data, we'll: - parse the unixtimestamp string to an integer - convert it to a valid date - format date to RFC-3339 (ISO 8601-ish) format Now AWS-SDK parser is happy.
In release 3.30.0 (aws/aws-sdk-js-v3#2737) AWS-SDK put stricter timestamp parsing. Our stub data for cloudwatch metrics was outputing unix timestamp strings, so aws-sdk parser was throwing `TypeError: Invalid RFC-3339 date-time` In 7b1edc2 we updated the `getGappyRandomData` function to return unix timestamp to address Prometheus stub data formats. We don't want to change the output of the `getGappyRandomData` function, so for cloudwatch stub data, we'll: - parse the unixtimestamp string to an integer - convert it to a valid date - format date to RFC-3339 (ISO 8601-ish) format Now AWS-SDK parser is happy.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Description
From context, we know what the format of a timestamp should be, and we can
restrict the input to match that format rather than relying on the unspecified
behavior of new Date(string).
Testing
Unit tests, plus regeneration of clients, client protocol tests, server protocol tests. (those PRs to follow)
Additional context
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.