From 46a976d7e1038ec8e98b987d1f1024d55f2792b4 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 6 May 2014 16:52:08 -0400 Subject: [PATCH] fixed asset search query. moved location_to_son to LocationBase.py. --- .../contentstore/tests/test_core_caching.py | 2 +- .../xmodule/xmodule/contentstore/content.py | 14 +--------- .../lib/xmodule/xmodule/contentstore/mongo.py | 28 +++++++++++-------- .../lib/xmodule/xmodule/modulestore/django.py | 1 - .../xmodule/xmodule/modulestore/mongo/base.py | 26 +++++------------ .../courseware/tests/test_video_handlers.py | 2 +- 6 files changed, 27 insertions(+), 46 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_core_caching.py b/cms/djangoapps/contentstore/tests/test_core_caching.py index 9ad4dccceb..622afd2110 100644 --- a/cms/djangoapps/contentstore/tests/test_core_caching.py +++ b/cms/djangoapps/contentstore/tests/test_core_caching.py @@ -10,7 +10,7 @@ class Content: self.content = content def get_id(self): - return StaticContent.get_id_from_location(self.location) + return self.location.to_deprecated_son() class CachingTestCase(TestCase): diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 8782116acc..c6e8b58be4 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -1,4 +1,3 @@ -import bson.son import re XASSET_LOCATION_TAG = 'c4x' XASSET_SRCREF_PREFIX = 'xasset:' @@ -60,7 +59,7 @@ class StaticContent(object): ) def get_id(self): - return StaticContent.get_id_from_location(self.location) + return self.location.to_deprecated_son(tag=XASSET_LOCATION_TAG) def get_url_path(self): return self.location.to_deprecated_string() @@ -106,17 +105,6 @@ class StaticContent(object): assert(isinstance(course_key, SlashSeparatedCourseKey)) return course_key.make_asset_key('asset', '').to_deprecated_string() - @staticmethod - def get_id_from_location(location): - """ - Get the doc store's primary key repr for this location - """ - return bson.son.SON([ - ('tag', 'c4x'), ('org', location.org), ('course', location.course), - ('category', location.category), ('name', location.name), - ('revision', location.revision), - ]) - @staticmethod def get_location_from_path(path): """ diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 76fca88103..9cad2f1014 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -2,7 +2,7 @@ import pymongo import gridfs from gridfs.errors import NoFile -from xmodule.modulestore.mongo.base import location_to_query, MongoModuleStore, location_to_son +from xmodule.modulestore.mongo.base import location_to_query, MongoModuleStore from xmodule.contentstore.content import XASSET_LOCATION_TAG import logging @@ -69,9 +69,7 @@ class MongoContentStore(ContentStore): assert not self.fs.exists({"_id": content_id}) def find(self, location, throw_on_not_found=True, as_stream=False): - content_id = StaticContent.get_id_from_location(location) - # Use slow attr based lookup b/c we weren't careful to control the key order in _id before - content_id = {u'_id.{}'.format(key): value for key, value in content_id.iteritems()} + content_id = self.asset_db_key(location) fs_pointer = self.fs_files.find_one(content_id, fields={'_id': 1}) if fs_pointer is None: if throw_on_not_found: @@ -110,9 +108,7 @@ class MongoContentStore(ContentStore): return None def get_stream(self, location): - content_id = StaticContent.get_id_from_location(location) - # use slow attr based lookup because we weren't careful to control the key order in _id before - content_id = {u'_id.{}'.format(key): value for key, value in content_id.iteritems()} + content_id = self.asset_db_key(location) fs_pointer = self.fs_files.find_one(content_id, fields={'_id': 1}) try: @@ -247,8 +243,10 @@ class MongoContentStore(ContentStore): for attr in attr_dict.iterkeys(): if attr in ['_id', 'md5', 'uploadDate', 'length']: raise AttributeError("{} is a protected attribute.".format(attr)) - asset_db_key = {'_id': location_to_son(location, tag=XASSET_LOCATION_TAG)} - if self.fs_files.find(asset_db_key).count() == 0: + asset_db_key = self.asset_db_key(location) + # FIXME remove fetch and use a form of update which fails if doesn't exist + item = self.fs_files.find_one(asset_db_key) + if item is None: raise NotFoundError(asset_db_key) self.fs_files.update(asset_db_key, {"$set": attr_dict}) @@ -262,9 +260,10 @@ class MongoContentStore(ContentStore): :param location: a c4x asset location """ - item = self.fs_files.find_one({'_id': location_to_son(location, tag=XASSET_LOCATION_TAG)}) + asset_db_key = self.asset_db_key(location) + item = self.fs_files.find_one(asset_db_key) if item is None: - raise NotFoundError() + raise NotFoundError(asset_db_key) return item def delete_all_course_assets(self, course_key): @@ -277,3 +276,10 @@ class MongoContentStore(ContentStore): matching_assets = self.fs_files.find(course_query) for asset in matching_assets: self.fs.delete(asset['_id']) + + @staticmethod + def asset_db_key(location): + """ + Returns the database query to find the given asset location. + """ + return location.to_deprecated_son(tag=XASSET_LOCATION_TAG, prefix='_id.') diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index c0c67d06f6..ad17243662 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -10,7 +10,6 @@ import re from django.conf import settings from django.core.cache import get_cache, InvalidCacheBackendError -from django.dispatch import Signal import django.utils from xmodule.modulestore.loc_mapper_store import LocMapperStore diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index cbf21a253c..3e579e10b9 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -248,29 +248,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem): return jsonfields -def location_to_son(location, prefix='', tag='i4x'): - """ - Converts a location into a SON object with the same key order - """ - son = SON({prefix + 'tag': tag}) - for field_name in location.KEY_FIELDS: - # Filter the run, because the existing data doesn't have it stored - if field_name != 'run': - son[prefix + field_name] = getattr(location, field_name) - return son - - # The only thing using this w/ wildcards is contentstore.mongo for asset retrieval def location_to_query(location, wildcard=True, tag='i4x'): """ Takes a Location and returns a SON object that will query for that location by subfields - rather than subdoc. Note: location_to_son is faster in mongo as it does subdoc equivalence. - Fields in location that are None are ignored in the query + rather than subdoc. + Fields in location that are None are ignored in the query. If `wildcard` is True, then a None in a location is treated as a wildcard query. Otherwise, it is searched for literally """ - query = location_to_son(location, prefix='_id.', tag=tag) + query = location.to_deprecated_son(prefix='_id.', tag=tag) if wildcard: for key, value in query.items(): @@ -481,7 +469,7 @@ class MongoModuleStore(ModuleStoreWriteBase): # first get non-draft in a round-trip query = { '_id': {'$in': [ - location_to_son(course_key.make_usage_key_from_deprecated_string(item)) for item in items + course_key.make_usage_key_from_deprecated_string(item).to_deprecated_son() for item in items ]} } return list(self.collection.find(query)) @@ -601,7 +589,7 @@ class MongoModuleStore(ModuleStoreWriteBase): ''' assert isinstance(location, Location) item = self.collection.find_one( - {'_id': location_to_son(location)}, + {'_id': location.to_deprecated_son()}, sort=[('revision', pymongo.ASCENDING)], ) if item is None: @@ -886,7 +874,7 @@ class MongoModuleStore(ModuleStoreWriteBase): # See http://www.mongodb.org/display/DOCS/Updating for # atomic update syntax result = self.collection.update( - {'_id': location_to_son(location)}, + {'_id': location.to_deprecated_son()}, {'$set': update}, multi=False, upsert=True, @@ -975,7 +963,7 @@ class MongoModuleStore(ModuleStoreWriteBase): # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False") # from overriding our default value set in the init method. - self.collection.remove({'_id': location_to_son(location)}, safe=self.collection.safe) + self.collection.remove({'_id': location.to_deprecated_son()}, safe=self.collection.safe) # recompute (and update) the metadata inheritance tree which is cached self.refresh_cached_metadata_inheritance_tree(location.course_key) diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index d1ba4c2762..d626b55ccf 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -66,7 +66,7 @@ def _clear_assets(location): for asset in assets: asset_location = AssetLocation._from_deprecated_son(asset["_id"], location.course_key.run) del_cached_content(asset_location) - mongo_id = StaticContent.get_id_from_location(asset_location) + mongo_id = asset_location.to_deprecated_son() store.delete(mongo_id)