Dropped coverage on Django 1.11

[jleclanche]

Tracking issue for Django 1.11 dropped coverage.

On Django 1.11, coverage has dropped in one very specific place: Card.remove():

    def remove(self):
        """Removes a card from this customer's account."""

        try:
            self._api_delete()
        except InvalidRequestError as exc:
            if "No such source:" in str(exc) or "No such customer:" in str(exc):
                # The exception was thrown because the stripe customer or card was already
                # deleted on the stripe side, ignore the exception
                pass
            else:
                # The exception was raised for another reason, re-raise it
                six.reraise(*sys.exc_info())

        try:
            self.delete()
        except StripeCard.DoesNotExist:
            # The card has already been deleted (potentially during the API call)
            pass

In the following method, the final except is never raised. Now, this is supposedly harmless, but we want to understand why before we greenlight Django 1.11.

This try/except was added by yours truly in commit 942ced2. That commit added the following test which forces the DoesNotExist to trigger:

    @patch("stripe.Customer.retrieve", return_value=deepcopy(FAKE_CUSTOMER))
    def test_remove_already_deleted_card(self, customer_retrieve_mock):
        stripe_card = Card._api_create(customer=self.customer, source=FAKE_CARD["id"])
        Card.sync_from_stripe_data(stripe_card)

        self.assertEqual(self.customer.sources.count(), 1)
        card_object = self.customer.sources.first()
        Card.objects.filter(stripe_id=stripe_card["id"]).delete()
        self.assertEqual(self.customer.sources.count(), 0)
        card_object.remove()
        self.assertEqual(self.customer.sources.count(), 0)

This test can be reduced to the following, which fails on Django 1.10 and passes on Django 1.11:

    def test_django111_behaviour(self):
        args = {
            "stripe_id": "test_django111",
            "customer_id": Customer.objects.first().id,
            "exp_month": 0,
            "exp_year": 0,
        }
        card = Card.objects.create(**args)
        card2 = Card.objects.get(stripe_id=args["stripe_id"])
        card.delete()
        card2.delete()

And here is the traceback:

Traceback (most recent call last):
  File "/home/adys/src/git/hsreplaynet/dj-stripe/tests/test_card.py", line 122, in test_django111_behaviour
    card2.delete()
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/django/db/models/base.py", line 957, in delete
    collector.collect([self], keep_parents=keep_parents)
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/django/db/models/deletion.py", line 207, in collect
    parent_objs = [getattr(obj, ptr.name) for obj in new_objs]
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/django/db/models/deletion.py", line 207, in <listcomp>
    parent_objs = [getattr(obj, ptr.name) for obj in new_objs]
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/polymorphic/models.py", line 161, in accessor_function
    attr = model.base_objects.get(pk=self.pk)
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/adys/src/git/hsreplaynet/dj-stripe/.tox/py36-django110-bjson/lib/python3.6/site-packages/django/db/models/query.py", line 385, in get
    self.model._meta.object_name
djstripe.stripe_objects.DoesNotExist: StripeSource matching query does not exist.

Now, it's worth noting that raising DoesNotExist on a delete() is not normal behaviour. Easy enough to test with a random django model:

>>> Foo.objects.create()
<Foo: e15c448b-4be4-40f7-a11a-bf82988dfa43>
>>> a = _
>>> b = Foo.objects.get(id=a.id)
>>> a.delete()
(1, {'foo.Foo': 1})
>>> b.delete()
(0, {'foo.Foo': 0})

Indeed we see in the traceback that the DoesNotExist is raised not in the delete itself, but in the collector that gathers related models before performing the delete.

The Model.delete method in Django has not changed since 1.10, and for that matter neither has the Collector.delete method. The error happens in that last method in the list comprehension parent_objs = [getattr(obj, ptr.name) for obj in new_objs]. This accesses the name attribute, which is created as a property on lines 161 and 179 of polymorphic/models.py.

This jumps into django/db/models/manager.py which hasn't changed since 1.10 either. And finally ends in the get() method of query.py which... hasn't changed either.

