From 268d935a0a5adff7f0d59bdf3882210c49e45f5c Mon Sep 17 00:00:00 2001
From: "Ivan A. Melnikov" <imelnikov@griddynamics.com>
Date: Thu, 15 May 2014 22:18:45 +0400
Subject: [PATCH] Don't create fake LogBook when we can not fetch one

When there is no LogBook nor backend in ZookeeperJob or something is
wrong with book data, we now return None from `book` property instead
of new (fake) LogBook. Having two (or more) logbooks with same uuid but
different data is too confusing and makes root cause of the problem
(backend or jobboard misconfiguration) less obvious.

This change also adds `book_name` and `book_uuid` properties to Job
abstract class to provide a way to access book data even if book
itself cannot be fetched.

Change-Id: Iae1f918e35d41794fc348860e6aa3b52ae2211f4
---
 taskflow/jobs/backends/impl_zookeeper.py | 51 ++++++++++++++----------
 taskflow/jobs/job.py                     | 19 ++++++++-
 taskflow/tests/unit/jobs/base.py         |  2 +
 3 files changed, 51 insertions(+), 21 deletions(-)

diff --git a/taskflow/jobs/backends/impl_zookeeper.py b/taskflow/jobs/backends/impl_zookeeper.py
index 4de2091d..fd73a097 100644
--- a/taskflow/jobs/backends/impl_zookeeper.py
+++ b/taskflow/jobs/backends/impl_zookeeper.py
@@ -32,7 +32,6 @@ from taskflow.jobs import jobboard
 from taskflow.openstack.common import excutils
 from taskflow.openstack.common import jsonutils
 from taskflow.openstack.common import uuidutils
-from taskflow.persistence import logbook
 from taskflow import states
 from taskflow.utils import kazoo_utils
 from taskflow.utils import lock_utils
@@ -145,17 +144,18 @@ class ZookeeperJob(base_job.Job):
     def board(self):
         return self._board
 
-    def _load_book(self, book_uuid, book_name):
-        # No backend to attempt to fetch from :-(
-        if self._backend is None:
-            return logbook.LogBook(name=book_name, uuid=book_uuid)
-        # TODO(harlowja): we are currently limited by assuming that the job
-        # posted has the same backend as this loader (to start this seems to
-        # be a ok assumption, and can be adjusted in the future if we determine
-        # there is a use-case for multi-backend loaders, aka a registry of
-        # loaders).
-        with contextlib.closing(self._backend.get_connection()) as conn:
-            return conn.get_logbook(book_uuid)
+    def _load_book(self):
+        book_uuid = self.book_uuid
+        if self._backend is not None and book_uuid is not None:
+            # TODO(harlowja): we are currently limited by assuming that the
+            # job posted has the same backend as this loader (to start this
+            # seems to be a ok assumption, and can be adjusted in the future
+            # if we determine there is a use-case for multi-backend loaders,
+            # aka a registry of loaders).
+            with contextlib.closing(self._backend.get_connection()) as conn:
+                return conn.get_logbook(book_uuid)
+        # No backend to fetch from or no uuid specified
+        return None
 
     @property
     def state(self):
@@ -194,16 +194,27 @@ class ZookeeperJob(base_job.Job):
     @property
     def book(self):
         if self._book is None:
-            loaded_book = None
-            try:
-                book_uuid = self._book_data['uuid']
-                book_name = self._book_data['name']
-                loaded_book = self._load_book(book_uuid, book_name)
-            except (KeyError, TypeError):
-                pass
-            self._book = loaded_book
+            self._book = self._load_book()
         return self._book
 
+    @property
+    def book_uuid(self):
+        if self._book:
+            return self._book.uuid
+        if self._book_data:
+            return self._book_data.get('uuid')
+        else:
+            return None
+
+    @property
+    def book_name(self):
+        if self._book:
+            return self._book.name
+        if self._book_data:
+            return self._book_data.get('name')
+        else:
+            return None
+
 
 class ZookeeperJobBoardIterator(six.Iterator):
     """Iterator over a zookeeper jobboard.
diff --git a/taskflow/jobs/job.py b/taskflow/jobs/job.py
index a0264901..a4f0b416 100644
--- a/taskflow/jobs/job.py
+++ b/taskflow/jobs/job.py
@@ -66,7 +66,24 @@ class Job(object):
 
     @abc.abstractproperty
     def book(self):
-        """Any logbook associated with this job."""
+        """Logbook associated with this job.
+
+        If no logbook is associated with this job, this property is None.
+        """
+
+    @abc.abstractproperty
+    def book_uuid(self):
+        """UUID of logbook associated with this job.
+
+        If no logbook is associated with this job, this property is None.
+        """
+
+    @abc.abstractproperty
+    def book_name(self):
+        """Name of logbook associated with this job.
+
+        If no logbook is associated with this job, this property is None.
+        """
 
     @property
     def uuid(self):
diff --git a/taskflow/tests/unit/jobs/base.py b/taskflow/tests/unit/jobs/base.py
index addc7edf..e5faad6a 100644
--- a/taskflow/tests/unit/jobs/base.py
+++ b/taskflow/tests/unit/jobs/base.py
@@ -253,6 +253,8 @@ class BoardTestMixin(object):
             self.assertEqual(1, len(j.book))
             self.assertEqual(book.name, j.book.name)
             self.assertEqual(book.uuid, j.book.uuid)
+            self.assertEqual(book.name, j.book_name)
+            self.assertEqual(book.uuid, j.book_uuid)
 
             flow_details = list(j.book)
             self.assertEqual(flow_detail.uuid, flow_details[0].uuid)