fix: implement specific interfaces in ElementalNode classes #30
Conversation
WalkthroughAdds generics to UnsupportedJsonValueImpl and Elemental*Node classes, introduces an ASM-based post-processor that injects Elemental JSON interfaces into compiled classes, updates Maven to run the post-processor and exclude its artifacts, and adds tests verifying runtime type preservation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.java:
- Around line 41-57: The visit flow in ClassVisitorImpl must always call
super.visit(...) and must guard against null signatures and detectInterface
returning null: remove early returns that skip calling super.visit(...) (ensure
super.visit is invoked regardless of transformations), wrap SignatureReader
usage with a null-check on the signature variable before constructing new
SignatureReader(signature), and when using detectInterface(...) skip
appending/adding an interface if detectInterface returns null (do not add a null
entry to the interfaces array). Also stop relying on ClassWriter.COMPUTE_FRAMES
for masking invalid bytecode — ensure proper visit order and null-guards so
frames aren’t required to compensate.
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.java (1)
88-127: Consider handling type variables / wildcards in generic argument parsing.If
UnsupportedJsonValueImpl<T>is ever implemented with a type variable (or something like? extends JsonValue),visitClassTypemight not observe a concrete class internal name for the argument, and you’ll returnnull(or the wrong type). If you intend to support only the concreteJsonObject/JsonArray/...wrappers, explicitly validate the detected name and fail fast with a clear error.pom.xml (1)
139-162: Exec plugin wiring is OK; consider making the target list less brittle.Hardcoding 6 class paths means adding a new Elemental node type requires pom edits. If feasible, consider passing the output directory and scanning for
Elemental*Node.class(or scanning forUnsupportedJsonValueImplimplementers) instead.src/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.java (1)
37-91: Good coverage for the regression; consider asserting the cast path explicitly.Since the original issue is a
ClassCastException, consider adding an explicit cast in each test (even if you keep theinstanceofassertion) to match the real caller behavior more closely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pom.xmlsrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.544Z
Learnt from: javier-godoy
Repo: FlowingCode/JsonMigrationHelper PR: 11
File: src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java:281-301
Timestamp: 2025-11-25T16:35:42.544Z
Learning: In the JsonMigrationHelper project, code copied from the Vaadin codebase should be kept consistent with the original source unless there's a specific reason to deviate.
Applied to files:
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java
🧬 Code graph analysis (4)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (7)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(26-42)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(223-276)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(26-42)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (8)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
SuppressWarnings(68-73)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(223-276)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(26-42)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(26-37)
🔇 Additional comments (8)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
22-22: LGTM! Generic type parameter added correctly.The addition of the
JsonNulltype parameter toUnsupportedJsonValueImplis correct and enables the ASM post-processor to inject the appropriate Elemental interface at build time. This change is consistent with the pattern applied across all ElementalNode classes.Also applies to: 27-27
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
34-34: LGTM! Generic type parameter added correctly.The addition of the
JsonObjecttype parameter toUnsupportedJsonValueImplis correct and enables the ASM post-processor to inject the appropriate Elemental interface at build time.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
22-22: LGTM! Generic type parameter added correctly.The addition of the
JsonBooleantype parameter toUnsupportedJsonValueImplis correct and enables the ASM post-processor to inject the appropriate Elemental interface at build time. This change is consistent with the pattern applied across all ElementalNode classes.Also applies to: 27-27
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
32-32: LGTM! Generic type parameter added correctly.The addition of the
JsonArraytype parameter toUnsupportedJsonValueImplis correct and enables the ASM post-processor to inject the appropriate Elemental interface at build time.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
22-22: LGTM! Generic type parameter added correctly.The addition of the
JsonStringtype parameter toUnsupportedJsonValueImplis correct and enables the ASM post-processor to inject the appropriate Elemental interface at build time. This change is consistent with the pattern applied across all ElementalNode classes.Also applies to: 27-27
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
22-27: Consistent generic specialization; looks good.
UnsupportedJsonValueImpl<JsonNumber>aligns this node with the otherElemental*Nodewrappers and enables your ASM step to infer the intended Elemental interface.src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
25-25: Generic marker interface change is fine; confirm API impact scope.The type parameter isn’t used in code, but it’s valuable for downstream signature parsing. Just make sure this project doesn’t promise third-party implementations of
UnsupportedJsonValueImpl(this is a source-level breaking change for implementers).pom.xml (1)
243-246: Artifact exclusions only apply to thedirectoryprofile; confirm that’s intentional.If the goal is “never ship the post-processor in published artifacts”, you likely want these excludes in the main build (not just
directory). If it’s only an add-on packaging concern, this is fine.Also applies to: 259-261
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
25-25: Consider documenting the purpose of the unused type parameter.The generic type parameter
Tis never referenced within the interface body. This is intentional—it serves purely as metadata for the ASM post-processor to determine which Elemental JSON interface to inject at build time. However, this unusual pattern may confuse maintainers.📝 Suggested documentation
+/** + * Base interface for Elemental JSON wrappers around Jackson nodes. + * The type parameter T is used by {@code ElementalNodeAsmPostProcessor} at build time + * to inject the corresponding Elemental interface into the compiled class file. + */ interface UnsupportedJsonValueImpl<T extends JsonValue> extends JsonValue {src/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.java (1)
37-91: LGTM: Comprehensive type preservation tests.The test suite effectively verifies that
convertToClientCallableResultpreserves concreteJsonValuesubtypes for all six Elemental JSON types. Each test follows a clear pattern: create input, convert, and assert the result is an instance of the expected type.These tests appropriately align with the PR's objective of satisfying
instanceofchecks and explicit casts. Given that the injected interfaces do not implement bridge methods (as noted in the PR description), testing actual method invocation would fail, so focusing on type identity is correct.Optional enhancement: Consider adding tests that verify data preservation through the conversion, for example:
@Test public void testConvertToClientCallableResult_JsonObject_PreservesData() { JsonObject input = Json.createObject(); input.put("key", "value"); JsonValue result = helper.convertToClientCallableResult(input); assertTrue("Result should be an instance of JsonObject", result instanceof JsonObject); // Verify data is preserved through Jackson methods (which are implemented) assertEquals("value", ((JsonNode) result).get("key").asText()); }This would confirm that the underlying Jackson data remains accessible.
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
22-28: Correct specialization toUnsupportedJsonValueImpl<JsonBoolean>(enables post-processor inference).Consider adding a short comment that the generic parameter is intentionally used by the ASM post-processor (to avoid “cleanup” refactors removing it). Based on learnings, keep deviations from Vaadin-copied code explicitly justified.
Proposed comment to make the intent explicit
@@ @SuppressWarnings("serial") +// Type argument is used by the ASM post-processor to inject the corresponding Elemental Json* interface. class ElementalBooleanNode extends BooleanNode implements UnsupportedJsonValueImpl<JsonBoolean> {src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
22-28: Correct specialization toUnsupportedJsonValueImpl<JsonString>(enables post-processor inference).Same suggestion as the other nodes: add a brief comment that the generic parameter is intentionally used by the post-processor. Based on learnings, this also helps document why this file intentionally diverges from the upstream Vaadin source (if it was copied).
Proposed comment to make the intent explicit
@@ @SuppressWarnings("serial") +// Type argument is used by the ASM post-processor to inject the corresponding Elemental Json* interface. class ElementalStringNode extends StringNode implements UnsupportedJsonValueImpl<JsonString> {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pom.xmlsrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNodeAsmPostProcessor.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.544Z
Learnt from: javier-godoy
Repo: FlowingCode/JsonMigrationHelper PR: 11
File: src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java:281-301
Timestamp: 2025-11-25T16:35:42.544Z
Learning: In the JsonMigrationHelper project, code copied from the Vaadin codebase should be kept consistent with the original source unless there's a specific reason to deviate.
Applied to files:
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25Test.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java
🧬 Code graph analysis (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(26-37)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(26-37)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(26-42)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(31-57)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(26-42)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(26-47)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(33-60)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(26-37)
🔇 Additional comments (7)
src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
5-5: LGTM: Copyright year updated.The copyright range correctly reflects the 2025-2026 timeframe.
pom.xml (2)
243-260: LGTM: Post-processor correctly excluded from artifacts.The configuration properly excludes
ElementalNodeAsmPostProcessorfrom both Javadoc generation and the final JAR. This is appropriate since the post-processor is a build-time tool only.
139-162: Correct the release date for exec-maven-plugin.The exec-maven-plugin version 3.1.0 was released on July 15, 2022 (not January 2022). The ASM dependency version 9.8 (confirmed at line 71) has no publicly known CVEs or security vulnerabilities. Both versions are secure for use, though newer versions are available.
Likely an incorrect or invalid review comment.
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
32-32: LGTM: Generic type parameter correctly specified.The change from
implements UnsupportedJsonValueImpltoimplements UnsupportedJsonValueImpl<JsonArray>correctly specifies the target Elemental interface for the ASM post-processor to inject. This aligns with the array semantics of this class.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
22-27: LGTM: Generic type parameter correctly specified.The addition of the
JsonNullimport and the change fromimplements UnsupportedJsonValueImpltoimplements UnsupportedJsonValueImpl<JsonNull>correctly specifies the target Elemental interface for the ASM post-processor. This is consistent with the otherElemental*Nodeclasses in the PR (e.g.,ElementalArrayNode,ElementalBooleanNode,ElementalStringNode,ElementalNumberNode,ElementalObjectNode).src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
5-5: License header year bump looks fine.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
5-5: License header year bump looks fine.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #29
This commit implements a build-time transformation to automate the alignment of Jackson-backed nodes with specific JSON contracts. The process uses bytecode manipulation to inject interfaces into compiled classes, allowing them to satisfy type requirements that are otherwise incompatible at the source level.
Technical Implementation
The transformation is handled by a processor that intercepts class definitions during the build lifecycle to perform the following:
UnsupportedJsonValueImpl<T>relationship. By inspecting the class metadata, it extracts the specific typeT(such asJsonObjectorJsonArray).Runtime Behavior
Instrumented classes will successfully satisfy instanceof checks and explicit casts. This allows these objects to pass through result conversion logic that requires the specific JSON interface types. Because the transformation only modifies the class header and does not generate bridge methods, calling an Elemental JSON method will fail at runtime.
Since Vaadin 25 is built to work with Jackson nodes, the code that actually interacts with these objects is written to call the Jackson methods (which are physically present and fully implemented) rather than the Elemental interface methods. The missing Elemental methods are never actually invoked by the framework's execution path.
This does not introduce a breaking change, because a functional constraint was already established in JsonMigrationHelper 0.9.1, specifying that the
JsonValuereturned byconvertToClientCallableResultserves strictly as a transport mechanism for the underlying Jackson data and it is not intended for use as a functional Elemental JSON object (sorry, Liskov).Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.