Opaque Key Simplification?

[kdmccormick]

Related:

• Resolve Opaque Key coherency issues openedx-core#322

The purpose of that ^ ticket is whatever we need to get in order to get a openedx-core v1.0 where people aren't mixing up entity keys and usage keys.

The purpose of this ticket is to more radically "fix" the opaque_keys interface.

[bradenmacdonald]

Some ideas:

• Can we merge CourseKey and CourseLocator into a single class?
• Rename the org, course, and run fields into org_code, course_code, and run_code for consistency with An openedx_catalog app with a representation for CourseRuns [FC-0117] openedx-core#479 (keeping the old names as aliases so nothing breaks)
• Improve the import API: from opaque_keys.edx.keys import CourseKey -> from openedx_keys import CourseKey ?
• Move CCX keys from edx-ccx-keys into this repo.
• Remove i4xEncoder, SlashSeparatedCourseKey, LocationBase, Location, DeprecatedLocation, AssetLocation
• Remove the deprecated mode of all the remaining classes, including the deprecated parameter and methods like make_usage_key_from_deprecated_string, to_deprecated_string, from_deprecated_string, set_deprecated_fallback, clean_for_html, offering and revision. These have been marked deprecated for over 12 years!
• Can we remove CreatorMixin from OpaqueKeyField ?
• Remove this log-and-strip-newline code, which seems like it was added ~10 years ago to handle some very specific bug?

[kdmccormick]

I poked around with this a bit today: #431 . I'm putting it down for now, but here are my thoughts so far:

Can we merge CourseKey and CourseLocator into a single class?

CourseKey is the parent of both CourseLocator (course-v1) and LibraryLocator (library-v1). I think we need to keep all 3 for those for the forseeable future. But I think we can rename them to make the relationship more obvious.

• CourseKey -> ContextKeyV1
• CourseLocator -> CourseKeyV1
• LibraryLocator -> LibraryKeyV1

We would end up with many many instances of the awkwardly-named "ContextKeyV1", but I think that's OK. 90% of the occurrences of "ContextKeyV1" would be better as either CourseKeyV1 (course-specific stuff) or ContextKey (stuff that applies to any learning context).

Rename the org, course, and run fields into org_code, course_code, and run_code for consistency with openedx/openedx-core#479 (keeping the old names as aliases so nothing breaks)

I am in favor with the renames, although I'm unsure if we should rename the fields themselves, or just introduce the new names as read-only properties. I'm a bit intimidated by the amount of test code that would need to be updated if we changed the actual field names.

Improve the import API: from opaque_keys.edx.keys import CourseKey -> from openedx_keys import CourseKey ?
Move CCX keys from edx-ccx-keys into this repo.

Agreed. My PR puts everything at openedx_keys.api, but I'm fine with dropping the .api part too.

Remove i4xEncoder, SlashSeparatedCourseKey, LocationBase, Location, DeprecatedLocation, AssetLocation
Can we remove CreatorMixin from OpaqueKeyField ?

+1

Generally, for removing the old names and APIs, I propose this approach:

• Build a new API at openedx_keys[.api] which just wildcard-imports from the sources at opaque_keys
• DEPR the entire opaque_keys API, add warnings
• Wait a release or two
• Finally, delete opaque_keys and move the implementation into opaque_keys.impl (or something like that)

Remove the deprecated mode of all the remaining classes
Remove this log-and-strip-newline code, which seems like it was added ~10 years ago to handle some very specific bug?

Didn't have a chance to look into these, but they sound reasonable

[bradenmacdonald]

Can we merge CourseKey and CourseLocator into a single class?

CourseKey is the parent of both CourseLocator (course-v1) and LibraryLocator (library-v1). I think we need to keep all 3 for those for the forseeable future. But I think we can rename them to make the relationship more obvious.

• CourseKey -> ContextKeyV1
• CourseLocator -> CourseKeyV1
• LibraryLocator -> LibraryKeyV1

That's only because v1 libraries were a huge hack and were technically implemented as courses, and some modulestore methods that need to accept v1 library keys require a CourseKey. Since v1 libraries are deprecated and soon to be read-only, I suspect that we may be able to clean this up in a better way, either by making LibraryKeyV1 just a direct subclass of LearningContextKey, a subclass of your proposed CourseKeyV1 (but I'd rather just call it CourseKey), or an independent key type without any inheritance.

Also, how does LearningContextKey fit into this picture?

[kdmccormick]

My thinking was this:

• OpaqueKey
  • ContextKey (nee LearningContextKey)
    • ContextKeyV1 (nee CourseKey)
      • CourseKeyV1 (nee CourseLocator)
      • LibraryKeyV1 (nee LibraryLocator)
    • LibraryKeyV2 (nee LibraryLocatorV2)

We need to keep legacy libraries backend working until after the Xylon cutoff, and even after that we'll probably have some stub systems which still use LibraryKeyV1.

Since v1 libraries are deprecated and soon to be read-only, I suspect that we may be able to clean this up in a better way, either by making LibraryKeyV1 just a direct subclass of LearningContextKey, a subclass of your proposed CourseKeyV1

Maybe that'd work? But I'm seeing a lot of places in openedx-platform which either:

• Need to handle legacy libraries and courses (and their blocks) as if they're the same thing, which is easier if the keys have a common ancestor.
• Need to handle legacy libraries and courses differently, which is easier if one is not a subclass of the other.

but I'd rather just call it CourseKey

I hear you, but find-and-replace is more error-prone if there are is any overlap between the old names and the new names.

[bradenmacdonald]

That hierarchy makes sense, assuming we eventually could remove "ContextKeyV1". But the main issue I have is that most current usages of CourseKey really mean what you're calling CourseKeyV1 (nee CourseLocator), and don't actually want to accept either a library key or a course key. So I'd suggest:

• OpaqueKey
  • ContextKey (nee LearningContextKey)
    • CourseLikeKeyV1 (deprecated and temporarily used in the modulestore API only)
      • CourseKeyV1 (nee CourseLocator, CourseKey)
      • LibraryKeyV1 (nee LibraryLocator)
    • LibraryKeyV2 (nee LibraryLocatorV2)

[kdmccormick]

CourselikeKeyV1 (deprecated and temporarily used in the modulestore API only)

sound great. that matches with the existing "courselike" terminology used throughout modulestore which we hope to rip out after xylon

[kdmccormick]

Here's an alternate naming scheme which omits V1/V2 and still avoids any overlap between new and old names.

• ContextKey (nee LearningContextKey)
  • CourselikeKey (nee CourseKey)
    • CourseRunKey (nee CourseLocator)
    • LegacyLibraryKey (nee LibraryLocator)
  • LibraryKey (nee LibraryLocatorV2)
• UsageKey
  • ContentUsageKey (nee UsageKeyV2)
    • LibraryUsageKey (nee LibraryUsageLocatorV2)
  • CourseRunUsageKey (or, CourselikeUsageKey) (nee BlockUsageLocator)
    • LegacyLibraryUsageKey (nee LibraryUsageLocator)
• AssetKey
  • CourseRunAssetKey (nee AssetLocator)
• ContainerKey
  • LibraryContainerKey (nee LibraryContainerLocator)
• CollectionKey (nee LibraryCollectionLocator)
• CourseRunDefinitionKey (nee DefinitionLocator)

The only name I'm iffy on is "ContentUsageKey"