From fc1664c09d6b58e383bf8b028408d1808e2e7769 Mon Sep 17 00:00:00 2001 From: DemchaAV Date: Mon, 1 Jun 2026 13:09:10 +0100 Subject: [PATCH] fix(document): invalidate layout cache on registry().register(...) (I3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DocumentSession.registry() returned the raw NodeRegistry, so callers mutating it directly via `session.registry().register(...)` left the cached LayoutGraph pinned to the previous compile. Only the dedicated session.registerNodeDefinition(...) path called invalidate(); the two entry points had different caching semantics for what is the same underlying mutation. Fix: - Drop the `final` modifier from NodeRegistry so DocumentSession can install a session-owned subclass. Binary-compatible — japicmp reports the change as "semver PATCH, compatible bug fix". - Add a private InvalidatingNodeRegistry subclass inside DocumentSession that overrides register(...) to call super.register(...) followed by invalidate(). The session initialises its registry field with this subclass, so every path that mutates the registry goes through cache invalidation — including default-definitions setup at construction time (layoutCache is field-initialised before the constructor body, so the early invalidate() calls are safe no-ops on an empty cache). - Drop the now-redundant explicit invalidate() from registerNodeDefinition; the wrapper handles it. - Update Javadoc on registry() and registerNodeDefinition to reflect the equivalent caching semantics. Regression test added: - registryRegisterInvalidatesCachedLayoutFromPreviousCompile in DocumentSessionTest. Warms the cache by rendering one badge under the default BadgeNodeDefinition, swaps in a replacement definition through registry().register(...), and asserts the next render reflects the replacement. Before the fix the test fails with "alpha" instead of "UPPER-alpha" because the cached LayoutGraph is returned unchanged. Gates: 1032 tests pass; japicmp vs v1.6.6 reports semver PATCH (compatible bug fix), no incompatible changes. --- CHANGELOG.md | 23 ++++++ .../compose/document/api/DocumentSession.java | 40 ++++++++-- .../compose/document/layout/NodeRegistry.java | 14 +++- .../document/api/DocumentSessionTest.java | 79 +++++++++++++++++++ 4 files changed, 150 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 999e4f43..18e420dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,9 +54,32 @@ planned; the next minor with new canonical DSL primitives is - `ConfigLoader.loadConfigWithEnv` Javadoc now states the YAML path requires `jackson-dataformat-yaml` on the classpath and throws `NoClassDefFoundError` when the optional dep is absent. +- `DocumentSession.registry()` Javadoc now explains that the + returned registry is a session-owned wrapper whose + `register(...)` mutates the registry *and* invalidates the + layout cache, making the two registration entry points + (`session.registry().register(...)` and + `session.registerNodeDefinition(...)`) interchangeable. + +### Fixes + +- `DocumentSession.registry().register(...)` now invalidates the + layout cache the same way + `DocumentSession.registerNodeDefinition(...)` does. Previously, + registering a node definition through `registry()` mutated the + registry in place but left the cached `LayoutGraph` pinned to + the previous compile, so a follow-up call to `render(...)` or + `layoutGraph()` silently returned the stale graph routed through + the old definition. Implemented by wrapping the session's + `NodeRegistry` in a private session-owned subclass that funnels + every `register(...)` call through `invalidate()`. (Track I3.) ### Internal +- `NodeRegistry` is no longer `final` so `DocumentSession` can + install a session-owned subclass that auto-invalidates the + layout cache on mutation (see Fixes above). Standalone + `NodeRegistry` instances retain their previous behaviour. - Replaced eight residual `org.jetbrains.annotations.NotNull` / `@Nullable` usages with `lombok.NonNull` (where the surrounding file already used Lombok) or removed them entirely (private diff --git a/src/main/java/com/demcha/compose/document/api/DocumentSession.java b/src/main/java/com/demcha/compose/document/api/DocumentSession.java index dbbed9ef..a21d2567 100644 --- a/src/main/java/com/demcha/compose/document/api/DocumentSession.java +++ b/src/main/java/com/demcha/compose/document/api/DocumentSession.java @@ -109,7 +109,7 @@ public DocumentSession(Path defaultOutputFile, this.canvas = LayoutCanvas.from(pageSize.width(), pageSize.height(), toEngineMargin(this.margin)); this.markdown = markdown; this.guideLines = guideLines; - this.registry = BuiltInNodeDefinitions.registerDefaults(new NodeRegistry()); + this.registry = BuiltInNodeDefinitions.registerDefaults(new InvalidatingNodeRegistry()); this.compiler = new LayoutCompiler(registry); this.customFontFamilies.addAll(List.copyOf(customFontFamilies)); refreshMeasurementServices(); @@ -540,16 +540,20 @@ public DocumentSession registerFontFamily(FontFamilyDefinition definition) { } /** - * Registers a custom semantic node definition. + * Registers a custom semantic node definition. Equivalent to + * {@code session.registry().register(definition)} — the layout + * cache is invalidated either way (the registry returned by + * {@link #registry()} is a session-owned wrapper that funnels + * mutations through {@code invalidate()} since v1.6.7). * * @param definition node definition implementation * @param semantic node type handled by the definition * @return this session + * @throws IllegalStateException if this session has already been closed */ public DocumentSession registerNodeDefinition(NodeDefinition definition) { ensureOpen(); registry.register(Objects.requireNonNull(definition, "definition")); - invalidate(); return this; } @@ -590,9 +594,15 @@ public SessionLayoutApi layout() { } /** - * Returns the mutable node registry used by this session. + * Returns the live node registry backing this session. The returned + * instance is a session-owned subclass of {@link NodeRegistry} (since + * v1.6.7); calling {@link NodeRegistry#register(NodeDefinition)} on it + * mutates the registry and invalidates the layout cache, so it + * is interchangeable with {@link #registerNodeDefinition(NodeDefinition)}. + * Read-only access via {@link NodeRegistry#definitionFor(DocumentNode)} + * is unaffected. * - * @return active node registry + * @return live node registry (invalidates layout cache on mutation) */ public NodeRegistry registry() { return registry; @@ -901,6 +911,26 @@ private interface PdfRenderingBody { R run() throws Exception; } + /** + * Session-owned {@link NodeRegistry} subclass that funnels every + * {@link #register(NodeDefinition)} call through + * {@link DocumentSession#invalidate()}. Without this wrapper, callers + * that mutated the registry directly via {@code session.registry(). + * register(...)} would leave the layout cache pointing at a stale + * compile result — the cache invalidation only happened on the + * dedicated {@link DocumentSession#registerNodeDefinition(NodeDefinition)} + * path. Added in v1.6.7 (Track I3) so the two registration entry points + * have identical caching semantics. + */ + private final class InvalidatingNodeRegistry extends NodeRegistry { + @Override + public NodeRegistry register(NodeDefinition definition) { + super.register(definition); + invalidate(); + return this; + } + } + /** * Inner adapter exposing session-private state to {@link DocumentRenderingFacade} * without making the corresponding session methods public. diff --git a/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java b/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java index a2a09d17..8dfe68d6 100644 --- a/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java +++ b/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java @@ -8,8 +8,20 @@ /** * Registry of semantic node definitions. + * + *

Standalone {@code NodeRegistry} instances are safe to mutate + * directly via {@link #register(NodeDefinition)}. When a registry is + * owned by a {@link com.demcha.compose.document.api.DocumentSession}, + * however, the session wraps it with an internal subclass that calls + * {@code invalidate()} on every {@code register(...)} call so the + * layout cache cannot go stale behind the caller's back. Callers + * working through {@code session.registry().register(...)} therefore + * get the same caching semantics as + * {@code session.registerNodeDefinition(...)}.

+ * + * @since 1.6.0 */ -public final class NodeRegistry { +public class NodeRegistry { private final Map, NodeDefinition> definitions = new LinkedHashMap<>(); /** diff --git a/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java b/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java index f6c755dd..dfa95274 100644 --- a/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java +++ b/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java @@ -681,6 +681,85 @@ public List render(LayoutGraph graph, FixedLayoutRenderContext context) } } + /** + * Regression for v1.6.7 Track I3: {@code session.registry().register(...)} + * must invalidate the layout cache the same way + * {@link DocumentSession#registerNodeDefinition(NodeDefinition)} does. + * Before the fix the registry was mutated in place but the cached + * {@link LayoutGraph} stayed pinned to the previous compile, so a + * follow-up call to {@code render(...)} or {@code layoutGraph()} would + * silently return the stale graph that still routed through the old + * definition. + */ + @Test + void registryRegisterInvalidatesCachedLayoutFromPreviousCompile() throws Exception { + FixedLayoutBackend> backend = new FixedLayoutBackend<>() { + @Override + public String name() { + return "badge-backend"; + } + + @Override + public List render(LayoutGraph graph, FixedLayoutRenderContext context) { + return graph.fragments().stream() + .map(fragment -> (String) fragment.payload()) + .toList(); + } + }; + + // Replacement definition that prefixes the badge label so the + // before / after snapshots are trivially distinguishable. + NodeDefinition uppercaseDef = new NodeDefinition<>() { + @Override + public Class nodeType() { + return BadgeNode.class; + } + + @Override + public PreparedNode prepare(BadgeNode node, PrepareContext ctx, BoxConstraints constraints) { + return PreparedNode.leaf(node, new MeasureResult(40, 18)); + } + + @Override + public PaginationPolicy paginationPolicy(BadgeNode node) { + return PaginationPolicy.ATOMIC; + } + + @Override + public List emitFragments(PreparedNode prepared, FragmentContext ctx, FragmentPlacement placement) { + return List.of(new LayoutFragment( + placement.path(), + 0, + 0, + 0, + placement.width(), + placement.height(), + "UPPER-" + prepared.node().label())); + } + }; + + try (DocumentSession session = GraphCompose.document() + .pageSize(200, 160) + .margin(DocumentInsets.of(10)) + .create()) { + + session.registerNodeDefinition(new BadgeNodeDefinition()); + session.add(new BadgeNode("Badge", "alpha", DocumentInsets.zero(), DocumentInsets.zero())); + + // First render warms the layout cache against the original definition. + assertThat(session.render(backend)).containsExactly("alpha"); + + // Swap the definition through the registry() path (not the + // dedicated session.registerNodeDefinition path). Without the + // I3 invalidate-on-mutate guard the cache would still point at + // the previously compiled layout and the next render would + // return "alpha". + session.registry().register(uppercaseDef); + + assertThat(session.render(backend)).containsExactly("UPPER-alpha"); + } + } + @Test @DisabledIfSystemProperty(named = "no.poi", matches = "true", disabledReason = "Exercises DocxSemanticBackend; skipped under the no-poi profile that excludes poi-ooxml from the test classpath")