Skip to content

Commit b4720c6

Browse files
authored
fix(document): invalidate layout cache on registry().register(...) (I3) (#113)
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.
1 parent 63f13dd commit b4720c6

4 files changed

Lines changed: 150 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,32 @@ planned; the next minor with new canonical DSL primitives is
5454
- `ConfigLoader.loadConfigWithEnv` Javadoc now states the YAML
5555
path requires `jackson-dataformat-yaml` on the classpath and
5656
throws `NoClassDefFoundError` when the optional dep is absent.
57+
- `DocumentSession.registry()` Javadoc now explains that the
58+
returned registry is a session-owned wrapper whose
59+
`register(...)` mutates the registry *and* invalidates the
60+
layout cache, making the two registration entry points
61+
(`session.registry().register(...)` and
62+
`session.registerNodeDefinition(...)`) interchangeable.
63+
64+
### Fixes
65+
66+
- `DocumentSession.registry().register(...)` now invalidates the
67+
layout cache the same way
68+
`DocumentSession.registerNodeDefinition(...)` does. Previously,
69+
registering a node definition through `registry()` mutated the
70+
registry in place but left the cached `LayoutGraph` pinned to
71+
the previous compile, so a follow-up call to `render(...)` or
72+
`layoutGraph()` silently returned the stale graph routed through
73+
the old definition. Implemented by wrapping the session's
74+
`NodeRegistry` in a private session-owned subclass that funnels
75+
every `register(...)` call through `invalidate()`. (Track I3.)
5776

5877
### Internal
5978

79+
- `NodeRegistry` is no longer `final` so `DocumentSession` can
80+
install a session-owned subclass that auto-invalidates the
81+
layout cache on mutation (see Fixes above). Standalone
82+
`NodeRegistry` instances retain their previous behaviour.
6083
- Replaced eight residual `org.jetbrains.annotations.NotNull` /
6184
`@Nullable` usages with `lombok.NonNull` (where the surrounding
6285
file already used Lombok) or removed them entirely (private

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

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public DocumentSession(Path defaultOutputFile,
109109
this.canvas = LayoutCanvas.from(pageSize.width(), pageSize.height(), toEngineMargin(this.margin));
110110
this.markdown = markdown;
111111
this.guideLines = guideLines;
112-
this.registry = BuiltInNodeDefinitions.registerDefaults(new NodeRegistry());
112+
this.registry = BuiltInNodeDefinitions.registerDefaults(new InvalidatingNodeRegistry());
113113
this.compiler = new LayoutCompiler(registry);
114114
this.customFontFamilies.addAll(List.copyOf(customFontFamilies));
115115
refreshMeasurementServices();
@@ -540,16 +540,20 @@ public DocumentSession registerFontFamily(FontFamilyDefinition definition) {
540540
}
541541

542542
/**
543-
* Registers a custom semantic node definition.
543+
* Registers a custom semantic node definition. Equivalent to
544+
* {@code session.registry().register(definition)} — the layout
545+
* cache is invalidated either way (the registry returned by
546+
* {@link #registry()} is a session-owned wrapper that funnels
547+
* mutations through {@code invalidate()} since v1.6.7).
544548
*
545549
* @param definition node definition implementation
546550
* @param <E> semantic node type handled by the definition
547551
* @return this session
552+
* @throws IllegalStateException if this session has already been closed
548553
*/
549554
public <E extends DocumentNode> DocumentSession registerNodeDefinition(NodeDefinition<E> definition) {
550555
ensureOpen();
551556
registry.register(Objects.requireNonNull(definition, "definition"));
552-
invalidate();
553557
return this;
554558
}
555559

@@ -590,9 +594,15 @@ public SessionLayoutApi layout() {
590594
}
591595

592596
/**
593-
* Returns the mutable node registry used by this session.
597+
* Returns the live node registry backing this session. The returned
598+
* instance is a session-owned subclass of {@link NodeRegistry} (since
599+
* v1.6.7); calling {@link NodeRegistry#register(NodeDefinition)} on it
600+
* mutates the registry <em>and</em> invalidates the layout cache, so it
601+
* is interchangeable with {@link #registerNodeDefinition(NodeDefinition)}.
602+
* Read-only access via {@link NodeRegistry#definitionFor(DocumentNode)}
603+
* is unaffected.
594604
*
595-
* @return active node registry
605+
* @return live node registry (invalidates layout cache on mutation)
596606
*/
597607
public NodeRegistry registry() {
598608
return registry;
@@ -901,6 +911,26 @@ private interface PdfRenderingBody<R> {
901911
R run() throws Exception;
902912
}
903913

914+
/**
915+
* Session-owned {@link NodeRegistry} subclass that funnels every
916+
* {@link #register(NodeDefinition)} call through
917+
* {@link DocumentSession#invalidate()}. Without this wrapper, callers
918+
* that mutated the registry directly via {@code session.registry().
919+
* register(...)} would leave the layout cache pointing at a stale
920+
* compile result — the cache invalidation only happened on the
921+
* dedicated {@link DocumentSession#registerNodeDefinition(NodeDefinition)}
922+
* path. Added in v1.6.7 (Track I3) so the two registration entry points
923+
* have identical caching semantics.
924+
*/
925+
private final class InvalidatingNodeRegistry extends NodeRegistry {
926+
@Override
927+
public <E extends DocumentNode> NodeRegistry register(NodeDefinition<E> definition) {
928+
super.register(definition);
929+
invalidate();
930+
return this;
931+
}
932+
}
933+
904934
/**
905935
* Inner adapter exposing session-private state to {@link DocumentRenderingFacade}
906936
* without making the corresponding session methods public.

src/main/java/com/demcha/compose/document/layout/NodeRegistry.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,20 @@
88

99
/**
1010
* Registry of semantic node definitions.
11+
*
12+
* <p>Standalone {@code NodeRegistry} instances are safe to mutate
13+
* directly via {@link #register(NodeDefinition)}. When a registry is
14+
* owned by a {@link com.demcha.compose.document.api.DocumentSession},
15+
* however, the session wraps it with an internal subclass that calls
16+
* {@code invalidate()} on every {@code register(...)} call so the
17+
* layout cache cannot go stale behind the caller's back. Callers
18+
* working through {@code session.registry().register(...)} therefore
19+
* get the same caching semantics as
20+
* {@code session.registerNodeDefinition(...)}.</p>
21+
*
22+
* @since 1.6.0
1123
*/
12-
public final class NodeRegistry {
24+
public class NodeRegistry {
1325
private final Map<Class<?>, NodeDefinition<?>> definitions = new LinkedHashMap<>();
1426

1527
/**

src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -681,6 +681,85 @@ public List<String> render(LayoutGraph graph, FixedLayoutRenderContext context)
681681
}
682682
}
683683

684+
/**
685+
* Regression for v1.6.7 Track I3: {@code session.registry().register(...)}
686+
* must invalidate the layout cache the same way
687+
* {@link DocumentSession#registerNodeDefinition(NodeDefinition)} does.
688+
* Before the fix the registry was mutated in place but the cached
689+
* {@link LayoutGraph} stayed pinned to the previous compile, so a
690+
* follow-up call to {@code render(...)} or {@code layoutGraph()} would
691+
* silently return the stale graph that still routed through the old
692+
* definition.
693+
*/
694+
@Test
695+
void registryRegisterInvalidatesCachedLayoutFromPreviousCompile() throws Exception {
696+
FixedLayoutBackend<List<String>> backend = new FixedLayoutBackend<>() {
697+
@Override
698+
public String name() {
699+
return "badge-backend";
700+
}
701+
702+
@Override
703+
public List<String> render(LayoutGraph graph, FixedLayoutRenderContext context) {
704+
return graph.fragments().stream()
705+
.map(fragment -> (String) fragment.payload())
706+
.toList();
707+
}
708+
};
709+
710+
// Replacement definition that prefixes the badge label so the
711+
// before / after snapshots are trivially distinguishable.
712+
NodeDefinition<BadgeNode> uppercaseDef = new NodeDefinition<>() {
713+
@Override
714+
public Class<BadgeNode> nodeType() {
715+
return BadgeNode.class;
716+
}
717+
718+
@Override
719+
public PreparedNode<BadgeNode> prepare(BadgeNode node, PrepareContext ctx, BoxConstraints constraints) {
720+
return PreparedNode.leaf(node, new MeasureResult(40, 18));
721+
}
722+
723+
@Override
724+
public PaginationPolicy paginationPolicy(BadgeNode node) {
725+
return PaginationPolicy.ATOMIC;
726+
}
727+
728+
@Override
729+
public List<LayoutFragment> emitFragments(PreparedNode<BadgeNode> prepared, FragmentContext ctx, FragmentPlacement placement) {
730+
return List.of(new LayoutFragment(
731+
placement.path(),
732+
0,
733+
0,
734+
0,
735+
placement.width(),
736+
placement.height(),
737+
"UPPER-" + prepared.node().label()));
738+
}
739+
};
740+
741+
try (DocumentSession session = GraphCompose.document()
742+
.pageSize(200, 160)
743+
.margin(DocumentInsets.of(10))
744+
.create()) {
745+
746+
session.registerNodeDefinition(new BadgeNodeDefinition());
747+
session.add(new BadgeNode("Badge", "alpha", DocumentInsets.zero(), DocumentInsets.zero()));
748+
749+
// First render warms the layout cache against the original definition.
750+
assertThat(session.render(backend)).containsExactly("alpha");
751+
752+
// Swap the definition through the registry() path (not the
753+
// dedicated session.registerNodeDefinition path). Without the
754+
// I3 invalidate-on-mutate guard the cache would still point at
755+
// the previously compiled layout and the next render would
756+
// return "alpha".
757+
session.registry().register(uppercaseDef);
758+
759+
assertThat(session.render(backend)).containsExactly("UPPER-alpha");
760+
}
761+
}
762+
684763
@Test
685764
@DisabledIfSystemProperty(named = "no.poi", matches = "true",
686765
disabledReason = "Exercises DocxSemanticBackend; skipped under the no-poi profile that excludes poi-ooxml from the test classpath")

0 commit comments

Comments
 (0)