Some debugging lets me see the values of self, clone, args and kwargs. Django 1.10:

<QuerySet [<Card: <brand=, last4=, exp_month=0, exp_year=0, stripe_id=test_django111>>]> <QuerySet [<Card: <brand=, last4=, exp_month=0, exp_year=0, stripe_id=test_django111>>]> () {'stripe_id': 'test_django111'}
<QuerySet [<StripeSource: <stripe_id=test_django111>>]> <QuerySet [<StripeSource: <stripe_id=test_django111>>]> () {'pk': 1750}
<QuerySet []> <QuerySet []> () {'pk': 1750}

Django 1.11:

<PolymorphicQuerySet [<Card: <brand=, last4=, exp_month=0, exp_year=0, stripe_id=test_django111>>]> <PolymorphicQuerySet [<Card: <brand=, last4=, exp_month=0, exp_year=0, stripe_id=test_django111>>]> () {'stripe_id': 'test_django111'}

Okay, so on Django 1.10 it's called three times, and once it gets to the last call it errors out. On Django 1.11 it's only called once, because self is not a QuerySet but a PolymorphicQuerySet.

Digging further, this confusion happens in PolymorphicManager.get_queryset(). Both in Django 1.10 and Django 1.11, self.queryset_class is PolymorphicQuerySet, and yet, the following line returns different things:

qs = self.queryset_class(self.model, using=self._db, hints=self._hints)

# Django 1.10 example:
self=<polymorphic.managers.PolymorphicManager object at 0x7f00ca11fe48>, <class 'polymorphic.query.PolymorphicQuerySet'>(self.model=<class 'djstripe.models.Card'>, using=self._db=None, hints=self._hints={})
-> <QuerySet []>

# Django 1.11 example:
self=<polymorphic.managers.PolymorphicManager object at 0x7fe85f955c88>, <class 'polymorphic.query.PolymorphicQuerySet'>(self.model=<class 'djstripe.models.Card'>, using=self._db=None, hints=self._hints={})
-> <PolymorphicQuerySet []>

Phew, okay, so we can reduce the failure to the following:

from polymorphic.managers import PolymorphicQuerySet
p = PolymorphicQuerySet(Card, using=None, hints={})
print(p)

# Django 1.10: <QuerySet []>
# Django 1.11: <PolymorphicQuerySet []>

Well, this is where I'm stumped. None of django's manager.py changed in 1.10->1.11. And yet, this changed. Any ideas?

[jleclanche]

cc @vdboor

[lskillen]

Is the last bit a red herring, perhaps? It looks like they used to hardcode the class names but this was fixed in the following commit: django/django@48826aa#diff-5b0dda5eb9a242c15879dc9cd2121379 ... which is no help at all, I know. But if true, at least we know it's not because the QuerySet class is different! This is a frustrating one.

[twidi]

Do you use v1.2 of polymorphic that officially supports django 1.11?

[twidi]

It seems that they adapted the code following a change in django 1.11, related to the queryset iteration: jazzband/django-polymorphic@dbad7bd

(not sure if really related, though! I was looking for important changes in this release)

[lskillen]

@jleclanche: Looking at this from a different angle - In the Card.remove() code, we're saying that we expect that delete() might throw a StripeCard.DoesNotExist exception, which it does in Django 1.10. As pointed out this originates whendjango-polymorphic tries to refetch the object via attr = model.base_objects.get(pk=self.pk) in order to get the name attribute.

It doesn't throw the exception in Django 1.11, which is why we get the code coverage drop.

So let's examine some of the possibilities:

• The refetch never happens because Django 1.11 short-circuited the delete. Either it knows the delete has already been performed, or there's something else about navigating the parent models that has changed.
• The refetch still happens, but it succeeds on Django 1.11. This could be because the object wasn't properly deleted when calling delete via the queryset as Card.objects.filter(stripe_id=stripe_card["id"]).delete(). I did a bit research into this before, and I do recall saying to Jerome that it appeared the object was still there even though delete was called.

