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

Omit fractional seconds from http-date format #2989

Merged
merged 10 commits into from
Sep 21, 2023

Conversation

rschmitt
Copy link
Contributor

Motivation and Context

Closes #2831.

Description

Fractional seconds will still be accepted during parsing, but not emitted during serialization.

Testing

./gradlew codegen-core:check codegen-client:check codegen-server:check codegen-client-test:check codegen-server-test:check

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

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

@rschmitt rschmitt requested review from a team as code owners September 18, 2023 18:48
@rcoh
Copy link
Collaborator

rcoh commented Sep 19, 2023

doesn't need to be in this PR, but we should also add an S3-specific protocol test for awslabs/aws-sdk-rust#818

Can you also link to aws-sdk-rust#818 from the changelog?

It would be good to also verify that that API actually works after this change.

We should also contribute a protocol test upstream that mandates this behavior—currently there are non that I'm aware of.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have the wrong date format. This change should be applied to http-date rather than date-time. We definitely want fractional seconds on date-time.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch all on the HTTP Date, glad we actually tested the end-bug. I think we need to back out the changes to the RFC3339 serializer than we're g2g

@@ -150,3 +150,9 @@ message = "Fix regression with redacting sensitive HTTP response bodies."
references = ["smithy-rs#2926", "smithy-rs#2972"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
author = "ysaito1001"

[[smithy-rs]]
message = "Omit fractional seconds from `date-time` format."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message = "Omit fractional seconds from `date-time` format."
message = "Omit fractional seconds from `http-date` format."

@@ -309,7 +309,7 @@ mod tests {
object.finish();

assert_eq!(
r#"{"epoch_seconds":5.2,"date_time":"2021-05-24T15:34:50.123Z","http_date":"Wed, 21 Oct 2015 07:28:00 GMT"}"#,
r#"{"epoch_seconds":5.2,"date_time":"2021-05-24T15:34:50Z","http_date":"Wed, 21 Oct 2015 07:28:00 GMT"}"#,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change this test to put a fraction in http_date—and I think we need to back out the changes to date_time, only http_date omits the subsecond. My bad on that—good catch from @jdisanti

rust-runtime/aws-smithy-types/src/date_time/format.rs Outdated Show resolved Hide resolved
@rschmitt rschmitt changed the title Omit fractional seconds from date-time format Omit fractional seconds from http-date format Sep 20, 2023
@rcoh rcoh enabled auto-merge September 21, 2023 18:30
@rcoh rcoh added this pull request to the merge queue Sep 21, 2023
Merged via the queue into smithy-lang:main with commit 3265555 Sep 21, 2023
@rschmitt rschmitt deleted the issue-2831 branch September 21, 2023 22:54
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.

Update aws_smithy_types::date_time::Format::HttpDate to omit fractional seconds
3 participants