Backup and Restore Final Adjustments by dwong2708 · Pull Request #406 · openedx/openedx-core

dwong2708 · 2025-10-04T17:33:49Z

Resolves: #386

Description

This PR introduces improvements to the validation process for restoring learning packages, along with minor adjustments to the backup process.

Changes

Backup

Removed uuid field
Removed [entity.container] section from component TOML files

Restore

Improved validation logic
Added a learning package serializer to align with the pattern of extract → validate → save
Introduced a structured response for the load API to provide clearer information to the restore endpoint
Added a preliminary dump file validation to check file structure before restoring, enabling early validation utilities prior to invoking the async task

openedx-webhooks · 2025-10-04T17:33:54Z

Thanks for the pull request, @dwong2708!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

ormsbee · 2025-10-09T02:30:19Z

-            load_dump_zip_file(file_name)
-            message = f'{file_name} loaded successfully'
+            start_time = time.time()
+            response = load_dump_zip_file(file_name)


"response" might be misinterpreted to be related to HTTP request/response. If there's no better, more specific thing to call this, we can call this result for now. But load_dump_zip_file is also confusing function name, because "load" and "dump" are often opposite of each other (e.g. json.load(), json.dump())

Right. I have adjusted. Thanks

ormsbee · 2025-10-09T04:38:40Z

+            return {
+                "status": "error",
+                "log_file_error": self._write_errors(),  # return a StringIO with the errors
+                "general_info": None
+            }


It would be useful to define an actual data structure here to return, like a dataclass.

It's also not clear what "general_info" means in this context.

I agree. I've added it. Thanks

ormsbee · 2025-10-09T04:40:36Z

@@ -425,12 +429,23 @@ def __init__(self, zipf: zipfile.ZipFile) -> None:
    @transaction.atomic
    def load(self) -> dict[str, Any]:


We're going to want an arg here to allow the target key of the LearningPackage to be passed in here.

Though that can be a separate PR if you want.

Might also be an optional param to the init() instead.

Trying to do this before this PR can be merged. Otherwise I can do other PR.

According to this comment the staged key generation logic was added.

ormsbee · 2025-10-09T04:42:05Z

+            "general_info": {
+                "learning_package_key": learning_package.key,
+                "learning_package_title": learning_package.title,
+                "containers": num_containers,


We're probably going to want the breakdown of sections/subsections/units here, not just total containers. I think you can just leave this out until we get UX guidance.

ormsbee · 2025-10-09T04:44:36Z

        }

+    def preliminary_check(self) -> Tuple[list[dict[str, Any]], dict[str, Any]]:
+        """Performs a preliminary check of the zip file structure and mandatory files."""


If this function is only doing a check for the existence of the package file, then please rename it accordingly. The term "preliminary check" is overly vague.

ormsbee · 2025-10-09T04:48:25Z

        for valid_draft in containers.get("unit_drafts", []):
            entity_key = valid_draft.pop("entity_key")
+            version_num = valid_draft["version_num"]  # Should exist, validated earlier
+            entity_version_identifier = f"{entity_key}__v{version_num}"


Please use tuples instead, so that other code later doesn't have to try to parse it back out. Tuples work fine as dict keys.

Changes have been applied. Thanks

ormsbee · 2025-10-09T04:51:54Z

            entity_key = valid_draft.pop("entity_key")
+            version_num = valid_draft["version_num"]  # Should exist, validated earlier
+            entity_version_identifier = f"{entity_key}__v{version_num}"
+            if entity_version_identifier in self.all_published_entities_versions:


You could make a helper method here and avoid having to comment about the same pattern multiple times, e.g.

Suggested change

if entity_version_identifier in self.all_published_entities_versions:

if self.version_already_exists(entity_version_identifier): # then the comment goes in the helper

…omponents TOML files

- Added dataclass to represent load process result - Improved docstrings - Simplified _is_version_already_exists logic

ormsbee

A few minor requests to adjust the interface and try to reduce edge cases.

Thank you!

ormsbee · 2025-10-14T12:50:35Z



-def load_dump_zip_file(path: str) -> None:
+def load_library_from_zip(path: str, user: UserType | None = None, use_staged_lp_key: bool = False) -> dict:


A few things on this:

This is a LearningPackage, not a Library. The distinction isn't important now, but it will be once we start storing courses in LC in the coming months. load_learning_package() is fine.

Instead of making it take a used_stage_lp_key argument, please have it take a key arg and generate if key==None. The default behavior of this function should not trust the key that was dumped in the archive itself, and it will be a potential security issue if people do trust that later on.

So maybe something like:

Suggested change

def load_library_from_zip(path: str, user: UserType | None = None, use_staged_lp_key: bool = False) -> dict:

def load_learning_package(path: str, key: str = None, user: UserType | None = None) -> dict:

