Merge pull request #3658 from edx/opaque-keys-location-to-son

Opaque keys location to son

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):

common/lib/xmodule/xmodule/contentstore/content.py

-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()
@@ -107,17 +106,6 @@ class StaticContent(object):
         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):
         """
         Generate an AssetKey for the given path (old c4x/org/course/asset/name syntax)

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.')

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

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)
 

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)