[WIP] Re-implementing Restore to use Pydantic (attempt 2) - #792
Draft
ormsbee wants to merge 14 commits into
Draft
Conversation
This is a re-implementation of the restore part of backup_restore, with the goal of making it more robust and maintainable in the long term.
The restore pipeline (archive -> payload -> validation -> loading) was in place but had never run end to end. This finishes it, gives it a single error model, and covers it with tests. loading.py predated the "keys are now opaque refs" rename (5d4fbf4) and its siblings, so it still passed `key=`/`local_key=` and read `.key` off models that now expose `package_ref`/`entity_ref`/`component_code`/`collection_code`. mypy reported 38 errors against this applet; it now reports none. Error handling -------------- All errors now descend from a single `BackupRestoreError` in the new errors.py. The existing `ExtractionError` classes move there and re-parent onto it, so there is no translation layer between error models. `validate()` no longer lets a raw pydantic ValidationError escape the public API, and no longer discards the extraction errors that payload.py had carefully collected. It gathers everything -- extraction errors as-is, one `SchemaError` per pydantic entry, plus consistency checks -- and reports it all at once, so someone repairing an archive by hand sees every problem in a single pass. Pydantic locations are mapped back to the archive file they came from, which is what `entity_path_mapping` was always for. The five consistency checks (unresolved child, missing draft/published version, duplicate version_num, malformed component ref, unknown container type) were each an uncaught exception in the middle of a database write. Bugs fixed along the way ------------------------ * The container union could not discriminate. All three container models default their single field and inherit `extra="allow"`, so every one of them validated every dict and the leftmost union member always won -- meaning every container would have loaded as a Unit, silently. * REF_CONSTRAINTS and CODE_CONSTRAINTS had trailing commas, making them 1-tuples, so strictness, whitespace stripping and the code regex were inert. * Restored packages were named "Temp Title"; title, description and created now come from the archive. * lp_dump writes `user.email` unconditionally and Django defaults it to "", so a backup taken by a user with no email produced an archive our own restore rejected. Blank [meta] strings now read as absent. * `[meta]` fields other than format_version were effectively required. * CollectionInput declared `created` twice and never declared `entities`, which loading.py read via `extra="allow"`. API changes ----------- `load_learning_package` now returns a `RestoreResult` and raises `RestoreFailedError`. `load_learning_package_as_dict` is a compatibility shim returning the old dict shape for callers that still expect it. api.py declares `__all__` again -- it had stopped, which leaked `zipfile`, `attrs`, `atomic` and the pipeline modules into `openedx_content.api`. lp_load now uses the new pipeline, accepts a directory as well as a .zip, and takes an optional --package-ref. lp_load2 and load_learning_package_old are gone, as is the scratch `encode` command (it called an unimported tomli_w). Retiring the old read path -------------------------- LearningPackageUnzipper, serializers.py and toml.py's parse functions are removed. The write side (LearningPackageZipper, toml.py's writers, create_zip_file) is untouched and still to be migrated. Tests ----- test_restore.py is replaced by test_loading.py, which ports every case it covered plus directory loading, the draft/published resolution matrix, static asset round-tripping, and rollback-on-failure. Two of the old tests had been failing since they patched a method that does not exist; those cases now use real fixtures instead of mocks. New test_archive.py, test_schema.py and test_validation.py; test_payload.py gains collection extraction, whole-archive assembly, and the seven fixture files that were committed empty. Assertions that sat unreachable inside `assertRaises` blocks now actually run. 33 passing / 2 failing / 2 skipped -> 139 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r backwards compatibility
Encapsulates extraction in PayloadExtractor and finishes the two TODOs that
were sitting in that class.
Wrapper-folder archives
-----------------------
`zip -r MyLib.zip MyLib` compresses the folder rather than its contents, so
the archive has a single top-level directory with package.toml inside it.
That is a perfectly reasonable thing to hand us, but we rejected it with
"Root Package file not found at expected path".
`find_archive_root` now looks one level down: ignore archiving debris
(__MACOSX, dotfiles), keep the top-level directories that actually contain a
package.toml, and re-root if exactly one qualifies. Re-rooting is done by
wrapping the filesystem in a DirFileSystem, so every path expression
downstream keeps working unchanged and is unaware anything happened.
Requiring package.toml inside the candidate is load-bearing rather than
belt-and-braces: payload_test_data/entities/ contains exactly one
subdirectory, and a rule based only on "a single top-level folder" would
silently re-root into it and break every entity test. There is a named
regression test for that.
Two details worth recording. The previous sketch used `len(fs.ls('.')) == 1`,
which could never have fired -- ls(".") returns [] on a ZipFileSystem, which
is the case it was written for; ls("") is what lists the top level. And a zip
made with macOS Finder's "Compress" carries __MACOSX and often .DS_Store
beside the folder, so any rule counting total top-level entries would fail on
the most common way a non-technical user produces an archive.
Encapsulation
-------------
The module-level extraction functions become methods, with fs held as
constructor state and root_package_path as a class attribute. This is what
makes the class the seam its docstring always claimed it was, for teams whose
archives are laid out differently -- the module docstring cites MIT DELTA,
who encode much of the Section/Subsection/Unit hierarchy in a single file.
Beyond the mechanical move:
* Duplicate detection moves from extract_entity_data up into
extract_entities_data. A duplicate is a property of the *set* of files, not
of any one file, and this drops an optional parameter that only existed to
smuggle the mapping down.
* get_collection_file_paths() mirrors get_entity_file_paths(); extract() used
to inline the glob, which is exactly the layout knowledge a subclass wants
to override.
* The "Duplicate collections are a problem too" TODO is resolved, not
deferred: schema.CompletePackageInputData.check_for_duplicate_keys already
catches them, and can name both files because it has their data by then.
Reporting the detected root
---------------------------
UnvalidatedLearningPackageInput and ValidatedLearningPackageInput carry the
folder we picked, and RestoreFailedError.as_text() emits an "Archive root:"
line when there is one. Error paths themselves stay relative to that root, so
the "fs:" static-asset pointers, entity_path_mapping and SchemaError.path all
live in a single path space.
The one thing that had to be got right: extract() returns the *re-rooted*
filesystem, because loading.py resolves static assets against the "fs:"
pointers written during extraction. Handing back the original would break
images for wrapper archives only. Tested directly, both at the payload layer
and end to end.
Tests
-----
139 -> 165 passing; payload.py reaches 100% coverage, PayloadExtractor having
been the last uncovered code in the applet. New FindArchiveRootTest covers
package.toml at the top, a single wrapper, macOS debris, a wrapper beside a
stray file, a folder with no package.toml, two ambiguous candidates, two
levels of nesting and an empty archive -- over both a zip and a directory,
since ls("") differs between them. End-to-end tests confirm a wrapper zip and
a wrapper directory restore identically to a flat archive.
folder_to_zip_path gains prefix and extra_names, so wrapper archives are built
at test time from the existing fixture rather than duplicating 20+ files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both find_archive_root and ROOT_PACKAGE_PATH are implementation details of this particular payload format, so they should be contained within this class.
Entities and Collections were checked for duplicate keys in two different
places, for a reason that was an accident of data structure rather than a
decision:
* Collections were assembled into a list, so both copies survived extraction
and CompletePackageInputData.check_for_duplicate_keys caught the duplicate
during validation, naming both files via each CollectionInput.src_path.
* Entities were assembled into a dict keyed by entity ref, so a second
definition would silently overwrite the first. PayloadExtractor therefore
had to catch it during extraction, and UnvalidatedLearningPackageInput
carried an entity_path_mapping side-channel so that later stages could work
out which file an entity came from.
Entities are now a list, each carrying its own key and src_path, and the
duplicate check sits next to the Collections one, sharing a
_reject_duplicate_keys helper. entity_path_mapping is gone, and
_collection_path_at generalizes into _src_path_at(section, index, ...) now
that both branches of _source_for_loc do the same thing.
The list is the load-bearing part, and there is a comment in schema.py saying
so: validation cannot report a duplicate that extraction has already
destroyed, and a dict keyed by ref can only ever hold one of the two. Anyone
"tidying" this back into a dict would silently remove the check.
A missing entity key moves along with it. That was only checked during
extraction because the key became a dict key and there was nowhere to put an
entity without one; it is now a required field on EntityInputData, exactly as
it is on CollectionInput.
Deleted error classes
---------------------
DuplicateFoundError and FieldMissing both lose their only caller here. Rather
than leave two untested, uncovered exception classes behind, both are removed;
they are easy to reintroduce if an extractor for some other archive layout
turns out to need them.
Path-less errors
----------------
A duplicate is reported against the whole section rather than one file --
pydantic gives a field_validator failure a loc of ("entities",), with no index
to resolve to a path -- so SchemaError.path is None and the message names both
files instead. Collections have always behaved this way.
That surfaced a latent bug in the output: BackupRestoreError.__str__
interpolated self.path unconditionally, so any path-less error rendered as
"None: entities: ..." in the restore log. Duplicate Collections have been
producing that for as long as the check has existed; nothing asserted on it.
Both __str__ methods now omit an absent path, and there are tests for it.
Tests
-----
165 -> 175 passing in the applet, with payload.py, schema.py and errors.py
staying at 100%.
test_dupes inverts rather than moves: it now asserts that both duplicates
*survive* extraction with distinct src_paths, which is precisely the property
that lets validation do its job, so it earns its place as a payload-level
test. test_missing_entity_key becomes a validation test. The helpers in
test_validation take an explicit key and build lists, dropping the
path_mapping parameter. No fixture files changed -- every archive fixture is
still valid, only where the error surfaces has moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PayloadExtractor wrapped its source filesystem in a DirFileSystem only when
the archive turned out to have a wrapper folder:
self.root = self.find_archive_root(source_fs)
self.fs = DirFileSystem(path=self.root, fs=source_fs) if self.root else source_fs
DirFileSystem already stores its root as .path, so in the wrapped case that
value existed twice. And because the wrap was conditional, nothing downstream
could rely on it being there, which is why the root was stored on
PayloadExtractor, copied onto UnvalidatedLearningPackageInput, copied again
onto ValidatedLearningPackageInput, and finally read once in api.py. Four hops
to carry something the filesystem was already holding.
We now wrap unconditionally. self.fs is a DirFileSystem in every case, fs.path
is the archive-relative root, and all three stored copies are gone.
find_archive_root returns "" rather than None for "no wrapper", so there is one
spelling of that idea instead of two.
Both input classes are typed `fs: DirFileSystem` rather than
AbstractFileSystem. That narrowing is the point: it's what makes reading .path
a stated invariant rather than an assumption about whatever happens to be
there.
Two things worth recording
--------------------------
DirFileSystem(path="", ...) raises. Its __init__ does `path = path or fo`, so
an empty string becomes None and _strip_protocol(None) fails with an
AttributeError. "/" is the value that gives an identity wrap: both
DirFileSystem and ZipFileSystem normalize it to "", after which _join and
_relpath short-circuit and every path passes through untouched. There's a
comment at the call site, because "/" reads as arbitrary otherwise.
The local-directory case is now double-wrapped, deliberately.
archive.read_fs_for_path already returns a DirFileSystem for a directory,
rooted at an absolute local path. Reusing that directly would make fs.path mean
two different things depending on how the archive arrived -- an absolute path
for a directory, "MyLib" for a zip. The extra layer is what makes fs.path
uniformly "the wrapper folder inside the archive, or empty". It costs nothing:
with an empty root both hooks return their argument immediately.
Tests
-----
Mostly mechanical, `is None` -> `== ""`. The unvalidated() helper in
test_validation now builds its filesystem the way PayloadExtractor does, so its
root parameter still reads naturally at call sites.
Added AlwaysRerootedTest, which is the guard for this whole change: it asserts
the wrap happens for a flat zip *and* a flat directory, and checks that the
source filesystem's own path is non-empty in the directory case. That last
assertion is what fails if someone reintroduces the conditional wrap as an
optimisation -- every other test in the suite would still pass while fs.path
quietly went back to meaning two different things.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ormsbee
commented
Aug 31, 2026
Comment on lines
+205
to
+207
| # Weird edge case: If you create something, never publish it, and then do a | ||
| # "reset to published state", the resulting export in Ulmo would omit the | ||
| # [entity.draft] section entirely, rather than it being an empty dictionary. |
Contributor
Author
There was a problem hiding this comment.
Note to self, make sure we're actually testing this edge case.
Contributor
Author
|
I realize that this is an enormous PR. The pipeline approach is really different from the previous implementation, and I couldn't think of a useful way to gradually phase this in. A lot of this code is tests and fixtures, but it's true that the import code itself has gone up by something like 2X. My hope is that this will make for a more robust solution in the longer run. I'm finally working on this again (Claude has been helpful with some refactoring). I need to redo Thank you. |
ormsbee
commented
Aug 31, 2026
| # is listed in the __all__ entries below. Internal helper functions that are | ||
| # private to this module should start with an underscore. If a function does not | ||
| # start with an underscore AND it is not in __all__, that function is considered | ||
| # to be callable only by other applets in the openedx_content package. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This code will import things through the existing Studio interface. It replaces the existing restore code. The backup code has not been changed as part of this PR. You can also test with:
Starting point for all the logic is in
backup_restore.api.load_learning_package.New modules:
archive.pyhandles the container archive format, though this is mostly just delegating tofsspec.payload.pyis where the extraction of TOML files happens, and things get assembled into a big dict for validation. It understands the backup archive's internal format.validation.pyis where that's gets turned into a validated Pydantic model and errors will be accumulated.schema.pydefines the actual Pydantic models. It needs a lot of validation rules (just the bare field stuff exists now).loading.pyis the logic for actually loading validated input into the database. It initializes aLoaderobject with input data and then callsloader.load_into()to push that data into a targetLearningPackage.I think I'm comfortable with this as the basic structure. I intend to keep full compatibility with the existing REST endpoint and just do conversions as needed.
One note is that much of
validator.pywas Claude-generated, along with many of the tests. I'm not at all happy with the resulting code invalidation.py, and I plan to make major revisions before I open this for review.