Make backward-forward compatible version.

appengine/swarming/server/task_request.py

Index: appengine/swarming/server/task_request.py
diff --git a/appengine/swarming/server/task_request.py b/appengine/swarming/server/task_request.py
index 8bd19f5e12925f6aa300b27288b299f86661bb20..f9c0674fb834b31bc67f3ccb26799b8f5da96958 100644
--- a/appengine/swarming/server/task_request.py
+++ b/appengine/swarming/server/task_request.py
@@ -51,6 +51,7 @@ separate entity to clearly declare the boundary for task request deduplication.
 
 import datetime
 import hashlib
+import itertools
 import posixpath
 import random
 import re
@@ -150,11 +151,20 @@ def _validate_dict_of_strings(prop, value):
         '%s must be a dict of strings, not %r' % (prop._name, value))
 
 
-def _validate_dimensions(prop, value):
-  """Validates TaskProperties.dimensions."""
-  # pylint: disable=W0212
+def _validate_dimension_flat(_prop, value):
+  """Validates TaskProperties.dimensions_flat."""
   if not value:
-    raise datastore_errors.BadValueError(u'%s must be specified' % prop._name)
+    raise datastore_errors.BadValueError(u'dimensions must be specified')
+  key, val = value.split(u':', 1)
+  if not config.validate_dimension_key(key):
+    raise datastore_errors.BadValueError(u'dimension %r isn\'t valid' % key)
+  if not config.validate_dimension_value(val):
+    raise datastore_errors.BadValueError(
+        u'dimension %r:%r isn\'t valid' % (key, val))
+
+
+def _validate_dimensions_dict(prop, value):
+  """Validates old TaskProperties.dimensions_dict."""
   _validate_dict_of_strings(prop, value)
   for key, val in value.iteritems():
     if not config.validate_dimension_key(key):
@@ -162,12 +172,6 @@ def _validate_dimensions(prop, value):
     if not config.validate_dimension_value(val):
       raise datastore_errors.BadValueError(
           u'dimension %r:%r isn\'t valid' % (key, val))
-  if u'pool' not in value and u'id' not in value:
-    raise datastore_errors.BadValueError(
-        u'At least one of \'id\' or \'pool\' must be used as %s' % prop._name)
-  if len(value) > 64:
-    raise datastore_errors.BadValueError(
-        '%s can have up to 64 keys' % prop._name)
 
 
 def _validate_env(prop, value):
@@ -488,10 +492,14 @@ class TaskProperties(ndb.Model):
   cipd_input = ndb.LocalStructuredProperty(CipdInput)
 
   # Filter to use to determine the required properties on the bot to run on. For
-  # example, Windows or hostname. Encoded as json. Either 'pool' or 'id'
-  # dimension are required (see _validate_dimensions).
-  dimensions = datastore_utils.DeterministicJsonProperty(
-      validator=_validate_dimensions, json_type=dict, indexed=False)
+  # example, Windows or hostname. Either 'pool' or 'id' dimension is required.
+  dimensions_flat = ndb.StringProperty(
+      validator=_validate_dimension_flat, indexed=False, repeated=True)
+  # Old way to express dimensions. Not used anymore as this doesn't support
+  # repeated keys.
+  dimensions_dict = datastore_utils.DeterministicJsonProperty(
+      validator=_validate_dimensions_dict, json_type=dict, indexed=False,
+      name='dimensions')
 
   # Environment variables. Encoded as json. Optional.
   env = datastore_utils.DeterministicJsonProperty(
@@ -533,6 +541,13 @@ class TaskProperties(ndb.Model):
   has_secret_bytes = ndb.BooleanProperty(default=False, indexed=False)
 
   @property
+  def dimensions(self):
+    """Returns a dict(key, value)."""
+    if self.dimensions_dict:
+      return self.dimensions_dict
+    return dict(i.split(u':', 1) for i in self.dimensions_flat)
+
+  @property
   def is_terminate(self):
     """If True, it is a terminate request."""
     return (
@@ -550,6 +565,12 @@ class TaskProperties(ndb.Model):
         not self.outputs and
         not self.has_secret_bytes)
 
+  def to_dict(self):
+    out = super(TaskProperties, self).to_dict(
+        exclude=['dimensions_dict', 'dimensions_flat'])
+    out['dimensions'] = self.dimensions
+    return out
+
   def _pre_put_hook(self):
     super(TaskProperties, self)._pre_put_hook()
     if self.is_terminate:
@@ -557,6 +578,29 @@ class TaskProperties(ndb.Model):
       # already check those.
       return
 
+    # Dimensions.
+    if not self.dimensions:
+      raise datastore_errors.BadValueError(u'dimensions are required')
+    keys = self.dimensions.keys()
+    count_pool = sum(1 for k in keys if k == u'pool')
+    count_id = sum(1 for k in keys if k == u'id')
+    if not count_pool and not count_id:
+      raise datastore_errors.BadValueError(
+          u'At least one of \'id\' or \'pool\' must be used as dimensions')
+    if count_id > 1:
+      raise datastore_errors.BadValueError(
+          u'\'id\' cannot be specified more than once in dimensions')
+    if len(keys) > 64:
+      raise datastore_errors.BadValueError(
+          'dimensions can have up to 64 entries')
+    if not _is_sorted(self.dimensions_flat):
+      # Better check here than silently sort since operations (like calculating
+      # dimensions_hash) is done before storage, which would cause silent
+      # corruption.
+      raise datastore_errors.BadValueError(
+          u'internal error: dimensions_flat must be sorted')
+
+    # Isolated input and commands.
     isolated_input = self.inputs_ref and self.inputs_ref.isolated
     if not self.command and not isolated_input:
       raise datastore_errors.BadValueError(
@@ -766,6 +810,13 @@ class TaskRequest(ndb.Model):
           'pubsub_userdata requires pubsub_topic')
 
 
+### Private stuff.
+
+
+def _is_sorted(l):
+  return all(a <= b for a, b in itertools.izip(l[:-1], l[1:]))
+
+
 ### Public API.
 
 
@@ -776,7 +827,7 @@ def create_termination_task(bot_id, allow_high_priority):
     TaskRequest for priority 0 (highest) termination task.
   """
   properties = TaskProperties(
-      dimensions={u'id': unicode(bot_id)},
+      dimensions_dict={u'id': unicode(bot_id)},
       execution_timeout_secs=0,
       grace_period_secs=0,
       io_timeout_secs=0)
@@ -984,7 +1035,9 @@ def new_request_clone(original_request, original_secret_bytes,
   now = utils.utcnow()
   if original_request.properties.is_terminate:
     raise ValueError('cannot clone a terminate request')
-  properties = TaskProperties(**original_request.properties.to_dict())
+  orig_props = original_request.properties.to_dict()
+  dimensions_dict = orig_props.pop(u'dimensions')
+  properties = TaskProperties(dimensions_dict=dimensions_dict, **orig_props)
   properties.idempotent = False
   expiration_ts = (
       now + (original_request.expiration_ts - original_request.created_ts))