From a119b723fd8e22ba0b739f9d6ba9f9831c3c26c6 Mon Sep 17 00:00:00 2001 From: Ben McMorran Date: Wed, 8 Jul 2015 15:23:15 -0400 Subject: [PATCH 1/2] TNL-2389 Use discussion id map cache for thread creation and update --- .../django_comment_client/base/tests.py | 4 +- .../django_comment_client/base/views.py | 17 ++----- .../django_comment_client/forum/tests.py | 8 ++-- .../django_comment_client/tests/test_utils.py | 46 ++++++++++++++---- lms/djangoapps/django_comment_client/utils.py | 48 ++++++++++++++----- 5 files changed, 84 insertions(+), 39 deletions(-) diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index aef768e026..409b4b6363 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -379,7 +379,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): ) self.assertEqual(response.status_code, 200) - @patch('django_comment_client.base.views.get_discussion_categories_ids', return_value=["test_commentable"]) + @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"]) def test_update_thread_wrong_commentable_id(self, mock_get_discussion_id_map, mock_request): self._test_request_error( "update_thread", @@ -876,7 +876,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq self.student = UserFactory.create() CourseEnrollmentFactory(user=self.student, course_id=self.course.id) - @patch('django_comment_client.base.views.get_discussion_categories_ids', return_value=["test_commentable"]) + @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"]) @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request, mock_get_discussion_id_map): self._set_mock_request_data(mock_request, { diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index b930908d29..e035b29b45 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -27,8 +27,7 @@ from django_comment_client.utils import ( JsonResponse, prepare_content, get_group_id_for_comments_service, - get_discussion_categories_ids, - get_discussion_id_map, + discussion_category_id_access, get_cached_discussion_id_map, ) from django_comment_client.permissions import check_permissions_by_view, has_permission @@ -82,7 +81,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None): commentable_id = data['commentable_id'] if id_map is None: - id_map = get_cached_discussion_id_map(course, commentable_id, user) + id_map = get_cached_discussion_id_map(course, [commentable_id], user) if commentable_id in id_map: data['category_name'] = id_map[commentable_id]["title"] @@ -209,14 +208,9 @@ def create_thread(request, course_id, commentable_id): event_data = get_thread_created_event_data(thread, follow) data = thread.to_dict() - # Calls to id map are expensive, but we need this more than once. - # Prefetch it. - id_map = get_discussion_id_map(course, request.user) + add_courseware_context([data], course, request.user) - add_courseware_context([data], course, request.user, id_map=id_map) - - track_forum_event(request, THREAD_CREATED_EVENT_NAME, - course, thread, event_data, id_map=id_map) + track_forum_event(request, THREAD_CREATED_EVENT_NAME, course, thread, event_data) if request.is_ajax(): return ajax_content_response(request, course_key, data) @@ -246,8 +240,7 @@ def update_thread(request, course_id, thread_id): thread.thread_type = request.POST["thread_type"] if "commentable_id" in request.POST: course = get_course_with_access(request.user, 'load', course_key) - commentable_ids = get_discussion_categories_ids(course, request.user) - if request.POST.get("commentable_id") in commentable_ids: + if discussion_category_id_access(course, request.user, request.POST.get("commentable_id")): thread.commentable_id = request.POST["commentable_id"] else: return JsonError(_("Topic doesn't exist")) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index e9d0ff75b4..d4d3654c8f 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -317,11 +317,11 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): @ddt.data( # old mongo with cache - (ModuleStoreEnum.Type.mongo, 1, 7, 5, 12, 7), - (ModuleStoreEnum.Type.mongo, 50, 7, 5, 12, 7), + (ModuleStoreEnum.Type.mongo, 1, 7, 5, 13, 8), + (ModuleStoreEnum.Type.mongo, 50, 7, 5, 13, 8), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, 1, 3, 3, 12, 7), - (ModuleStoreEnum.Type.split, 50, 3, 3, 12, 7), + (ModuleStoreEnum.Type.split, 1, 3, 3, 13, 8), + (ModuleStoreEnum.Type.split, 50, 3, 3, 13, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index cabcb83c4d..83a45bfb6c 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -170,6 +170,13 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase): discussion_category='Chapter', discussion_target='Discussion 1' ) + self.discussion2 = ItemFactory.create( + parent_location=self.course.location, + category='discussion', + discussion_id='test_discussion_id_2', + discussion_category='Chapter 2', + discussion_target='Discussion 2' + ) self.private_discussion = ItemFactory.create( parent_location=self.course.location, category='discussion', @@ -212,11 +219,18 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase): self.assertFalse(utils.has_required_keys(self.bad_discussion)) def verify_discussion_metadata(self): - """Retrieves the metadata for self.discussion and verifies that it is correct""" - metadata = utils.get_cached_discussion_id_map(self.course, 'test_discussion_id', self.user) - metadata = metadata[self.discussion.discussion_id] - self.assertEqual(metadata['location'], self.discussion.location) - self.assertEqual(metadata['title'], 'Chapter / Discussion 1') + """Retrieves the metadata for self.discussion and self.discussion2 and verifies that it is correct""" + metadata = utils.get_cached_discussion_id_map( + self.course, + ['test_discussion_id', 'test_discussion_id_2'], + self.user + ) + discussion1 = metadata[self.discussion.discussion_id] + discussion2 = metadata[self.discussion2.discussion_id] + self.assertEqual(discussion1['location'], self.discussion.location) + self.assertEqual(discussion1['title'], 'Chapter / Discussion 1') + self.assertEqual(discussion2['location'], self.discussion2.location) + self.assertEqual(discussion2['title'], 'Chapter 2 / Discussion 2') def test_get_discussion_id_map_from_cache(self): self.verify_discussion_metadata() @@ -226,22 +240,36 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase): self.verify_discussion_metadata() def test_get_missing_discussion_id_map_from_cache(self): - metadata = utils.get_cached_discussion_id_map(self.course, 'bogus_id', self.user) + metadata = utils.get_cached_discussion_id_map(self.course, ['bogus_id'], self.user) self.assertEqual(metadata, {}) def test_get_discussion_id_map_from_cache_without_access(self): user = UserFactory.create() - metadata = utils.get_cached_discussion_id_map(self.course, 'private_discussion_id', self.user) + metadata = utils.get_cached_discussion_id_map(self.course, ['private_discussion_id'], self.user) self.assertEqual(metadata['private_discussion_id']['title'], 'Chapter 3 / Beta Testing') - metadata = utils.get_cached_discussion_id_map(self.course, 'private_discussion_id', user) + metadata = utils.get_cached_discussion_id_map(self.course, ['private_discussion_id'], user) self.assertEqual(metadata, {}) def test_get_bad_discussion_id(self): - metadata = utils.get_cached_discussion_id_map(self.course, 'bad_discussion_id', self.user) + metadata = utils.get_cached_discussion_id_map(self.course, ['bad_discussion_id'], self.user) self.assertEqual(metadata, {}) + def test_discussion_id_accessible(self): + self.assertTrue(utils.discussion_category_id_access(self.course, self.user, 'test_discussion_id')) + + def test_bad_discussion_id_not_accessible(self): + self.assertFalse(utils.discussion_category_id_access(self.course, self.user, 'bad_discussion_id')) + + def test_missing_discussion_id_not_accessible(self): + self.assertFalse(utils.discussion_category_id_access(self.course, self.user, 'bogus_id')) + + def test_discussion_id_not_accessible_without_access(self): + user = UserFactory.create() + self.assertTrue(utils.discussion_category_id_access(self.course, self.user, 'private_discussion_id')) + self.assertFalse(utils.discussion_category_id_access(self.course, user, 'private_discussion_id')) + class CategoryMapTestMixin(object): """ diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 3bfce30039..43e18bf517 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -118,19 +118,22 @@ def get_cached_discussion_key(course, discussion_id): raise DiscussionIdMapIsNotCached() -def get_cached_discussion_id_map(course, discussion_id, user): +def get_cached_discussion_id_map(course, discussion_ids, user): """ - Returns a dict mapping discussion_id to discussion module metadata if it is cached and visible to the user. - If not, returns the result of get_discussion_id_map + Returns a dict mapping discussion_ids to respective discussion module metadata if it is cached and visible to the + user. If not, returns the result of get_discussion_id_map """ try: - key = get_cached_discussion_key(course, discussion_id) - if not key: - return {} - module = modulestore().get_item(key) - if not (has_required_keys(module) and has_access(user, 'load', module, course.id)): - return {} - return dict([get_discussion_id_map_entry(module)]) + entries = [] + for discussion_id in discussion_ids: + key = get_cached_discussion_key(course, discussion_id) + if not key: + continue + module = modulestore().get_item(key) + if not (has_required_keys(module) and has_access(user, 'load', module, course.id)): + continue + entries.append(get_discussion_id_map_entry(module)) + return dict(entries) except DiscussionIdMapIsNotCached: return get_discussion_id_map(course, user) @@ -324,6 +327,23 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude return _filter_unstarted_categories(category_map) if exclude_unstarted else category_map +def discussion_category_id_access(course, user, discussion_id): + """ + Returns True iff the given discussion_id is accessible for user in course. Uses the discussion id cache if available + falling back to get_discussion_categories_ids if there is no cache. + """ + if discussion_id in course.top_level_discussion_topic_ids: + return True + try: + key = get_cached_discussion_key(course, discussion_id) + if not key: + return False + module = modulestore().get_item(key) + return has_required_keys(module) and has_access(user, 'load', module, course.id) + except DiscussionIdMapIsNotCached: + return discussion_id in get_discussion_categories_ids(course, user) + + def get_discussion_categories_ids(course, user, include_all=False): """ Returns a list of available ids of categories for the course that @@ -490,10 +510,14 @@ def extend_content(content): def add_courseware_context(content_list, course, user, id_map=None): """ - Decorates `content_list` with courseware metadata. + Decorates `content_list` with courseware metadata using the discussion id map cache if available. """ if id_map is None: - id_map = get_discussion_id_map(course, user) + id_map = get_cached_discussion_id_map( + course, + [content['commentable_id'] for content in content_list], + user + ) for content in content_list: commentable_id = content['commentable_id'] From d36eb83a03414920e847b8ab5eb66a8d06455659 Mon Sep 17 00:00:00 2001 From: Ben McMorran Date: Wed, 15 Jul 2015 14:56:04 +0000 Subject: [PATCH 2/2] Add query count tests --- .../django_comment_client/base/tests.py | 160 ++++++++++++++---- lms/djangoapps/django_comment_client/utils.py | 5 +- 2 files changed, 131 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 409b4b6363..041b6e0e24 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -1,10 +1,14 @@ import logging import json +import ddt +from django.conf import settings +from django.core.cache import get_cache from django.test.client import Client, RequestFactory from django.contrib.auth.models import User from django.core.management import call_command from django.core.urlresolvers import reverse +from request_cache.middleware import RequestCache from mock import patch, ANY, Mock from nose.tools import assert_true, assert_equal # pylint: disable=no-name-in-module from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -18,8 +22,11 @@ from django_comment_common.models import Role from django_comment_common.utils import seed_permissions_roles from student.tests.factories import CourseEnrollmentFactory, UserFactory, CourseAccessRoleFactory from util.testing import UrlResetMixin -from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import check_mongo_calls +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import ModuleStoreEnum log = logging.getLogger(__name__) @@ -161,16 +168,19 @@ class ThreadActionGroupIdTestCase( ) -@patch('lms.lib.comment_client.utils.requests.request') -class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): +class ViewsTestCaseMixin(object): + """ + This class is used by both ViewsQueryCountTestCase and ViewsTestCase. By + breaking out set_up_course into its own method, ViewsQueryCountTestCase + can build a course in a particular modulestore, while ViewsTestCase can + just run it in setUp for all tests. + """ - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) - def setUp(self): - - # Patching the ENABLE_DISCUSSION_SERVICE value affects the contents of urls.py, - # so we need to call super.setUp() which reloads urls.py (because - # of the UrlResetMixin) - super(ViewsTestCase, self).setUp(create_user=False) + def set_up_course(self, module_count=0): + """ + Creates a course, optionally with module_count discussion modules, and + a user with appropriate permissions. + """ # create a course self.course = CourseFactory.create( @@ -179,6 +189,17 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): display_name='Robot Super Course', ) self.course_id = self.course.id + + # add some discussion modules + for i in range(module_count): + ItemFactory.create( + parent_location=self.course.location, + category='discussion', + discussion_id='id_module_{}'.format(i), + discussion_category='Category {}'.format(i), + discussion_target='Discussion {}'.format(i) + ) + # seed the forums permissions and roles call_command('seed_permissions_roles', self.course_id.to_deprecated_string()) @@ -201,7 +222,24 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): self.client = Client() assert_true(self.client.login(username='student', password='test')) - def test_create_thread(self, mock_request): + def _setup_mock_request(self, mock_request, include_depth=False): + """ + Ensure that mock_request returns the data necessary to make views + function correctly + """ + mock_request.return_value.status_code = 200 + data = { + "user_id": str(self.student.id), + "closed": False, + } + if include_depth: + data["depth"] = 0 + self._set_mock_request_data(mock_request, data) + + def create_thread_helper(self, mock_request): + """ + Issues a request to create a thread and verifies the result. + """ mock_request.return_value.status_code = 200 self._set_mock_request_data(mock_request, { "thread_type": "discussion", @@ -255,7 +293,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): 'title': u'Hello', 'commentable_id': u'i4x-MITx-999-course-Robot_Super_Course', 'anonymous': False, - 'course_id': u'MITx/999/Robot_Super_Course', + 'course_id': unicode(self.course_id), }, params={'request_id': ANY}, headers=ANY, @@ -263,6 +301,83 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): ) assert_equal(response.status_code, 200) + def update_thread_helper(self, mock_request): + """ + Issues a request to update a thread and verifies the result. + """ + self._setup_mock_request(mock_request) + response = self.client.post( + reverse("update_thread", kwargs={"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}), + data={"body": "foo", "title": "foo", "commentable_id": "some_topic"} + ) + self.assertEqual(response.status_code, 200) + + +@ddt.ddt +@patch('lms.lib.comment_client.utils.requests.request') +class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin): + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(ViewsQueryCountTestCase, self).setUp(create_user=False) + + def clear_caches(self): + """Clears caches so that query count numbers are accurate.""" + for cache in settings.CACHES: + get_cache(cache).clear() + RequestCache.clear_request_cache() + + def count_queries(func): # pylint: disable=no-self-argument + """ + Decorates test methods to count mongo and SQL calls for a + particular modulestore. + """ + def inner(self, default_store, module_count, mongo_calls, sql_queries, *args, **kwargs): + with modulestore().default_store(default_store): + self.set_up_course(module_count=module_count) + self.clear_caches() + with self.assertNumQueries(sql_queries): + with check_mongo_calls(mongo_calls): + func(self, *args, **kwargs) + return inner + + @ddt.data( + (ModuleStoreEnum.Type.mongo, 3, 4, 21), + (ModuleStoreEnum.Type.mongo, 20, 4, 21), + (ModuleStoreEnum.Type.split, 3, 13, 21), + (ModuleStoreEnum.Type.split, 20, 13, 21), + ) + @ddt.unpack + @count_queries + def test_create_thread(self, mock_request): + self.create_thread_helper(mock_request) + + @ddt.data( + (ModuleStoreEnum.Type.mongo, 3, 3, 16), + (ModuleStoreEnum.Type.mongo, 20, 3, 16), + (ModuleStoreEnum.Type.split, 3, 10, 16), + (ModuleStoreEnum.Type.split, 20, 10, 16), + ) + @ddt.unpack + @count_queries + def test_update_thread(self, mock_request): + self.update_thread_helper(mock_request) + + +@patch('lms.lib.comment_client.utils.requests.request') +class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin): + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + # Patching the ENABLE_DISCUSSION_SERVICE value affects the contents of urls.py, + # so we need to call super.setUp() which reloads urls.py (because + # of the UrlResetMixin) + super(ViewsTestCase, self).setUp(create_user=False) + self.set_up_course() + + def test_create_thread(self, mock_request): + self.create_thread_helper(mock_request) + def test_delete_comment(self, mock_request): self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), @@ -280,20 +395,6 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): self.assertEqual(args[0], "delete") self.assertTrue(args[1].endswith("/{}".format(test_comment_id))) - def _setup_mock_request(self, mock_request, include_depth=False): - """ - Ensure that mock_request returns the data necessary to make views - function correctly - """ - mock_request.return_value.status_code = 200 - data = { - "user_id": str(self.student.id), - "closed": False, - } - if include_depth: - data["depth"] = 0 - self._set_mock_request_data(mock_request, data) - def _test_request_error(self, view_name, view_kwargs, data, mock_request): """ Submit a request against the given view with the given data and ensure @@ -372,12 +473,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): ) def test_update_thread_course_topic(self, mock_request): - self._setup_mock_request(mock_request) - response = self.client.post( - reverse("update_thread", kwargs={"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}), - data={"body": "foo", "title": "foo", "commentable_id": "some_topic"} - ) - self.assertEqual(response.status_code, 200) + self.update_thread_helper(mock_request) @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"]) def test_update_thread_wrong_commentable_id(self, mock_get_discussion_id_map, mock_request): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 43e18bf517..8785d76999 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -329,8 +329,9 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude def discussion_category_id_access(course, user, discussion_id): """ - Returns True iff the given discussion_id is accessible for user in course. Uses the discussion id cache if available - falling back to get_discussion_categories_ids if there is no cache. + Returns True iff the given discussion_id is accessible for user in course. + Uses the discussion id cache if available, falling back to + get_discussion_categories_ids if there is no cache. """ if discussion_id in course.top_level_discussion_topic_ids: return True