feat(convert): make attachment names round-trip image paths - #57
Merged
Conversation
Attachment names were derived by replacing "/" with "_", which was neither
injective nor decodable. "a/b.png" and "a_b.png" both flattened to "a_b.png",
and since the encoded name is also the dedupe key, the second file was silently
dropped -- so the doc comment's "collision-free" claim was false.
Percent-encode instead ("%" -> "%25", then "/" -> "%2F"). Escaping the escape
character makes the mapping bijective, which fixes the collision by
construction and lets a later read recover an image's original path rather than
a flattened one. Confluence stores such names verbatim and matches ri:filename
literally, verified against Cloud.
A substitution like "/" -> "__" was considered and rejected: it never escapes
its own delimiter, so "__a.png" would decode to the absolute path "/a.png".
Also record the normalized source path on each collected attachment, and bound
publishing by the documentation root: image resolution stays page-relative (as
GitHub does), so a shared "../assets" directory works, but an image resolving
outside the root markfluence was run from is now IMAGE BROKEN.
StorageToMarkdown used ri:filename verbatim as the markdown src, so reading a page back produced a flattened path naming a file that does not exist on disk. Decode the attachment name, and prefer the source path markfluence records on the attachment when it is available, since that is exact rather than inferred. An absolute result is refused in both cases and falls back to the raw attachment name: markfluence never produces one, so it means the attachment came from somewhere else -- and export (#37) will write these paths to disk. StorageToMarkdown takes a sources map to carry that data; nil keeps the old name-decoding behavior. read builds it from ListAttachments, but only when the body actually references an attachment, and tolerates a failed lookup rather than failing the read. Threading the map required making the render chain methods on an mdRenderer. Rewriting ri:filename in the parsed tree would have been smaller but would corrupt attachment references inside unknown macros that pass through as raw storage.
Decoding an attachment name is inference; the comment is truth. Stamp uploads with "markfluence: sha256=<hex> path=<path>" so reading a page back recovers an image's original location exactly, and so an attachment can be identified as markfluence-managed. The old "mzcld:checksum: <hex>" form is still parsed, and skip/update now compares the *parsed* checksum rather than the whole comment string. Without that, changing the format would re-upload every attachment whose name did not change; with it, an unchanged file keeps its old comment and is still correctly skipped. The path is written last and unquoted so it may contain spaces.
Cover the percent-encoding, that image paths resolve relative to the markdown file the way GitHub renders them, that markfluence should be run from the root of the documentation tree, and that republishing a page leaves its old underscore-named attachment behind unreferenced -- markfluence never deletes.
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
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.
Fixes #56.
Makes the mapping between a markdown image's source path and its Confluence attachment name bijective, and records the source path on the attachment, so publishing a page and reading it back recovers the image's original location instead of a flattened approximation.
Design rationale, rejected alternatives, and the live-API probe results are written up in
_plans/018_attachment-name-encoding.md.Why
Attachment names were derived by replacing
/with_, which produced two defects.The "collision-free" claim in the doc comment was false.
a/b.pnganda_b.pngboth flattened toa_b.png, and since the encoded name is also the dedupe key, the second file was silently dropped — one set of bytes uploaded, both images resolving to it.Directory structure was unrecoverable.
readusedri:filenameverbatim, so the publish → read → publish cycle was a stable fixed point but a lossy one:docs/img.pngcame back asdocs_img.png, naming a file that doesn't exist on disk. This is what made #37 (export) incoherent — it can't lay attachments out so the markdown's image links resolve.What changed
Percent-encoding (
%→%25, then/→%2F). Escaping the escape character is what makes it bijective, which fixes the collision by construction and letsreadrecover the original path./→__was the obvious fix and is wrong — it never escapes its own delimiter, so it can't be bijective.__a.pngwould decode to the absolute path/a.png, which #37's export would then write to.a_/b.pngand a literala__b.pngalso decode wrong. All three are pinned as tests.Decode in
storage_to_md.go, preferring the source path recorded on the attachment (exact) over decoding the name (inference). An absolute result is refused in both cases — markfluence never produces one, so it means the attachment came from elsewhere.Comment format
markfluence: sha256=… path=…, retiring the deadmzcldname. The legacy form is still parsed, and skip/update now compares the parsed checksum rather than the raw comment string — without that, the format change would re-upload every attachment whose name didn't change.Documentation root. Image resolution stays page-relative, the way GitHub renders a repo's markdown, so a shared
../assetsdirectory works. But the root markfluence is run from now bounds what may be published: an image outside it isIMAGE BROKENrather than uploaded.Notable implementation choices
..never appears in a name (letting decode refuse escaping paths outright), but it makes the attachment name depend on the invocation directory — run fromdocs/guide/instead ofdocs/and the same image gets a different name, silently orphaning the old attachment. That's the exact migration pain this PR exists to stop repeating.StorageToMarkdowngained asources map[string]stringparam (nil = decode names).readbuilds it fromListAttachments, but only when the body actually references an attachment, and tolerates a failed lookup rather than failing the read.mdRendererto thread that map. Rewritingri:filenamein the parsed tree would have needed no signature changes, but would corrupt attachment references inside unknown macros, which pass through as raw storage and are re-serialized verbatim.Migration
markfluence never deletes, so republishing a page with a subdirectory image uploads under the new name, rewrites the body to match, and leaves the old
assets_x.pngattached but unreferenced. Nothing breaks; the cruft is permanent. Documented in the README, and #9'sattachment-listwill give users a direct way to find and remove them.Verification
Atlassian documents none of the attachment-name rules, so percent-encoding was probed against a live Cloud instance before any code was written (fallback would have been an escaped-underscore scheme). Full results in the issue comment:
%is stored verbatim — not rejected, stripped, or normalized back to a slash.ri:filenamematching is literal: a%2Fname renders as a real embedded image, while a nonexistent attachment renders theunknown-attachmentplaceholder, so the test discriminates. Confluence double-encodes to%252Fin the image URL, which is correct._links.downloadreturns byte-identical content.End-to-end against a real page after implementation:
assets/markfluence-test.pngpublished asassets%2Fmarkfluence-test.png, the page renders 3 embedded images with 0 placeholders,readreturnedwith the directory recovered, and a re-run correctly skips.Tests cover every edge case in the tables above, injectivity (the property the dedupe depends on), absolute-path refusal, and both comment formats. Two new regression cases pin the shared-parent layout and outside-root rejection. The pre-existing skip/update client tests were repointed at the legacy constant, so they now serve as legacy-tolerance coverage.
Out of scope
..when writing files — belongs to Implement an export subcommand (page + attachments to the filesystem) #37, where export actually creates files.attachment-listcovers it better.attachment-uploadneeds settled guidance on what remote name to use. The probe already answered several of its open questions.