Skip to content

fix(output): make file rendering non-destructive and the PPTX target explicit#439

Merged
DemchaAV merged 1 commit into
developfrom
fix/output-api-safety
Jul 25, 2026
Merged

fix(output): make file rendering non-destructive and the PPTX target explicit#439
DemchaAV merged 1 commit into
developfrom
fix/output-api-safety

Conversation

@DemchaAV

Copy link
Copy Markdown
Owner

Stacked on #436. Base is chore/release-2.1-line so the diff shows only this change — both branches edit the v2.1.0 CHANGELOG section. Retarget to develop before #436 merges, or GitHub closes this PR.

Why

Rendering to a file opened the destination before the backend produced a byte, so the file was truncated first and written second. A render that then failed — an atomic node taller than the page, a missing backend, a full disk — replaced a published document with an empty one. That is the exact shape of the library's main use case: a server re-rendering a document it is already serving.

Separately, DocumentSession holds one configured output path shared with buildPdf(), so the no-arg buildPptx() wrote deck bytes into whatever that path was. Its own Javadoc had to warn that a .pdf default would receive a PPTX.

And BackendProviders.fixedLayout(format) took the first ServiceLoader match, so a third-party provider declaring an already-registered format silently decided which backend rendered the document, depending on classpath order.

What changed

  • Non-destructive file output. Both file paths render into a scratch file in the destination's own directory, moved onto the destination only after the render returns normally. The move is atomic where the filesystem supports it, so a concurrent reader never sees a half-written document, and the scratch file is discarded on failure without masking the render's own exception. Two details the naive form gets wrong are handled explicitly: Files.createTempFile creates owner-only and Files.move carries that mode onto the destination — silently narrowing a served file — so POSIX permissions are aligned to the destination's existing mode (or rw-r--r-- for a new file) before the move; and replacing the destination entry replaces a symlink rather than writing through it, which the contract now states.
  • DocumentSession.buildPptx() (no-arg) removed; buildPptx(Path) stays. Naming the destination is also what lets one session emit both formats. The surface is @Beta and unpublished — graph-compose-core at the 2.0.0 baseline does not declare the method, which is why the binary-compatibility gate stays green. The real risk was javadoc:javadoc: two {@link} references lived in the examples module.
  • Duplicate backend formats fail fast. Both the by-format lookup and the no-arg default share one resolver that throws IllegalStateException naming the competing provider classes.

Verification

  • ./mvnw -B -ntp clean verify — BUILD SUCCESS, 1518 tests (from 1508), 0 failures, 2 skipped off POSIX.
  • The regression test was proven, not assumed: reverting DocumentRenderingFacade to Files.newOutputStream(target) turns DocumentOutputFileSafetyTest red with expected: "previously published document" but was: "".
  • ./mvnw -P japicmp verify -pl :graph-compose-core — green; git show v2.0.0:…/DocumentSession.java | grep -c buildPptx is 0.
  • javadoc:javadoc green on the six published modules and on examples.
  • New coverage: mid-stream failure, scratch cleanup, first-time creation, missing parent directory, POSIX permission alignment, and two ServiceLoader fakes sharing one format with a companion test proving unrelated formats still resolve.

@DemchaAV
DemchaAV changed the base branch from chore/release-2.1-line to develop July 25, 2026 08:14
…explicit

Rendering to a file opened the destination before the backend produced a byte,
so the file was truncated first and written second. A render that then failed —
an atomic node taller than the page, a missing backend, a full disk — replaced a
published document with an empty one. That is the exact shape of the library's
main use case: a server re-rendering a document that is already being served.

Route both file paths through a scratch file in the destination's own directory,
moved onto the destination only after the render returns normally. The move is
atomic where the filesystem supports it, so a concurrent reader never sees a
half-written document, and the scratch file is discarded on failure without
masking the render's own exception. Two details the naive form gets wrong are
handled explicitly: a temp file is created owner-only and `Files.move` carries
that mode onto the destination, which would silently narrow a served file, so
POSIX permissions are aligned to the destination's existing mode (or rw-r--r--
for a new file) before the move; and replacing the destination entry replaces a
symlink rather than writing through it, which the contract now states.

Remove the no-arg `DocumentSession.buildPptx()`. The session holds one configured
output path, shared with `buildPdf()`, so the no-arg form wrote deck bytes into
whatever that path was — its own Javadoc had to warn that a `.pdf` default would
receive a PPTX. Naming the destination is also what lets one session emit both
formats. The surface is `@Beta` and unpublished, so nothing released can depend
on it; `graph-compose-core` at the 2.0.0 baseline does not declare the method at
all, which is why the binary-compatibility gate stays green.

Make duplicate backend registrations fail instead of resolving by classpath
order: a third-party provider declaring an already-registered format silently
decided which backend rendered the document. Both the by-format lookup and the
no-arg default now share one resolver that throws `IllegalStateException` naming
the competing classes.

Tests: the qa regression writes a sentinel document, fails a render on an
oversized node, and asserts the sentinel survives — it fails on the previous
implementation with `expected: "previously published document" but was: ""`.
Helper-level tests cover mid-stream failure, scratch cleanup, first-time
creation, a missing parent directory, and the permission alignment (skipped off
POSIX). Two ServiceLoader fakes sharing one format cover the ambiguity, with a
companion test proving unrelated formats still resolve.

Verified: full reactor `clean verify` green (1518 tests, 0 failures, 2 skipped
off POSIX); japicmp against the 2.0.0 baseline green; `javadoc:javadoc` green on
the six published modules and on examples, where two `{@link}` references to the
removed overload lived.
@DemchaAV
DemchaAV force-pushed the fix/output-api-safety branch from daa79d4 to ddf03f8 Compare July 25, 2026 08:21
@DemchaAV
DemchaAV merged commit e75bf5b into develop Jul 25, 2026
12 checks passed
@DemchaAV
DemchaAV deleted the fix/output-api-safety branch July 25, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant