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

[transfer] Fix date_imported to use a correct ISO 8601 date #8095

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Sep 7, 2024

Fix date_imported key to use a proper ISO 8601 format (with hyphens and colons) rather than concatenated numbers. Botocore expects the former rather than the latter, and when given a bare number it incorrectly interprets it as UNIX timestamp rather than %Y%m%d%H%M%S date.

This can particularly be seen on systems with 32-bit time_t, where the resulting timestamp overflows (it would correspond to year 643378 today), and causes tests/test_transfer/test_transfer.py failures such as:

E       RuntimeError: Unable to calculate correct timezone offset for "20240907201715"

When using %Y-%m-%d %H:%M:%S format, the relevant tests pass both on systems with 64-bit and 32-bit time_t.

Copy link

codecov bot commented Sep 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.45%. Comparing base (924f5ec) to head (a4aace1).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8095   +/-   ##
=======================================
  Coverage   94.45%   94.45%           
=======================================
  Files        1141     1141           
  Lines       98198    98198           
=======================================
  Hits        92757    92757           
  Misses       5441     5441           
Flag Coverage Δ
servertests 28.95% <0.00%> (ø)
unittests 94.43% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Hi @mgorny! I've just tested this against AWS, and they actually return an epoch timestamp here - so I think we should do the same.

We have an existing utility method for that:

from moto.core.utils import unix_time
now = unix_time()

I don't know whether that affects 32-bit systems? If so, we may just have to disable the tests instead.

@mgorny
Copy link
Contributor Author

mgorny commented Sep 8, 2024

Sure, updated it to do that. It's also going to work on 32-bit arches, at least until 2038 (and then, everything else will fail too, so not an issue).

Fix `date_imported` key to use a proper UNIX timestamp rather than
concatenated digits of an ISO-8501 date.  Botocore expects the former
rather than the latter, and interprets the latter it as UNIX timestamp.

This can particularly be seen on systems with 32-bit time_t, where
the resulting timestamp overflows (it would correspond to year 643378
today), and causes `tests/test_transfer/test_transfer.py` failures
such as:

```
E       RuntimeError: Unable to calculate correct timezone offset for "20240907201715"
```

When passing a correct timestamp, the relevant tests pass both
on systems with 64-bit and 32-bit time_t (at least until 2038).
@bblommers bblommers added this to the 5.0.14 milestone Sep 8, 2024
Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Great, thank you @mgorny!

@bblommers bblommers merged commit a15f866 into getmoto:master Sep 8, 2024
51 checks passed
@mgorny mgorny deleted the fix-date-imported branch September 8, 2024 13:34
@mgorny
Copy link
Contributor Author

mgorny commented Sep 8, 2024

Thanks!

Copy link
Contributor

github-actions bot commented Sep 8, 2024

This is now part of moto >= 5.0.14.dev70

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.

2 participants