661617 - [traceback] AttributeError: 'NoneType' object has no attribute 'package'

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Builder, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

https://builder-addons-next.allizom.org/addon/1000365/latest/ is returning a 500:

Traceback (most recent call last):

  File "/data/amo_python/www/builder.next/flightdeck/vendor/lib/python/django/core/handlers/base.py", line 100, in get_response
    response = callback(request, *callback_args, **callback_kwargs)

  File "/data/amo_python/www/builder.next/flightdeck/apps/jetpack/views.py", line 99, in package_view_or_edit
    if revision.package.deleted:

AttributeError: 'NoneType' object has no attribute 'package'

Comment 1

[Piotr Zalewa [:zalun]]

different than ObjectNotFound exception is raised.
I've fixed it - waiting for the day to start in US to review changes.

https://github.com/zalun/FlightDeck/commit/7286b797896e42f96993ad9b96ea5eac6c09e02a

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Now https://builder-addons.allizom.org/addon/1004872/latest/ is bombing out with the same stack.

Comment 3

[Piotr Zalewa [:zalun]]

In review
https://github.com/zalun/FlightDeck/commit/c14471a2b71855280e384cb63a60487c4f4dfcb5

Comment 5

[Piotr Zalewa [:zalun]]

It happens when the Package item has no ``latest`` or ``version``.
On Package creation both of these attributes are linked with the first PackageRevision.

Code now leaves a critical log Package %s by %s has no latest or version revision

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Although https://builder-addons.allizom.org/addon/1004872/latest/ is a 404, I ran Netsparker against trunk/dev, and didn't get any 500s.

Verified FIXED.

Comment 7

[Zac C (:zac)]

Re-opening:

This traceback can be replicated by double-clicking the copy button from inside an add-on or library editor page.

Perhaps this UI bug was the source of the original corrupt data.

Comment 8

[Piotr Zalewa [:zalun]]

I can't replicate it on the localhost dev box.
Let's try that:
https://github.com/mozilla/FlightDeck/pull/85

Comment 9

[Piotr Zalewa [:zalun]]

The double-click raises a following exception:

```
File "/data/www/builder-addons-dev.allizom.org/flightdeck/apps/jetpack/models.py", line 1697, in copy
  new_p.latest.origin = self.latest

AttributeError: 'NoneType' object has no attribute 'origin'
```

Although transaction decorator should roll back the changes in copy view, it would be good to prevent this from happening. 

new_p = Package( ... )
new_p.save()

shouldn't result with new_p == None

I've added a bit of logging:

It's unclear to me. After doubleclick following lines do show a log:

```
if new_p:
    log.debug('[copy: %s] Package saved (%s)' % (self.pk, new_p.
else:
    log.critical('[copy: %s] Package save(?) error' % self.pk)
```

```
Dec 14 04:24:04 f.jetpack:DEBUG [copy: 18368] Package saved (None) :/data/www/builder-addons-dev.allizom.org/flightdeck/apps/jetpack/models.py:1699
[...]
Dec 14 04:24:05 f.jetpack:DEBUG [copy: 18368] Package saved (29542) :/data/www/builder-addons-dev.allizom.org/flightdeck/apps/jetpack/models.py:1699

```

And later certainly already known ``AttributeError: 'NoneType' object has no attribute 'origin'``

Comment 10

[Piotr Zalewa [:zalun]]

That's debugging I've added: https://github.com/mozilla/FlightDeck/commit/00170985011e414e54c55a33679a6d81dc37d005

Comment 11

[Piotr Zalewa [:zalun]]

As ``new_p.pk == None``and ``new_p.latest`` is None it indicates that the save method failed

Comment 12

[Piotr Zalewa [:zalun]]

Back-end is not 100% fixed yet - dblclick can create 2 add-ons with the same name (run within transaction)
We prevented dblclick in the JavaScript

Comment 13

[Sean McArthur [:seanmonstar]]

Best solution would be to add a unique constraint: UNIQUE KEY (name, author_id). This should stop the race conditions we're getting.

However, we might have an issue trying to add a unique constraint since there are already some addon's that have managed to break this uniqueness (unintentionally). How do we determine which dupes to delete beforehand?