Relegate `chrono` to an optional feature in a new conversion crate #849

jdisanti · 2021-11-10T01:29:23Z

Motivation and Context

This PR removes the aws-smithy-types and aws-sigv4 dependency on chrono for date-time parsing/formatting since it is no longer maintained and has a CVE.

Description

aws-smithy-types and aws-sigv4 were refactored to use the time crate rather than chrono.
Instant was renamed to DateTime to avoid confusion with the standard library type.
DateTime formatting now returns a result instead of panicking when the DateTime can't be represented in the requested format.
DateTime is now re-exported from service crates so that an explicit dependency on aws-smithy-types isn't required anymore.
The chrono conversions were retained in a new aws-smithy-types-convert crate behind an optional feature. Conversions to the time crate were also added.

Testing

Ran existing tests and added new ones where necessary.

Checklist

I have updated CHANGELOG.md
I have updated aws/SDK_CHANGELOG.md if applicable

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

Rename methods on Instant to have a consistent naming scheme. Remove built-in Instant conversions.

github-actions · 2021-11-10T01:33:48Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

github-actions · 2021-11-10T01:35:22Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

jdisanti · 2021-11-10T01:44:27Z

Looks like I need to revisit DateTime <-> SystemTime conversions to work on Windows, and we should check MacOS too.

rcoh

LGTM! Notes are all extremely minor, this looks great.

...tlin/software/amazon/smithy/rust/codegen/smithy/customizations/SmithyTypesPubUseGenerator.kt

rust-runtime/aws-smithy-types/src/instant/mod.rs

aws/rust-runtime/aws-sig-auth/src/signer.rs

rcoh · 2021-11-10T14:48:56Z

rust-runtime/aws-smithy-types-convert/src/instant.rs

