847474 - Runs that don't exist can cause a 500 error

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P3)

Description

[Peter Bengtsson [:peterbe]]

For example: https://l10n-dev-sj.mozilla.org/dashboard/compare?run=123

This leads to https://errormill.mozilla.org/webtools/elmo-dev/group/15017/

Comment 1

[Peter Bengtsson [:peterbe]]

In case the exception expires from errormill, the error was:

  ValueError: No JSON object could be decoded

  File "django/core/handlers/base.py", line 111, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "l10nstats/views.py", line 286, in compare
    json = simplejson.loads(json)
  File "simplejson/__init__.py", line 307, in loads
    return _default_decoder.decode(s)
  File "simplejson/decoder.py", line 335, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "simplejson/decoder.py", line 353, in raw_decode
    raise ValueError("No JSON object could be decoded")

Comment 2

[Peter Bengtsson [:peterbe]]

Attachment: Don't proceed if there are no builds

We can't return anything but a 200 error. The client did their part correctly.

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 720846 [details] [diff] [review]
Don't proceed if there are no builds

Review of attachment 720846 [details] [diff] [review]:
-----------------------------------------------------------------

So what happens here really is that there's a build, with steps, which have logs, all in the database.

But the actual log contents can't be read, because we've thrown them away.

I guess that a 404 is actually more appropriate?

Comment 4

[Peter Bengtsson [:peterbe]]

I think the error happened because of the `simplejson.loads('')`. 
So perhaps you're right, there are Build items but `generateLog()` returns nothing. (an empty iterable).

However, I still don't think a 404 is right. It's misleading in that the client hasn't actually done anything wrong. And a raised exception isn't good either.

Comment 5

[Peter Bengtsson [:peterbe]]

Hmm.. I've looked at this again and thought some more. I can't think of a better way to handle this. 
What we could potentially do is return one message if generateLog() returns nothing and another message if the queryset returns nothing. 

Either way, it would be wrong to allow json.loads() run on something nonsensical. And it would be wrong to tell the client their did something they could do better. 

So, if I amend the patch with a second explicit HttpResponse would you land it?

Comment 6

[Axel Hecht [:Pike]]

I got confused in my analysis.

This is what happens:

If a log entry exists in the database, but the file is gone, generateLog raises a 404.
If there files for all log entries in the db are there, but none return something for the json channel, we end up with an empty json.

In our case, the log entries in the db are gone (as are the files, I assume).

If the json is empty, I guess we should replace all the shebang in compare.html after the <p id="blurb"> with a message that further details couldn't be found. Something like "Detailed information for this comparison are not available."

Comment 7

[Peter Bengtsson [:peterbe]]

You mean something like::

+     if not json:
+         return render(request, 'l10nstats/compare.html', {'no_json': True})
-     json = simplejson.loads(json)

and then deal with it in the template. 

I like that.

Comment 8

[Axel Hecht [:Pike]]

Yeah, I'd go as far as do just leave nodes = None, and not change more. Not sure if a 100% good locale leaves nodes as something not-None, though.

The stats and all are still valid, and in the db, so there's actually some data to show.

Comment 9

[Peter Bengtsson [:peterbe]]

Attachment: Render the template even if there is no log data

It's not good enough to just set nodes=None since the template also depends on `widths` and `summary`.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 733049 [details] [diff] [review]
Render the template even if there is no log data

Review of attachment 733049 [details] [diff] [review]:
-----------------------------------------------------------------

We do have data for this run in the database, and we should show it. That's what my comment 8 was about.

Comment 11

[Peter Bengtsson [:peterbe]]

I'm sorry. I don't know why I'm struggling to see what you see. 

Here's the code::

    json = simplejson.loads(json)
    nodes = JSONAdaptor.adaptChildren(json['details'].get('children', []))
    summary = json['summary']
    if 'keys' not in summary:
        summary['keys'] = 0
    # create table widths for the progress bar
    widths = {}
    for k in ('changed', 'missing', 'missingInFiles', 'report', 'unchanged'):
        widths[k] = summary.get(k, 0) * 300 / summary['total']

If there's no json (first variable) there's no `nodes`, `summary` or `widths`. So all of those variables are out of the question. 

So, basically all use of `summary`, `widths` or `nodes` in compare.html needs to be *not* used. 

However, preparing for this comment I noticed I put the `{% if summary %}` statement 2 lines higher than necessary. The lines::

  <p id="backlink"><a href="{% url 'tinder_show_build' run.build.builder.name run.build.buildnumber %}">Build {{ run.build.buildnumber }}</a> from
  {{ run.build.starttime|date }} {{ run.build.starttime|time }}</p>

actually don't depend on the above mentioned variables.

Comment 12

[Peter Bengtsson [:peterbe]]

Note-to-self: Pike thinks that the only actual data we can't get out of the JSON (the log data) is the nodes. The summary stuff should all be in the database. 

The correct solution would be to populate `summary` from the database instead of from the parsed JSON.

Comment 13

[Peter Bengtsson [:peterbe]]

Attachment: Now it gets the summary numbers from the Run instance

Comment 14

[Peter Bengtsson [:peterbe]]

It hit me how to best test this. I just scp'ed the one missing file when I clicked on a random compare from the dashboard.

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 739287 [details] [diff] [review]
Now it gets the summary numbers from the Run instance

Review of attachment 739287 [details] [diff] [review]:
-----------------------------------------------------------------

Doh, I wrote shitty code back then. So, summary is the same thing as run, just that run doesn't need all the hacks around missing entries.

Can you just drop summary alltogether, and just use the run object instead? I'd suggest to do that both in the view and in the template.

Comment 16

[Peter Bengtsson [:peterbe]]

Attachment: no more summary

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 742079 [details] [diff] [review]
no more summary

Review of attachment 742079 [details] [diff] [review]:
-----------------------------------------------------------------

The pieces in the patch are all OK, but I just realized that we forgot about replacing the "Below you see the files and localizable strings missing and obsolete. The obsolete ones are striked through and grey. The data is organized hierarchically, the full path for a file is available as an tooltip." with "Detailed information for this comparison are not available." if nodes is None. That should be different from nodes being [], which would happen for a 100% translated run.

See also comment 6.

Comment 18

[Peter Bengtsson [:peterbe]]

Attachment: smarter ifs in the template

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 743262 [details] [diff] [review]
smarter ifs in the template

Review of attachment 743262 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with one more nit, I'd like to make the difference between "can't load" and "there's nothing" explicit.

Suggestion below:

::: apps/l10nstats/templates/l10nstats/compare.html
@@ +73,5 @@
>  </div>
>  {% endfor %}
>  {% endrecurse %}
> +</div>
> +{% else %}

I'd do a 

{% elif nodes|length_is:"0" %}
<p>No further details for this comparison.</p>
{% else %}

Comment 20

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/69e19f9414c5b831ec6d46d1b19a2019d7475450
fixes bug 847474 - Runs that don't exist can cause a 500 error, r=Pike