[buildbucket] Implement properties update

Extend UpdateBuild RPC to support output properties.

Bug: 843714
Change-Id: Ibcf19cb2b63a887962d435e2e55676d2027d6f82
Reviewed-on: https://chromium-review.googlesource.com/c/1322230
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Jao-ke Chin-Lee <jchinlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#18884}
diff --git a/appengine/cr-buildbucket/model.py b/appengine/cr-buildbucket/model.py
index 4c296a1..0cceca0 100644
--- a/appengine/cr-buildbucket/model.py
+++ b/appengine/cr-buildbucket/model.py
@@ -26,6 +26,7 @@
 
 # Key in Build.parameters that specifies the builder name.
 BUILDER_PARAMETER = 'builder_name'
+PROPERTIES_PARAMETER = 'properties'
 
 
 class BuildStatus(messages.Enum):
diff --git a/appengine/cr-buildbucket/v2/api.py b/appengine/cr-buildbucket/v2/api.py
index a63c20a..d30f465 100644
--- a/appengine/cr-buildbucket/v2/api.py
+++ b/appengine/cr-buildbucket/v2/api.py
@@ -2,9 +2,11 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import json
 import functools
 
 from google.appengine.ext import ndb
+from google.protobuf import json_format
 from google.protobuf import symbol_database
 
 from components import auth
@@ -255,6 +257,8 @@
     )
   validation.validate_update_build_request(req)
 
+  update_paths = set(req.update_mask.paths)
+
   @ndb.transactional_tasklet
   def txn_async():
     build_proto = req.build
@@ -265,18 +269,28 @@
       raise not_found(
           'Cannot update nonexisting build with id %s', build_proto.id
       )
+    to_put = [build]
 
-    # Update build steps.
-    build_steps = model.BuildSteps(
-        key=model.BuildSteps.key_for(build.key),
-        step_container=build_pb2.Build(steps=build_proto.steps),
-    )
+    if 'build.steps' not in update_paths:
+      build_steps = yield model.BuildSteps.key_for(build.key).get_async()
+    else:
+      # Update build steps.
+      build_steps = model.BuildSteps(
+          key=model.BuildSteps.key_for(build.key),
+          step_container=build_pb2.Build(steps=build_proto.steps),
+      )
+      to_put.append(build_steps)
 
-    # Currently, only build steps are supported; later, update other fields
-    # specified in req.update_mask.
+    if 'build.output.properties' in update_paths:
+      # TODO(nodir): persist it in a separate entity, in Struct binary format.
+      # The following is inefficient.
+      build.result_details = build.result_details or {}
+      build.result_details[model.PROPERTIES_PARAMETER] = json.loads(
+          json_format.MessageToJson(build_proto.output.properties)
+      )
 
     # Store and convert back to build_pb2.Build proto for return.
-    yield ndb.put_multi_async([build, build_steps])
+    yield ndb.put_multi_async(to_put)
     raise ndb.Return(v2.build_to_v2(build, build_steps))
 
   build = yield txn_async()
diff --git a/appengine/cr-buildbucket/v2/test/api_test.py b/appengine/cr-buildbucket/v2/test/api_test.py
index 04c4cde..520c673 100644
--- a/appengine/cr-buildbucket/v2/test/api_test.py
+++ b/appengine/cr-buildbucket/v2/test/api_test.py
@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import contextlib
+import copy
 import os
 import datetime
 
@@ -135,7 +137,7 @@
   @mock.patch('service.get_async', autospec=True)
   def test_trimming_exclude(self, get_async):
     get_async.return_value = future(
-        self.new_build_v1(parameters={'properties': {'a': 'b'}})
+        self.new_build_v1(parameters={model.PROPERTIES_PARAMETER: {'a': 'b'}})
     )
     req = rpc_pb2.GetBuildRequest(id=1)
     res = self.call(self.api.GetBuild, req)
