feat!: Package and Entity keys are now opaque refs#546
feat!: Package and Entity keys are now opaque refs#546kdmccormick wants to merge 1 commit intokdmccormick/keys-containerfrom
keys are now opaque refs#546Conversation
845c3c7 to
fbe4b4b
Compare
1fcd411 to
aba969c
Compare
0f70804 to
266d7f3
Compare
aba969c to
0b01361
Compare
The `{LearningPackage,PublishableEntity}.key` fields were always
meant to be externally-supplied refs whose format does not matter.
However, they were effectively being used as parseable psuedo-keys
for identifying containers and components, with more assumptions to their
structure being baked in over time, both within and outside openedx-core.
This commit renames them to `{package,entity}_ref`, and removes all
parsing of them from the API and from the internals (except for where
required for backcompat with Ulmo ZIP backups). Instead, the pairings
of (component_type,component_code) and (container_type,container_code)
are used to identify components and containers; their respective
entity_refs are derived from those *by conventional default* but also
are customizable by callers.
BREAKING CHANGE: Renamed key_field(...) -> ref_field(...) in openedx_django_lib.
BREAKING CHANGE: Renamed PublishableEntity.key -> PublishableEntityKey.entity_ref
BREAKING CHANGE: Renamed PublishableEntityMixin.key -> PublishableEntityMixin.entity_ref
BREAKING CHANGE: In create_publishable_entity(...), renamed param key -> entity_ref
BREAKING CHANGE: Renamed get_publishable_entity_by_key(...) ->
get_publishable_entity_by_ref(...), and renamed param key -> entity_ref
BREAKING CHANGE: In get_entity_collections(...), renamed param entity_key -> entity_ref
BREAKING CHANGE: In create_component(...)/create_container(...):
* Positional key argument removed
* Optional entity_ref argument added
* Defaults to defaults to "{namespace}:{type_name}:{component_code}" if unspecified
BREAKING CHANGE: Function get_or_create_component_type_by_entity_key() removed.
BREAKING CHANGE: Rename get_container_children_entities_keys(...) ->
get_container_children_entity_refs(...)
BREAKING CHANGE: Renamed get_container_by_key(...) -> get_container_by_ref(..)
and renamed param key -> entity_ref.
BREAKING CHANGE: Renamed get_container_children_entities_keys(...) ->
get_container_children_entity_refs(...)
BRAEKING CHANGE: Renamed LearningPackage.key -> LearningPackage.package_ref.
BREAKING CHANGE: In create_learning_package(...), renamed param key -> package_ref
BREAKING CHANGE: In update_learning_package(...), renamed param key -> package_ref
BREAKING CHANGE: Renamed get_learning_package_by_key(...) ->
get_learning_package_by_ref(...), and renamed param key -> package_ref
BREAKING CHANGE: In learning_package_exists(...), renamed param key -> package_ref
BREAKING CHANGE: In look_up_component_version_media(...):
* Renamed param learning_package_key -> learning_package_ref
* Renamed param component_key -> entity_ref
BREAKING CHANGE: In add_assets_to_component management command:
* Renamed argument learning_package_key -> learning_package_ref
* Renamed argument component_key -> entity_ref
BREAKING CHANGE In load_learning_package:
* Renamed param key -> package_ref
* In return dict, within sub-dict ["lp_restored_data"]:
* Renamed ["key"] -> ["package_ref"]
* Renamed ["archive_lp_key"] -> ["archive_package_ref"]
* Renamed ["archive_org_key"] -> ["archive_org_code"]
* Renamed ["archive_slug"] -> ["archive_package_code"]
* If archive_package_ref cannot be parsed into "{_prefix}:{org_code}:{package_code}", then
archvie_org_code and archive_package_code will both be None. Callers should handle this case.
(Previously, a ValueError would be raised by backup-restore code.)
Backup/Restore format changes (non-breaking):
* In [learning_package], key field is renamed -> package_ref.
* In [entity], key field is renamed -> entity_ref.
* For compatibility with Ulmo instances, both key and {package,entity}_ref
will be written in all new archives.
* For compatibility with Ulmo archives, both key and {package,entity}_ref
will be read, with priority given to the latter.
Part of: #322
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
0b01361 to
52fe205
Compare
|
|
||
| def create_subsection_and_version( | ||
| learning_package_id: LearningPackage.ID, | ||
| key: str, |
There was a problem hiding this comment.
I realize this is still in draft, but just a quick note here: The entity_ref should replace key directly here. One of the big reasons why this arg is positional-only was because we thought it was likely to get renamed at some point and wanted to make it so that we didn't have to update callers when that happened.
There was a problem hiding this comment.
@ormsbee @bradenmacdonald , I think this is something we need to align on. My thinking was that entity_ref would become an optional parameter in the Container and Component APIs, defaulting to and {container_code} and {component_type}:{component_code}, respectively. That would preserve the current convention by default (entity refs and opaque keys match) but would allow API callers to customize entity_ref if they care to.
Dave, it sounds like you'd prefer entity_ref to always be an explicit required parameter?
There was a problem hiding this comment.
I realize this is still in draft
They're only in Draft because I don't have testing instructions posted yet-- review comments are all welcome!
There was a problem hiding this comment.
Ah, I see. I have no objection then.
There was a problem hiding this comment.
would allow API callers to customize entity_ref if they care to.
When would they want to?
Unrelated, is too late to switch containers to use the convention {container_type}:{container_code} ?
There was a problem hiding this comment.
@bradenmacdonald Not too late, but I don't want to clog this PR stack up with it. I'm open to doing it in a followup PR.
There was a problem hiding this comment.
OK. I'm still wondering if we should just drop the ability to customize entity_ref. Do we have a use case in mind?
| Uses SeparateDatabaseAndState with RunSQL (RENAME COLUMN) for | ||
| SQLite/MySQL compatibility. The table is named |
There was a problem hiding this comment.
As in the other PR, I don't understand from this explanation why Django's auto-generated migration wouldn't be "SQLite/MySQL" compatible ?
Description
The
{LearningPackage,PublishableEntity}.keyfields were always meant to be externally-supplied refs whose format does not matter. However, they were effectively being used as parseable psuedo-keys for identifying containers and components, with more assumptions to their structure being baked in over time, both within and outside openedx-core.This commit renames them to
{package,entity}_ref, and removes all parsing of them from the API and from the internals (except for where required for backcompat with Ulmo ZIP backups). Instead, the pairings of (component_type,component_code) and (container_type,container_code) are used to identify components and containers; their respective entity_refs are derived from those by conventional default but also are customizable by callers.BREAKING CHANGE: Renamed key_field(...) -> ref_field(...) in openedx_django_lib.
BREAKING CHANGE: Renamed PublishableEntity.key -> PublishableEntityKey.entity_ref BREAKING CHANGE: Renamed PublishableEntityMixin.key -> PublishableEntityMixin.entity_ref BREAKING CHANGE: In create_publishable_entity(...), renamed param key -> entity_ref
BREAKING CHANGE: Renamed get_publishable_entity_by_key(...) ->
get_publishable_entity_by_ref(...), and renamed param key -> entity_ref
BREAKING CHANGE: In get_entity_collections(...), renamed param entity_key -> entity_ref BREAKING CHANGE: In create_component(...)/create_container(...):
BREAKING CHANGE: Rename get_container_children_entities_keys(...) ->
get_container_children_entity_refs(...)
BREAKING CHANGE: Renamed get_container_by_key(...) -> get_container_by_ref(..)
and renamed param key -> entity_ref.
BREAKING CHANGE: Renamed get_container_children_entities_keys(...) ->
get_container_children_entity_refs(...)
BRAEKING CHANGE: Renamed LearningPackage.key -> LearningPackage.package_ref. BREAKING CHANGE: In create_learning_package(...), renamed param key -> package_ref BREAKING CHANGE: In update_learning_package(...), renamed param key -> package_ref
BREAKING CHANGE: Renamed get_learning_package_by_key(...) ->
get_learning_package_by_ref(...), and renamed param key -> package_ref
BREAKING CHANGE: In learning_package_exists(...), renamed param key -> package_ref
BREAKING CHANGE: In look_up_component_version_media(...):
BREAKING CHANGE: In add_assets_to_component management command:
BREAKING CHANGE In load_learning_package:
Backup/Restore format changes (non-breaking):
Part of: #322
Full series of PRs:
keysare now opaquerefs#546Testing
TBC
AI disclosure
Claude helped me write this PR. See #322 for details.