From c74215cb9c3120f6fd86c56a88f2ba60c5fd01a9 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Mar 03 2018 15:34:38 +0000 Subject: Don't use datetime.strftime('%s') This is dangerous for two major reasons: it may not be entirely portable, and it doesn't do the thing you'd expect it to do. If you, for instance, do: datetime.datetime.utcnow().strftime('%s') you do not get the Unix timestamp for the current UTC time; you get the Unix timestamp for whatever the current UTC wall clock time is *in the system's current time zone*. It doesn't even work with explicitly timezone-aware datetime objects, because the timezone information is lost somewhere between Python and glibc's strftime. See: https://bugs.python.org/issue12750 for more details on this. So, instead of doing that, we'll use arrow's `timestamp` attribute instead. Python 3.3+ datetime instances have a `timestamp()` method that works properly on aware instances, but it's not available on Python 2.7 and you still have to make sure you're using an aware instance. arrow instances are always aware (they default to UTC timezone) and `arrow.Arrow.timestamp` is available in Python 2 and 3. This issue would have real consequences on any Pagure server on a system whose timezone is *not* UTC - when getting JSON representations of these things, the timestamps would not be correct. For instance if an issue was created at 16:00 UTC, the database would have 16:00, but on a system which is, say, an hour behind UTC, when producing the JSON representation of the issue, the timestamp would match 16:00 *local time* - so it'd be the timestamp of 17:00 UTC. Signed-off-by: Adam Williamson --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 4c1fe5d..420eefd 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -11,6 +11,7 @@ __requires__ = ['SQLAlchemy >= 0.8', 'jinja2 >= 2.4'] # noqa import pkg_resources # noqa: E402,F401 +import arrow import datetime import collections import logging @@ -883,8 +884,8 @@ class Project(BASE): 'namespace': self.namespace, 'parent': self.parent.to_json( public=public, api=api) if self.parent else None, - 'date_created': self.date_created.strftime('%s'), - 'date_modified': self.date_modified.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), + 'date_modified': str(arrow.get(self.date_modified).timestamp), 'user': self.user.to_json(public=public), 'access_users': self.access_users_json, 'access_groups': self.access_groups_json, @@ -1256,10 +1257,10 @@ class Issue(BASE): 'content': self.content, 'status': self.status, 'close_status': self.close_status, - 'date_created': self.date_created.strftime('%s'), - 'last_updated': self.last_updated.strftime('%s'), - 'closed_at': self.closed_at.strftime( - '%s') if self.closed_at else None, + 'date_created': str(arrow.get(self.date_created).timestamp), + 'last_updated': str(arrow.get(self.last_updated).timestamp), + 'closed_at': str(arrow.get(self.closed_at).timestamp) + if self.closed_at else None, 'user': self.user.to_json(public=public), 'private': self.private, 'tags': self.tags_text, @@ -1389,9 +1390,9 @@ class IssueComment(BASE): 'id': self.id, 'comment': self.comment, 'parent': self.parent_id, - 'date_created': self.date_created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'user': self.user.to_json(public=public), - 'edited_on': self.edited_on.strftime('%s') + 'edited_on': str(arrow.get(self.edited_on).timestamp) if self.edited_on else None, 'editor': self.editor.to_json(public=public) if self.editor_id else None, @@ -1846,11 +1847,11 @@ class PullRequest(BASE): 'repo_from': self.project_from.to_json( public=public, api=api) if self.project_from else None, 'remote_git': self.remote_git, - 'date_created': self.date_created.strftime('%s'), - 'updated_on': self.updated_on.strftime('%s'), - 'last_updated': self.last_updated.strftime('%s'), - 'closed_at': self.closed_at.strftime( - '%s') if self.closed_at else None, + 'date_created': str(arrow.get(self.date_created).timestamp), + 'updated_on': str(arrow.get(self.updated_on).timestamp), + 'last_updated': str(arrow.get(self.last_updated).timestamp), + 'closed_at': str(arrow.get(self.closed_at).timestamp) + if self.closed_at else None, 'user': self.user.to_json(public=public), 'assignee': self.assignee.to_json( public=public) if self.assignee else None, @@ -1970,9 +1971,9 @@ class PullRequestComment(BASE): 'line': self.line, 'comment': self.comment, 'parent': self.parent_id, - 'date_created': self.date_created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'user': self.user.to_json(public=public), - 'edited_on': self.edited_on.strftime('%s') + 'edited_on': str(arrow.get(self.edited_on).timestamp) if self.edited_on else None, 'editor': self.editor.to_json(public=public) if self.editor_id else None, @@ -2062,7 +2063,7 @@ class PullRequestFlag(BASE): 'comment': self.comment, 'status': self.status, 'url': self.url, - 'date_created': self.date_created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'user': self.user.to_json(public=public), } @@ -2156,7 +2157,7 @@ class CommitFlag(BASE): 'comment': self.comment, 'status': self.status, 'url': self.url, - 'date_created': self.date_created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'user': self.user.to_json(public=public), } @@ -2274,7 +2275,7 @@ class PagureGroup(BASE): 'description': self.description, 'group_type': self.group_type, 'creator': self.creator.to_json(public=public), - 'date_created': self.created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'members': [user.username for user in self.users] } @@ -2483,7 +2484,7 @@ class PagureLog(BASE): 'type': self.log_type, 'ref_id': self.ref_id, 'date': self.date.strftime('%Y-%m-%d'), - 'date_created': self.date_created.strftime('%s'), + 'date_created': str(arrow.get(self.date_created).timestamp), 'user': self.user.to_json(public=public), } return output diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 14a9741..2365a4b 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -11,6 +11,7 @@ __requires__ = ['SQLAlchemy >= 0.8'] import pkg_resources +import arrow import copy import datetime import unittest @@ -1187,7 +1188,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): repo = pagure.lib.get_authorized_project(self.session, 'test') # Create 2 tickets but only 1 has a milestone - start = datetime.datetime.utcnow().strftime('%s') + start = arrow.utcnow().timestamp issue = pagure.lib.model.Issue( id=pagure.lib.get_next_id(self.session, repo.id), project_id=repo.id, @@ -1280,7 +1281,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): repo = pagure.lib.get_authorized_project(self.session, 'test') # Create 2 tickets but only 1 has a priority - start = datetime.datetime.utcnow().strftime('%s') + start = arrow.utcnow().timestamp issue = pagure.lib.model.Issue( id=pagure.lib.get_next_id(self.session, repo.id), project_id=repo.id, @@ -1425,7 +1426,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): repo = pagure.lib.get_authorized_project(self.session, 'test') # Create 2 tickets but only 1 has a milestone - start = datetime.datetime.utcnow().strftime('%s') + start = arrow.utcnow().timestamp issue = pagure.lib.model.Issue( id=pagure.lib.get_next_id(self.session, repo.id), project_id=repo.id, @@ -1543,7 +1544,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): repo = pagure.lib.get_authorized_project(self.session, 'test') # Create 1st tickets - start = datetime.datetime.utcnow().strftime('%s') + start = arrow.utcnow().timestamp issue = pagure.lib.model.Issue( id=pagure.lib.get_next_id(self.session, repo.id), project_id=repo.id, @@ -1557,7 +1558,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): self.session.commit() time.sleep(1) - middle = datetime.datetime.utcnow().strftime('%s') + middle = arrow.utcnow().timestamp # Create 2nd tickets issue = pagure.lib.model.Issue( @@ -1573,7 +1574,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): self.session.commit() time.sleep(1) - final = datetime.datetime.utcnow().strftime('%s') + final = arrow.utcnow().timestamp # Create private issue issue = pagure.lib.model.Issue( @@ -1627,7 +1628,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): ) time.sleep(1) - late = datetime.datetime.utcnow().strftime('%s') + late = arrow.utcnow().timestamp # List all opened issues from the start output = self.app.get('/api/0/test/issues?since=%s' % start)