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

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

When json parsing fails it can be hard to see the reason why. Adding
some explicit handling of common error cases (empty file and None input)
makes it easier to see the issue.

The error output should always be included even when add_json_log is
false otherwise things like empty or missing json files are silently
ignored and cause very weird and hard to debug issues later in recipes
code. Adding this reveals numerous issues where this state is being
accidentally ignored already.

With large json files there is the temptation to use add_json_log=False
to reduce the output. This makes it very hard to debug issues with the
json as you have no idea what the contents even looked like. By adding a
max output size allows making this trade off more easily. The line
snipping code is in utilities to allow it to be used in other similar
places.

[mithro, 2017-03-23 11:51:08]

Description was changed from

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

When json parsing fails it can be hard to see the reason why. Adding
some explicit handling of common error cases (empty file and None input)
makes it easier to see the issue.

The error output should always be included even when add_json_log is
false otherwise things like empty or missing json files are silently
ignored and cause very weird and hard to debug issues later in recipes
code. Adding this reveals numerous issues where this state is being
accidentally ignored already.

With large json files there is the temptation to use add_json_log=False
to reduce the output. This makes it very hard to debug issues with the
json as you have no idea what the contents even looked like. By adding a
max output size allows making this trade off more easily. The line
snipping code is in utilities to allow it to be used in other similar
places.

BUG=

json: Always output error on an exception.
==========

to

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

When json parsing fails it can be hard to see the reason why. Adding
some explicit handling of common error cases (empty file and None input)
makes it easier to see the issue.

The error output should always be included even when add_json_log is
false otherwise things like empty or missing json files are silently
ignored and cause very weird and hard to debug issues later in recipes
code. Adding this reveals numerous issues where this state is being
accidentally ignored already.

With large json files there is the temptation to use add_json_log=False
to reduce the output. This makes it very hard to debug issues with the
json as you have no idea what the contents even looked like. By adding a
max output size allows making this trade off more easily. The line
snipping code is in utilities to allow it to be used in other similar
places.
==========

[mithro, 2017-03-23 11:51:08]

tansell@chromium.org changed reviewers:
+ estaab@chromium.org, iannucci@chromium.org

[mithro, 2017-03-23 11:52:03]

Hi,

I've recently had a whole bunch of issues dealing with json files in recipes.
This CL tries to make things better.

I can't figure out how to test the case where self.raw.result is returning None?

Tim 'mithro' Ansell

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

phajdan.jr@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

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

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py
File recipe_modules/json/api.py (right):

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:5: import sys
nit: sort

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:73: explain_invalid = 'input was invalid'
Why not propagate the original exception then?

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:89: log_data =
recipe_util.snip_output_lines(log_data, self.max_output_bytes)
nit: 80 cols

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:137: def output(self, add_json_log=True,
force_full_log=False, name=None, leak_to=None):
Remember to propagate force_full_log.

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:148: leak_to - WTF does this do?
See comment above ("If leak_to is provided").

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/test_ap...
File recipe_modules/json/test_api.py (right):

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/test_ap...
recipe_modules/json/test_api.py:33: # FIXME: How do I get None into the data?
What happens if you just use None below instead of '' ?

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

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py
File recipe_modules/json/api.py (right):

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:69: explain_invalid = 'input was None'
This isn't a good check; 'null' is a perfectly valid json document which will
translate to None.

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:71: explain_invalid = 'input was empty'
This is also a bad check. The following json documents will incorrectly fail:
  * ""
  * 0
  * null
  * {}
  * []

[iannucci, 2017-03-23 19:38:54]

On 2017/03/23 19:38:00, iannucci wrote:
> https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py
> File recipe_modules/json/api.py (right):
> 
>
https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
> recipe_modules/json/api.py:69: explain_invalid = 'input was None'
> This isn't a good check; 'null' is a perfectly valid json document which will
> translate to None.
> 
>
https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
> recipe_modules/json/api.py:71: explain_invalid = 'input was empty'
> This is also a bad check. The following json documents will incorrectly fail:
>   * ""
>   * 0
>   * null
>   * {}
>   * []

Er, nevermind, this is checking raw_data not ret :(

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

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py
File recipe_modules/json/api.py (right):

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:73: explain_invalid = 'input was invalid'
On 2017/03/23 12:22:12, Paweł Hajdan Jr. wrote:
> Why not propagate the original exception then?

The exception is reported below.

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

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/api.py#...
recipe_modules/json/api.py:137: def output(self, add_json_log=True,
force_full_log=False, name=None, leak_to=None):
On 2017/03/23 12:22:12, Paweł Hajdan Jr. wrote:
> Remember to propagate force_full_log.

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.

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/test_ap...
File recipe_modules/json/test_api.py (right):

https://codereview.chromium.org/2768883004/diff/1/recipe_modules/json/test_ap...
recipe_modules/json/test_api.py:33: # FIXME: How do I get None into the data?
On 2017/03/23 12:22:12, Paweł Hajdan Jr. wrote:
> What happens if you just use None below instead of '' ?

That's what I would expect as well... however I would also expect that passing
None to invalid() above will also work?