From 01e66ecd07aa53e88328ef059b5b3568aa7ad91a Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 9 Dec 2013 16:34:07 -0500 Subject: [PATCH 1/2] Reduce sql queries for groupname tests. Conflicts: cms/djangoapps/auth/authz.py --- cms/djangoapps/auth/authz.py | 15 ++++++--------- lms/djangoapps/courseware/roles.py | 1 - 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 1a1f138cb588..5ec7beba2ef4 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -63,7 +63,7 @@ def get_all_course_role_groupnames(location, role, use_filter=True): # filter to the ones which exist default = groupnames[0] if use_filter: - groupnames = [group for group in groupnames if Group.objects.filter(name=group).exists()] + groupnames = [group.name for group in Group.objects.filter(name__in=groupnames)] return groupnames, default @@ -203,12 +203,9 @@ def remove_user_from_course_group(caller, user, location, role): # see if the user is actually in that role, if not then we don't have to do anything groupnames, _ = get_all_course_role_groupnames(location, role) - for groupname in groupnames: - groups = user.groups.filter(name=groupname) - if groups: - # will only be one with that name - user.groups.remove(groups[0]) - user.save() + for group in user.groups.filter(name__in=groupnames): + user.groups.remove(group) + user.save() def remove_user_from_creator_group(caller, user): @@ -243,7 +240,7 @@ def is_user_in_course_group_role(user, location, role, check_staff=True): if check_staff and user.is_staff: return True groupnames, _ = get_all_course_role_groupnames(location, role) - return any(user.groups.filter(name=groupname).exists() for groupname in groupnames) + return user.groups.filter(name__in=groupnames).exists() return False @@ -266,7 +263,7 @@ def is_user_in_creator_group(user): # Feature flag for using the creator group setting. Will be removed once the feature is complete. if settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False): - return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).count() > 0 + return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).exists() return True diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 110bb9f3623b..60853e8090d0 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/lms/djangoapps/courseware/roles.py @@ -4,7 +4,6 @@ """ from abc import ABCMeta, abstractmethod -from functools import partial from django.contrib.auth.models import User, Group From 050900661329c4ad56270e1169263f732785ef2f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 10 Dec 2013 12:15:16 -0500 Subject: [PATCH 2/2] Use mongo indices for all queries STUD-1039 --- cms/djangoapps/contentstore/views/course.py | 3 +- .../xmodule/modulestore/loc_mapper_store.py | 64 +++++++++++++------ .../modulestore/tests/test_location_mapper.py | 10 +-- mongo_indexes.md | 21 ++++++ 4 files changed, 72 insertions(+), 26 deletions(-) create mode 100644 mongo_indexes.md diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index c7e379869b30..39695fc66e7f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -116,7 +116,8 @@ def course_listing(request): """ List all courses available to the logged in user """ - courses = modulestore('direct').get_items(['i4x', None, None, 'course', None]) + # there's an index on category which will be used if none of its antecedents are set + courses = modulestore('direct').get_items([None, None, None, 'course', None]) # filter out courses that we don't have access too def course_filter(course): diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index c7355c2f29a3..4bf5f1c64b7c 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -4,10 +4,10 @@ from random import randint import re import pymongo +import bson.son from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError from xmodule.modulestore.locator import BlockUsageLocator -from xmodule.modulestore.mongo import draft from xmodule.modulestore import Location import urllib @@ -91,9 +91,10 @@ def create_map_entry(self, course_location, course_id=None, draft_branch='draft' else: course_id = "{0.org}.{0.course}".format(course_location) # very like _interpret_location_id but w/o the _id - location_id = {'org': course_location.org, 'course': course_location.course} - if course_location.category == 'course': - location_id['name'] = course_location.name + location_id = self._construct_location_son( + course_location.org, course_location.course, + course_location.name if course_location.category == 'course' else None + ) self.location_map.insert({ '_id': location_id, @@ -128,20 +129,25 @@ def translate_location(self, old_style_course_id, location, published=True, add_ """ location_id = self._interpret_location_course_id(old_style_course_id, location) - maps = self.location_map.find(location_id).sort('_id.name', pymongo.ASCENDING) - if maps.count() == 0: + maps = self.location_map.find(location_id) + maps = list(maps) + if len(maps) == 0: if add_entry_if_missing: # create a new map - course_location = location.replace(category='course', name=location_id['_id.name']) + course_location = location.replace(category='course', name=location_id['_id']['name']) self.create_map_entry(course_location) entry = self.location_map.find_one(location_id) else: raise ItemNotFoundError() - elif maps.count() > 1: - # if more than one, prefer the one w/o a name if that exists. Otherwise, choose the first (alphabetically) + elif len(maps) == 1: entry = maps[0] else: + # find entry w/o name, if any; otherwise, pick arbitrary entry = maps[0] + for item in maps: + if 'name' not in item['_id']: + entry = item + break if published: branch = entry['prod_branch'] @@ -242,11 +248,11 @@ def add_block_location_translator(self, location, old_course_id=None, usage_id=N location_id = self._interpret_location_course_id(old_course_id, location) maps = self.location_map.find(location_id) - if maps.count() == 0: - raise ItemNotFoundError() - # turn maps from cursor to list map_list = list(maps) + if len(map_list) == 0: + raise ItemNotFoundError() + encoded_location_name = self._encode_for_mongo(location.name) # check whether there's already a usage_id for this location (and it agrees w/ any passed in or found) for map_entry in map_list: @@ -279,7 +285,10 @@ def add_block_location_translator(self, location, old_course_id=None, usage_id=N ) map_entry['block_map'].setdefault(encoded_location_name, {})[location.category] = computed_usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return computed_usage_id @@ -317,7 +326,10 @@ def update_block_location_translator(self, location, usage_id, old_course_id=Non if location.category in map_entry['block_map'].setdefault(encoded_location_name, {}): map_entry['block_map'][encoded_location_name][location.category] = usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return usage_id @@ -338,7 +350,10 @@ def delete_block_location_translator(self, location, old_course_id=None): del map_entry['block_map'][encoded_location_name] else: del map_entry['block_map'][encoded_location_name][location.category] - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) def _add_to_block_map(self, location, location_id, block_map): '''add the given location to the block_map and persist it''' @@ -357,7 +372,7 @@ def _add_to_block_map(self, location, location_id, block_map): def _interpret_location_course_id(self, course_id, location): """ - Take the old style course id (org/course/run) and return a dict for querying the mapping table. + Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table. If the course_id is empty, it uses location, but this may result in an inadequate id. :param course_id: old style 'org/course/run' id from Location.course_id where Location.category = 'course' @@ -367,12 +382,21 @@ def _interpret_location_course_id(self, course_id, location): if course_id: # re doesn't allow ?P<_id.org> and ilk matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id) - return dict(zip(['_id.org', '_id.course', '_id.name'], matched.groups())) + return {'_id': self._construct_location_son(*matched.groups())} - location_id = {'_id.org': location.org, '_id.course': location.course} if location.category == 'course': - location_id['_id.name'] = location.name - return location_id + return {'_id': self._construct_location_son(location.org, location.course, location.name)} + else: + return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)]) + + def _construct_location_son(self, org, course, name=None): + """ + Construct the SON needed to repr the location for either a query or an insertion + """ + if name: + return bson.son.SON([('org', org), ('course', course), ('name', name)]) + else: + return bson.son.SON([('org', org), ('course', course)]) def _block_id_is_guid(self, name): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 2fbff1042389..f5505264938c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -36,8 +36,9 @@ def test_create_map(self): org = 'foo_org' course = 'bar_course' loc_mapper().create_map_entry(Location('i4x', org, course, 'course', 'baz_run')) + # pylint: disable=protected-access entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course, 'name': 'baz_run'} + '_id': loc_mapper()._construct_location_son(org, course, 'baz_run') }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}.baz_run'.format(org, course)) @@ -48,8 +49,9 @@ def test_create_map(self): # ensure create_entry does the right thing when not given a course (creates org/course # rather than org/course/run course_id) loc_mapper().create_map_entry(Location('i4x', org, course, 'vertical', 'baz_vert')) + # find the one which has no name entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} + '_id' : loc_mapper()._construct_location_son(org, course, None) }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}'.format(org, course)) @@ -63,9 +65,7 @@ def test_create_map(self): 'wip', 'live', block_map) - entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} - }) + entry = loc_mapper().location_map.find_one({'_id.org': org, '_id.course': course}) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], 'foo_org.geek_dept.quux_course.baz_run') self.assertEqual(entry['draft_branch'], 'wip') diff --git a/mongo_indexes.md b/mongo_indexes.md new file mode 100644 index 000000000000..513367473772 --- /dev/null +++ b/mongo_indexes.md @@ -0,0 +1,21 @@ +These are the indexes each mongo db should have in order to perform well. +Each section states the collection name and then the indexes. To create an index, +you'll typically either use the mongohq type web interface or a standard terminal console. +If a terminal, this assumes you've logged in and gotten to the mongo prompt +``` +mongo mydatabasename +``` + +If using the terminal, to add an index to a collection, you'll need to prefix ```ensureIndex``` with +``` +db.collection_name +``` +as in ```db.location_map.ensureIndex({'course_id': 1}{background: true})``` + +location_map: +============= + +``` +ensureIndex({​'_id.org': 1, '_id.course': 1}) +ensureIndex({​'course_id': 1}) +```