Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce repeated calls to django cache while calculating utm_source for listing endpoints #4462

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions course_discovery/apps/api/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from django.conf import settings
from django.core.cache import cache
from django.http.response import HttpResponse
from edx_django_utils.cache import get_cache_key
from rest_framework.renderers import JSONRenderer
from rest_framework_extensions.cache.decorators import CacheResponse
from rest_framework_extensions.key_constructor.bits import KeyBitBase, QueryParamsKeyBit
Expand Down Expand Up @@ -165,3 +166,7 @@ def list(self, request, *args, **kwargs):
)
def retrieve(self, request, *args, **kwargs):
return super().retrieve(request, *args, **kwargs)


def get_utm_source_request_cache_key(partner, user):
return get_cache_key(partner=partner.id, user=user.id)
4 changes: 3 additions & 1 deletion course_discovery/apps/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@
from taxonomy.choices import ProductTypes
from taxonomy.utils import get_whitelisted_serialized_skills

from course_discovery.apps.api.cache import get_utm_source_request_cache_key
from course_discovery.apps.api.fields import (
HtmlField, ImageField, SlugRelatedFieldWithReadSerializer, SlugRelatedTranslatableField, StdImageSerializerField
)
from course_discovery.apps.api.utils import StudioAPI, get_excluded_restriction_types
from course_discovery.apps.api.utils import StudioAPI, get_excluded_restriction_types, use_request_cache
from course_discovery.apps.catalogs.models import Catalog
from course_discovery.apps.core.api_client.lms import LMSAPIClient
from course_discovery.apps.core.utils import update_instance
Expand Down Expand Up @@ -147,6 +148,7 @@ def get_lms_course_url_for_archived(partner, course_key):
return f'{lms_url}/courses/{course_key}/course/'


@use_request_cache("utm_source_cache", get_utm_source_request_cache_key)
def get_utm_source_for_user(partner, user):
"""
Return the utm source for the user.
Expand Down
18 changes: 18 additions & 0 deletions course_discovery/apps/api/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from django.core.files.base import ContentFile
from django.db.models.fields.related import ManyToManyField
from django.utils.translation import gettext as _
from edx_django_utils.cache import RequestCache
from opaque_keys.edx.keys import CourseKey
from requests.exceptions import HTTPError
from sortedm2m.fields import SortedManyToManyField
Expand Down Expand Up @@ -352,3 +353,20 @@ def push_to_studio(self, course_run, create=False, old_course_run_key=None, user
self.create_course_run_in_studio(course_run, user=user)
else:
self.update_course_run_details_in_studio(course_run)


def use_request_cache(cache_name, key_func):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A docstring explaining it's usage would be nice.

def inner(fn):
@functools.wraps(fn)
def wrapped(*args, **kwargs):
cache = RequestCache(cache_name)
cache_key = key_func(*args, **kwargs)
cached_response = cache.get_cached_response(cache_key)
if cached_response.is_found:
return cached_response.value

ret_val = fn(*args, **kwargs)
cache.set(cache_key, ret_val)
return ret_val
Comment on lines +359 to +370
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming and readability can be improved like:
Rename inner to decorator to make it clear that this is a decorator function.
Rename wrapped to wrapper to follow common convention in Python decorator patterns.
Rename ret_val to something like result

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of lines here n there to improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious] Was the first of these comments generated through some AI tool?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this myself although I got the naming conventions from GPT :D

return wrapped
return inner
21 changes: 21 additions & 0 deletions course_discovery/apps/api/v1/tests/test_views/test_courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from edx_toggles.toggles.testutils import override_waffle_switch
from rest_framework.reverse import reverse
from testfixtures import LogCapture
from waffle.testutils import override_switch

from course_discovery.apps.api.v1.exceptions import EditableAndQUnsupported
from course_discovery.apps.api.v1.tests.test_views.mixins import APITestCase, OAuth2Mixin, SerializationMixin
Expand Down Expand Up @@ -352,6 +353,26 @@ def test_list(self):
self.serialize_course(Course.objects.all(), many=True)
)

def test_no_repeated_cache_calls_for_utm_calculation(self):
"""
Test that utm source calculation is done only once per request, and not per
serialized object on a listing endpoint. Since each utm source calculation
requires two cache calls, this reduces the number of calls to the django cache.
"""
CourseFactory(
partner=self.partner, title='Fake Test 1', key='edX+Fake102', type=self.audit_type
)
self.partner.lms_url = "http://localhost:8000"
self.partner.save()
with mock.patch('course_discovery.apps.core.api_client.lms.LMSAPIClient.get_api_access_request', return_value={
"company_name": "Example Inc",
"company_address": "Example Address",
}) as mocked_api_access_request:
with override_switch('use_company_name_as_utm_source_value', True):
url = reverse('api:v1:course-list')
self.client.get(url)
mocked_api_access_request.assert_called_once()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we not be checking utm call's count here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_utm_source_for_user function will still be called as many times as it is being currently called. (In this case twice). However, the stuff inside of it (which includes get_api_access_request) will only be called once. And any subsequent invocations will fetch the result from the RequestCache


@pytest.mark.skip(reason="https://github.com/openedx/course-discovery/issues/4431")
@responses.activate
def test_list_query(self):
Expand Down
Loading