From: Michał Dulko <michal.dulko@intel.com>
Date: Thu, 22 Oct 2015 12:54:35 +0000 (+0200)
Subject: Make relationships in objects consistent
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=288ca5d8d6384926c2c3b2cb84b02be843846d2d;p=openstack-build%2Fcinder-build.git

Make relationships in objects consistent

In Volume object 1:n relationship on volume_attachment was modeled
by ListOfObjectsField. Moreover _from_db_object and obj_load_attr
actually were setting VolumeAttachmentList as value of that field,
which is wrong behavior.

In CGSnapshot similar relationship on snapshots was done by ObjectField
with SnapshotList inside.

This commit unifies the approach to use the latter. Also unit test is
added to prevent mismatch of field type and value set on it.

Change-Id: I802fc8807d7d4c42680bb19866c3e90c866d3f26
Closes-Bug: 1508889
---

diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index 69435c308..e669a45bf 100644
--- a/cinder/objects/volume.py
+++ b/cinder/objects/volume.py
@@ -84,8 +84,8 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
         'metadata': fields.DictOfStringsField(nullable=True),
         'admin_metadata': fields.DictOfStringsField(nullable=True),
         'volume_type': fields.ObjectField('VolumeType', nullable=True),
-        'volume_attachment': fields.ListOfObjectsField('VolumeAttachment',
-                                                       nullable=True),
+        'volume_attachment': fields.ObjectField('VolumeAttachmentList',
+                                                nullable=True),
     }
 
     # NOTE(thangp): obj_extra_fields is used to hold properties that are not
@@ -176,7 +176,7 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
                 context, objects.VolumeAttachmentList(context),
                 objects.VolumeAttachment,
                 db_volume.get('volume_attachment'))
-            volume.volume_attachment = attachments.objects
+            volume.volume_attachment = attachments
 
         volume._context = context
         volume.obj_reset_changes()
@@ -242,10 +242,9 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
             self.volume_type = objects.VolumeType.get_by_id(
                 self._context, self.volume_type_id)
         elif attrname == 'volume_attachment':
-            attachments = (
-                objects.VolumeAttachmentList.get_all_by_volume_id(
-                    self._context, self.id))
-            self.volume_attachment = attachments.objects
+            attachments = objects.VolumeAttachmentList.get_all_by_volume_id(
+                self._context, self.id)
+            self.volume_attachment = attachments
 
         self.obj_reset_changes(fields=[attrname])
 
diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py
index 0e75c8d2c..8cadcafde 100644
--- a/cinder/tests/unit/objects/test_volume.py
+++ b/cinder/tests/unit/objects/test_volume.py
@@ -134,6 +134,53 @@ class TestVolume(test_objects.BaseObjectsTestCase):
         self.assertEqual({'key1': 'value1'}, volume.metadata)
         metadata_delete.assert_called_once_with(self.context, '1', 'key2')
 
+    @mock.patch('cinder.db.volume_metadata_get')
+    @mock.patch('cinder.db.volume_admin_metadata_get')
+    @mock.patch('cinder.objects.volume_type.VolumeType.get_by_id')
+    @mock.patch('cinder.objects.volume_attachment.VolumeAttachmentList.'
+                'get_all_by_volume_id')
+    def test_obj_load_attr(self, mock_va_get_all_by_vol, mock_vt_get_by_id,
+                           mock_admin_metadata_get, mock_metadata_get):
+        volume = objects.Volume._from_db_object(
+            self.context, objects.Volume(), fake_volume.fake_db_volume())
+
+        # Test metadata lazy-loaded field
+        metadata = {'foo': 'bar'}
+        mock_metadata_get.return_value = metadata
+        self.assertEqual(metadata, volume.metadata)
+        mock_metadata_get.assert_called_once_with(self.context, volume.id)
+
+        # Test volume_type lazy-loaded field
+        volume_type = objects.VolumeType(context=self.context, id=5)
+        mock_vt_get_by_id.return_value = volume_type
+        self.assertEqual(volume_type, volume.volume_type)
+        mock_vt_get_by_id.assert_called_once_with(self.context,
+                                                  volume.volume_type_id)
+
+        # Test volume_attachment lazy-loaded field
+        va_objs = [objects.VolumeAttachment(context=self.context, id=i)
+                   for i in [3, 4, 5]]
+        va_list = objects.VolumeAttachmentList(context=self.context,
+                                               objects=va_objs)
+        mock_va_get_all_by_vol.return_value = va_list
+        self.assertEqual(va_list, volume.volume_attachment)
+        mock_va_get_all_by_vol.assert_called_once_with(self.context, volume.id)
+
+        # Test admin_metadata lazy-loaded field - user context
+        adm_metadata = {'bar': 'foo'}
+        mock_admin_metadata_get.return_value = adm_metadata
+        self.assertEqual({}, volume.admin_metadata)
+        self.assertFalse(mock_admin_metadata_get.called)
+
+        # Test admin_metadata lazy-loaded field - admin context
+        adm_context = self.context.elevated()
+        volume = objects.Volume._from_db_object(adm_context, objects.Volume(),
+                                                fake_volume.fake_db_volume())
+        adm_metadata = {'bar': 'foo'}
+        mock_admin_metadata_get.return_value = adm_metadata
+        self.assertEqual(adm_metadata, volume.admin_metadata)
+        mock_admin_metadata_get.assert_called_once_with(adm_context, volume.id)
+
 
 class TestVolumeList(test_objects.BaseObjectsTestCase):
     @mock.patch('cinder.db.volume_glance_metadata_get', return_value={})