fix(document): invalidate layout cache on registry().register(...) (I3)#113
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the v1.6.7 Track I3 item from the readiness taskboard. Fixes
a latent cache-staleness bug on
DocumentSession.registry()and addsthe regression test the taskboard asked for.
The bug
DocumentSession.registry()returned the rawNodeRegistry, socallers mutating it directly via
session.registry().register(...)left the cached
LayoutGraphpointing at the previous compile —only
session.registerNodeDefinition(...)invalidated the cache.The two entry points had different caching semantics for what is
the same underlying mutation. A subsequent
render(...)/layoutGraph()call would silently return the stale graph routedthrough the old definition.
The fix (Variant B — additive, non-breaking)
finalmodifier fromNodeRegistrysoDocumentSessioncan install a session-owned subclass. Binary-compatible — japicmp
reports the change as
semver PATCH, compatible bug fix.InvalidatingNodeRegistrysubclass insideDocumentSessionwhoseregister(...)callssuper.register(...)followed by
invalidate(). The session'sregistryfield isinitialised with this subclass, so every path that mutates the
registry goes through cache invalidation (the early
default-definitions setup is safe —
layoutCacheis field-initialised before the constructor body).
invalidate()fromregisterNodeDefinition; the wrapper handles it.registry()andregisterNodeDefinitionupdated toreflect the equivalent semantics.
Why Variant B, not Variant A
The taskboard offered "read-only view OR deprecate with invalidate-
on-mutate guard". Variant A (read-only view) would have changed the
registry()return type to an immutable subset, breaking any callerthat already typed it as
NodeRegistry. Variant B keeps the publicshape, fixes the bug for the broken call site, and leaves the cleaner
"immutable view" redesign as a 2.0 candidate.
Test plan
./mvnw test -pl . -Dtest=DocumentSessionTest#registryRegisterInvalidatesCachedLayoutFromPreviousCompilepasses after the fix.
./mvnw verify -pl . -P japicmp— 1032 tests (+1 new),0 failures; japicmp reports
semver PATCH(compatible bugfix), no incompatible changes vs the v1.6.6 baseline.