[Findit] Extending versioned_model.py to support versioning multiple entities of the same class.

appengine/findit/model/versioned_model.py

Index: appengine/findit/model/versioned_model.py
diff --git a/appengine/findit/model/versioned_model.py b/appengine/findit/model/versioned_model.py
index f45a10ad3ddabe23fe969ef739c3e600b6ea82be..7a923a180ce8133a44d1f06559dcd7d3512cacfc 100644
--- a/appengine/findit/model/versioned_model.py
+++ b/appengine/findit/model/versioned_model.py
@@ -31,16 +31,46 @@ class VersionedModel(ndb.Model):
   """
 
   @property
+  def root_string_id(self):

[stgao, 2016/09/22 00:44:01]

Why do we care whether the root id is a string or an integer?

[stgao, 2016/09/22 00:44:01]

Also not make it a public property.

[lijeffrey, 2016/09/22 17:36:15]

Done.

+    if not self.key:
+      return None
+
+    key_pairs = self.key.pairs()
+    root_string_id = key_pairs[0][1]
+
+    if len(key_pairs) > 1:
+      # The key is expected to have format Key(root, string_id, kind, version)
+      return root_string_id
+
+    if isinstance(root_string_id, basestring):
+      # The key has format Key(kind, string_id)
+      return root_string_id
+
+    # Otherwise, the key has format Key(root, version_number).
+    return None
+
+  @property
   def version(self):
-    return self.key.integer_id() if self.key else 0
+    if not self.key:
+      return 0
+
+    key_pairs = self.key.pairs()
+    if len(key_pairs) == 1:
+      # Key(root, version_number) or Key(kind, string_id). The latter is used

[stgao, 2016/09/22 00:44:01]

This is the child entity of the root.

Per discussion, this change is not needed. The original code should work as
expected.

[lijeffrey, 2016/09/22 17:36:14]

A slight change is actually needed:

When creating a versioned entity with a key specified, such as MFA, the design
of this code gives an initial key before the entity is saved looks like

ndb.Key('MFA', 'master/builder/number/step/test')

once it's saved it then gets assigned a new key:
ndb.Key('MFARoot', 'master/builder/number/step/test', 'MFA', 1)

In the pre-saved version, the integer_id() component of key is None since a
string_id is supplied, but we want that to return 0.

In the post-saved version, an integer_id is assigned as the version number and
the string_id supplied component is assigned to the root key.

Changed to:

return self.key.integer_id() or 0 if self.key else 0

[stgao, 2016/09/23 06:27:14]

Should we fix the problem in the subclasses (MFA here) of this VersionedModel
instead?

[stgao, 2016/09/23 16:35:53]

Another option is to add a function here for instance creation, and make it an
API contract between this class and its subclasses.

+      # only when an entity has been created but not yet written to ndb.
+      return self.key.integer_id() or 0
+
+    # Key(root, string_id, type, version_number).
+    return key_pairs[-1][-1]
 
   @classmethod
-  def GetVersion(cls, version=None):
+  def GetVersion(cls, root_string_id=None, version=None):
     """Returns a version of the entity, the latest if version=None."""
     assert not ndb.in_transaction()
 
-    root_key = cls._GetRootKey()
+    root_key = cls._GetRootKey(root_string_id)
     root = root_key.get()
+
     if not root or not root.current:
       return None
 
@@ -53,20 +83,32 @@ class VersionedModel(ndb.Model):
     return ndb.Key(cls, version, parent=root_key).get()
 
   @classmethod
-  def GetLatestVersionNumber(cls):
-    root_entity = cls._GetRootKey().get()
+  def GetLatestVersionNumber(cls, root_string_id=None):
+    root_entity = cls._GetRootKey(root_string_id).get()
     if not root_entity:
       return -1
     return root_entity.current
 
-  def Save(self):
-    """Saves the current entity, but as a new version."""
-    root_key = self._GetRootKey()
+  def Save(self, should_try_next_version_number=True):

[stgao, 2016/09/22 00:44:01]

name nit: too long. How about retry_on_conflict or a shorter one?

[lijeffrey, 2016/09/22 17:36:15]

Done.

+    """Saves the current entity, but as a new version.
+
+    Args:
+      should_try_next_version_number: Boolean whether or not the next version

[stgao, 2016/09/22 00:44:01]

Format nit:

Args:
  param (type): comment.

[lijeffrey, 2016/09/22 17:36:14]

Done.

+        number should automatically be tried in case another transaction writes
+        the entity first with the same proposed new version number.
+
+    Returns:
+      The key of the newly written version, and a boolean whether or not this
+      call to Save() was responsible for creating it.
+    """
+    root_string_id = self.root_string_id

[stgao, 2016/09/22 00:44:01]

nit: root_string_id is used only once.

[lijeffrey, 2016/09/22 17:36:14]

Done.

+    root_key = self._GetRootKey(root_string_id)
     root = root_key.get() or self._GetRootModel()(key=root_key)
 
     def SaveData():
       if self.key.get():
         return False  # The entity exists, should retry.
+
       ndb.put_multi([self, root])
       return True
 
@@ -77,11 +119,17 @@ class VersionedModel(ndb.Model):
     SetNewKey()
     while True:
       while self.key.get():
-        SetNewKey()
+        if should_try_next_version_number:
+          SetNewKey()
+        else:
+          # Another transaction had already written the proposed new version, so
+          # return that version's key and False indicating this call to Save()
+          # was not responsible for creating it.
+          return self.key, False
 
       try:
         if ndb.transaction(SaveData, retries=0):
-          return self.key
+          return self.key, True
       except (
           datastore_errors.InternalError,
           datastore_errors.Timeout,
@@ -95,7 +143,13 @@ class VersionedModel(ndb.Model):
           RuntimeError) as e:
         logging.info('Transaction failure: %s', e)
       else:
-        SetNewKey()
+        if should_try_next_version_number:
+          SetNewKey()
+        else:
+          # Another transaction had already written the proposed new version, so
+          # return that version's key and False indicating this call to Save()
+          # was not responsible for creating it.
+          return self.key, False

[chanli, 2016/09/22 00:11:25]

Based on https://docs.python.org/3/tutorial/errors.html#handling-exceptions :
'The try ... except statement has an optional else clause, which, when present,
must follow all except clauses. It is useful for code that must be executed if
the try clause does not raise an exception. '

In our case, if try clause does not raise an exception, the function will
already complete by the return sentence. So this else clause here will never be
reached, right? Besides, why do we want to set a new key when we've done with
save anyway?

[stgao, 2016/09/22 00:44:01]

There is a if in line #131. We return only when we succeed in creating and
saving. Otherwise, we need to try the next key.

 
   @classmethod
   def _GetRootModel(cls):
@@ -111,5 +165,6 @@ class VersionedModel(ndb.Model):
     return _RootModel
 
   @classmethod
-  def _GetRootKey(cls):
-    return ndb.Key(cls._GetRootModel(), 1)
+  def _GetRootKey(cls, string_id=None):

[stgao, 2016/09/22 00:44:01]

name nit: root_id to be consistent.

[lijeffrey, 2016/09/22 17:36:15]

Done.

+    return ndb.Key(
+        cls._GetRootModel(), string_id if string_id is not None else 1)