With this update, we’ll have two cases:

Key provided: The user doesn’t need to generate the staged key.

Key not provided: The user must generate the staged key.

Changes applied.

ormsbee · 2025-10-14T13:01:18Z

+
+    The timestamp at the end ensures the key is unique.
+    """
+    username = user.username if user else DEFAULT_USERNAME


Don't use a DEFAULT_USERNAME because "command" (or anything like that) is a valid username that someone could plausibly pick. It may be that nothing you have here is a security concern, but at some point, someone is likely to add something that assumes that a user has access to things that are under their username and won't check this edge case. It's also likely to not get cleaned up properly.

Honestly, I'd just force the user to put in a username when invoking the management command--it will reduce edge cases, and if someone has shell access to run management commands, they basically have access to everything anyway.

ormsbee · 2025-10-14T13:02:58Z

+    Generate a staged learning package key based on the given base key.
+
+    Arguments:
+        lp_key (str): The base key of the learning package.


We should specify that this is the archive's LearningPackage key. Maybe even call it archive_lp_key to make sure it's not confused.

ormsbee · 2025-10-14T13:06:47Z

+
+    _, org_slug, lp_slug = parts[:3]
+    timestamp = int(time.time() * 1000)  # Current time in milliseconds
+    return f"lib-restore:{username}:{org_slug}:{lp_slug}:{timestamp}"


Since this temp name is coming from LC now instead of from libraries code (when I initially sketched it out), please make a prefix that's not "lib", so something like lp-restore.

Applied. Thanks

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 4, 2025

openedx-webhooks added this to Contributions Oct 4, 2025

github-project-automation Bot moved this to Needs Triage in Contributions Oct 4, 2025

dwong2708 marked this pull request as ready for review October 5, 2025 19:01

dwong2708 requested a review from ormsbee October 5, 2025 19:01

mphilbrick211 added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label Oct 6, 2025

mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Oct 6, 2025

wgu-taylor-payne mentioned this pull request Oct 7, 2025

feat: add library restore endpoint openedx/openedx-platform#37439

Merged

ormsbee requested changes Oct 9, 2025

View reviewed changes

ormsbee moved this from Ready for Review to In Eng Review in Contributions Oct 9, 2025

dwong2708 requested a review from ormsbee October 10, 2025 17:06

dwong2708 added 6 commits October 10, 2025 11:48

fix: remove uuid from TOML files and unnecessary container table in c…

2ccf7d2

…omponents TOML files

test: remove uuid verification from backup test

2c485e1

feat: add success and error responses to the restore process

9932939

feat: improve validation for restore process inputs

1f74f97

fix: handle case when draft and published versions are the same

77cf6be

feat: introduce dataclass for load result and refactor version check

0931130

- Added dataclass to represent load process result - Improved docstrings - Simplified _is_version_already_exists logic

dwong2708 force-pushed the dwong2708/backup_restore_lib_adjustments branch from b25c186 to 0931130 Compare October 10, 2025 17:48

dwong2708 added 2 commits October 10, 2025 11:56

test: fix test_error_preliminary_check test

04c514c

feat: include staged key generation in the load process

2d328a3

ormsbee requested changes Oct 14, 2025

View reviewed changes

feat: extend load process with updated input and output fields

43e5159

dwong2708 requested a review from ormsbee October 16, 2025 05:34

ormsbee approved these changes Oct 16, 2025

View reviewed changes

ormsbee merged commit 8e71a58 into openedx:main Oct 16, 2025
11 checks passed

github-project-automation Bot moved this from In Eng Review to Done in Contributions Oct 16, 2025

ormsbee mentioned this pull request Dec 30, 2025

Unify smaller apps into one openedx_content app #454

Merged

		@@ -425,12 +429,23 @@ def __init__(self, zipf: zipfile.ZipFile) -> None:
		@transaction.atomic
		def load(self) -> dict[str, Any]:

	if entity_version_identifier in self.all_published_entities_versions:
	if self.version_already_exists(entity_version_identifier): # then the comment goes in the helper



		def load_dump_zip_file(path: str) -> None:
		def load_library_from_zip(path: str, user: UserType \| None = None, use_staged_lp_key: bool = False) -> dict:

	def load_library_from_zip(path: str, user: UserType \| None = None, use_staged_lp_key: bool = False) -> dict:
	def load_learning_package(path: str, key: str = None, user: UserType \| None = None) -> dict:

Conversation

dwong2708 commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Backup

Restore

Uh oh!

openedx-webhooks commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwong2708 Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dwong2708 commented Oct 4, 2025 •

edited

Loading

openedx-webhooks commented Oct 4, 2025 •

edited

Loading

dwong2708 Oct 10, 2025 •

edited

Loading