@@ -145,7 +147,7 @@
   def test_trimming_include(self, get_async):
     get_async.return_value = future(
         self.new_build_v1(parameters={
-            'properties': {'a': 'b'},
+            model.PROPERTIES_PARAMETER: {'a': 'b'},
         }),
     )
     req = rpc_pb2.GetBuildRequest(
@@ -298,32 +300,62 @@
       metadata.append((api.BUILD_TOKEN_HEADER, token))
     return build_req, ctx
 
-  @mock.patch('service.get_async', autospec=True)
-  def test_update_steps(self, mock_get_async):
-    build = model.Build(
-        id=123,
-        status=model.BuildStatus.STARTED,
-        bucket_id='chromium/try',
-        created_by=auth.Identity('user', 'foo@google.com'),
-        create_time=utils.utcnow(),
-        start_time=utils.utcnow(),
-        parameters_actual=dict(),
-    )
-    mock_get_async.return_value = future(build)
+  @contextlib.contextmanager
+  def mock_build(self, build_id):
+    with mock.patch('service.get_async', autospec=True) as mock_get_async:
+      build = model.Build(
+          id=build_id,
+          status=model.BuildStatus.STARTED,
+          bucket_id='chromium/try',
+          created_by=auth.Identity('user', 'foo@google.com'),
+          create_time=utils.utcnow(),
+          start_time=utils.utcnow(),
+          parameters_actual=dict(),
+      )
+      mock_get_async.return_value = future(build)
+      yield build
 
-    build_proto = build_pb2.Build(id=123)
-    with open(os.path.join(THIS_DIR, 'steps.pb.txt')) as f:
-      text = protoutil.parse_multiline(f.read())
-      text_format.Merge(text, build_proto)
+  def test_update_steps(self):
+    with self.mock_build(build_id=123) as build:
+      build_proto = build_pb2.Build(id=123)
+      with open(os.path.join(THIS_DIR, 'steps.pb.txt')) as f:
+        text = protoutil.parse_multiline(f.read())
+        text_format.Merge(text, build_proto)
 
-    req, ctx = self._mk_update_req(build_proto)
-    req.fields.paths[:] = ['id', 'steps']
+      req, ctx = self._mk_update_req(build_proto)
+      req.fields.paths[:] = ['id', 'steps']
+      actual = self.call(self.api.UpdateBuild, req, ctx=ctx)
+      self.assertEqual(actual, build_proto)
 
-    actual = self.call(self.api.UpdateBuild, req, ctx=ctx)
-    self.assertEqual(actual, build_proto)
+      persisted = model.BuildSteps.key_for(build.key).get()
+      self.assertEqual(persisted.step_container.steps, build_proto.steps)
 
-    persisted = model.BuildSteps.key_for(build.key).get()
-    self.assertEqual(persisted.step_container.steps, build_proto.steps)
+  def test_update_properties(self):
+    with self.mock_build(build_id=123) as build:
+      expected_props = {'a': 1}
+      build_steps = model.BuildSteps(
+          key=model.BuildSteps.key_for(build.key),
+          step_container=build_pb2.Build(
+              steps=[step_pb2.Step(name='bot_update')],
+          ),
+      )
+      build_steps.put()
+
+      build_proto = build_pb2.Build(id=123)
+      build_proto.output.properties.update(expected_props)
+
+      req, ctx = self._mk_update_req(build_proto)
+      req.update_mask.paths[:] = ['build.output.properties']
+      req.fields.paths[:] = ['id', 'steps', 'output.properties']
+      actual = self.call(self.api.UpdateBuild, req, ctx=ctx)
+      expected = copy.deepcopy(build_proto)
+      expected.MergeFrom(build_steps.step_container)
+      self.assertEqual(actual, expected)
+
+      build = build.key.get()
+      self.assertEqual(
+          build.result_details[model.PROPERTIES_PARAMETER], expected_props
+      )
 
   def test_missing_token(self):
     build = build_pb2.Build(
diff --git a/appengine/cr-buildbucket/v2/test/validation_test.py b/appengine/cr-buildbucket/v2/test/validation_test.py
index d4c578d..22f46e7 100644
--- a/appengine/cr-buildbucket/v2/test/validation_test.py
+++ b/appengine/cr-buildbucket/v2/test/validation_test.py
@@ -423,9 +423,14 @@
     )
     self.assert_invalid(msg, 'required')
 
-  def test_missing_update_mask(self):
-    msg = rpc_pb2.UpdateBuildRequest(build=build_pb2.Build())
-    self.assert_invalid(msg, 'update only supports build.steps path currently')
+  def test_unsupported_paths(self):
+    msg = rpc_pb2.UpdateBuildRequest(
+        build=build_pb2.Build(),
+        update_mask=field_mask_pb2.FieldMask(paths=['build.input'],)
+    )
+    self.assert_invalid(
+        msg, r'update_mask\.paths: unsupported path\(s\) .+build\.input.+'
+    )
 
   @mock.patch('model.BuildSteps', autospec=True)
   def test_steps_too_big(self, mock_steps_mod):
diff --git a/appengine/cr-buildbucket/v2/validation.py b/appengine/cr-buildbucket/v2/validation.py
index d357846..792038e 100644
--- a/appengine/cr-buildbucket/v2/validation.py
+++ b/appengine/cr-buildbucket/v2/validation.py
@@ -197,9 +197,11 @@
   if not req.HasField('build'):
     _enter_err('build', 'required')
 
-  with _enter('update_mask'):
-    if req.update_mask.paths != ['build.steps']:
-      _err('update only supports build.steps path currently')
+  with _enter('update_mask', 'paths'):
+    supported = {'build.steps', 'build.output.properties'}
+    unsupported = set(req.update_mask.paths) - supported
+    if unsupported:
+      _err('unsupported path(s) %r', sorted(unsupported))
 
   with _enter('build'):
     with _enter('steps'):
@@ -416,12 +418,12 @@
 
 
 @contextlib.contextmanager
-def _enter(name):
-  _field_stack().append(name)
+def _enter(*names):
+  _field_stack().extend(names)
   try:
     yield
   finally:
-    _field_stack().pop()
+    _field_stack()[-len(names):] = []
 
 
 def _err(fmt, *args):