json: Improve error handling and adding ability to limit log output.

recipe_modules/json/api.py

Index: recipe_modules/json/api.py
diff --git a/recipe_modules/json/api.py b/recipe_modules/json/api.py
index 812f71ad459db1ca8ec72a7163cc0bfa8f17afd8..0609c3d95d03ac4059d39766c0a6fd0a19aaf7c6 100644
--- a/recipe_modules/json/api.py
+++ b/recipe_modules/json/api.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
+import sys

[Paweł Hajdan Jr., 2017/03/23 12:22:12]

nit: sort

 import functools
 import collections
 import contextlib
@@ -30,8 +31,14 @@ class JsonOutputPlaceholder(recipe_util.OutputPlaceholder):
 
   See the example recipe (./example.py) for some more uses.
   """
-  def __init__(self, api, add_json_log, name=None, leak_to=None):
+  MAX_OUTPUT_BYTES = 5000
+
+  def __init__(self, api, add_json_log, force_full_log=False, name=None, leak_to=None):
     self.raw = api.m.raw_io.output_text('.json', leak_to=leak_to)
+    if force_full_log:
+      self.max_output_bytes = None
+    else:
+      self.max_output_bytes = self.MAX_OUTPUT_BYTES
     self.add_json_log = add_json_log
     super(JsonOutputPlaceholder, self).__init__(name=name)
 
@@ -57,15 +64,30 @@ class JsonOutputPlaceholder(recipe_util.OutputPlaceholder):
     except (ValueError, TypeError) as ex:  # pragma: no cover
       invalid_error = str(ex)
 
+    if not valid:
+      if raw_data is None:
+        explain_invalid = 'input was None'

[iannucci, 2017/03/23 19:38:00]

This isn't a good check; 'null' is a perfectly valid json document which will
translate to None.

+      elif not raw_data:
+        explain_invalid = 'input was empty'

[iannucci, 2017/03/23 19:38:00]

This is also a bad check. The following json documents will incorrectly fail:
  * ""
  * 0
  * null
  * {}
  * []

+      else:
+        explain_invalid = 'input was invalid'

[Paweł Hajdan Jr., 2017/03/23 12:22:12]

Why not propagate the original exception then?

[iannucci, 2017/03/23 19:44:13]

The exception is reported below.

But yes; it might make sense to raise StepError("json output was invalid:
<explaination>")?

+
+      presentation.logs[self.label + ' (exception)'] = [
+          explain_invalid] + invalid_error.splitlines()
+
     if self.add_json_log:
+      # If valid, output a pretty-printed version of the JSON file, otherwise
+      # just use the raw data.
       if valid:
         with contextlib.closing(recipe_util.StringListIO()) as listio:
           json.dump(ret, listio, indent=2, sort_keys=True)
-        presentation.logs[self.label] = listio.lines
+        log_data = listio.lines
       else:
-        presentation.logs[self.label + ' (invalid)'] = raw_data.splitlines()
-        presentation.logs[self.label + ' (exception)'] = (
-          invalid_error.splitlines())
+        log_data = raw_data.splitlines()
+
+      if self.max_output_bytes:
+        log_data = recipe_util.snip_output_lines(log_data, self.max_output_bytes)

[Paweł Hajdan Jr., 2017/03/23 12:22:12]

nit: 80 cols

+      presentation.logs[self.label] = log_data
 
     return ret
 
@@ -112,12 +134,18 @@ class JsonApi(recipe_api.RecipeApi):
     return self.m.raw_io.input_text(self.dumps(data), '.json')
 
   @recipe_util.returns_placeholder
-  def output(self, add_json_log=True, name=None, leak_to=None):
+  def output(self, add_json_log=True, force_full_log=False, name=None, leak_to=None):

[Paweł Hajdan Jr., 2017/03/23 12:22:12]

Remember to propagate force_full_log.

[iannucci, 2017/03/23 19:44:13]

Maybe `limit_log=<default amount with None meaning unlimited>`?

I would also make this the last kwarg in case someone is using this method with
positional arguments.

     """A placeholder which will expand to '/tmp/file'.
 
     If leak_to is provided, it must be a Path object. This path will be used in
     place of a random temporary file, and the file will not be deleted at the
     end of the step.
+
+    Args:
+      add_json_log - Output the JSON in the step output.
+      force_full_log - Output the full JSON even if data is huge
+      name - Override the name of placeholder, default name is 'json.output'
+      leak_to - WTF does this do?

[Paweł Hajdan Jr., 2017/03/23 12:22:12]

See comment above ("If leak_to is provided").

     """
     return JsonOutputPlaceholder(self, add_json_log, name=name, leak_to=leak_to)