From 7974dd69a37a9532a00e91df37bccbbdf84100f4 Mon Sep 17 00:00:00 2001 From: Alexander Dusenbery Date: Tue, 18 Feb 2025 11:01:11 -0500 Subject: [PATCH] fix: properly avoid race condition when provisioning licenses ENT-10064 --- license_manager/apps/subscriptions/admin.py | 17 +- .../apps/subscriptions/tests/test_admin.py | 146 +++++++++--------- 2 files changed, 84 insertions(+), 79 deletions(-) diff --git a/license_manager/apps/subscriptions/admin.py b/license_manager/apps/subscriptions/admin.py index a47365ca..06736a35 100644 --- a/license_manager/apps/subscriptions/admin.py +++ b/license_manager/apps/subscriptions/admin.py @@ -391,16 +391,17 @@ def save_model(self, request, obj, form, change): if not change: obj.desired_num_licenses = form.cleaned_data.get('num_licenses', 0) - # If the desired number of licenses is large enough, we trigger an async celery task - # after the creation of this record. We wrap that creation in a transaction here - # to force a commit, so that the async task does not encounter a race condition - # where the plan it expects to read from the DB does not yet exist. - with transaction.atomic(): - super().save_model(request, obj, form, change) + super().save_model(request, obj, form, change) - # Finally, if we're creating the model instance, go ahead and create the related license records. + # ``not change`` implies that we're creating the model instance, + # so go ahead and create the related license records. if not change: - obj.provision_licenses() + # If the desired number of licenses is large enough, ``provision_licenses()` will + # submit an async celery task after this record is saved. + # We defer submitting the task until a successful commit + # occurs, so that the async task does not encounter a race condition + # where the plan it expects to read from the DB does not yet exist. + transaction.on_commit(obj.provision_licenses) @admin.register(CustomSubscriptionExpirationMessaging) diff --git a/license_manager/apps/subscriptions/tests/test_admin.py b/license_manager/apps/subscriptions/tests/test_admin.py index 019db949..681c689f 100644 --- a/license_manager/apps/subscriptions/tests/test_admin.py +++ b/license_manager/apps/subscriptions/tests/test_admin.py @@ -4,7 +4,7 @@ import pytest from django.contrib import messages from django.contrib.admin.sites import AdminSite -from django.test import RequestFactory +from django.test import RequestFactory, TestCase from license_manager.apps.subscriptions.admin import ( CustomerAgreementAdmin, @@ -29,78 +29,82 @@ from ..constants import REVOKED -@pytest.mark.django_db -def test_licenses_subscription_creation(): +class SubscriptionCreationTests(TestCase): """ - Verify that creating a SubscriptionPlan creates its associated Licenses after it is created. + Tests cases for creating new plans/licenses. """ - subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite()) - request = RequestFactory() - request.user = UserFactory() - num_licenses = 5 - form = make_bound_subscription_form(num_licenses=num_licenses) - obj = form.save() # Get the object returned from saving the form to save to the database - assert obj.licenses.count() == 0 # Verify no Licenses have been created yet - change = False - subscription_admin.save_model(request, obj, form, change) - assert obj.licenses.count() == num_licenses - - -@pytest.mark.django_db -@mock.patch('license_manager.apps.subscriptions.admin.messages.add_message') -def test_subscription_licenses_create_action(mock_add_message): - """ - Verify that running the create licenses action will create licenses. - """ - # Setup an existing plan - customer_agreement = CustomerAgreementFactory() - subscription_plan = SubscriptionPlanFactory.create( - customer_agreement=customer_agreement, - desired_num_licenses=10, - ) - assert subscription_plan.licenses.count() == 0 # Verify no Licenses have been created yet - - # setup the admin form - subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite()) - request = RequestFactory() - request.user = UserFactory() - - # doesn't really matter what we put for num_licenses in here, save_model - # will read the desired number of license from the existing object on save. - form = make_bound_subscription_form(num_licenses=10) - form.save() - - # save the form as a modify instead of create - subscription_admin.save_model(request, subscription_plan, form, True) - subscription_plan.refresh_from_db() - # Desired number of licenses won't actually be created until we run the action - assert subscription_plan.licenses.count() == 0 - - # Now run the action... - subscription_admin.create_actual_licenses_action( - request, - SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid), - ) - subscription_plan.refresh_from_db() - # Actual number of licenses should now equal the desired number (ten) - assert subscription_plan.licenses.count() == 10 - - mock_add_message.assert_called_once_with( - request, messages.SUCCESS, 'Successfully created license records for selected Subscription Plans.', - ) - - # check that freezing the plan means running the create licenses action has no effect - subscription_plan.last_freeze_timestamp = localized_utcnow() - subscription_plan.desired_num_licenses = 5000 - subscription_plan.save() - - subscription_admin.create_actual_licenses_action( - request, - SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid), - ) - subscription_plan.refresh_from_db() - # Actual number of licenses should STILL equal 10, because the plan is frozen - assert subscription_plan.licenses.count() == 10 + def test_licenses_subscription_creation(self): + """ + Verify that creating a SubscriptionPlan creates its associated Licenses after it is created. + """ + subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite()) + request = RequestFactory() + request.user = UserFactory() + num_licenses = 5 + form = make_bound_subscription_form(num_licenses=num_licenses) + obj = form.save() # Get the object returned from saving the form to save to the database + assert obj.licenses.count() == 0 # Verify no Licenses have been created yet + change = False + + with self.captureOnCommitCallbacks(execute=True): + subscription_admin.save_model(request, obj, form, change) + + assert obj.licenses.count() == num_licenses + + @mock.patch('license_manager.apps.subscriptions.admin.messages.add_message') + def test_subscription_licenses_create_action(self, mock_add_message): + """ + Verify that running the create licenses action will create licenses. + """ + # Setup an existing plan + customer_agreement = CustomerAgreementFactory() + subscription_plan = SubscriptionPlanFactory.create( + customer_agreement=customer_agreement, + desired_num_licenses=10, + ) + assert subscription_plan.licenses.count() == 0 # Verify no Licenses have been created yet + + # setup the admin form + subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite()) + request = RequestFactory() + request.user = UserFactory() + + # doesn't really matter what we put for num_licenses in here, save_model + # will read the desired number of license from the existing object on save. + form = make_bound_subscription_form(num_licenses=10) + form.save() + + # save the form as a modify instead of create + subscription_admin.save_model(request, subscription_plan, form, True) + subscription_plan.refresh_from_db() + # Desired number of licenses won't actually be created until we run the action + assert subscription_plan.licenses.count() == 0 + + # Now run the action... + subscription_admin.create_actual_licenses_action( + request, + SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid), + ) + subscription_plan.refresh_from_db() + # Actual number of licenses should now equal the desired number (ten) + assert subscription_plan.licenses.count() == 10 + + mock_add_message.assert_called_once_with( + request, messages.SUCCESS, 'Successfully created license records for selected Subscription Plans.', + ) + + # check that freezing the plan means running the create licenses action has no effect + subscription_plan.last_freeze_timestamp = localized_utcnow() + subscription_plan.desired_num_licenses = 5000 + subscription_plan.save() + + subscription_admin.create_actual_licenses_action( + request, + SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid), + ) + subscription_plan.refresh_from_db() + # Actual number of licenses should STILL equal 10, because the plan is frozen + assert subscription_plan.licenses.count() == 10 @pytest.mark.django_db