From 288ca5d8d6384926c2c3b2cb84b02be843846d2d Mon Sep 17 00:00:00 2001
From: =?utf8?q?Micha=C5=82=20Dulko?= <michal.dulko@intel.com>
Date: Thu, 22 Oct 2015 14:54:35 +0200
Subject: [PATCH] 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
---
 cinder/objects/volume.py                 | 13 +++----
 cinder/tests/unit/objects/test_volume.py | 47 ++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 7 deletions(-)

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={})
-- 
2.45.2