fix: resolve Zotero linked-file attachments via base directory - #138
fix: resolve Zotero linked-file attachments via base directory#138petermingjing wants to merge 2 commits into
Conversation
Zotero stores linked-file attachments with an "attachments:" prefix that is
relative to the "Linked Attachment Base Directory" configured in the Zotero
profile (extensions.zotero.baseAttachmentPath in prefs.js, not in
zotero.sqlite). _find_local_pdf only handled the "storage:" prefix and
absolute paths, so Path("attachments:sub/paper.pdf") never existed and the
function returned None.
For users who keep PDFs outside Zotero's own storage directory, this made
`scholaraio import-zotero --local` silently import metadata only: no PDF was
copied, no paper.md was produced, and no warning was emitted. On a 350-item
library this was the difference between 0 and 76 resolved PDFs.
Add _base_attachment_path() to read the base directory from prefs.js in the
macOS/Linux/Windows profile locations, and resolve the "attachments:" prefix
against it. Existing "storage:" and absolute-path behaviour is unchanged.
ZimoLiao
left a comment
ZimoLiao
left a comment
There was a problem hiding this comment.
Review summary
Decision: Request changes
This is a well-scoped fix with a clear diagnosis, useful regression coverage, and an appropriate changelog entry. The primary ASCII-path scenario works, and the existing storage: and absolute-path behavior remains covered. Before merge, however, two correctness/acceptance gaps need to be addressed.
Scope and validation
Reviewed against issue #137 at head 511784a626554bbff83a51f63d98268b0c1ce601.
- Focused attachment tests: 7 passed
- Full test suite: 1634 passed
- Ruff lint and format checks: passed
mypy scholaraio/providers/zotero.py: passed- Additional non-ASCII-path reproduction: failed (
中文文献resolved asNone) - GitHub Actions: pending maintainer approval for this first-time-contributor run; no CI result is available yet
Required changes
- [P1][Correctness / i18n] Preserve non-ASCII characters when decoding the preference value. The current
unicode_escaperound-trip corrupts literal UTF-8 paths, leaving users with non-ASCII home or attachment-directory names in the original metadata-only failure mode. See the inline comment for the reproduction and requested regression test. - [P1][Observability / acceptance] Surface unresolved
attachments:entries. Missing or unreadable profile configuration, an unavailable base directory, or a missing target file still falls through silently. This does not satisfy #137's explicit fallback expectation. Please report an aggregated warning rather than emitting one warning per item.
Non-blocking follow-up
[P2][Data integrity] Multiple Zotero profiles are currently resolved by returning the first lexicographically discovered profile with an existing base directory. That profile may not correspond to the supplied zotero.sqlite; in the worst case, the same relative filename under another base directory could resolve to the wrong PDF. Matching the database to its profile may be larger than this bug fix, so this can be tracked separately, but ambiguous discovery should not remain an undocumented silent choice.
Merge acceptance criteria
- Preserve literal Unicode while decoding only the preference string's actual escape sequences.
- Add regression coverage for a non-ASCII linked attachment base directory.
- Add an aggregated warning, with coverage, for unresolved
attachments:entries. - Run the repository checks successfully on the updated head, including GitHub CI.
Thanks for the strong initial contribution. The core direction is right; these changes will make the fix reliable for international paths and diagnosable when local Zotero configuration cannot be resolved.
| continue | ||
| m = _PREFS_BASE_PATH_RE.search(text) | ||
| if m: | ||
| base = Path(m.group(1).encode().decode("unicode_escape")).expanduser() |
There was a problem hiding this comment.
[P1][Correctness / i18n] Preserve literal Unicode while unescaping prefs.js values.
text has already been decoded as UTF-8, so m.group(1).encode().decode("unicode_escape") reinterprets each UTF-8 byte as a Unicode code point. A valid linked directory containing non-ASCII characters therefore becomes mojibake and fails base.is_dir().
I reproduced this with a base directory named 中文文献:
expected=/tmp/.../中文文献
resolved=None
matches=False
Please decode only the JavaScript string escapes while preserving characters that are already Unicode (for example, with a JSON-compatible string parser or a targeted unescaper), and add a regression test with a non-ASCII directory name. This is blocking because users with internationalized home or attachment paths remain in the original silent metadata-only failure mode.
| elif raw_path.startswith("attachments:"): | ||
| # Linked file relative to Zotero's linked attachment base directory | ||
| base = _base_attachment_path() | ||
| if base is not None: | ||
| pdf_path = base / raw_path[len("attachments:") :] | ||
| if pdf_path.exists(): | ||
| return pdf_path |
There was a problem hiding this comment.
[P1][Observability / acceptance] Do not silently discard unresolved linked attachments.
If the profile cannot be found/read, the base directory is unavailable, or the referenced file is missing, this branch still falls through to None; the caller then reports a normal metadata-only import. That preserves the silent-success behavior called out in #137 for every unresolved case outside the happy path.
Please propagate enough resolution information for parse_zotero_local() (or the CLI layer) to emit one aggregated warning such as N linked-file attachments could not be resolved, ideally including the likely configuration/file causes. Aggregating at the import level avoids one warning per record while making the failure actionable. Please cover the warning behavior in a test.
2 participants
What does this PR do?
Fixes #137 —
import-zotero --localsilently importing metadata only when Zotero uses a Linked Attachment Base Directory.Zotero stores linked-file attachments as
attachments:<relative path>, resolved againstextensions.zotero.baseAttachmentPath. That preference lives in the profile'sprefs.js, not inzotero.sqlite, so_find_local_pdfhad no way to expand it: theattachments:value fell through to the absolute/relative branch,Path("attachments:2024/paper.pdf").exists()was alwaysFalse, and the function returnedNonewith no warning.This adds
_base_attachment_path(), which reads that preference from the macOS / Linux / Windows profile locations, and resolves theattachments:prefix against it. Behaviour forstorage:prefixes and absolute paths is unchanged.The helper takes an optional
homeargument (defaulting toPath.home()) purely so the tests can inject a fixture directory, and is memoized withlru_cacheso a 350-item import readsprefs.jsonce rather than once per attachment.Before / after, same fixture, against the reproduction in #137:
On a real 350-item library the resolved-PDF count went from 0 to 76.
Type of change
Checklist
ruff checkpassespytestpassesVerification
ruff check scholaraio/ tests/— clean;ruff format --check— 232 files already formatted.mypy scholaraio/providers/zotero.py— clean.Full suite: 1628 passed, 4 skipped, plus 2 pre-existing failures that also fail on an unmodified
mastercheckout on macOS and are unrelated to this change:tests/test_agent_setup.py::test_shell_setup_quotes_paths_without_command_expansion— runsshwithenv={"PATH": "/usr/bin"}, but macOS shipsshat/bin/sh, not/usr/bin/sh.tests/test_backup.py::test_instance_backup_and_manifest_scoped_restore_round_trip— recent macOS shipsopenrsync(rsync 2.6.9 compatible) rather than GNU rsync.Both are macOS-only and CI runs
ubuntu-latest, so they are invisible there. Happy to open a separate issue for them if useful.New tests in
tests/test_zotero_attachments.pycover theattachments:resolution, the missing-file and missing-profile cases, and regression coverage for the existingstorage:and absolute-path branches.