+pub enum Error {
+    /// Conversion failed because the value being converted is out of range for its destination
+    #[non_exhaustive]
+    OutOfRange(Box<dyn StdError + Send + Sync + 'static>),


is this error type actually unknown? I figured it would always be DateTimeParseError

I made this one open ended so that we can add additional library conversions to the aws-smithy-types-convert crate without having to change the error type at all. In this case, the only error being fed into Error::OutOfRange is the error from time::OffsetDateTime::from_unix_timestamp_nanos.

rcoh · 2021-11-10T14:53:29Z

aws/SDK_CHANGELOG.md

@@ -1,6 +1,56 @@
 vNext (Month Day, Year)
 =======================

+**Breaking Changes**
+
+Several breaking changes around `aws_smithy_types::Instant` were introduced by smithy-rs#TODO:


can you:

list out the crates with breaking changes as a top level item (for ease of release cutting)

add the changes to aws-sigv4 and aws-sig-auth

list out the crates with breaking changes as a top level item (for ease of release cutting)

I'm not sure I understand how this makes releasing eaiser.

Added a changelog entry for aws-sigv4. There weren't any breaking changes to aws-sig-auth.

…rono

github-actions · 2021-11-11T00:32:55Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

github-actions · 2021-11-11T00:35:17Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

github-actions · 2021-11-11T00:48:18Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

crisidev

LGTM. Nice change!

crisidev · 2021-11-11T10:56:06Z

Some of the examples in the Tier2 CI need to be updated:

error[E0599]: no method named `to_chrono` found for reference `&DateTime` in the current scope
  --> examples/apigateway/src/bin/get_rest_apis.rs:30:65
   |
30 |         println!("Created:     {}", api.created_date().unwrap().to_chrono());
   |                                                                 ^^^^^^^^^ method not found in `&DateTime`

error: aborting due to previous error

david-perez · 2021-11-11T10:42:05Z

rust-runtime/aws-smithy-types-convert/src/date_time.rs

+use std::error::Error as StdError;
+use std::fmt;
+
+/// Conversion erro/Insta


Unfinished comment.

Good catch! Thanks.

david-perez · 2021-11-11T10:42:33Z

rust-runtime/aws-smithy-types-convert/src/date_time.rs

+}
+
+/// Adds functions to [`DateTime`] to convert it to `time` or `chrono` types.
+///


Suggested change

///

david-perez · 2021-11-11T10:43:09Z

rust-runtime/aws-smithy-types-convert/src/date_time.rs

+    doc = r##"
+# Example with `time`
+
+Make sure your *Cargo.toml* enables the `convert-time` feature:


Suggested change

Make sure your *Cargo.toml* enables the `convert-time` feature:

Make sure your `Cargo.toml` enables the `convert-time` feature:

note that the suggested change is missing a space after your

This raises the question of what our API doc style is. Our AWS SDK Guide Documentation uses bold for file names, and I'm inclined to be consistent with that. Open to debate though.

david-perez · 2021-11-11T10:43:29Z

rust-runtime/aws-smithy-types-convert/src/date_time.rs

+    doc = r##"
+# Example with `chrono`
+
+Make sure your *Cargo.toml* enables the `convert-chrono` feature:


Suggested change

Make sure your *Cargo.toml* enables the `convert-chrono` feature:

Make sure your `Cargo.toml` enables the `convert-chrono` feature:

david-perez · 2021-11-11T10:59:03Z

rust-runtime/aws-smithy-types/src/date_time/mod.rs

+ * SPDX-License-Identifier: Apache-2.0.
+ */
+
+//! DateTime value for representing Smithy timestamps.


Suggested change

//! DateTime value for representing Smithy timestamps.

//! DateTime type for representing Smithy timestamps.

david-perez · 2021-11-11T11:00:05Z

rust-runtime/aws-smithy-types/src/date_time/mod.rs

+const NANOS_PER_SECOND: i128 = 1_000_000_000;
+const NANOS_PER_SECOND_U32: u32 = 1_000_000_000;
+
+/* ANCHOR: date_time */


What are these "anchors" about?

when we generate the mdbook of smithy docs, these code snippets get included

david-perez · 2021-11-11T11:25:06Z

rust-runtime/aws-smithy-types/src/date_time/format.rs

    // This code is taken from https://github.com/pyfisch/httpdate and modified under an
    // Apache 2.0 License. Modifications:
    // - Removed use of unsafe
    // - Add serialization and deserialization of subsecond nanos


My first reaction upon seeing this code was "why aren't we using httpdate"? These strike me as valuable additions to contribute upstream. Can we? Also, can't we simply depend on httpdate and wrap it in a newtype to provide our additional functionality?

Also, I'm not a lawyer, but I thought that if one copied MIT-licensed code, a copy of the MIT license has to be included with it.

// - Add serialization and deserialization of subsecond nanos this is outside of the HTTP date spec (as in, "wrong" if you're actually parsing and serializing http dates.

…rono

github-actions · 2021-11-11T21:13:13Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

…vely

github-actions · 2021-11-11T21:45:55Z

A new generated diff is ready to view: __generated-main...__generated-jdisanti-replace-chrono

jdisanti added 7 commits November 9, 2021 17:15

Refactor Instant to use time instead of chrono

9f40691

Rename methods on Instant to have a consistent naming scheme. Remove built-in Instant conversions.

Remove chrono from aws-sigv4

1430f0c

Re-export Instant from service crates

9de6e15

Implement aws-smithy-types-convert

605913e

Rename Instant to DateTime

9b431f1

Make date-time formatting operations fallible

fb692d0

Add initial changelog entries

6511c84

jdisanti requested review from a team as code owners November 10, 2021 01:29

Update changelog

d4025e3

rcoh approved these changes Nov 10, 2021

View reviewed changes

jdisanti added 3 commits November 10, 2021 12:31

Make DateTime to SystemTime conversion fallible

6ec67a8

Incorporate review feedback

6eae81f

Merge remote-tracking branch 'upstream/main' into jdisanti-replace-ch…

1cba9b8

…rono

Fix merge issues

c4d82a5

jdisanti force-pushed the jdisanti-replace-chrono branch from b427b2e to c4d82a5 Compare November 11, 2021 00:45

crisidev approved these changes Nov 11, 2021

View reviewed changes

david-perez reviewed Nov 11, 2021

View reviewed changes

jdisanti added 3 commits November 11, 2021 13:04

Fix examples

a226221

Fix doc comments

5a2a0b3

Merge remote-tracking branch 'upstream/main' into jdisanti-replace-ch…

935a41a

…rono

Fix unused import warning when using convert-chrono feature exclusi…

94edb56

…vely

jdisanti merged commit 72eae55 into main Nov 12, 2021

jdisanti deleted the jdisanti-replace-chrono branch November 12, 2021 00:01

jdisanti mentioned this pull request Dec 6, 2021

DateTime/HttpDate formatting shouldn't panic for out of range instants #494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relegate `chrono` to an optional feature in a new conversion crate #849

Relegate `chrono` to an optional feature in a new conversion crate #849

jdisanti commented Nov 10, 2021 •

edited

github-actions bot commented Nov 10, 2021

github-actions bot commented Nov 10, 2021

jdisanti commented Nov 10, 2021

rcoh left a comment

rcoh Nov 10, 2021

jdisanti Nov 11, 2021

rcoh Nov 10, 2021

jdisanti Nov 10, 2021

jdisanti Nov 11, 2021

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

crisidev left a comment

crisidev commented Nov 11, 2021

david-perez Nov 11, 2021

jdisanti Nov 11, 2021

david-perez Nov 11, 2021

david-perez Nov 11, 2021 •

edited

rcoh Nov 11, 2021

jdisanti Nov 11, 2021

david-perez Nov 11, 2021

david-perez Nov 11, 2021

david-perez Nov 11, 2021

rcoh Nov 11, 2021

david-perez Nov 11, 2021

rcoh Nov 11, 2021

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

	Make sure your Cargo.toml enables the `convert-time` feature:
	Make sure your `Cargo.toml` enables the `convert-time` feature:

	//! DateTime value for representing Smithy timestamps.
	//! DateTime type for representing Smithy timestamps.

Relegate chrono to an optional feature in a new conversion crate #849

Relegate chrono to an optional feature in a new conversion crate #849

Conversation

jdisanti commented Nov 10, 2021 • edited

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Nov 10, 2021

github-actions bot commented Nov 10, 2021

jdisanti commented Nov 10, 2021

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

crisidev left a comment

Choose a reason for hiding this comment

crisidev commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez Nov 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2021

github-actions bot commented Nov 11, 2021

Relegate `chrono` to an optional feature in a new conversion crate #849

Relegate `chrono` to an optional feature in a new conversion crate #849

jdisanti commented Nov 10, 2021 •

edited

david-perez Nov 11, 2021 •

edited