Repository: django-oscar/django-oscar

Issue:
Performance issue in cases where selecting strategy for product
Found a bug? Please fill out the sections below.

### Issue Summary

In Oscar 2.1, Selecting a strategy for product in [`UseFirstStockRecord`](https://github.com/django-oscar/django-oscar/blob/2.1.0b0/src/oscar/apps/partner/strategy.py#L188) used to (correctly, for performance reasons) index on the related stock records for a product to get the first. This meant, of course, that the `prefetch_related` call to `stockrecords` on `Product` was used and no additional queries generated.

Now, in Oscar 3.1 [this was changed to use the queryset's `first` method](https://github.com/django-oscar/django-oscar/blob/3.1/src/oscar/apps/partner/strategy.py#L188), which unfortunately makes the prefetch invalid and causes a new query each time a product shows price info (for various reasons, this has a huge performance impact on my use case).

This change was introduced in [this commit](https://github.com/django-oscar/django-oscar/commit/3a826796d01e5cba345aa646dbc5c8ae466cbf4a), which also implemented the same pattern over various related queries, and therefore I assume has similar performance implications (but I haven't checked).

### Steps to Reproduce

Try to render a page with various snippets of pricing info for a collection of hundreds of products - use a tool like Django Debug Toolbar to see the query multiplication :)

### Technical details

* Python version: Run `python --version`: 3.10
* Django version: Look in your requirements.txt, or run `pip show django | grep Version` 3.2.12
* Oscar version: Look in your requirements.txt, or run `pip show django-oscar | grep Version`. 3.1



Discussion:
Interesting, I would have thought that the [0] indexing was done because queryset.first() didn't exist at the time this was written.
could you show us a screenshot of your usecase or add a url? I think it would be good to be able to replicate the scenario.
Hey @specialunderwear 

I don't have a public URL but you can see the docs for `prefetch_related` that this is the expected behaviour (that the prefetch is redundant because we've chained a new clause to the query):

https://docs.djangoproject.com/en/4.0/ref/models/querysets/#prefetch-related

The "note" section there clearly illustrates, using `.filter`, but the principle is the same as using `.first`, and easily observed by looking at the quanity of queries fired if one switches from using `.first` to `[0]`.

So, using `.first()` has negative performance implications with this change in Oscar - it means the optimization of prefetching the stock records is redundant, and that the query performance from that optimization is therefore no longer present).
Hah, this is interesting. I too had just assumed that this was legacy code that was written before `first()` was introduced. I think we need to revert the changes made to this query in commit, but also add docstrings explaining why it's done that way, as it's not immediately obvious. Having had a look at the whole commit through, I think it needs to be only partially reverted - other instances of using this pattern do just seem to be legacy code.
@solarissmoke 

Yes, it is not obvious from the code, and a way to document it better would also be to emphasize where Oscar does `select_related` and `prefetch_related` queries - I assume there will be Oscar users who don't know the details of Django's ORM and won't be aware that any customization they may do to queries will make Oscar's use of `select_related` and `prefetch_related` redundant, impacting performance.

Also see the [docs](https://docs.djangoproject.com/en/4.0/ref/models/querysets/#first) and the [implementation](https://github.com/django/django/blob/main/django/db/models/query.py#L862) for `.first()` - Because of the stuff around ordering, I would be careful about using it in Oscar to be honest - `first()` and `all()[0]` are *not* guaranteed to return the same instance due to how ordering may or may not be implemented.

Fix patch:
diff --git a/src/oscar/apps/partner/strategy.py b/src/oscar/apps/partner/strategy.py
--- a/src/oscar/apps/partner/strategy.py
+++ b/src/oscar/apps/partner/strategy.py
@@ -185,17 +185,19 @@ def parent_availability_policy(self, product, children_stock):
 # Mixins - these can be used to construct the appropriate strategy class
 
 
-class UseFirstStockRecord(object):
+class UseFirstStockRecord:
     """
     Stockrecord selection mixin for use with the ``Structured`` base strategy.
     This mixin picks the first (normally only) stockrecord to fulfil a product.
-
-    This is backwards compatible with Oscar<0.6 where only one stockrecord per
-    product was permitted.
     """
 
     def select_stockrecord(self, product):
-        return product.stockrecords.first()
+        # We deliberately fetch by index here, to ensure that no additional database queries are made
+        # when stockrecords have already been prefetched in a queryset annotated using ProductQuerySet.base_queryset
+        try:
+            return product.stockrecords.all()[0]
+        except IndexError:
+            pass
 
 
 class StockRequired(object):


Test patch:
diff --git a/tests/integration/partner/test_selector_mixin.py b/tests/integration/partner/test_selector_mixin.py
--- a/tests/integration/partner/test_selector_mixin.py
+++ b/tests/integration/partner/test_selector_mixin.py
@@ -1,5 +1,6 @@
 from django.test import TestCase
 
+from oscar.apps.catalogue.models import Product
 from oscar.apps.partner import strategy
 from oscar.test import factories
 
@@ -17,3 +18,12 @@ def test_selects_first_stockrecord_for_product(self):
 
     def test_returns_none_when_no_stock_records(self):
         self.assertIsNone(self.mixin.select_stockrecord(self.product))
+
+    def test_does_not_generate_additional_query_when_passed_product_from_base_queryset(self):
+        product = Product.objects.base_queryset().first()
+        # Regression test for https://github.com/django-oscar/django-oscar/issues/3875
+        # If passed a product from a queryset annotated by base_queryset, then
+        # the selector should not trigger any additional database queries because
+        # it should rely on the prefetched stock records.
+        with self.assertNumQueries(0):
+            self.mixin.select_stockrecord(product)