From d24a777067b48a7a0345041aa64d861faf89232e Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 15 May 2014 13:35:55 -0400 Subject: [PATCH] Remove package_id and remove lower_ stuff from migrator STUD-1603 --- .../xmodule/modulestore/loc_mapper_store.py | 91 ++++++------------- .../xmodule/xmodule/modulestore/locator.py | 13 +-- 2 files changed, 31 insertions(+), 73 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 0f04298685..d38da854ef 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -100,9 +100,7 @@ class LocMapperStore(object): self.location_map.insert({ '_id': course_son, 'org': org, - 'lower_org': org.lower(), 'offering': offering, - 'lower_offering': offering.lower(), 'draft_branch': draft_branch, 'prod_branch': prod_branch, 'block_map': block_map or {}, @@ -197,7 +195,7 @@ class LocMapperStore(object): self._cache_location_map_entry(location, published_usage, draft_usage) return result - def translate_locator_to_location(self, locator, get_course=False, lower_only=False): + def translate_locator_to_location(self, locator, get_course=False): """ Returns an old style Location for the given Locator if there's an appropriate entry in the mapping collection. Note, it requires that the course was previously mapped (a side effect of @@ -211,53 +209,31 @@ class LocMapperStore(object): locator: a BlockUsageLocator to translate get_course: rather than finding the map for this locator, returns the CourseKey for the mapped course. - lower_only: (obsolete?) the locator's fields are lowercased and not the actual case - for the identifier (e.g., came from a sql db which lowercases all ids). Find the actual - case Location for the desired object """ if get_course: cached_value = self._get_course_location_from_cache( # if locator is already a course_key it won't have course_key attr - getattr(locator, 'course_key', locator), - lower_only + getattr(locator, 'course_key', locator) ) else: cached_value = self._get_location_from_cache(locator) if cached_value: return cached_value - # This does not require that the course exist in any modulestore - # only that it has a mapping entry. - if lower_only: - # migrate any records which don't have the lower_org and lower_offering fields as - # this won't be able to find what it wants. (only needs to be run once ever per db, - # I'm not sure how to control that, but I'm putting some check here for once per launch) - if not getattr(self, 'lower_offering_migrated', False): - obsolete = self.location_map.find( - {'lower_org': {"$exists": False}, "lower_offering": {"$exists": False}, } - ) - self._migrate_if_necessary(obsolete) - setattr(self, 'lower_offering_migrated', True) + # migrate any records which don't have the org and offering fields as + # this won't be able to find what it wants. (only needs to be run once ever per db, + # I'm not sure how to control that, but I'm putting some check here for once per launch) + if not getattr(self, 'offering_migrated', False): + obsolete = self.location_map.find( + {'org': {"$exists": False}, "offering": {"$exists": False}, } + ) + self._migrate_if_necessary(obsolete) + setattr(self, 'offering_migrated', True) - entry = self.location_map.find_one(bson.son.SON([ - ('lower_org', locator.org.lower()), - ('lower_offering', locator.offering.lower()), - ])) - else: - # migrate any records which don't have the lower_org and lower_offering fields as - # this won't be able to find what it wants. (only needs to be run once ever per db, - # I'm not sure how to control that, but I'm putting some check here for once per launch) - if not getattr(self, 'offering_migrated', False): - obsolete = self.location_map.find( - {'org': {"$exists": False}, "offering": {"$exists": False}, } - ) - self._migrate_if_necessary(obsolete) - setattr(self, 'offering_migrated', True) - - entry = self.location_map.find_one(bson.son.SON([ - ('org', locator.org), - ('lower_offering', locator.offering), - ])) + entry = self.location_map.find_one(bson.son.SON([ + ('org', locator.org), + ('offering', locator.offering), + ])) # look for one which maps to this block block_id if entry is None: @@ -277,12 +253,8 @@ class LocMapperStore(object): self.decode_key_from_mongo(old_name) ) - if lower_only: - entry_org = "lower_org" - entry_offering = "lower_offering" - else: - entry_org = "org" - entry_offering = "offering" + entry_org = "org" + entry_offering = "offering" published_locator = BlockUsageLocator( CourseLocator( @@ -454,18 +426,20 @@ class LocMapperStore(object): """ return self.cache.get(unicode(locator)) - def _get_course_location_from_cache(self, locator_package_id, lower_only=False): + def _get_course_location_from_cache(self, course_key): """ - See if the package_id is in the cache. If so, return the mapped location to the + See if the course_key is in the cache. If so, return the mapped location to the course root. """ - if lower_only: - cache_key = u'courseIdLower+{}'.format(locator_package_id.lower()) - else: - cache_key = u'courseId+{}'.format(locator_package_id) - + cache_key = self._course_key_cache_string(course_key) return self.cache.get(cache_key) + def _course_key_cache_string(self, course_key): + """ + Return the string used to cache the course key + """ + return u'{0.org}+{0.offering}'.format(course_key) + def _cache_course_locator(self, old_course_id, published_course_locator, draft_course_locator): """ For quick lookup of courses @@ -482,8 +456,7 @@ class LocMapperStore(object): """ setmany = {} if location.category == 'course': - setmany[u'courseId+{}'.format(published_usage.package_id)] = location - setmany[u'courseIdLower+{}'.format(published_usage.package_id.lower())] = location + setmany[self._course_key_cache_string(published_usage)] = location.course_key setmany[unicode(published_usage)] = location setmany[unicode(draft_usage)] = location setmany[unicode(location)] = (published_usage, draft_usage) @@ -504,8 +477,7 @@ class LocMapperStore(object): delete_keys = [] published_locator = unicode(cached_key[0].course_key) course_location = self._course_location_from_cache(published_locator) - delete_keys.append(u'courseId+{}'.format(published_locator)) - delete_keys.append(u'courseIdLower+{}'.format(unicode(cached_key[0].course_key).lower())) + delete_keys.append(self._course_key_cache_string(course_key)) delete_keys.append(published_locator) delete_keys.append(unicode(cached_key[1].course_key)) delete_keys.append(unicode(course_location)) @@ -535,8 +507,7 @@ class LocMapperStore(object): """ delete_keys = [] if location.category == 'course': - delete_keys.append(u'courseId+{}'.format(published_usage.package_id)) - delete_keys.append(u'courseIdLower+{}'.format(published_usage.package_id.lower())) + delete_keys.append(self._course_key_cache_string(published_usage.course_key)) delete_keys.append(unicode(published_usage)) delete_keys.append(unicode(draft_usage)) @@ -560,7 +531,7 @@ class LocMapperStore(object): """ If entry had an '_id' without a run, remove the whole record. - Add fields: schema, org, offering, lower_org, and lower_offering + Add fields: schema, org, offering Remove: course_id, lower_course_id :param entry: """ @@ -579,9 +550,7 @@ class LocMapperStore(object): entry.pop('lower_course_id', None) old_course_id = SlashSeparatedCourseKey(entry['_id']['org'], entry['_id']['course'], entry['_id']['name']) entry['org'] = old_course_id.org - entry['lower_org'] = old_course_id.org.lower() entry['offering'] = old_course_id.offering.replace('/', '+') - entry['lower_offering'] = entry['offering'].lower() return self._migrate_1(entry, True) # insert new migrations just before _migrate_top. _migrate_top sets the schema version and diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index e164216552..ff73c540e8 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -111,13 +111,6 @@ class BlockLocatorBase(Locator): raise InvalidKeyError(cls, string) return match.groupdict() - @property - def package_id(self): - if self.org and self.offering: - return u'{}{}{}'.format(self.org, self.ORG_SEPARATOR, self.offering) - else: - return None - class CourseLocator(BlockLocatorBase, CourseKey): """ @@ -267,7 +260,7 @@ class CourseLocator(BlockLocatorBase, CourseKey): """ parts = [] if self.offering: - parts.append(unicode(self.package_id)) + parts.extend([self.org, self.offering]) if self.branch: parts.append(u"{prefix}+{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch)) if self.version_guid: @@ -390,10 +383,6 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey): def offering(self): return self.course_key.offering - @property - def package_id(self): - return self.course_key.package_id - @property def branch(self): return self.course_key.branch