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

appengine/findit/model/versioned_model.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides a model to support versioned entities in datastore.
 
 Idea: use a root model entity to keep track of the most recent version of a
 versioned entity, and make the versioned entities and the root model entity in
 the same entity group so that they could be read and written in a transaction.
 """
@@ skipping 13 matching lines @@
 
 
 class VersionedModel(ndb.Model):
   """A model that supports versioning.
 
   Subclass will automatically be versioned, if use GetVersion() to
   read and use Save() to write.
   """
 
   @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
 
     if version is None:
       version = root.current
     elif version < 1:
       #  Return None for versions < 1, which causes exceptions in ndb.Key()
       return None
 
     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
 
     def SetNewKey():
       root.current += 1
       self.key = ndb.Key(self.__class__, root.current, parent=root_key)
 
     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,
           datastore_errors.TransactionFailedError) as e:
         # https://cloud.google.com/appengine/docs/python/datastore/transactions
         # states the result is ambiguous, it could have succeeded.
         logging.info('Transaction likely failed: %s', e)
       except (
           apiproxy_errors.CancelledError,
           datastore_errors.BadRequestError,
           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):
     """Returns a root model that can be used for versioned entities."""
     root_model_name = '%sRoot' % cls.__name__
 
     class _RootModel(_GroupRoot):
 
       @classmethod
       def _get_kind(cls):
         return root_model_name
 
     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)