From 7bf5e8707201811e4ddd85dd6d33cb4d0c719bbb Mon Sep 17 00:00:00 2001 From: DemchaAV Date: Mon, 1 Jun 2026 17:47:45 +0100 Subject: [PATCH] chore(api): InvalidatingNodeRegistry ensureOpen + negative test + NodeRegistry @since note (J2/J3/J4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small carry-over items from the v1.6.7 senior review, bundled into one PR because they share a topic (the I3 auto-invalidating registry wrapper) and a CHANGELOG section. J2 — symmetric closed-session behaviour: InvalidatingNodeRegistry.register now calls ensureOpen() before super.register(). Previously the two registration entry points (session.registry().register and session.registerNodeDefinition) disagreed on what happens after session.close(): the dedicated method threw IllegalStateException, the registry().register path silently mutated and invalidated a closed-session cache. After J2 both throw the same exception type with the same message. ensureOpen() inside the constructor's default- definitions setup is safe because the `closed` flag is field- initialised to false before the constructor body runs. J3 — negative test: DocumentSessionTest.registryRegisterOnClosedSessionThrowsIllegalStateException pins the new contract. Pairs with the existing positive cache- invalidation test from I3 so the senior-bar "positive + negative" pattern (cf PR-7.3) is complete. J4 — NodeRegistry @since note: Class-level Javadoc on NodeRegistry now explicitly calls out the v1.6.7 non-final relaxation. The class became non-final in v1.6.7 (Track I3) so DocumentSession could install the auto- invalidating subclass; the change was already binary-compatible (japicmp: semver PATCH). The Javadoc just makes the rationale discoverable without reading the CHANGELOG. Test plan: - DocumentSessionTest now has 26 tests (25 prev + 1 new negative). - ./mvnw verify -pl . -P japicmp — 1058 tests, 0 failures. japicmp vs v1.6.7 baseline: semver PATCH (compatible — no public signature changes; ensureOpen() is a behavioural strengthening on an existing public method, well-known semver-PATCH territory). --- CHANGELOG.md | 23 ++++++++++++++++ .../compose/document/api/DocumentSession.java | 21 +++++++++------ .../compose/document/layout/NodeRegistry.java | 6 +++++ .../document/api/DocumentSessionTest.java | 26 +++++++++++++++++++ 4 files changed, 68 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94806a0d..21b3b0ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,29 @@ follow-ups carried over from the v1.6.7 senior review (see [ROADMAP.md](ROADMAP.md) and the private taskboard). No breaking changes are planned. +### Fixes + +- The two `DocumentSession` registration entry points are now + **fully** interchangeable, not just cache-equivalent. + `session.registry().register(...)` now calls `ensureOpen()` + before mutating, matching the behaviour of + `session.registerNodeDefinition(...)`. Previously + `registry().register(...)` on a closed session silently mutated + the registry and invalidated a closed-session cache (harmless + but semantically odd). After this change both paths throw + `IllegalStateException` on a closed session. (Track J2 — carry- + over polish from the v1.6.7 senior review.) + +### Internal + +- `NodeRegistry` Javadoc updated to call out the v1.6.7 non-final + relaxation explicitly (Track J4). The class became non-final + in v1.6.7 (Track I3) so `DocumentSession` could install the + auto-invalidating subclass; the change was already binary- + compatible (japicmp classified it as `semver PATCH`). The + Javadoc just makes the rationale discoverable without reading + the CHANGELOG. + ### Public API - `MarkdownInline.append(...)` (the inline-markdown adapter used by 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 a21d2567..17f15d58 100644 --- a/src/main/java/com/demcha/compose/document/api/DocumentSession.java +++ b/src/main/java/com/demcha/compose/document/api/DocumentSession.java @@ -913,18 +913,23 @@ private interface PdfRenderingBody { /** * 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. + * {@link #register(NodeDefinition)} call through both + * {@link DocumentSession#ensureOpen()} and + * {@link DocumentSession#invalidate()}. The two registration entry + * points — {@code session.registry().register(...)} and + * {@link DocumentSession#registerNodeDefinition(NodeDefinition)} — + * are now fully interchangeable: both refuse to mutate a closed + * session and both invalidate the cached compile. + * + *

Added in v1.6.7 (Track I3) as cache-invalidation only; the + * {@code ensureOpen()} symmetry landed in v1.6.8 (Track J2) after + * the senior review noted that the two entry points still + * disagreed on closed-session behaviour.

*/ private final class InvalidatingNodeRegistry extends NodeRegistry { @Override public NodeRegistry register(NodeDefinition definition) { + ensureOpen(); super.register(definition); invalidate(); return this; 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 8dfe68d6..9c435885 100644 --- a/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java +++ b/src/main/java/com/demcha/compose/document/layout/NodeRegistry.java @@ -19,6 +19,12 @@ * get the same caching semantics as * {@code session.registerNodeDefinition(...)}.

* + *

Non-final since v1.6.7. The class lost its + * {@code final} modifier in v1.6.7 (Track I3) so {@code DocumentSession} + * could install the auto-invalidating subclass described above. The + * change is binary-compatible (japicmp classifies it as {@code semver + * PATCH}); standalone-registry callers see no behavioural change.

+ * * @since 1.6.0 */ public class NodeRegistry { 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 dfa95274..8c67484e 100644 --- a/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java +++ b/src/test/java/com/demcha/compose/document/api/DocumentSessionTest.java @@ -760,6 +760,32 @@ public List emitFragments(PreparedNode prepared, Frag } } + /** + * Negative-path companion to the I3 positive test above. After + * closing the session, mutating the registry through + * {@code session.registry().register(...)} must throw + * {@link IllegalStateException} — symmetric with + * {@link DocumentSession#registerNodeDefinition(NodeDefinition)}, + * which has always thrown on a closed session. Added in v1.6.8 + * (Track J2/J3) after the v1.6.7 senior review flagged that the + * two entry points still disagreed on closed-session behaviour. + */ + @Test + void registryRegisterOnClosedSessionThrowsIllegalStateException() { + DocumentSession session = GraphCompose.document() + .pageSize(200, 160) + .margin(DocumentInsets.of(10)) + .create(); + // Close via try-with-resources is the canonical path; close + // explicitly here so the assertion runs against a closed + // session. + session.close(); + + assertThatThrownBy(() -> session.registry() + .register(new BadgeNodeDefinition())) + .isInstanceOf(IllegalStateException.class); + } + @Test @DisabledIfSystemProperty(named = "no.poi", matches = "true", disabledReason = "Exercises DocxSemanticBackend; skipped under the no-poi profile that excludes poi-ooxml from the test classpath")