The former is good, and we can put this down as innocuous, and maybe remove the try/except because Django 1.11 is superior (etc.). The latter is bad, and we'll need to figure out if it's something that we can fix, or that we can fix upstream within either django-polymorphic or Django itself.

Noticed that @twidi jumped in here to mention the upgrade use the newer style iteration. Yep, noticed that too. It's possible that executing delete() against the result set that comes back from Card.objects.filter(stripe_id=stripe_card["id"]) in the newer iteration code is what is breaking it.

Need to do a couple of things to confirm further:

• Get answers for above regarding whether refetch is happening or not on Django 1.11 vs 1.10.
• Step into Card.objects.filter(stripe_id=stripe_card["id"]).delete() to see if it's actually getting through to the delete() properly, and if not, why not?

[jleclanche]

@lskillen Unless I misunderstand you, it's not a refetch, it's a prefetch of the related objects. In Django 1.11, the prefetch succeeds (into a PolymorphicQuerySet) whereas on Django 1.10, it fails.

I haven't looked at what changed in polymorphic itself because I used the same version for both 1.10 and 1.11 (the latest one, 1.2).

[lskillen]

Confirmed on the red herring at least (this is Django 1.10):

(Pdb) p self
<QuerySet [<Card: <brand=Visa, last4=4242, exp_month=12, exp_year=2016, stripe_id=card_16YKQh2eZvKYlo2Cblc5Feoo>>]>
(Pdb) p self.__class__
<class 'polymorphic.query.PolymorphicQuerySet'>
(Pdb)

As for the refetch, said this on gitter:

By refetch I meant this line: model.base_objects.get(pk=self.pk) - django-polymorphic forces Django to use the ORM to refetch the object when accessing parent class/object attributes.

[jleclanche]

Wee! We figured it out together on Gitter.

Card.stripesource_ptr is, on Django 1.10, a ForwardManyToOneDescriptor. On Django 1.11 however, it is a ForwardOneToOneDescriptor which was added in django/django@38575b0 and is a subclass of the former.

That means that this if type(...) check falls through.

Signs point to the if check having to be replaced with the following:

if issubclass(type(orig_accessor), (ReverseOneToOneDescriptor, ForwardManyToOneDescriptor])):

I will submit a pull request to django-polymorphic.

[lskillen]

@jleclanche and I did a bit of sleuthing this morning, and it's due to ForwardOneToOneDescriptor being added in Django 1.11 in django/django@38575b0 - which breaks the following code that adds attribute wrappers to superclasses within django-polymorphic:

        for name, model in subclasses_and_superclasses_accessors.items():
            orig_accessor = getattr(self.__class__, name, None)
            if type(orig_accessor) in [ReverseOneToOneDescriptor, ForwardManyToOneDescriptor]:
                # print >>sys.stderr, '---------- replacing', name, orig_accessor, '->', model
                setattr(self.__class__, name, property(create_accessor_function_for_model(model, name)))

The proposal is to change the type check to a subclass check instead as ForwardOneToOneDescriptor is derived from ForwardManyToOneDescriptor:

        for name, model in subclasses_and_superclasses_accessors.items():
            orig_accessor = getattr(self.__class__, name, None)
            if issubclass(type(orig_accessor), (ReverseOneToOneDescriptor, ForwardManyToOneDescriptor)):
                # print >>sys.stderr, '---------- replacing', name, orig_accessor, '->', model
                setattr(self.__class__, name, property(create_accessor_function_for_model(model, name)))

What we're not certain about is:

• What behaviour breaks if this is missing in Django 1.11? It presumably affects something outside of our (simplistic) use case of deleting a polymorphic object via PolymorphicQuerySet.
• The original behaviour with Django 1.10 results in a refetch via the ORM, which .. isn't great if it turns out it isn't necessary. The above changes would cause that behaviour to happen again with Django 1.11.

[jleclanche]

PR submitted: jazzband/django-polymorphic#291

[jleclanche]

I'm closing this, it's in the hands of polymorphic now.