-
Notifications
You must be signed in to change notification settings - Fork 863
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
[V4] DateTime handling adjustments for inconsistencies between local and UTC times. #3572
base: v4-development
Are you sure you want to change the base?
Conversation
sdk/test/UnitTests/Custom/Marshalling/BackwardsCompatibilityForDateTimePropertiesTests.cs
Show resolved
Hide resolved
sdk/test/NetStandard/IntegrationTests/IntegrationTests/DynamoDB/DataModelTests.cs
Show resolved
Hide resolved
generator/.DevConfigs/460d1afb-6912-4244-82e2-cfb811a84743.json
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/DownloadDirectoryCommand.cs
Outdated
Show resolved
Hide resolved
Excluding my minor comments I already added I think the change looks good. I have gone through the credential expiration a couple times and everything looks like it is doing comparisons by UTC. I have done some interop testing with V3 and this branch saving and retrieving data from DynamoDB. I tested storing dates as both strings and epochs. I can confirm the data in the table was persisted the same no matter if I used V3 or this branch. The We should look Roslyn/CodeAnalyzers rules that we can use to enforce the correct date behavior going forward but that can be a separate task. |
Adjustments applied and a new commit has been pushed up. I will do another dry-run but nothing changed that should impact that. |
TestFromJsonCanHandleAllDataTypes is the only integ test that failed. Investigating. |
… or context config the value was still defaulted to false.
Description
This PR addresses the inconsistencies of using LocalTime and UniversalTime within the SDK to consistently use UniversalTime (UTC) whenever possible as a first choice and only using LocalTime when appropriate to do so.
DevConfig commit: 8884a55
Manual changes are in these commits only: cb4f74d, 5f88660, 3f93a6d, and c072d6d
Generated changes commit: 9611cba
Note for reviewers:
Motivation and Context
Breaking changes: Changes where the behavior is now different and may or may not produce a compile time error.
EndTime
then changedEndTimeUtc
toEndTime
. This could lead to offset times if anyone was still using the marked obsolete originalEndTime
for example. A compile time error will occur for anyone usingEndTimeUtc
. Removing BackwardCompatibility properties and code in the generator is part of this change. Also id this to the manual S3 models.TimestampFormat.ISO8601 || TimestampFormat.RFC822
datetimes were being parsed into local times. Adjusted DateTime parsing to return UTC times. (BaseResponseUnmarshaller.tt, MultiValueHeaderParser.cs)true
as the default.Cosmetic changes: These changes were mainly so that I could keep track and ensure I looked at and ensured all code using DateTime.Now / .ToUniversalTime, .ToLocalTime, new DateTime, etc, was for a correct and valid reason. Updating to UTC will assist with validation errors at a later time.
var elapsed = DateTime.UtcNow - StartTime;
DateTime.Now.Ticks
andDateTime.UtcNow.Ticks
produces the same long.DateTime.Now.ToFileTime
=>DateTime.UtcNow.ToFileTime
.ToUniversalTime()
calls. Example:string epochSeconds = AWSSDKUtils.ConvertToUnixEpochSecondsString(expiresOn.ToUniversalTime());
-- AWSSDKUtils.ConvertToUnixEpochSecondsString already ensures the DateTime is UTC.IsDateTime
toIsTimeStamp
. This is because the shape is actually modeled as aTimeStamp
not a C# language specificDateTime
.Amazon.EC2.Internal.CustomMarshallTransformations.ConvertDateTimeISOWithoutMillisecondsUtc
=>Amazon.Runtime.Internal.Util.StringUtils.FromDateTimeToISO8601NoMs
. Deleted the service specific DateTime utility methods.#pragma warning disable CS0612, CS0618 // Type or member is obsolete
aroundAWSSDKUtils.CorrectedUtcNow
because the property isn't marked obsolete.Minor bug fixes: These are not breaking changes but fix slight issues in the SDK.
Testing
DryRun: Passed
Manual Testing: In progress -- Example (PresignedURLs)
Adding more tests where appropriate: Pending
Screenshots (if appropriate)
Types of changes
Checklist
License