feat: add utility for class instrumentation AND feat: add JsonCodec encodeWithoutTypeInfo #16
Conversation
WalkthroughAdds runtime bytecode instrumentation to produce instrumented Vaadin component subclasses that adapt ClientCallable/LegacyClientCallable methods for JsonValue handling; introduces instrumentation helpers, route registration support, new annotations, JsonCodec/JsonMigration API extensions, README examples, and an optional ASM dependency. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
README.md (1)
16-17: Clarify the purpose of JsonSerializer and JsonCodec in the features bullet.Line 17 describes these classes but doesn't explain their purpose. Consider revising to clarify what serialization/deserialization value they provide to users.
-- **JsonSerializer and JsonCodec**: Includes `JsonSerializer` and `JsonCodec` classes for serialization and deserialization of elemental JSON values. +- **JSON serialization/deserialization utilities**: Includes `JsonSerializer` and `JsonCodec` classes to convert between Java objects and elemental JSON values.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
39-41: Remove unused imports.The imports for
NodeandReturnChannelRegistrationare not used anywhere in this file.Apply this diff to remove the unused imports:
-import com.vaadin.flow.dom.Node; import com.vaadin.flow.internal.ReflectTools; -import com.vaadin.flow.internal.nodefeature.ReturnChannelRegistration;src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentedRoute.java (1)
33-35: Consider adding@Documentedfor consistency.
LegacyClientCallableincludes@Documentedbut this annotation does not. SinceInstrumentedRouteis part of the public API and will appear in component class declarations, adding@Documentedwould ensure it appears in generated Javadoc.+import java.lang.annotation.Documented; ... @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.TYPE) +@Documented public @interface InstrumentedRoute {src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
56-60: Avoid reassigning method parameter.Reassigning
navigationTargeton line 58 is a code smell that can make the method harder to reason about. Consider using a local variable instead.String route = annotation.value(); + Class<? extends Component> targetClass = navigationTarget; if (Version.getMajorVersion() > 24) { - navigationTarget = JsonMigration.instrumentClass(navigationTarget); + targetClass = JsonMigration.instrumentClass(navigationTarget); } - RouteConfiguration.forApplicationScope().setRoute(route, navigationTarget); + RouteConfiguration.forApplicationScope().setRoute(route, targetClass);src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
348-379: Consider usingType.getDescriptor()from ASM.The
getTypeDescriptormethod manually builds type descriptors, but ASM'sType.getDescriptor(Class<?>)already provides this functionality (and is already imported). This would reduce code duplication and potential for errors.private String getTypeDescriptor(Class<?> type) { - if (type == Void.TYPE) { - return "V"; - } - if (type == Boolean.TYPE) { - return "Z"; - } - // ... all the manual cases - return "L" + type.getName().replace('.', '/') + ";"; + return Type.getDescriptor(type); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(2 hunks)pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentedRoute.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java(3 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(2 hunks)
🧰 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/JsonCodec.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.javaREADME.mdsrc/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
🧬 Code graph analysis (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
JsonMigration(44-208)
🔇 Additional comments (9)
README.md (1)
128-151: I'll verify the public API classes and annotations referenced in the instrumentation example by searching the codebase for their definitions.
<tool_calls>
#!/bin/bashSearch for the public instrumentation-related classes and annotations
Find @InstrumentedRoute annotation
echo "=== Searching for @InstrumentedRoute ==="
rg -n '@interface InstrumentedRoute|class InstrumentedRoute' --type java -A 5echo ""
echo "=== Searching for @LegacyClientCallable ==="
rg -n '@interface LegacyClientCallable|class LegacyClientCallable' --type java -A 5echo ""
echo "=== Searching for InstrumentationViewInitializer ==="
rg -n 'class InstrumentationViewInitializer' --type java -A 10echo ""
echo "=== Searching for registerInstrumentedRoute method ==="
rg -n 'registerInstrumentedRoute' --type java -B 2 -A 3
</function_calls>pom.xml (1)
71-76: ASM 9.8 dependency addition looks good.ASM 9.8 is a current, stable release and appropriately marked as optional since bytecode instrumentation is an optional feature. The exact version pinning provides reproducible builds. No known security concerns identified for this version.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (2)
64-64: LGTM!Making the class public is appropriate for the new API surface supporting class instrumentation.
64-153: New public API methods are not tested, documented, or used within the codebase.The methods
canEncodeWithoutTypeInfo()andencodeWithoutTypeInfo()are newly made public but lack integration with the rest of the project:
- No test coverage: The repository contains no test files (no
src/testdirectory). These new public methods have no dedicated tests.- No internal usage: The methods are not used anywhere in the instrumentation code or elsewhere in the codebase.
encodeWithoutTypeInfo()only callscanEncodeWithoutTypeInfo()internally (lines 138, 150).- No documentation: While the README lists
JsonCodecas a feature for "serialization and deserialization," the new public methods are not documented in the Developer Guide or README.Consider:
- Adding test coverage for the new public methods
- Clarifying the intended use case (external API? future internal use?)
- Documenting these methods in the Developer Guide if they are part of the public API contract
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
39-40: Well-designed interface method for instrumentation.The method signature correctly uses
Class<? extends T>return type, allowing implementations to return either the original class (legacy path) or an instrumented subclass (Vaadin 25+ path). This is consistent with the strategy pattern used elsewhere in this interface.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
45-48: Clean delegation to the instrumentation utility.The implementation correctly delegates to
ClassInstrumentationUtil.instrumentClass, maintaining separation of concerns.src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
34-37: Correct no-op implementation for legacy Vaadin versions.Returning the original class unchanged is the expected behavior for Vaadin 24 and earlier, where the elemental.json API is natively supported.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
172-206: Excellent documentation for the public instrumentation API.The Javadoc thoroughly documents the version-specific behavior, return value semantics, exception conditions, and cross-references to related APIs. This provides good guidance for library consumers.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
144-168: The review comment is based on a misunderstanding of the instrumentation design.The use of
getDeclaredMethods()is intentional and correct. The class documentation (lines 53-72) clearly states that this utility creates dynamic subclasses that override methods annotated with@ClientCallable. The instrumentation works by creating a wrapper subclass (e.g.,ClassName$Instrumented) for the specific class passed toinstrumentClass().If a parent class has
@ClientCallablemethods returningJsonValue, that parent class is instrumented separately when it's passed toinstrumentClass(). Child classes only need their own declared methods instrumented. This is not a limitation—it's the correct design for a wrapper-based instrumentation system. Including inherited methods would result in redundant or incorrect overrides.Likely an incorrect or invalid review comment.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
instrumentation
branch
from
353df8c to
b433298
Compare
javier-godoy
force-pushed
the
instrumentation
branch
from
b433298 to
3353729
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
145-151: Consider adding service registration note.The
ViewInitializerImplexample shows extendingInstrumentationViewInitializer, but doesn't mention that the implementation needs to be registered as a Vaadin service. Users may need to add aMETA-INF/services/com.vaadin.flow.server.VaadinServiceInitListenerfile or use@SpringComponentannotation (for Spring) to ensure the initializer is discovered.Consider adding a note like:
**Note:** Register `ViewInitializerImpl` via `META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener` or use `@SpringComponent` with Spring.src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.java (1)
29-37: Annotation is properly defined.The annotation has the correct retention policy, target, and is documented. Consider expanding the Javadoc to explain when to use
@LegacyClientCallableinstead of@ClientCallable(i.e., when methods haveJsonValueparameters).Optional enhancement for the Javadoc:
/** - * When instrumented, publishes the annotated method as if {@link ClientCallable} has been used. + * Marks a method to be published as callable from client-side JavaScript when the containing + * class is instrumented. Use this annotation instead of {@link ClientCallable} when the method + * has {@code JsonValue} parameters, which are not directly supported by {@code @ClientCallable} + * in Vaadin 25+. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentedRoute.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentedRoute.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java
- pom.xml
🧰 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/ClassInstrumentationUtil.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.javaREADME.md
🧬 Code graph analysis (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
JsonMigration(44-208)
🔇 Additional comments (8)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
39-40: LGTM!The new
instrumentClassmethod signature is well-designed with proper generic bounds, ensuring type safety when returning an instrumented subclass of the provided component class.README.md (1)
128-134: Good documentation of the instrumentation complexity.The note about instrumentation being a complex mechanism and suggesting a rewrite of affected code is helpful for users to understand the tradeoffs.
src/main/java/com/flowingcode/vaadin/jsonmigration/InstrumentationViewInitializer.java (1)
48-61: Well-structured route registration logic.The implementation correctly:
- Validates the
@InstrumentedRouteannotation presence before proceeding- Only instruments classes when Vaadin major version > 24
- Registers routes at application scope
The Javadoc accurately describes the behavior.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (5)
65-66: Good use of WeakHashMap with proper synchronization.The
WeakHashMapforclassLoaderCacheallows classloaders to be garbage collected, preventing memory leaks in dynamic classloader environments. The synchronized block ingetOrCreateInstrumentedClassLoaderensures thread-safe access.Also applies to: 193-197
95-145: Comprehensive validation and error handling.The method properly validates input constraints (null, interface, primitive, array, final class) and handles constructor accessibility checks. The exception handling for
setAccessibleis appropriately wrapped.
267-284: Potential runtime ClassCastException on JsonValue subtype parameters.When
paramType != JsonValue.class(e.g.,JsonObject), the code emits aCHECKCASTafterconvertToJsonValue. If the actual JSON structure doesn't match the expected subtype (e.g., method expectsJsonObjectbut receives aJsonArray), this will throwClassCastExceptionat runtime. This is acceptable behavior but consider documenting this constraint.
347-362: Type mapping covers common cases.The
getConvertedTypeDescriptormethod maps elemental JSON types to Jackson equivalents. The fallback toJsonNodefor unrecognizedJsonValuesubtypes is appropriate.
151-175: I'll help you verify whether thegetInstrumentableMethodsmethod should include inherited methods from superclasses. Let me investigate the codebase to understand the design intent and if this is truly a concern.
<function_calls>
#!/bin/bashFirst, let's understand the broader context of class instrumentation
Search for usages of getInstrumentableMethods and related method collection patterns
rg -n "getInstrumentableMethods|getDeclaredMethods" --type=java -B3 -A3 | head -80
</function_calls><function_calls>
#!/bin/bashSearch for @ClientCallable and @LegacyClientCallable annotations to understand usage patterns
rg -n "@ClientCallable|@LegacyClientCallable" --type=java -B2 -A2 | head -150
</function_calls><function_calls>
#!/bin/bashLook for class hierarchy/inheritance patterns in tests related to instrumentation
fd -e java -type f | xargs rg -l "extends.@ClientCallable|extends.@LegacyClientCallable|class.extends.{" | head -20
</function_calls><function_calls>
#!/bin/bashFind the complete context of getInstrumentableMethods and how it's used
rg -n "getInstrumentableMethods" --type=java -B5 -A10
</function_calls>
javier-godoy
force-pushed
the
instrumentation
branch
from
3353729 to
ac50922
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
128-153: Consider clarifying instrumentation guidance and META-INF/services registration.The instrumentation section explains the what but could benefit from more explanation of the why. Users might better understand the need for instrumentation if you briefly explain that
@ClientCallabledoesn't supportJsonValueparameters due to framework limitations, and instrumentation is one way to work around this.Additionally, the META-INF/services registration comment could be more explicit. Consider adding a brief example of the actual configuration file structure (e.g., a file named
com.vaadin.flow.server.VaadinServiceInitListenerinMETA-INF/services/with the class name as content).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 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:
README.md
🔇 Additional comments (3)
README.md (3)
16-17: Approve feature bullets.These additions appropriately highlight the new ClientCallable and JSON serialization capabilities. The descriptions are clear and concise.
128-153: I'll help you verify the instrumentation API classes and the completeness of the documentation. Let me search the codebase for these classes and methods.
<function_calls>
#!/bin/bashSearch for the instrumentation-related classes
echo "=== Searching for LegacyClientCallable ==="
rg -n 'class LegacyClientCallable|@interface LegacyClientCallable' --type=java -B 2 -A 5echo -e "\n=== Searching for InstrumentedRoute ==="
rg -n 'class InstrumentedRoute|@interface InstrumentedRoute' --type=java -B 2 -A 5echo -e "\n=== Searching for InstrumentationViewInitializer ==="
rg -n 'class InstrumentationViewInitializer' --type=java -B 2 -A 10echo -e "\n=== Searching for JsonMigration.instrumentClass ==="
rg -n 'instrumentClass|JsonMigration' --type=java -B 2 -A 5 | head -50echo -e "\n=== Searching for META-INF/services ==="
find . -path '/META-INF/services/' 2>/dev/null | head -20
</function_calls>
122-126: ClientCallable example is accurate.The verification confirms both claims:
@ClientCallablemethods can returnJsonValuedirectly (as evidenced byClassInstrumentationUtil.java:166checkingisCallable && hasJsonValueReturn), andJsonMigration.convertToClientCallableResult()is the correct public static helper method for this purpose (defined inJsonMigration.java:76-78with generic signature<T extends JsonValue> T convertToClientCallableResult(T object)).
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.