dj-stripe: _get_or_create_from_stripe_object is not atomic

Code:

    @classmethod
    def _get_or_create_from_stripe_object(cls, data, field_name="id", refetch=True, save=True):
        field = data.get(field_name)

        if isinstance(field, six.string_types):
            # A field like {"subscription": "sub_6lsC8pt7IcFpjA", ...}
            stripe_id = field
        elif field:
            # A field like {"subscription": {"id": sub_6lsC8pt7IcFpjA", ...}}
            data = field
            stripe_id = field.get("id")
        else:
            # An empty field - We need to return nothing here because there is
            # no way of knowing what needs to be fetched!
            return None, False

        try:
            return cls.stripe_objects.get(stripe_id=stripe_id), False
        except cls.DoesNotExist:
            if refetch and field_name != "id":
                cls_instance = cls(stripe_id=stripe_id)
                data = cls_instance.api_retrieve()

        return cls._create_from_stripe_object(data, save=save), True

Doing a try/except is a source of race conditions, which are actually very likely to happen at high loads with webhooks turned on.

The proper way to do this would be with a get_or_create with a defaults keyword argument like I do in this commit.

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 1
Comments: 27 (23 by maintainers)

Commits related to this issue

Make get_or_create behave more like django's versions. Ref #429. — committed to kandoio/dj-stripe by kbrownlees 7 years ago
Add comments to explain _get_or_create_from_stripe_object logic See #429, #903 — committed to dj-stripe/dj-stripe by therefromhere 5 years ago

Most upvoted comments

@lskillen @kavdev @kbrownlees I had some time to look at this issue more closely. I think this solution is good enough and has full integrity coverage after all.

Simplified patch:

diff --git a/djstripe/models.py b/djstripe/models.py
index 86a8fa12..22f4cf26 100644
--- a/djstripe/models.py
+++ b/djstripe/models.py
@@ -302,7 +302,7 @@ class StripeObject(models.Model):
         instance._attach_objects_hook(cls, data)
 
         if save:
-            instance.save()
+            instance.save(force_insert=True)
 
         instance._attach_objects_post_save_hook(cls, data)
 
@@ -347,7 +347,10 @@ class StripeObject(models.Model):
         # If this happens when syncing Stripe data, it's a djstripe bug. Report it!
         assert not should_expand, "No data to create {} from {}".format(cls.__name__, field_name)
 
-        return cls._create_from_stripe_object(data, save=save), True
+        try:
+            return cls._create_from_stripe_object(data, save=save), True
+        except IntegrityError:
+            return cls.stripe_objects.get(stripe_id=stripe_id), False
 
     @classmethod
     def _stripe_object_to_customer(cls, target_cls, data):

I’m going to test it in production now.

jleclanche on Dec 5, 2017

mock_django is gone.

jleclanche on May 30, 2017

If we can get rid of the mock_django dependency that’d be great… it’s only used for UpcomingInvoice, nasty stuff.

jleclanche on Mar 13, 2017