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 69435c308ad9c5bd1b7584b16fa59134d4d251b9..e669a45bfebd698c60eba408134b9ee0a33b4416 100644 (file)
--- 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 0e75c8d2cf598203586ea057215b5c98e05a54a3..8cadcafde327506cd27aa1a320692c542cd7b943 100644 (file)
--- 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={})