From f42b6e4bb41f7a8aca37a84d25ec0917888e37ef Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 21 Aug 2025 16:17:39 -0600 Subject: [PATCH 1/3] feat: add slugified filenames with short hash suffix for entity keys --- .../apps/authoring/backup_restore/zipper.py | 10 +++-- openedx_learning/lib/dump_naming.py | 39 +++++++++++++++++++ .../authoring/backup_restore/test_backup.py | 24 ++++++------ 3 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 openedx_learning/lib/dump_naming.py diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index 5386e9aff..de74c4667 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -15,6 +15,7 @@ PublishableEntity, PublishableEntityVersion, ) +from openedx_learning.lib.dump_naming import hash_slugify_filename TOML_PACKAGE_NAME = "package.toml" @@ -73,7 +74,8 @@ def create_zip(self, path: str) -> None: entity_toml_content: str = toml_publishable_entity(entity) if hasattr(entity, 'container'): - entity_toml_filename = f"{entity.key}.toml" + entity_slugify_hash = hash_slugify_filename(entity.key) + entity_toml_filename = f"{entity_slugify_hash}.toml" entity_toml_path = entities_folder / entity_toml_filename zipf.writestr(str(entity_toml_path), entity_toml_content) @@ -87,7 +89,7 @@ def create_zip(self, path: str) -> None: # component_versions/ # v1/ # static/ - + entity_slugify_hash = hash_slugify_filename(entity.component.local_key) component_namespace_folder = entities_folder / entity.component.component_type.namespace # Example of component namespace is: "xblock.v1" self.create_folder(component_namespace_folder, zipf) @@ -96,12 +98,12 @@ def create_zip(self, path: str) -> None: # Example of component type is: "html" self.create_folder(component_type_folder, zipf) - component_id_folder = component_type_folder / entity.component.local_key # entity.key + component_id_folder = component_type_folder / entity_slugify_hash # Example of component id is: "i-dont-like-the-sidebar-aa1645ade4a7" self.create_folder(component_id_folder, zipf) # Add the entity TOML file inside the component type folder as well - component_entity_toml_path = component_type_folder / f"{entity.component.local_key}.toml" + component_entity_toml_path = component_type_folder / f"{entity_slugify_hash}.toml" zipf.writestr(str(component_entity_toml_path), entity_toml_content) # Add component version folder into the component id folder diff --git a/openedx_learning/lib/dump_naming.py b/openedx_learning/lib/dump_naming.py new file mode 100644 index 000000000..7f1b3d585 --- /dev/null +++ b/openedx_learning/lib/dump_naming.py @@ -0,0 +1,39 @@ +""" +This module provides utilities for generating safe and consistent filenames for +dumping learning package content. + +Examples +print(hash_slugify_filename("My:Component")) # → my_component_a12f4c.toml +print(hash_slugify_filename("Math/Algebra+V2")) # → math_algebra+v2_8d31a.toml +print(hash_slugify_filename("User@Library#1")) # → user@library#1_c9a0f.toml +""" +import hashlib +import re +from typing import Optional + +# Windows-invalid filename characters +INVALID_CHARS = r'<>:"/\\|?*' + + +def safe_slugify(value: str, separator: str = "_") -> str: + """ + Make a filesystem-safe slug: + - Lowercase + - Replace only invalid filename characters with separator + - Collapse multiple separators + - Strip leading/trailing separators + """ + value = value.lower() + value = re.sub(f"[{re.escape(INVALID_CHARS)}]", separator, value) + value = re.sub(f"{separator}+", separator, value) # collapse duplicates + return value.strip(separator) + + +def hash_slugify_filename(identifier: str, ext: Optional[str] = "") -> str: + """ + Hybrid filename = slugified identifier + short hash. + Keeps most special characters intact except filesystem-invalid ones. + """ + slug = safe_slugify(identifier) + short_hash = hashlib.sha1(identifier.encode("utf-8")).hexdigest()[:5] + return f"{slug}_{short_hash}{ext}" diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index a40bc3c99..0d9719dfb 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -117,17 +117,17 @@ def check_zip_file_structure(self, zip_path: Path): "collections/", "entities/xblock.v1/", "entities/xblock.v1/html/", - "entities/xblock.v1/html/my_draft_example.toml", - "entities/xblock.v1/html/my_draft_example/", - "entities/xblock.v1/html/my_draft_example/component_versions/", - "entities/xblock.v1/html/my_draft_example/component_versions/v2/", - "entities/xblock.v1/html/my_draft_example/component_versions/v2/static/", + "entities/xblock.v1/html/my_draft_example_ac6d7.toml", + "entities/xblock.v1/html/my_draft_example_ac6d7/", + "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/", + "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/v2/", + "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/v2/static/", "entities/xblock.v1/problem/", - "entities/xblock.v1/problem/my_published_example/", - "entities/xblock.v1/problem/my_published_example.toml", - "entities/xblock.v1/problem/my_published_example/component_versions/", - "entities/xblock.v1/problem/my_published_example/component_versions/v1/", - "entities/xblock.v1/problem/my_published_example/component_versions/v1/static/", + "entities/xblock.v1/problem/my_published_example_fff05/", + "entities/xblock.v1/problem/my_published_example_fff05.toml", + "entities/xblock.v1/problem/my_published_example_fff05/component_versions/", + "entities/xblock.v1/problem/my_published_example_fff05/component_versions/v1/", + "entities/xblock.v1/problem/my_published_example_fff05/component_versions/v1/static/", ] # Check that all expected paths are present @@ -166,7 +166,7 @@ def test_lp_dump_command(self): # Check the content of the entity TOML files expected_files = { - "entities/xblock.v1/problem/my_published_example.toml": [ + "entities/xblock.v1/problem/my_published_example_fff05.toml": [ '[entity]', f'uuid = "{self.published_component.uuid}"', 'can_stand_alone = true', @@ -175,7 +175,7 @@ def test_lp_dump_command(self): '[entity.published]', 'version_num = 1', ], - "entities/xblock.v1/html/my_draft_example.toml": [ + "entities/xblock.v1/html/my_draft_example_ac6d7.toml": [ '[entity]', f'uuid = "{self.draft_component.uuid}"', 'can_stand_alone = true', From 89e579c7c1053f496e7b35e51ed79c34d9f7ace7 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 22 Aug 2025 13:24:33 -0600 Subject: [PATCH 2/3] refactor: improve slugify_hashed_filename function based on review comments --- .../apps/authoring/backup_restore/zipper.py | 33 ++++++++++++++-- openedx_learning/lib/dump_naming.py | 39 ------------------- .../authoring/backup_restore/test_backup.py | 24 ++++++------ .../backup_restore/test_slug_hash.py | 37 ++++++++++++++++++ 4 files changed, 79 insertions(+), 54 deletions(-) delete mode 100644 openedx_learning/lib/dump_naming.py create mode 100644 tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index de74c4667..6dada7998 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -2,11 +2,13 @@ This module provides functionality to create a zip file containing the learning package data, including a TOML representation of the learning package and its entities. """ +import hashlib import zipfile from pathlib import Path from typing import Optional from django.db.models import QuerySet +from django.utils.text import slugify from openedx_learning.apps.authoring.backup_restore.toml import toml_learning_package, toml_publishable_entity from openedx_learning.apps.authoring.publishing import api as publishing_api @@ -15,11 +17,36 @@ PublishableEntity, PublishableEntityVersion, ) -from openedx_learning.lib.dump_naming import hash_slugify_filename TOML_PACKAGE_NAME = "package.toml" +def slugify_hashed_filename(identifier: str) -> str: + """ + Generate a filesystem-safe filename from an identifier. + + Why: + Identifiers may contain characters that are invalid or ambiguous + in filesystems (e.g., slashes, colons, case differences). + Additionally, two different identifiers might normalize to the same + slug after cleaning. To avoid collisions and ensure uniqueness, + we append a short blake2b hash. + + What: + - Slugify the identifier (preserves most characters, only strips + filesystem-invalid ones). + - Append a short hash for uniqueness. + - Result: human-readable but still unique and filesystem-safe filename. + """ + slug = slugify(identifier) + # Short digest ensures uniqueness without overly long filenames + short_hash = hashlib.blake2b( + identifier.encode("utf-8"), + digest_size=3 + ).hexdigest() + return f"{slug}_{short_hash}" + + class LearningPackageZipper: """ A class to handle the zipping of learning content for backup and restore. @@ -74,7 +101,7 @@ def create_zip(self, path: str) -> None: entity_toml_content: str = toml_publishable_entity(entity) if hasattr(entity, 'container'): - entity_slugify_hash = hash_slugify_filename(entity.key) + entity_slugify_hash = slugify_hashed_filename(entity.key) entity_toml_filename = f"{entity_slugify_hash}.toml" entity_toml_path = entities_folder / entity_toml_filename zipf.writestr(str(entity_toml_path), entity_toml_content) @@ -89,7 +116,7 @@ def create_zip(self, path: str) -> None: # component_versions/ # v1/ # static/ - entity_slugify_hash = hash_slugify_filename(entity.component.local_key) + entity_slugify_hash = slugify_hashed_filename(entity.component.local_key) component_namespace_folder = entities_folder / entity.component.component_type.namespace # Example of component namespace is: "xblock.v1" self.create_folder(component_namespace_folder, zipf) diff --git a/openedx_learning/lib/dump_naming.py b/openedx_learning/lib/dump_naming.py deleted file mode 100644 index 7f1b3d585..000000000 --- a/openedx_learning/lib/dump_naming.py +++ /dev/null @@ -1,39 +0,0 @@ -""" -This module provides utilities for generating safe and consistent filenames for -dumping learning package content. - -Examples -print(hash_slugify_filename("My:Component")) # → my_component_a12f4c.toml -print(hash_slugify_filename("Math/Algebra+V2")) # → math_algebra+v2_8d31a.toml -print(hash_slugify_filename("User@Library#1")) # → user@library#1_c9a0f.toml -""" -import hashlib -import re -from typing import Optional - -# Windows-invalid filename characters -INVALID_CHARS = r'<>:"/\\|?*' - - -def safe_slugify(value: str, separator: str = "_") -> str: - """ - Make a filesystem-safe slug: - - Lowercase - - Replace only invalid filename characters with separator - - Collapse multiple separators - - Strip leading/trailing separators - """ - value = value.lower() - value = re.sub(f"[{re.escape(INVALID_CHARS)}]", separator, value) - value = re.sub(f"{separator}+", separator, value) # collapse duplicates - return value.strip(separator) - - -def hash_slugify_filename(identifier: str, ext: Optional[str] = "") -> str: - """ - Hybrid filename = slugified identifier + short hash. - Keeps most special characters intact except filesystem-invalid ones. - """ - slug = safe_slugify(identifier) - short_hash = hashlib.sha1(identifier.encode("utf-8")).hexdigest()[:5] - return f"{slug}_{short_hash}{ext}" diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index 0d9719dfb..b51b3e12a 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -117,17 +117,17 @@ def check_zip_file_structure(self, zip_path: Path): "collections/", "entities/xblock.v1/", "entities/xblock.v1/html/", - "entities/xblock.v1/html/my_draft_example_ac6d7.toml", - "entities/xblock.v1/html/my_draft_example_ac6d7/", - "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/", - "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/v2/", - "entities/xblock.v1/html/my_draft_example_ac6d7/component_versions/v2/static/", + "entities/xblock.v1/html/my_draft_example_af06e1.toml", + "entities/xblock.v1/html/my_draft_example_af06e1/", + "entities/xblock.v1/html/my_draft_example_af06e1/component_versions/", + "entities/xblock.v1/html/my_draft_example_af06e1/component_versions/v2/", + "entities/xblock.v1/html/my_draft_example_af06e1/component_versions/v2/static/", "entities/xblock.v1/problem/", - "entities/xblock.v1/problem/my_published_example_fff05/", - "entities/xblock.v1/problem/my_published_example_fff05.toml", - "entities/xblock.v1/problem/my_published_example_fff05/component_versions/", - "entities/xblock.v1/problem/my_published_example_fff05/component_versions/v1/", - "entities/xblock.v1/problem/my_published_example_fff05/component_versions/v1/static/", + "entities/xblock.v1/problem/my_published_example_386dce/", + "entities/xblock.v1/problem/my_published_example_386dce.toml", + "entities/xblock.v1/problem/my_published_example_386dce/component_versions/", + "entities/xblock.v1/problem/my_published_example_386dce/component_versions/v1/", + "entities/xblock.v1/problem/my_published_example_386dce/component_versions/v1/static/", ] # Check that all expected paths are present @@ -166,7 +166,7 @@ def test_lp_dump_command(self): # Check the content of the entity TOML files expected_files = { - "entities/xblock.v1/problem/my_published_example_fff05.toml": [ + "entities/xblock.v1/problem/my_published_example_386dce.toml": [ '[entity]', f'uuid = "{self.published_component.uuid}"', 'can_stand_alone = true', @@ -175,7 +175,7 @@ def test_lp_dump_command(self): '[entity.published]', 'version_num = 1', ], - "entities/xblock.v1/html/my_draft_example_ac6d7.toml": [ + "entities/xblock.v1/html/my_draft_example_af06e1.toml": [ '[entity]', f'uuid = "{self.draft_component.uuid}"', 'can_stand_alone = true', diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py b/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py new file mode 100644 index 000000000..e2d9e3681 --- /dev/null +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py @@ -0,0 +1,37 @@ +""" +Test cases for the slugify_hashed_filename function. + +These tests ensure consistent and predictable behavior when +generating slugified, hash-based filenames. +""" + +from openedx_learning.apps.authoring.backup_restore.zipper import slugify_hashed_filename +from openedx_learning.lib.test_utils import TestCase + + +class SlugHashTestCase(TestCase): + """ + Test the slugify_hashed_filename function. + """ + + def test_slugify_hashed_filename(self): + # Test the slugify_hashed_filename function + self.assertEqual(slugify_hashed_filename("my_example"), "my_example_880989") + + def test_slugify_hashed_filename_special_chars(self): + # Test the slugify_hashed_filename function with special characters + self.assertEqual(slugify_hashed_filename("my@ex#ample!"), "myexample_3366b5") + + def test_slugify_hashed_filename_long_string(self): + # Test the slugify_hashed_filename function with a long string + long_string = "a" * 100 + self.assertEqual(slugify_hashed_filename(long_string), f"{long_string}_4e84b3") + + def test_slugify_hashed_filename_case_insensitivity(self): + # Test the slugify_hashed_filename function for case insensitivity + self.assertEqual(slugify_hashed_filename("My_Example"), "my_example_4f859c") + self.assertEqual(slugify_hashed_filename("MY_EXAMPLE"), "my_example_49be65") + self.assertEqual(slugify_hashed_filename("my_example"), "my_example_880989") + self.assertEqual(slugify_hashed_filename("mY_eXamPle"), "my_example_d28c02") + self.assertEqual(slugify_hashed_filename("My_ExAmPlE"), "my_example_79232e") + self.assertEqual(slugify_hashed_filename("mY_EXAMPLE"), "my_example_b91dc0") From 1991fa885b938c34e7ffba558280eb07d0160f4d Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Mon, 25 Aug 2025 11:43:40 -0600 Subject: [PATCH 3/3] test: add edge cases for unicode and forbidden filesystem characters --- .../apps/authoring/backup_restore/zipper.py | 4 +- .../backup_restore/test_slug_hash.py | 51 ++++++++++++++++--- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index 6dada7998..1443b63f3 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -38,11 +38,11 @@ def slugify_hashed_filename(identifier: str) -> str: - Append a short hash for uniqueness. - Result: human-readable but still unique and filesystem-safe filename. """ - slug = slugify(identifier) + slug = slugify(identifier, allow_unicode=True) # Short digest ensures uniqueness without overly long filenames short_hash = hashlib.blake2b( identifier.encode("utf-8"), - digest_size=3 + digest_size=3, ).hexdigest() return f"{slug}_{short_hash}" diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py b/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py index e2d9e3681..292d08af1 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_slug_hash.py @@ -22,6 +22,47 @@ def test_slugify_hashed_filename_special_chars(self): # Test the slugify_hashed_filename function with special characters self.assertEqual(slugify_hashed_filename("my@ex#ample!"), "myexample_3366b5") + def test_slugify_hashed_filename_invalid_characters_common_filesystems(self): + # Test the slugify_hashed_filename function with invalid characters for common filesystems + self.assertEqual( + slugify_hashed_filename("xblock.v1:problem:my_component"), "xblockv1problemmy_component_d346b1" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1/problem/my_component"), "xblockv1problemmy_component_2648c6" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1?problem?my_component"), "xblockv1problemmy_component_12a86d" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1\\problem_[my_component]"), "xblockv1problem_my_component_a59bb3" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1>problem_>my_component"), "xblockv1problem_my_component_8497eb" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1*problem*EndsWith|"), "xblockv1problemendswith_b88aab" + ) + self.assertEqual( + slugify_hashed_filename("xblock.v1*problem*EndsWith."), "xblockv1problemendswith_7fa0cf" + ) + + def test_slugify_hashed_filename_unicode(self): + # Test the slugify_hashed_filename function with unicode characters + + # Example with accents + self.assertEqual(slugify_hashed_filename("café"), "café_07f4df") + self.assertEqual(slugify_hashed_filename("naïve"), "naïve_308c7e") + + # Example with non-latin (e.g., Japanese) + identifier = "テスト用" + result = slugify_hashed_filename(identifier) + self.assertEqual(result, "テスト用_be48ab") + + # Example with mixed characters + identifier = "café_テスト用" + result = slugify_hashed_filename(identifier) + self.assertEqual(result, "café_テスト用_3cf9ef") + def test_slugify_hashed_filename_long_string(self): # Test the slugify_hashed_filename function with a long string long_string = "a" * 100 @@ -29,9 +70,7 @@ def test_slugify_hashed_filename_long_string(self): def test_slugify_hashed_filename_case_insensitivity(self): # Test the slugify_hashed_filename function for case insensitivity - self.assertEqual(slugify_hashed_filename("My_Example"), "my_example_4f859c") - self.assertEqual(slugify_hashed_filename("MY_EXAMPLE"), "my_example_49be65") - self.assertEqual(slugify_hashed_filename("my_example"), "my_example_880989") - self.assertEqual(slugify_hashed_filename("mY_eXamPle"), "my_example_d28c02") - self.assertEqual(slugify_hashed_filename("My_ExAmPlE"), "my_example_79232e") - self.assertEqual(slugify_hashed_filename("mY_EXAMPLE"), "my_example_b91dc0") + upper_case_value = slugify_hashed_filename("MY_EXAMPLE") + lower_case_value = slugify_hashed_filename("my_example") + # The values should be different even though they are the same but with different cases + self.assertNotEqual(upper_case_value, lower_case_value)