Module import from a URL and through Content Hub (#10 part 2c) #90
No reviewers
Labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
schaefera/utStudyBIble!90
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/module-import"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Part of #10 (part 2c).
What
DataPathis where another installed module's data is, so no package replaces another module's files.qml/controls/TextEntry); page bars get a ⋮ menu. Newcontent_exchangeAppArmor policy group.Blast radius
Tier: trunk
Trunk paths touched:
core/src/db/(migration 004),core/src/tasks/,sword/src/install*(package, package/data_path, http),qt/glue,utstudybible.apparmor,tools/e2e/Testing tier: elevated (migration, downloads, writes to the module folder), also: standard UI
Gate / rollback: Migrations are forward-only;
app.sqlite3is backed up (VACUUM INTO) before migration 004. Rollback: reinstall the previous.clickplus that backup.Proof
e0ae57f: all 6 success (rust: fmt, clippy, tests+coverage, release, deny, audit; app: lint, -Werror, clang-tidy, mem-check; e2e incl.import-url; click; presets). First e2e attempt failed one unrelated case (typed-reference menu tap), green on re-run: #98.cargo makelocally one0ae57f: green, 87 sword + 234 core tests.check_sizeremoved → 1 test fails);check_urlallowing http → 2 fail;valid_nameaccepting a leading dot → 1 fails; clash check disabled → the sword and core clash tests fail.an_import_killed_mid_download_goes_on_from_its_part). The clash fix is Rust-only and not tried on the phone.Design notes
Staged::any+commitinstalls a zip of an unknown module (one conf, name from the conf, same checks);commitfirst refuses aDataPathoverlapping another installed conf's (DataPath::overlaps, now inpackage/data_path.rs).source, nullable module),Command::ImportUrl/ImportFile, resumable; the file is copied to<cache>/importson the tasks thread before it is queued.PageBar.menu(⋮, Lucideimport/refresh-cwicons); shared controls live inqml/controls/.Review
Reviewer: fresh-context
code-reviewat high (282) + delta/re-reviews at medium (283–286): 10 findings, 7 fixed, 3 declined (283-3: covered by the http layer's cut tests; 286-3: mirrorsowns, no CrossWire pair affected; 286-4: an unparseable conf is no module our reader opens), 0 deferred, 2 fixed after the delta review (286-1, 286-2 ine0ae57f), 0 left open. The first audit's missed defect (import over another module's data) is fixed in681ac3e.Review-Confidence: 70/100, review of the clash fix stopped at
681ac3e(e0ae57fis unreviewed trunk) and 286-4's skip path has no test.Confidence by part
Staged::commit,DataPath::overlaps): medium; tested, the CrossWire check found no false refusals, bute0ae57fhad no reviewer. Readpackage.rsclash().import-url, owner's phone and desktop checks).🤖 Generated with Claude Code
Review level: high (trunk). Reviewed commit:
25057629fb. Range: origin/main...HEAD.Files read in context: core/src/db.rs, db/004_task_import.sql, db/tasks.rs, paths.rs, tasks.rs, tasks/exec.rs, tasks/source.rs, tasks/progress.rs, sword/src/install/package.rs, qt/src/installer.rs, qt/src/module_manager.rs, qml/platform/FileImport.qml, qml/platform/ut/FileImport.qml, ModulesPage.qml (import parts), utstudybible.apparmor, tools/e2e/cases/import-url.sh. Docs, po and tests skimmed.
No high or medium defects found. Checked: migration 004 table rebuild keeps rows, CHECKs and both unique indexes; resume after a kill between set_task_module and commit (staging is rebuilt from the kept zip or copy); cancel before and after the Named event; retry of a named failed import by its source; orphan sweep of
~import-Nfiles and the imports folder; http only allowed in a dev build with the test repo; Staged::any name and DataPath checks.Finding outside the diff (low):
Re the low finding (sword/src/install/http.rs, no byte cap on ImportUrl): fixed in
3db8f6d. A download that announces more than 1 GiB (Content-Range total or Content-Length) is now refused as a fatal protocol error before any body byte is stored. 1 GiB matches the unpack limit, so no valid module zip is affected. Test: an_answer_larger_than_the_cap_is_refused_before_any_byte_is_stored.cargo makeis green. A server that sends a body beyond its announced length is already bounded by the per-chunk read ofwantbytes.Phone try (docs/09). PR head
3db8f6dis installed on the Plinius over USB. The app starts: the journal showsfeat/module-import@3db8f6d18and no AppArmor denials. I putASV.zip(CrossWire rawzip, 3.3 MB) in~/Downloadsfor the file test.Checklist (footer menu → Modules → Import):
https://crosswire.org/ftpmirror/pub/sword/packages/rawzip/YLT.zipand tap Import. A row "Importing …" shows progress, then YLT is installed and listed in the footer's Bible switcher.https://crosswire.org/ftpmirror/pub/sword/packages/rawzip/KJV.zip. It is refused with "KJV is already installed; remove it first", and KJV still opens.https://crosswire.org/ftpmirror/pub/sword/packages/rawzip/NOPE.zip. The row fails with a plain error and offers Retry and Cancel; Cancel removes it.Downloads/ASV.zip. ASV installs and opens.🤖 Generated with Claude Code
Review level: medium (delta + previously unreviewed test files).
Reviewed commit:
3db8f6d183Range: 2505762..3db8f6d, plus origin/main...HEAD for the four trunk test files.
Files reviewed: sword/src/install/http.rs, sword/src/install/http/tests.rs, sword/src/install/package/tests.rs, core/src/db/tasks/tests.rs, core/src/tasks/tests.rs (plus core/src/tasks/exec.rs import_file/enqueue_import for ordering).
Findings
MEDIUM - sword/src/install/http/tests.rs:346. The cap test calls check_size() directly. Nothing exercises it through download(), so deleting either call (http.rs:392 on the 206 path, http.rs:423 on the 200 path) leaves every test green. The name says "refused before any byte is stored", but no test checks that. Scenario: a later refactor of accept_range/accept_whole drops the check, a server announces a 5 GiB Content-Length, and the whole file is written to disk with CI green. Add two server tests, one for a 206 with Content-Range total > cap and one for a 200 with Content-Length > cap. The body should be small or absent. Assert Error::Protocol and no .part bytes.
LOW - sword/src/install/http.rs:342-356 (ordering). For a 200 whose Content-Length is above the chunk size, a second GET with a whole_timeout(total) budget is issued before accept_whole runs check_size. A server that ignores Range and announces more than the cap is therefore asked twice, with a recv budget of total/SLOWEST seconds, before it is refused. Call check_size(total) in the
if let (200, Some(total))block before re-requesting.LOW - core/src/tasks/tests.rs:747-775 (an_import_killed_mid_download_goes_on_from_its_part). The "kill" is runner.pause() followed by drop, a graceful stop that syncs the part file. It does not cover a write cut off mid-chunk. The resume check (bytes={len}-) is real. The file name also hard-codes task id 1 (~import-1.zip.part).
LOW - core/src/tasks/tests.rs:743.
assert!(n >= 1)on the return of send() is vacuous. The second refusal (ftp://) only waits for any ImportRefused and does not check its reason.Checked and fine: migration 004 test (data kept, .bak-v3, constraint rebuild); the already-installed refusal (bytes are compared against a different-size import, so it can fail); copy-before-queue (exec.rs:370-374 copies and syncs before enqueue_import, and the test checks that the copy survives deleting the original and a restart); the bad-zip cases (each fails on a distinct Staged::any rule); package limits and name tests.
Replies to the delta review (review 283). Fixes are in
4e3ff2a.check_size: fixed. The cap is nowOptions::max_download, defaulting to 1 GiB. Three tests rundownload()against a 4000- or 500-byte cap: a 206 Range answer, a 200 whole answer larger than a chunk, and a 200 within one chunk. Each assertsError::Protocol, an empty or absent.partand no destination file. A fourth test downloads a file exactly at the cap. Fails without the fix: I removed each of the threecheck_sizecalls in turn, and each time one test failed (25 passed; 1 failed).a_whole_answer_larger_than_the_cap_is_refused_without_asking_againasserts a single request.cut_next_after). This test is about the task layer resuming a URL import from its part. The part name is the task's key, which is what the test checks.assert!(n >= 1)and an unchecked ftp:// reason: fixed. The useless assert is gone, and both refusals now check their sentence. Doing that showed that ftp:// gets "That is not a web address.", which is the intended reason.cargo makepasses (233 core and 83 sword tests).Level: medium re-review (answers review 283).
Reviewed commit:
4e3ff2a996Range: 3db8f6d..4e3ff2a
Files reviewed: sword/src/install.rs, sword/src/install/http.rs (with chunk, accept_range, accept_whole, copy_body, run), sword/src/install/http/tests.rs, core/src/tasks/tests.rs; grep of other Options users (core/src/catalog, core/src/tasks.rs, qt/, tools/, spikes/).
Findings: none (no defects with a concrete failure scenario).
Checked:
Not run: cargo build/test (read-only review).
Feedback:
Copy-pasting from the browser doesn't work: I get no option to paste when I long press, therefore I can't test 1-3.
5 works. Didn't test 6.
Thanks. There was no paste because Controls 2 text fields have no edit menu, and the phone keyboard has no paste key. In
c02b581, pressing and holding a text field opens a menu with Cut, Copy, Paste and Select all. Import's address field, the module search and the typed reference in Go to all have it, with German strings.c02b581is installed on the phone. Please open the app from the launcher: starting it over adb can't reach the display. Then retry steps 1–3:Could you also confirm step 4: did picking
Downloads/ASV.zipthrough "Choose a file…" install ASV?All three checks succeeded.
File picker also successfully came up in the 4th case.
Additional feedback: multiple text options in our limited space menus (in this case: Import Refresh) are ugly. Since we don't work with multiple bars, we likely need another context menu. Other UI/UX suggestions would be good here if anything comes to mind.
Thanks, all three checks and the picker passing is great.
Import / Refresh → a ⋮ menu (
c1e080c).PageBarnow has a shared ⋮ page menu. A page passesmenu: [MenuItem…]and the ⋮ shows last in the bar, one touch target wide, the same as on the reading pane. The Modules bar now reads‹ Back · Modules · ⋮, and the menu has Import a module… and Refresh the lists (greyed while loading), each touch-height. Any page that gets a second action later uses the same menu, so this is the first piece of #91. The e2e cases open it with the samee2e_menu <name>as the reading pane's. import-url, module-manager and modules-offline pass, and I looked at the shots (the menu opens above the bar at the right edge).Other UX ideas (not in this PR; say which you want and I'll file them):
Desktop preview: good idea, noted as a working rule. Before the next UI change reaches CI or the phone, I'll start
clickable desktophere so you can look at it first. The phone install stays for what only shows there (Suru, AppArmor).Note: locally, mem-check went over its 165 MB start-up budget, but an interleaved A/B showed the base QML over by the same amount (166–173 MB vs 167–169 MB), so the host was busy. The change doesn't load at start-up (pages load when pushed), and CI's app job passed mem-check on
edcaee5.Delta review (medium), commit
97f46c23a5, range 4e3ff2a..97f46c2No high, medium or low findings. Checked: PageBar menu alias/Menu.count, the pageMenu e2e log lines against e2e_menu/e2e_tap_menu, removal of the "modules importbtn/refresh" report lines (no remaining consumers), qrc entries for the icons and controls/qmldir + TextEntry.qml, TextEntry (pressAndHold, canPaste), po/pot msgids against qsTr use, docs and NOTICES claims.
Files reviewed:
NOTICES.md
assets/icons/import.svg
assets/icons/refresh-cw.svg
docs/04-features.md
docs/06-sources.md
po/de.po
po/utstudybible.schaefera.pot
qml/controls/TextEntry.qml
qml/controls/qmldir
qml/icons/import.png
qml/icons/refresh-cw.png
qml/pages/ModulesPage.qml
qml/pages/PageBar.qml
qml/pages/PickerPage.qml
qml/qml.qrc
tools/e2e/cases/import-url.sh
tools/e2e/lib.sh (context)
qml/pages/ReadingPane.qml (context)
Delta review, level medium
Commit:
681ac3ebf1Range: 97f46c2..681ac3e
No high or medium findings. The clash check runs in commit, so it covers downloads and imports. A kill mid-commit and a reinstall are handled: the module's own conf is skipped by case-insensitive name, and the conf is written last. Three low findings, two inline and one below.
Files reviewed: core/src/tasks/tests.rs, docs/04-features.md, sword/src/install/package.rs, sword/src/install/package/data_path.rs, sword/src/install/package/tests.rs, core/src/tasks/exec.rs (callers), CLAUDE.md
Low, outside the diff:
@ -55,0 +109,4 @@fn clash(&self, sword: &Path) -> Result<Option<String>> {let Ok(entries) = fs::read_dir(sword.join("mods.d")) else { return Ok(None) };for entry in entries {let path = entry?.path();low ·
entry?aborts the whole install with an I/O error if one directory entry is unreadable, while an unreadable or unparseable conf is skipped (.ok()). The error handling is inconsistent. Treat an unreadable entry as a skip, or an unreadable conf as an error.@ -55,0 +110,4 @@let Ok(entries) = fs::read_dir(sword.join("mods.d")) else { return Ok(None) };for entry in entries {let path = entry?.path();if path.extension().is_none_or(|e| e != "conf") {low · The
confextension test is case-sensitive, so an installedFOO.CONFis never checked for a clash.installed()andconf_pathuse lower-case names, so this only affects confs placed by other tools.@ -0,0 +96,4 @@(None, Some(tree)) => self.folder().starts_with(tree),(None, None) => match (self, other) {(DataPath::Prefix { dir: a, stem: s }, DataPath::Prefix { dir: b, stem: t }) => {a == b && (s.starts_with(t.as_str()) || t.starts_with(s.as_str()))low · In a shared folder, stems
strongsandstrongsgreekcount as overlapping and the later install is refused.remove_dataonly deletesstemandstem.*, so removal would not clash. This is consistent withowns, which matches by stem prefix. It could wrongly refuse a legitimate pair of modules whose stems share a prefix in one shared folder. Worth a comment, or a check against CrossWire's real confs.Audit (score 60) missed defect — an imported zip whose
DataPathnames an installed module's folder was renamed over it: fixed in681ac3e.Staged::commitrefuses, nothing moved, when another installed conf'sDataPathoverlaps (installs from a repo get it too). Tests: sworda_package_whose_data_is_where_an_installed_modules_is_is_refused(Dir, the audit's prefix case, sub-folder), corean_import_whose_data_is_where_an_installed_modules_is_is_refused; both fail with the check disabled. Checked for false refusals: CrossWire main + Beta, 434 confs, 0 overlapping pairs under the same rule.Review 286 (delta,
681ac3e):e0ae57f: every unreadable entry/conf is passed over alike..CONFcase — fixed ine0ae57f, testa_conf_another_tool_named_in_capitals_is_still_seen.owns, which lets a package with stemstrongsshipstrongsgreek.dat, so treating them as overlapping is what keeps that from replacing files; the CrossWire check above shows no real pair is refused.WIP: Module import from a URL and through Content Hub (#10 part 2c)to Module import from a URL and through Content Hub (#10 part 2c)Merge gate at
e0ae57f(trunk, elevated + standard UI). Approved.3db8f6d, with check_size at every size point in sword/src/install/http.rs; 283 medium (cap tested only via check_size) fixed in4e3ff2a; 283-3 refuted soundly (the http layer's cut tests cover mid-chunk cuts); the first audit's missed defect (DataPath over another module's folder) fixed in681ac3e. 284 and 285 had no findings. The 286 lows are fixed (1, 2) or declined with reasons (3, 4). Nothing high or medium is open.e0ae57fread in full. clash() is now infallible. read_dir failure, unreadable entries, unreadable or unparseable confs and refused DataPaths are all passed over. The extension match is case-insensitive. The module's own name is excluded, case-insensitively, so updates and reruns after a kill are not clashes. I found no concrete failure scenario. The commit adds a test for OTHER.CONF that asserts the refusal and that nothing was installed. The skip path of 286-4 is still untested, but it is a conservative skip and the 70 confidence score already counts that.