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")