1191300 - celery errors: Task kuma.wiki.tasks.build_json_data_for_document: 'AttributeError("\'NoneType\' object has no attribute \'summary\'", )'

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: General, defect)

Description

[Luke Crouch [:groovecoder]]

What did you do?
================
1. Woke up and checked my email

What happened?
==============
Got 11 emails with this error

What should have happened?
==========================
No emails

Is there anything else we should know?
======================================
We can diagnose and fix, or squelch the errors - whichever makes the most sense.

Comment 1

[Jannis Leidel [:jezdez]]

There were 5 different wiki documents (all of them were translations) that hadn't a current revision assigned, which I would assume is a sign of either:

- a race condition in the translate view
- an issue with the db cluster going away every once in a and resulting in an unclean state during save
- or something else

In other parts of the code base we heavily check if the document has a current revision, which doesn't make sense to me since the whole architecture expects it to be there. I would assume that those conditions are "fixes" from previous instances of this error that were applied in favor of fixing the actual issue in the wiki editing code.

I must say, it's not without dread that I look through the translate view, it's so gobbled code with different Django forms dealing with the posted data that I'm afraid to touch it. OTOH I'm in awe that this fragile function actually has only produced so few errors so far. But maybe it just haven't showed up as much since we hide that unclean state behind dozens of condition checks for the current revision.

I've now fixed the immediate source of errors by manually saving 4 of the 5 documents, creating new revisions (the first for those documents) and setting the "current_revision" field successfully. The 5th I've deleted since it was not even capable of being loaded in the edit view and threw an error right away. It didn't have any content to store in a current revision anyway.

So, @groovecoder, @hoosteeno, I consider this to be a major issue, not minor as it implies we have a leak in our wiki editing interface. But I don't know how to fix it. What should we do?

Comment 2

[Justin Crawford [:hoosteeno] [:jcrawford]]

> But I don't know
> how to fix it. What should we do?

I suspect the solution to this requires a depth of knowledge about kuma that I don't have. But when I see denormalization (as in "wiki_document.current_revision_field", which can be derived) causing consistency errors, I have to ask whether the denormalization is strictly necessary.

Comment 3

[Jannis Leidel [:jezdez]]

:hoosteeno The code comment next to the column definition reads:

    # Latest approved revision. L10n dashboard depends on this being so (rather
    # than being able to set it to earlier approved revisions).

So I think this is used in the translation review process to indicate which - if any - revision is "current" of a translated document. IMO that's a pretty in-transparent way of doing that, but at least would explain why there are so many checks in the code base to check for a "current revision" (read: "approved revision").

Oddly enough that approval system doesn't apply to original English documents that are automatically being assigned the "current revision" after it was created.

So to be precise "current_revision" contains information about which revision to show as well if it was approved depending on its language. Why this wasn't stored in separate fields that would allow a query for "give me the latest revision of a document in language x that is also approved" is beyond me. Maybe Luke can shed some light on this.

Comment 4

[Luke Crouch [:groovecoder]]

Just got to the needinfo? here ...

Not sure I would call wiki_document.current_revision a denormalization? It's a FK to wiki_revision.

We do have a wiki_revision.is_approved field from SUMO days when revisions were not automatically approved. IIRC SUMO set wiki_revision.is_approved = True and wiki_document.current_revision at the same time. We do the same, we just do it automatically.

So, we *could* remove the is_approved field, since it's always True for us anyway. But we need wiki_document.current_revision.

Make sense?

Comment 5

[Justin Crawford [:hoosteeno] [:jcrawford]]

OK, as a foreign key it makes sense. Right now the column is defined as an indexed integer field with default value NULL, not a foreign key with foreign key constraints. Which means the application itself has to be responsible for referential integrity. And this bug suggests that while the application is depending on referential integrity, it's not capable of maintaining it for some reason. 

I'm not sure what effect adding foreign key constraints would have -- maybe, an error when we try to set NULL instead of a revision ID. That might be preferable to an error when we retrieve NULL, because it'd help us debug the source of the issue rather than the symptom.

P.S. I still don't know enough about the application to intelligently debug.

Comment 6

[Peter Bengtsson [:peterbe]]

Ryan,
Are you aware of any similar exceptions still happening around build_json_data_for_document() ?

I'm tempted to resolve this due to so much else has changed. If it does crop up again we should have it linked to Sentry and a Traceback.

Comment 7

[Ryan Johnson (:rjohnson)]

No, I don't know of any similar exceptions still happening. I agree that we should resolve this.