Skip to content

Commit e75bf5b

Browse files
authored
fix(output): make file rendering non-destructive and the PPTX target explicit (#439)
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.
1 parent e7a5495 commit e75bf5b

17 files changed

Lines changed: 528 additions & 64 deletions

File tree

CHANGELOG.md

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,26 @@ follow semantic versioning; release dates are ISO 8601.
77

88
### Public API
99

10+
- **A failed render no longer destroys the document it was replacing.**
11+
`buildPdf(Path)`, `buildPptx(Path)` and multi-section `buildPdf(Path)` render into
12+
a scratch file in the destination's own directory and move it onto the destination
13+
only after the render returns. Previously the destination was opened — and therefore
14+
truncated — before the backend produced a byte, so an oversized node, a missing
15+
backend, or a full disk left an empty file where a published document used to be.
16+
The move is atomic where the filesystem supports it, so a concurrent reader never
17+
observes a half-written document; on POSIX the destination keeps the permissions it
18+
already had, or gets `rw-r--r--` when it is new.
19+
- **`DocumentSession.buildPptx()` (no-arg) is removed**; use `buildPptx(Path)`. The
20+
session has a single configured output path, shared with `buildPdf()`, so the no-arg
21+
form wrote deck bytes into whatever that path was — including a file named `.pdf`.
22+
PPTX output now always names its destination, which is also what lets one session
23+
emit both formats. The PPTX surface is `@Beta` (Experimental) and was never published,
24+
so no released code can depend on the removed overload.
25+
- **Two backends registered for one format now fail loudly.**
26+
`BackendProviders.fixedLayout(format)` and the no-arg default previously took the
27+
first `ServiceLoader` match, so a third-party provider declaring an existing format
28+
silently decided the renderer by classpath order. Both entry points now throw
29+
`IllegalStateException` naming the competing provider classes.
1030
- **Keep a heading with its content**`SectionBuilder.keepWithNext()`. A section
1131
marked keep-with-next is never left stranded as the last block on a page apart from
1232
the content it introduces: when the section plus the first slice of the following
@@ -184,7 +204,7 @@ follow semantic versioning; release dates are ISO 8601.
184204
documents that custom handlers do not apply inside rasterized clip
185205
composites.
186206
- `DocumentSession.toPptxBytes()` / `writePptx(OutputStream)` /
187-
`buildPptx()` / `buildPptx(Path)` — the PPTX counterparts of the PDF
207+
`buildPptx(Path)` / `buildPptx(Path)` — the PPTX counterparts of the PDF
188208
convenience trio. The backend resolves through the format-keyed provider
189209
(`BackendProviders.fixedLayout("pptx")`), so the core stays free of a PPTX
190210
dependency: with `graph-compose-render-pptx` on the classpath the session's
@@ -243,7 +263,7 @@ follow semantic versioning; release dates are ISO 8601.
243263
AUTO-PAGINATION` tags and a `code → layout → PDF/PPTX` diagram whose amber
244264
connectors branch to both backends — composed as one full-bleed
245265
`CanvasLayerNode` and emitted as an editable PowerPoint slide via
246-
`buildPptx()`. Registered in `GenerateAllExamples` and listed in the
266+
`buildPptx(Path)`. Registered in `GenerateAllExamples` and listed in the
247267
examples gallery with committed PDF/PPTX previews; a native-shape guard
248268
(`MavenBannerNativeShapeTest`) pins the slide to a single picture (the badge
249269
checkmark) with the panels, tags, code and diagram all native.

core/src/main/java/com/demcha/compose/GraphCompose.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,11 @@ public static DocumentBuilder document() {
8585

8686
/**
8787
* Starts the canonical semantic document composition flow with a default output target
88-
* used by {@link DocumentSession#buildPdf()} and {@link DocumentSession#buildPptx()}.
88+
* used by {@link DocumentSession#buildPdf()}. PPTX output always takes an explicit
89+
* path — see {@link DocumentSession#buildPptx(Path)} — so one session can emit both
90+
* formats without the deck overwriting the PDF.
8991
*
90-
* @param outputFile default output path for the no-arg build methods
92+
* @param outputFile default output path for {@code buildPdf()}
9193
* @return builder for creating a semantic document session
9294
*/
9395
public static DocumentBuilder document(Path outputFile) {
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package com.demcha.compose.document.api;
2+
3+
import org.slf4j.Logger;
4+
import org.slf4j.LoggerFactory;
5+
6+
import java.io.IOException;
7+
import java.io.OutputStream;
8+
import java.nio.file.AtomicMoveNotSupportedException;
9+
import java.nio.file.Files;
10+
import java.nio.file.NoSuchFileException;
11+
import java.nio.file.Path;
12+
import java.nio.file.StandardCopyOption;
13+
import java.nio.file.attribute.PosixFilePermission;
14+
import java.nio.file.attribute.PosixFilePermissions;
15+
import java.util.Set;
16+
17+
/**
18+
* Writes a rendered document to a file without destroying the previous
19+
* contents when the render fails.
20+
*
21+
* <p>Rendering straight into the destination truncates it the moment the
22+
* stream opens, long before the backend has produced a single byte. A render
23+
* that then fails — a missing backend, an unsupported payload, a full disk —
24+
* leaves the caller with an empty or half-written file where a good document
25+
* used to be. That matters most for the case this library is built for: a
26+
* server that overwrites a previously published document in place.</p>
27+
*
28+
* <p>So the bytes go to a temporary file in the destination's own directory
29+
* and are moved onto the destination only after the writer returns normally.
30+
* The move is atomic where the filesystem supports it, which also means a
31+
* concurrent reader never observes a partially written document. On failure
32+
* the temporary file is removed and the destination is left untouched.</p>
33+
*
34+
* <p><b>Permissions.</b> {@link Files#createTempFile} deliberately creates an
35+
* owner-only file, and {@link Files#move} carries those permissions onto the
36+
* destination — which would silently narrow a served file from world-readable
37+
* to owner-only. On POSIX filesystems the temporary file is therefore widened
38+
* before the move: to the permissions the destination already had when it
39+
* exists, and to {@code rw-r--r--} when it does not. Filesystems without POSIX
40+
* support (Windows) are unaffected.</p>
41+
*
42+
* <p><b>Symbolic links.</b> Publishing replaces the destination <em>entry</em>.
43+
* When the destination is a symlink, the link itself is replaced by the rendered
44+
* file rather than the render being written through it to the link's target.
45+
* Render to the real path when a symlink must survive.</p>
46+
*
47+
* @author Artem Demchyshyn
48+
*/
49+
final class AtomicFileOutput {
50+
51+
private static final Logger LOG = LoggerFactory.getLogger("com.demcha.compose.document.lifecycle");
52+
53+
private static final Set<PosixFilePermission> DEFAULT_PERMISSIONS =
54+
Set.copyOf(PosixFilePermissions.fromString("rw-r--r--"));
55+
56+
private AtomicFileOutput() {
57+
}
58+
59+
/**
60+
* Writes bytes into a caller-owned stream.
61+
*/
62+
@FunctionalInterface
63+
interface StreamWriter {
64+
/**
65+
* @param output stream to write to; closed by the caller, not the writer
66+
* @throws Exception if rendering fails
67+
*/
68+
void writeTo(OutputStream output) throws Exception;
69+
}
70+
71+
/**
72+
* Renders through {@code writer} and publishes the result at {@code target}.
73+
*
74+
* @param target destination file, replaced only after a successful render
75+
* @param writer produces the document bytes
76+
* @throws NoSuchFileException if the destination's parent directory does not exist
77+
* @throws Exception whatever the writer throws, with {@code target} untouched
78+
*/
79+
static void write(Path target, StreamWriter writer) throws Exception {
80+
Path directory = target.toAbsolutePath().getParent();
81+
if (directory == null || !Files.isDirectory(directory)) {
82+
throw new NoSuchFileException(
83+
target.toString(),
84+
null,
85+
"The parent directory does not exist. Create it before rendering.");
86+
}
87+
88+
Path temporary = Files.createTempFile(directory, ".graphcompose-", ".tmp");
89+
boolean published = false;
90+
try {
91+
try (OutputStream output = Files.newOutputStream(temporary)) {
92+
writer.writeTo(output);
93+
}
94+
alignPermissions(temporary, target);
95+
move(temporary, target);
96+
published = true;
97+
} finally {
98+
if (!published) {
99+
discard(temporary);
100+
}
101+
}
102+
}
103+
104+
/**
105+
* Removes the scratch file without masking the failure that caused it to be
106+
* abandoned. A cleanup {@link IOException} — a scanner still holding the
107+
* handle, a revoked mount — must not replace the render diagnostic the
108+
* caller actually needs.
109+
*/
110+
private static void discard(Path temporary) {
111+
try {
112+
Files.deleteIfExists(temporary);
113+
} catch (IOException ex) {
114+
LOG.debug("document.output.scratch-cleanup-failed path={}", temporary, ex);
115+
}
116+
}
117+
118+
private static void move(Path temporary, Path target) throws IOException {
119+
try {
120+
Files.move(temporary, target,
121+
StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
122+
} catch (AtomicMoveNotSupportedException ex) {
123+
// Some network and FUSE filesystems reject ATOMIC_MOVE. Replacing
124+
// without the atomicity guarantee is still strictly better than
125+
// truncating the destination up front.
126+
Files.move(temporary, target, StandardCopyOption.REPLACE_EXISTING);
127+
}
128+
}
129+
130+
private static void alignPermissions(Path temporary, Path target) {
131+
if (!temporary.getFileSystem().supportedFileAttributeViews().contains("posix")) {
132+
return;
133+
}
134+
try {
135+
Set<PosixFilePermission> permissions = Files.exists(target)
136+
? Files.getPosixFilePermissions(target)
137+
: DEFAULT_PERMISSIONS;
138+
Files.setPosixFilePermissions(temporary, permissions);
139+
} catch (IOException | UnsupportedOperationException ex) {
140+
// A render that succeeded must not fail over file metadata; the
141+
// document is still published, just with the temp file's mode.
142+
}
143+
}
144+
}

core/src/main/java/com/demcha/compose/document/api/DocumentRenderingFacade.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,8 @@ private void buildFixedLayout(String format, Path outputFile) throws Exception {
189189
long startNanos = System.nanoTime();
190190
LIFECYCLE_LOG.debug("document.{}.build.start sessionId={} revision={} roots={}",
191191
format, context.sessionId(), context.revision(), context.rootCount());
192-
try (OutputStream output = Files.newOutputStream(target)) {
193-
writeFixedLayout(format, output);
192+
try {
193+
AtomicFileOutput.write(target, output -> writeFixedLayout(format, output));
194194
LIFECYCLE_LOG.debug(
195195
"document.{}.build.end sessionId={} revision={} durationMs={}",
196196
format,

core/src/main/java/com/demcha/compose/document/api/DocumentSession.java

Lines changed: 1 addition & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
* <li>inspect {@link #layoutGraph()} / {@link #layoutSnapshot()} as needed</li>
5555
* <li>render with {@link #writePdf(OutputStream)}, {@link #toPdfBytes()}, {@link #buildPdf()},
5656
* their PPTX counterparts ({@link #writePptx(OutputStream)}, {@link #toPptxBytes()},
57-
* {@link #buildPptx()}), or a custom backend</li>
57+
* {@link #buildPptx(Path)}), or a custom backend</li>
5858
* </ol>
5959
*
6060
* <p><b>Thread-safety:</b> this type is mutable and not thread-safe.</p>
@@ -989,32 +989,6 @@ public void writePptx(OutputStream output) throws DocumentRenderingException {
989989
});
990990
}
991991

992-
/**
993-
* Builds the current document as a .pptx deck into the default output file
994-
* configured on the builder. See {@link #toPptxBytes()} for the geometry
995-
* and classpath contract.
996-
*
997-
* <p>The default file is shared with {@link #buildPdf()}: the session has
998-
* one configured output path, and this method writes deck bytes to it
999-
* as-is — pass an explicit {@code .pptx} path to
1000-
* {@link #buildPptx(Path)} when the default is named for PDF output.</p>
1001-
*
1002-
* @throws IllegalStateException if no default output file was configured
1003-
* @throws DocumentRenderingException if PPTX rendering fails
1004-
* @since 2.1.0
1005-
*/
1006-
@Beta
1007-
public void buildPptx() throws DocumentRenderingException {
1008-
ensureOpen();
1009-
if (defaultOutputFile == null) {
1010-
throw new IllegalStateException("No default output file was configured for this document session.");
1011-
}
1012-
wrapRendering("build PPTX at '" + defaultOutputFile + "'", () -> {
1013-
renderingFacade.buildPptx(defaultOutputFile);
1014-
return null;
1015-
});
1016-
}
1017-
1018992
/**
1019993
* Builds the current document as a .pptx deck into the supplied output
1020994
* file. See {@link #toPptxBytes()} for the geometry and classpath contract.

core/src/main/java/com/demcha/compose/document/api/MultiSectionDocument.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,7 @@ public void buildPdf() throws DocumentRenderingException {
140140
public void buildPdf(Path outputFile) throws DocumentRenderingException {
141141
Objects.requireNonNull(outputFile, "outputFile");
142142
render("build PDF at '" + outputFile + "'", () -> {
143-
try (OutputStream output = Files.newOutputStream(outputFile)) {
144-
backend.writeSections(renderUnits(), output);
145-
}
143+
AtomicFileOutput.write(outputFile, output -> backend.writeSections(renderUnits(), output));
146144
return null;
147145
});
148146
}

core/src/main/java/com/demcha/compose/document/backend/fixed/BackendProviders.java

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import java.util.ServiceLoader;
1212
import java.util.concurrent.ConcurrentHashMap;
1313
import java.util.function.Supplier;
14+
import java.util.stream.Collectors;
1415

1516
/**
1617
* Locates the fixed-layout backend service providers registered on the
@@ -76,13 +77,16 @@ public static FixedLayoutBackendProvider fixedLayout() {
7677
* format, resolving and caching it on first use.
7778
*
7879
* <p>Matching against {@link FixedLayoutBackendProvider#format()} is
79-
* case-insensitive. When several providers declare the same format the
80-
* first one enumerated by {@link ServiceLoader} wins.</p>
80+
* case-insensitive. Two providers claiming the same format is a classpath
81+
* mistake rather than a preference, so it fails loudly instead of letting
82+
* {@link ServiceLoader} enumeration order silently decide which backend
83+
* renders the document.</p>
8184
*
8285
* @param format output format identifier such as {@code "pdf"} or {@code "pptx"}
8386
* @return the provider rendering that format
8487
* @throws MissingBackendException if no provider for the format is registered
8588
* on the classpath
89+
* @throws IllegalStateException if more than one provider declares the format
8690
* @since 2.1.0
8791
*/
8892
public static FixedLayoutBackendProvider fixedLayout(String format) {
@@ -92,11 +96,31 @@ public static FixedLayoutBackendProvider fixedLayout(String format) {
9296
if (cached != null) {
9397
return cached;
9498
}
95-
FixedLayoutBackendProvider resolved = fixedLayoutProviders().stream()
99+
return cacheByFormat(key, resolveExactlyOne(key));
100+
}
101+
102+
/**
103+
* Resolves the single provider declaring {@code key}, refusing an ambiguous
104+
* classpath. Shared by the by-format lookup and the default lookup so both
105+
* entry points agree about what "registered" means.
106+
*/
107+
private static FixedLayoutBackendProvider resolveExactlyOne(String key) {
108+
List<FixedLayoutBackendProvider> matches = fixedLayoutProviders().stream()
96109
.filter(provider -> key.equals(normalizedFormat(provider)))
97-
.findFirst()
98-
.orElseThrow(() -> new MissingBackendException(missingFormatMessage(key)));
99-
return cacheByFormat(key, resolved);
110+
.toList();
111+
if (matches.isEmpty()) {
112+
throw new MissingBackendException(missingFormatMessage(key));
113+
}
114+
if (matches.size() > 1) {
115+
String names = matches.stream()
116+
.map(provider -> provider.getClass().getName())
117+
.collect(Collectors.joining(", "));
118+
throw new IllegalStateException(
119+
"Multiple fixed-layout backends are registered for format \"" + key + "\": " + names
120+
+ ". Remove the duplicate artifact from the classpath, or select a backend "
121+
+ "explicitly instead of resolving it by format.");
122+
}
123+
return matches.get(0);
100124
}
101125

102126
/**
@@ -122,7 +146,10 @@ private static FixedLayoutBackendProvider defaultFixedLayout() {
122146
!DEFAULT_FIXED_LAYOUT_FORMAT.equals(normalizedFormat(provider)))
123147
.thenComparing(BackendProviders::normalizedFormat))
124148
.orElseThrow(missingBackend());
125-
return cacheByFormat(normalizedFormat(chosen), chosen);
149+
// The comparator picks a format deterministically, but two providers
150+
// claiming that format would still make the winner arbitrary.
151+
String key = normalizedFormat(chosen);
152+
return cacheByFormat(key, resolveExactlyOne(key));
126153
}
127154

128155
/**

core/src/main/java/com/demcha/compose/document/exceptions/MissingBackendException.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* {@code io.github.demchaav:graph-compose-render-pptx}) discovered at
1111
* runtime through {@link java.util.ServiceLoader}. Calling a convenience output
1212
* method — {@code toPdfBytes()}, {@code buildPdf()}, {@code toImages()},
13-
* {@code toPptxBytes()}, {@code buildPptx()} — or requesting
13+
* {@code toPptxBytes()}, {@code buildPptx(Path)} — or requesting
1414
* {@code layoutSnapshot()} without the matching artifact on the classpath
1515
* fails with this exception. The message names the artifact to add.</p>
1616
*

0 commit comments

Comments
 (0)