Adjust how shipping methods are handled in the repository class

The old Repository class wasn't thread safe as the method instances were
created at compile time.  Since the instance is configured by setting
the basket for each request, this poses thread safety issues as two
requests could share the same method instance (one of which would have
the wrong basket assigned).

This change fiddles with how the methods are handled, ensuring that they
are instanciated within the instance methods so that thread-safety
issues go away.

Shipping methods need a gentle rewrite for v0.7 to ensure they aren't
vulnerable to such issues.

docs/source/releases/v0.6.rst

@@ -239,7 +239,38 @@ Several classes and functions have had signature changes:
   :class:`oscar.apps.catalogue.reviews.forms.ProductReviewForm``.
 
 
-Address model changes
+Loading shipping methods
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+The default implementation of the shipping method repository has been adjusted
+to avoid thread-safety issues.  If you define your own shipping ``Repository``
+class, ensure that your shipping methods are instantiated per-request and not
+at compile time.
+
+For example, avoid this:
+
+.. code-block:: python
+
+   from oscar.apps.shipping import repository
+
+   class Repository(repository.Repository)
+       methods = [SomeMethod(), AnotherMethod()]
+
+Instead, instantiate the methods within ``get_shipping_methods``:
+
+.. code-block:: python
+
+   from oscar.apps.shipping import repository
+
+   class Repository(repository.Repository)
+       # Note, methods are not instantiated
+       methods = [SomeMethod, AnotherMethod]
+
+Beware of shipping methods that are configured via constructor arguments, like 
+:class:`~oscar.apps.shipping.methods.FixedPrice`.  Shipping methods will be
+reworked for Oscar 0.7.
+
+Address Model changes
 ~~~~~~~~~~~~~~~~~~~~~
 
 * The ``UserAddress.salutation`` and ``UserAddress.name`` methods are now

oscar/apps/shipping/repository.py

@@ -11,7 +11,9 @@ class Repository(object):
     Repository class responsible for returning ShippingMethod
     objects for a given user, basket etc
     """
-    methods = (methods.Free(),)
+    # Note, don't instantiate your shipping methods here (at class-level) as
+    # that isn't thread safe.
+    methods = (methods.Free,)
 
     def get_shipping_methods(self, user, basket, shipping_addr=None,
                              request=None, **kwargs):
@@ -23,7 +25,10 @@ class Repository(object):
         but this behaviour can easily be overridden by subclassing this class
         and overriding this method.
         """
-        return self.prime_methods(basket, self.methods)
+        # We need to instantiate each method class to avoid thread-safety
+        # issues
+        methods = [klass() for klass in self.methods]
+        return self.prime_methods(basket, methods)
 
     def get_default_shipping_method(self, user, basket, shipping_addr=None,
                                     request=None, **kwargs):
@@ -69,8 +74,9 @@ class Repository(object):
         """
         Return the appropriate Method object for the given code
         """
-        for method in self.methods:
-            if method.code == code:
+        for method_class in self.methods:
+            if method_class.code == code:
+                method = method_class()
                 return self.prime_method(basket, method)
 
         # Check for NoShippingRequired as that is a special case

tests/integration/order/creator_tests.py

@@ -8,7 +8,7 @@ from oscar.apps.catalogue.models import ProductClass, Product
 from oscar.apps.offer.utils import Applicator
 from oscar.apps.order.models import Order
 from oscar.apps.order.utils import OrderCreator
-from oscar.apps.shipping.methods import FixedPrice, Free
+from oscar.apps.shipping.methods import FixedPrice, Free, Base
 from oscar.apps.shipping.repository import Repository
 from oscar.core.loading import get_class
 from oscar.test import factories
@@ -138,9 +138,15 @@ class TestPlacingOrderForDigitalGoods(TestCase):
         self.assertTrue(stockrecord.num_allocated is None)
 
 
+class Fixed(Base):
+    code = 'test'
+    charge_incl_tax = charge_excl_tax = D('5.00')
+    is_tax_known = True
+
+
 class StubRepository(Repository):
     """ Custom shipping methods """
-    methods = (FixedPrice(D('5.00'), D('5.00')), Free())
+    methods = (Fixed, Free)
 
 
 class TestShippingOfferForOrder(TestCase):
@@ -152,7 +158,7 @@ class TestShippingOfferForOrder(TestCase):
     def apply_20percent_shipping_offer(self):
         """Shipping offer 20% off"""
         range = Range.objects.create(name="All products range",
-                                    includes_all_products=True)
+                                     includes_all_products=True)
         benefit = Benefit.objects.create(
             range=range, type=Benefit.SHIPPING_PERCENTAGE, value=20)
         offer = factories.create_offer(range=range, benefit=benefit)
@@ -163,7 +169,7 @@ class TestShippingOfferForOrder(TestCase):
         self.apply_20percent_shipping_offer()
 
         # Normal shipping 5.00
-        shipping = StubRepository().find_by_code(FixedPrice.code, self.basket)
+        shipping = StubRepository().find_by_code('test', self.basket)
 
         place_order(self.creator,
                     basket=self.basket,