From 2d7d86fccd0923ac17f55431cf756f980a5abc9c Mon Sep 17 00:00:00 2001
From: Thang Pham <thang.g.pham@gmail.com>
Date: Tue, 17 Mar 2015 10:24:08 -0400
Subject: [PATCH] Properly use obj_extra_fields in objects

The following patch fixes a bug in the snapshot object,
where fields that are not part of the Snapshot model,
i.e. name and volume_name, were not specified as
obj_extra_fields in the object. It also fixes the same
problem in the volume object, i.e. name and name_id.
With this patch, those extra fields are properly loaded,
e.g. snapshot.volume_name or volume.name.

Change-Id: Ie2b8dc66c897f89f941e9ad3b80ec4f4ab346430
Closes-Bug: #1429889
---
 cinder/objects/snapshot.py            | 17 +++++++++++++----
 cinder/objects/volume.py              |  4 ++++
 cinder/tests/objects/test_snapshot.py | 19 +++++++++++++++++++
 cinder/tests/objects/test_volume.py   |  6 ++++++
 4 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py
index 06accd758..cbf72b612 100644
--- a/cinder/objects/snapshot.py
+++ b/cinder/objects/snapshot.py
@@ -37,8 +37,6 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
 
     fields = {
         'id': fields.UUIDField(),
-        'name': fields.StringField(nullable=True),
-        'volume_name': fields.StringField(nullable=True),
 
         'user_id': fields.UUIDField(nullable=True),
         'project_id': fields.UUIDField(nullable=True),
@@ -61,10 +59,18 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
         'volume': fields.ObjectField('Volume', nullable=True),
     }
 
+    # NOTE(thangp): obj_extra_fields is used to hold properties that are not
+    # usually part of the model
+    obj_extra_fields = ['name', 'volume_name']
+
     @property
     def name(self):
         return CONF.snapshot_name_template % self.id
 
+    @property
+    def volume_name(self):
+        return self.volume.name
+
     def __init__(self, *args, **kwargs):
         super(Snapshot, self).__init__(*args, **kwargs)
         self._orig_metadata = {}
@@ -167,8 +173,11 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
             raise exception.OrphanedObjectError(method='obj_load_attr',
                                                 objtype=self.obj_name())
 
-        self.volume = objects.Volume.get_by_id(self._context, self.volume_id)
-        self.obj_reset_changes(fields=['volume'])
+        if attrname == 'volume':
+            self.volume = objects.Volume.get_by_id(self._context,
+                                                   self.volume_id)
+
+        self.obj_reset_changes(fields=[attrname])
 
     def delete_metadata_key(self, context, key):
         db.snapshot_metadata_delete(context, self.id, key)
diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index ccc53e938..8fb2e68b9 100644
--- a/cinder/objects/volume.py
+++ b/cinder/objects/volume.py
@@ -74,6 +74,10 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
         'replication_driver_data': fields.StringField(nullable=True),
     }
 
+    # NOTE(thangp): obj_extra_fields is used to hold properties that are not
+    # usually part of the model
+    obj_extra_fields = ['name', 'name_id']
+
     @property
     def name_id(self):
         return self.id if not self._name_id else self._name_id
diff --git a/cinder/tests/objects/test_snapshot.py b/cinder/tests/objects/test_snapshot.py
index f5bf7f7c9..56d306904 100644
--- a/cinder/tests/objects/test_snapshot.py
+++ b/cinder/tests/objects/test_snapshot.py
@@ -15,6 +15,7 @@
 import mock
 
 from cinder.objects import snapshot as snapshot_obj
+from cinder.objects import volume as volume_obj
 from cinder.tests import fake_volume
 from cinder.tests.objects import test_objects
 
@@ -99,6 +100,24 @@ class TestSnapshot(test_objects._LocalTest):
         snapshot_metadata_delete.assert_called_once_with(self.context, '1',
                                                          'key2')
 
+    def test_obj_fields(self):
+        volume = volume_obj.Volume(context=self.context, id=2, _name_id=2)
+        snapshot = snapshot_obj.Snapshot(context=self.context, id=1,
+                                         volume=volume)
+        self.assertEqual(['name', 'volume_name'], snapshot.obj_extra_fields)
+        self.assertEqual('snapshot-1', snapshot.name)
+        self.assertEqual('volume-2', snapshot.volume_name)
+
+    @mock.patch('cinder.objects.volume.Volume.get_by_id')
+    def test_obj_load_attr(self, volume_get_by_id):
+        snapshot = snapshot_obj.Snapshot._from_db_object(
+            self.context, snapshot_obj.Snapshot(), fake_snapshot)
+        volume = volume_obj.Volume(context=self.context, id=2)
+        volume_get_by_id.return_value = volume
+        self.assertEqual(volume, snapshot.volume)
+        volume_get_by_id.assert_called_once_with(self.context,
+                                                 snapshot.volume_id)
+
 
 class TestSnapshotList(test_objects._LocalTest):
     @mock.patch('cinder.db.snapshot_metadata_get', return_value={})
diff --git a/cinder/tests/objects/test_volume.py b/cinder/tests/objects/test_volume.py
index 840f30c13..901855a40 100644
--- a/cinder/tests/objects/test_volume.py
+++ b/cinder/tests/objects/test_volume.py
@@ -58,6 +58,12 @@ class TestVolume(test_objects._LocalTest):
         volume.destroy()
         volume_destroy.assert_called_once_with(self.context, '1')
 
+    def test_obj_fields(self):
+        volume = objects.Volume(context=self.context, id=2, _name_id=2)
+        self.assertEqual(['name', 'name_id'], volume.obj_extra_fields)
+        self.assertEqual('volume-2', volume.name)
+        self.assertEqual('2', volume.name_id)
+
 
 class TestVolumeList(test_objects._LocalTest):
     @mock.patch('cinder.db.volume_get_all')
-- 
2.45.2