Fix saving tz aware datetimes in Versioned Objects

Currently we cannot save date fields in Versioned Objects because they
are timezone aware and our database is not.

If we look at our objects definition, we are not giving any value to
tzinfo_aware argument, so it takes DateTime default value (True).

In our sqlalchemy model definitions we are defining our datetime fields
as Column(DateTime) which will take sqlalchemy default: class
sqlalchemy.types.DateTime(timezone=False)

In most cases we are accessing those fields directly through the DB
methods, so we don't see the problem, but if we were to modify any of
those fields using versioned object, for example scheduled_at, and then
save the object we'll get an exception:

 File ".../sqlalchemy/sql/type_api.py", line 278, in compare_values
 return x == y
 TypeError: can't compare offset-naive and offset-aware datetimes

Because we are trying to save a timezone aware date to a timezone
unaware DB field.

Following Dan's advice we create a specific cinder_obj_get_changes,
instead of overwriting obj_get_changes, that returns all datefields as
timezone naive UTC datetimes ready for saving.

Change-Id: Ie7b0249e3f6850e7c70d23fc53cfaf27fe04afbb
Closes-Bug: #1469120

diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py
index 760a633fcac428fcf743d76c2c74c7db0bea04fa..3fdc0b04b824784883e4923719ae59ae07a81966 100644 (file)
--- a/cinder/objects/backup.py
+++ b/cinder/objects/backup.py
@@ -90,14 +90,14 @@ class Backup(base.CinderPersistentObject, base.CinderObject,
         if self.obj_attr_is_set('id'):
             raise exception.ObjectActionError(action='create',
                                               reason='already created')
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
 
         db_backup = db.backup_create(self._context, updates)
         self._from_db_object(self._context, self, db_backup)
 
     @base.remotable
     def save(self):
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
         if updates:
             db.backup_update(self._context, self.id, updates)
 

diff --git a/cinder/objects/base.py b/cinder/objects/base.py
index 7b687190cda696c014d3f9a2e4d10b43e11cff54..240cb61793263e7a70b08e2420596b9b642dd302 100644 (file)
--- a/cinder/objects/base.py
+++ b/cinder/objects/base.py
@@ -46,6 +46,28 @@ class CinderObject(base.VersionedObject):
     # from one another.
     OBJ_PROJECT_NAMESPACE = 'cinder'
 
+    def cinder_obj_get_changes(self):
+        """Returns a dict of changed fields with tz unaware datetimes.
+
+        Any timezone aware datetime field will be converted to UTC timezone
+        and returned as timezone unaware datetime.
+
+        This will allow us to pass these fields directly to a db update
+        method as they can't have timezone information.
+        """
+        # Get dirtied/changed fields
+        changes = self.obj_get_changes()
+
+        # Look for datetime objects that contain timezone information
+        for k, v in changes.items():
+            if isinstance(v, datetime.datetime) and v.tzinfo:
+                # Remove timezone information and adjust the time according to
+                # the timezone information's offset.
+                changes[k] = v.replace(tzinfo=None) - v.utcoffset()
+
+        # Return modified dict
+        return changes
+
 
 class CinderObjectDictCompat(base.VersionedObjectDictCompat):
     """Mix-in to provide dictionary key access compat.

diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py
index c9d84f147da96de08b6486ce346f55b7398cf638..3e69d3eda5e62d484b7c62bdd2b864c169e1ffe9 100644 (file)
--- a/cinder/objects/snapshot.py
+++ b/cinder/objects/snapshot.py
@@ -136,7 +136,7 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
         if self.obj_attr_is_set('id'):
             raise exception.ObjectActionError(action='create',
                                               reason=_('already created'))
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
 
         if 'volume' in updates:
             raise exception.ObjectActionError(action='create',
@@ -147,7 +147,7 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
 
     @base.remotable
     def save(self):
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
         if updates:
             if 'volume' in updates:
                 raise exception.ObjectActionError(action='save',

diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index d32aff26af73ce9d18b4e641d71fd901e260e64b..1dab3a234913cce5dbe7c8c2cafdf5cfe5922eda 100644 (file)
--- a/cinder/objects/volume.py
+++ b/cinder/objects/volume.py
@@ -121,13 +121,13 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
         if self.obj_attr_is_set('id'):
             raise exception.ObjectActionError(action='create',
                                               reason=_('already created'))
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
         db_volume = db.volume_create(self._context, updates)
         self._from_db_object(self._context, self, db_volume)
 
     @base.remotable
     def save(self):
-        updates = self.obj_get_changes()
+        updates = self.cinder_obj_get_changes()
         if updates:
             db.volume_update(self._context, self.id, updates)
 

diff --git a/cinder/tests/unit/objects/test_base.py b/cinder/tests/unit/objects/test_base.py
new file mode 100644 (file)
index 0000000..aa0b83c
--- /dev/null
+++ b/cinder/tests/unit/objects/test_base.py
@@ -0,0 +1,77 @@
+#    Copyright 2015 Red Hat, Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import datetime
+import uuid
+
+from iso8601 import iso8601
+
+from cinder.objects import base
+from cinder.tests.unit import objects as test_objects
+
+
+@base.CinderObjectRegistry.register
+class TestObject(base.CinderObject):
+    fields = {
+        'scheduled_at': base.fields.DateTimeField(nullable=True),
+        'uuid': base.fields.UUIDField(),
+        'text': base.fields.StringField(nullable=True),
+    }
+
+
+class TestCinderObject(test_objects.BaseObjectsTestCase):
+    """Tests methods from CinderObject."""
+
+    def setUp(self):
+        super(TestCinderObject, self).setUp()
+        self.obj = TestObject(
+            scheduled_at=None,
+            uuid=uuid.uuid4(),
+            text='text')
+        self.obj.obj_reset_changes()
+
+    def test_cinder_obj_get_changes_no_changes(self):
+        self.assertDictEqual({}, self.obj.cinder_obj_get_changes())
+
+    def test_cinder_obj_get_changes_other_changes(self):
+        self.obj.text = 'text2'
+        self.assertDictEqual({'text': 'text2'},
+                             self.obj.cinder_obj_get_changes())
+
+    def test_cinder_obj_get_changes_datetime_no_tz(self):
+        now = datetime.datetime.utcnow()
+        self.obj.scheduled_at = now
+        self.assertDictEqual({'scheduled_at': now},
+                             self.obj.cinder_obj_get_changes())
+
+    def test_cinder_obj_get_changes_datetime_tz_utc(self):
+        now_tz = iso8601.parse_date('2015-06-26T22:00:01Z')
+        now = now_tz.replace(tzinfo=None)
+        self.obj.scheduled_at = now_tz
+        self.assertDictEqual({'scheduled_at': now},
+                             self.obj.cinder_obj_get_changes())
+
+    def test_cinder_obj_get_changes_datetime_tz_non_utc_positive(self):
+        now_tz = iso8601.parse_date('2015-06-26T22:00:01+01')
+        now = now_tz.replace(tzinfo=None) - datetime.timedelta(hours=1)
+        self.obj.scheduled_at = now_tz
+        self.assertDictEqual({'scheduled_at': now},
+                             self.obj.cinder_obj_get_changes())
+
+    def test_cinder_obj_get_changes_datetime_tz_non_utc_negative(self):
+        now_tz = iso8601.parse_date('2015-06-26T10:00:01-05')
+        now = now_tz.replace(tzinfo=None) + datetime.timedelta(hours=5)
+        self.obj.scheduled_at = now_tz
+        self.assertDictEqual({'scheduled_at': now},
+                             self.obj.cinder_obj_get_changes())