feat: add support for private callable methods #18
Conversation
WalkthroughAggregates callable handler methods across class hierarchies, enables instrumentation of private Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas needing extra attention:
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 (4)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java (1)
6-12: JsonNumber fixture looks good; double‑check visibility vs. “Private” namingThe test setup (trace +
Json.create(0)asJsonNumber) is correct and aligned with the otherLegacyClientCallablefixtures.One thing to confirm: the class is named
LegacyClientCallablePrivate__JsonNumber, but the annotatedtestmethod ispublic. If otherLegacyClientCallablePrivate__*fixtures useprivatemethods to exercise the new private‑callable support, you might want to make this methodprivateas well for consistency, unless this one is intentionally public for a specific scenario.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java (1)
4-13: Consider adding a conditional test skip mechanism.The comment on line 12 indicates this test requires Vaadin 25 on the classpath and won't work otherwise. Consider using JUnit's
@Ignorewith a clear reason, or anAssume.assumeTrue()check to gracefully skip when Vaadin 25 is not available:+import org.junit.Assume; +import org.junit.Before; ... // this test doesn't work as-in because it requires Vaadin 25 in the classpath public class LegacyClientCallablesPrivateTest25 extends LegacyClientCallablesPrivateTest { + + @Before + public void checkVaadin25Available() { + try { + Class.forName("tools.jackson.databind.node.NullNode"); + } catch (ClassNotFoundException e) { + Assume.assumeTrue("Vaadin 25 not available", false); + } + }Also, the
tools.jackson.databindpackage path appears to be Vaadin 25's repackaged Jackson. Please verify this is the correct package for your target Vaadin 25 version.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
359-364: Minor: Fix typo in comment.The comment says "unresolve" but the method being invoked is
unreflect.- // Invoke Lookup.unresolve(method) + // Invoke Lookup.unreflect(method)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (1)
42-43: Consider usingassertNotNullfor clearer intent.- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);This more clearly expresses the assertion intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(8 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java(1 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/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java
🔇 Additional comments (25)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java (1)
5-11: LGTM!The test fixture correctly implements a private method with
@LegacyClientCallableannotation, consistent with the class naming convention for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java (1)
5-11: LGTM!The test fixture correctly implements a private method with
@LegacyClientCallableannotation forJsonBooleanparameter type testing.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java (1)
3-9: LGTM!The test fixture correctly implements a private method with
@LegacyClientCallableannotation forStringparameter type testing.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java (1)
1-9: LGTM!This test fixture correctly defines a private method with a boolean parameter annotated with
@LegacyClientCallable. The implementation follows the expected pattern for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java (1)
1-5: LGTM!This empty extension class serves as a test fixture to verify that private callable instrumentation works correctly with inheritance hierarchies.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java (1)
1-13: LGTM!The test fixture correctly returns a JsonString value. Note that while the class name includes "Private", this method is public, which aligns with the test suite's goal of validating that instrumentation doesn't break existing public callables alongside the new private callable support.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java (1)
1-11: LGTM!This test fixture correctly defines a private method with a JsonArray parameter. The implementation follows the expected pattern for testing private callable support with JSON types.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java (1)
1-9: LGTM!This test fixture correctly defines a private method with a double parameter. The implementation follows the expected pattern for testing private callable support with primitive types.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java (1)
1-13: LGTM!The test fixture correctly returns a JsonObject value. Consistent with the test suite pattern, this public method verifies that instrumentation maintains compatibility with existing public callables.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java (1)
1-11: LGTM!This test fixture correctly defines a private method with a JsonObject parameter. The implementation follows the expected pattern for testing private callable support with JSON types.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java (1)
1-10: LGTM!The test fixture correctly returns an int value. This public method fixture ensures that instrumentation maintains compatibility with existing public callables alongside the new private callable support.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java (1)
3-8: Private void fixture looks correct
LegacyClientCallablePrivate__Vcorrectly defines a private, no-arg, void@LegacyClientCallablethat just callstrace(), matching the intended private-callable test pattern.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java (1)
3-10: Private JsonNull-arg fixture is consistent
LegacyClientCallablePrivate_JsonNull__Vdefines a private@LegacyClientCallablewith aJsonNullparameter and void return, which matches the private-parameter coverage pattern for these tests.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java (1)
3-8: Int-arg private void fixture is correct
LegacyClientCallablePrivate_I__Vproperly declares a private@LegacyClientCallablewith anintparameter and void return, which aligns with the private-callable coverage pattern.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java (1)
3-10: JsonString-arg private fixture matches pattern
LegacyClientCallablePrivate_JsonString__Vcorrectly defines a private@LegacyClientCallabletaking aJsonStringand returning void, consistent with the rest of the private-parameter fixtures.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java (1)
6-12: Method visibility may not match class naming convention.The class name
LegacyClientCallablePrivate__JsonValuesuggests this is a test fixture for private callables, but thetest()method is declared aspublic. Other fixtures in this PR (e.g.,LegacyClientCallablePrivate_JsonNumber__V) declare their methods asprivate. Please verify whether this should beprivatefor consistency with the naming pattern, or if the public visibility is intentional.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java (1)
5-11: LGTM!The test fixture correctly implements a private method annotated with
@LegacyClientCallable, with proper naming convention and trace verification pattern.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (4)
167-185: LGTM!The
getAllCallablesmethod correctly aggregates callable methods from the class hierarchy, detects conflicts when methods share a name but have different parameter types, and ensures subclass methods take precedence. The error message is clear and actionable.
280-311: LGTM!The implementation correctly generates static fields for MethodHandles and initializes them in
<clinit>. The field naming by method name is safe becausegetAllCallablesprevents same-name methods with different parameter types.
474-482: LGTM!The
pushInthelper correctly handles the various int constant opcodes:ICONST_*for -1 to 5,BIPUSHfor byte range, andSIPUSHfor short range.
484-508: LGTM!The
loadClassConstanthelper correctly handles all primitive types by loading the correspondingTYPEfield from wrapper classes, and usesType.getType()for reference types.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (2)
60-67: LGTM!The
testExtends__Vtest properly validates that instrumentation works for subclasses of private callable fixtures.
109-127: No changes needed. The fixture classes declaretest()aspublic, notprivate. The "Private" in class names (LegacyClientCallablePrivate__Z, etc.) refers to the callable's metadata designation, not the Java access modifier. Direct calls likenonInstrumented.test()on lines 116 and 126 are valid and will compile correctly.Likely an incorrect or invalid review comment.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java (2)
6-11: LGTM!The class correctly extends the base test class and overrides
instrumentClassto useLegacyJsonMigrationHelper, which is appropriate for Vaadin 24 testing. Creating a new helper instance per call ensures test isolation.
13-41: No issues identified. The JSON factory methods correctly useelemental.json.Jsonfor Vaadin 14, which is the project's target version as specified in pom.xml. The "Test24" suffix refers to a test case variant, not Vaadin 24. TheJsonMigrationHelperis explicitly designed to handle legacy elemental.json APIs, making these methods appropriate for this codebase.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java
Show resolved
Hide resolved
...test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
private-callables
branch
from
0937288 to
b5a58b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
421-430: Useinvoke()instead ofinvokeExactfor MethodHandle invocation of private methods.The
invokeExactcall requires exact type matching. Since the instrumented class is a subclass ofmethod.getDeclaringClass(), the receiver (this) will be a subtype, causingWrongMethodTypeExceptionat runtime. ReplaceinvokeExactwithinvoke, which allows subtype compatibility.Apply this diff:
if (isPrivate) { // Call private method String descriptor = "(" + Type.getDescriptor(method.getDeclaringClass()) + superDescriptor.substring(1); mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/invoke/MethodHandle", - "invokeExact", + "invoke", descriptor, false);
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (1)
43-44: Consider usingassertNotNullfor null checks.
assertNotNull(MESSAGE, testMethod)is more idiomatic and provides clearer failure diagnostics thanassertTrue(MESSAGE, testMethod != null).- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);Also add the import:
import static org.junit.Assert.assertNotNull;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(8 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java
🚧 Files skipped from review as they are similar to previous changes (15)
- src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.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/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java
🔇 Additional comments (17)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java (1)
1-12: Private JsonNull callable fixture looks correctClass name, private visibility of
test(),@LegacyClientCallableusage,trace()call, and returningJson.createNull()are all consistent and appropriate for this private-callable fixture.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java (1)
1-11: LGTM! Test fixture is well-structured.The test class correctly validates private method instrumentation for
@LegacyClientCallablewith aJsonValueparameter. The unused parameter is expected in this context—the test verifies that the instrumentation framework can invoke the private method with the correct parameter type, withtrace()confirming successful invocation.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java (1)
8-12: LGTM! Method visibility corrected to private.The method is now correctly declared as
private, matching the fixture naming convention and ensuring the private-method callable path is exercised.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java (1)
8-12: LGTM! Private method correctly implements test fixture.The private method declaration and JsonValue return type correctly exercise the private callable instrumentation path.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java (1)
8-12: LGTM! Private method correctly implements test fixture.The private method declaration correctly matches the fixture intent and will exercise the private callable path.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (4)
155-165: LGTM! Correctly updated to include private methods.The parameter rename and filter adjustment correctly enable private methods to be considered as callable candidates.
280-311: LGTM! Well-designed MethodHandle caching mechanism.The static field initialization and
<clinit>generation correctly implement a caching strategy for private method MethodHandles, avoiding repeated reflection overhead.
313-371: LGTM! Correct bytecode generation for MethodHandle lookup.The
createLookupHelpermethod correctly generates bytecode that:
- Obtains a
MethodHandles.Lookupinstance- Reflectively retrieves the private method
- Makes it accessible
- Unreflects it to obtain a MethodHandle
The implementation properly handles primitive and object parameter types.
474-508: LGTM! Correct bytecode helper utilities.Both
pushIntandloadClassConstantcorrectly generate appropriate bytecode instructions for their respective purposes, properly handling primitives and objects.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java (1)
8-12: LGTM! Method visibility corrected to private.The method is now correctly declared as
private, matching the fixture naming convention and ensuring the private-method callable path is properly tested.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java (1)
5-8: LGTM! Private void-return test fixture correctly implemented.The private method declaration properly exercises the void-return private callable path.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java (1)
5-8: LGTM! Private method with String parameter correctly implemented.The private method declaration properly tests the String parameter handling in the private callable instrumentation path.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java (1)
5-9: LGTM! Method visibility corrected to private.The method is now correctly declared as
private, consistent with the fixture naming convention and other similar test classes.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java (1)
1-10: LGTM!Test fixture follows the established pattern for private callable method testing. The naming convention
__Icorrectly indicates the return type (int), and the implementation properly callstrace()for verification.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (3)
1-14: LGTM!Clean import structure and well-designed abstract method with bounded type parameter for type safety.
56-66: LGTM!Using
Objectas the return type provides flexibility for concrete implementations to return version-specific JSON types (e.g.,elemental.jsonvsjakarta.json).
68-281: LGTM!Comprehensive test coverage for various parameter and return type combinations including primitives, wrappers, and JSON types. The inheritance test (
testExtends__V) is a valuable addition for verifying that instrumentation works correctly with class hierarchies.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
private-callables
branch
from
b5a58b4 to
067186b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
420-429: Useinvoke()instead ofinvokeExactfor private method invocation.This issue was flagged in a previous review. The
invokeExactcall requires exact type matching, but the receiver (thisloaded at line 395) is an instance of the instrumented subclass, while the descriptor expects the declaring class. This will causeWrongMethodTypeExceptionat runtime.Apply this diff:
if (isPrivate) { // Call private method String descriptor = "(" + Type.getDescriptor(method.getDeclaringClass()) + superDescriptor.substring(1); mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/invoke/MethodHandle", - "invokeExact", + "invoke", descriptor, false); }src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (1)
49-53: Add null check fortestMethodto avoid NPE.Unlike
invokeTestMethod, this method doesn't verify that the method was found before callingsetAccessible. If no method with@LegacyClientCallableexists,testMethodwill be null and line 51 will throwNullPointerException.Apply this diff:
private static Object invokeLegacyMethod(BaseClientCallable instrumented, Object... args) throws Exception { Method testMethod = getAnnotatedTestMethod(instrumented, LegacyClientCallable.class); + assertTrue("instrumented method is not annotated with @LegacyClientCallable", testMethod != null); testMethod.setAccessible(true); return testMethod.invoke(instrumented, args); }
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
312-370: Consider adding exception handling for robust initialization.The
lookup_*helper methods perform reflection operations that can throw checked exceptions (NoSuchMethodException,SecurityException,IllegalAccessException). Since these are invoked in<clinit>, any exception wraps inExceptionInInitializerError, which is difficult to diagnose.Consider wrapping the reflection calls in a try-catch that re-throws as a more descriptive
RuntimeException:// In the generated bytecode, wrap in try-catch // and throw RuntimeException with context about which method failedAlternatively, document that initialization failures are intentional fail-fast behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(8 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.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/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java
🔇 Additional comments (17)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java (1)
1-11: Private JsonArray callable test helper looks correctThe class wiring, annotation usage, visibility (
privatemethod), andJsonArrayimport all look consistent with the described pattern for otherLegacyClientCallablePrivate_*__Vtest classes. No changes requested.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java (2)
1-3: LGTM!Package declaration and import are appropriate for this test fixture.
5-10: LGTM!This test fixture correctly demonstrates a private method annotated with
@LegacyClientCallableaccepting aJsonNumberparameter. The implementation is appropriate for testing the MethodHandle-based instrumentation of private methods.The
argparameter is unused within the method body, which is acceptable for a test fixture focused on verifying invocation mechanics rather than parameter handling.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java (1)
1-11: JsonObject private void fixture looks correctClass cleanly exercises a private
@LegacyClientCallablewithJsonObjectparameter and void return viatrace(). No changes needed.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java (1)
1-11: JsonBoolean private void fixture is consistent and validPrivate
@LegacyClientCallablewithJsonBooleanargument correctly mirrors the other void-return variants and usestrace()for verification.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java (1)
1-13: JsonNumber private-return fixture is correctThis class neatly covers the private JsonNumber-return scenario (
Json.create(0)), aligning with the rest of the private callable matrix.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java (1)
1-13: JsonNull private-return fixture matches private-callable intent
test()is private, annotated, and returnsJson.createNull(), so the fixture now correctly exercises the private JsonNull-return path.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java (1)
1-10: Double-return private fixture is consistentPrivate
test()with double return andtrace()matches the pattern for primitive private-callable fixtures.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java (1)
1-9: Int-parameter private void fixture is fineThis provides the private int-parameter, void-return case and aligns with other
*_I__V-style tests.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java (1)
1-10: Boolean-return private fixture correctly configured
test()is private, annotated, and returns a constant boolean aftertrace(), matching the private-callable boolean coverage goal.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java (1)
1-10: Int-return private fixture looks goodPrivate
@LegacyClientCallablereturningintvia a simple constant plustrace()is aligned with the rest of the primitive fixtures.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java (1)
6-12: LGTM!The test fixture correctly implements a private
@LegacyClientCallablemethod, matching the fixture naming convention. The implementation is straightforward and aligns with the PR's goal of supporting private callable methods.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (3)
167-184: LGTM - Method overriding logic correctly preserves subclass methods.The logic now properly:
- Adds the method if no existing entry (subclass version seen first)
- Throws if same name but different parameter types (conflict)
- Silently skips if same name and parameters (keeps subclass version)
This correctly implements the expected override semantics when iterating from subclass to superclass.
473-507: LGTM!The bytecode utility methods are correctly implemented:
pushIntproperly selects betweenICONST_*,BIPUSH, andSIPUSHbased on value rangeloadClassConstantcorrectly handles all 8 primitive types via wrapperTYPEfields and reference types viaLDC
279-310: LGTM - Static initialization pattern for MethodHandle fields.The generated
<clinit>correctly:
- Iterates private method names
- Invokes each
lookup_*helper to obtain the MethodHandle- Stores the result in the corresponding static field
This ensures MethodHandles are initialized once at class load time.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (1)
67-280: LGTM - Comprehensive test coverage.The test suite thoroughly covers:
- Void return types with various parameter types (primitives, String, JsonValue subtypes)
- Primitive and wrapper return types (boolean, int, double, Integer)
- All JsonValue subtypes (JsonBoolean, JsonNumber, JsonString, JsonNull, JsonArray, JsonObject)
- Inheritance scenario (
ExtendsLegacyClientCallablePrivate__V)Each test properly verifies that the method was traced and compares results against non-instrumented instances.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java (1)
6-43: LGTM!Clean implementation of the abstract test base:
- Correctly delegates
instrumentClasstoLegacyJsonMigrationHelper- All JSON factory methods properly use
elemental.json.Jsonto create appropriate values
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
411-420: Useinvoke()instead ofinvokeExactfor MethodHandle invocation of private methods.This issue was previously flagged but appears unresolved. The
invokeExactcall requires exact type matching, butthisis a subclass ofmethod.getDeclaringClass(), which will causeWrongMethodTypeExceptionat runtime.mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/invoke/MethodHandle", - "invokeExact", + "invoke", descriptor, false);Note:
invokereturnsObjectand throwsThrowable, so adjust the descriptor accordingly:- String descriptor = - "(" + Type.getDescriptor(method.getDeclaringClass()) - + superDescriptor.substring(1); + String descriptor = "(" + Type.getDescriptor(Object.class); + for (Class<?> pt : method.getParameterTypes()) { + descriptor += Type.getDescriptor(Object.class); + } + descriptor += ")Ljava/lang/Object;";Then cast/unbox the result as needed based on the return type.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
315-315: Consider simplifying class constant loading.The current approach is functionally correct but verbose. You can simplify:
- mv.visitLdcInsn(Type.getObjectType(Type.getInternalName(method.getDeclaringClass()))); + mv.visitLdcInsn(Type.getType(method.getDeclaringClass()));Both produce the same bytecode, but
Type.getType(Class)is more direct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(9 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/ClassInstrumentationUtil.java
🔇 Additional comments (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (5)
155-165: LGTM!The filter correctly includes private methods by only checking for non-static modifiers, enabling the new private callable instrumentation feature.
167-184: LGTM!The method correctly aggregates callable methods across the class hierarchy, preserving the subclass version when overrides exist and properly detecting conflicts when parameter types differ.
186-207: LGTM!The method correctly uses
getAllCallablesto gather methods from the entire hierarchy, and the filtering logic properly distinguishes between legacy and current Vaadin behavior.
270-301: LGTM!The static field generation and class initializer correctly set up MethodHandle references for private methods. The approach of using method names as field names is safe since
getAllCallablesalready prevents duplicate method names.
464-498: LGTM!The
pushInthelper correctly selects optimal opcodes based on value range. TheloadClassConstanthelper properly handles all primitive types via wrapper class TYPE fields and usesType.getTypefor object types.
javier-godoy
force-pushed
the
private-callables
branch
from
ba81018 to
b8ebbcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
411-420: Useinvoke()instead ofinvokeExactfor MethodHandle invocation of private methods.The
invokeExactcall requires exact type matching. Since the instrumented class is a subclass ofmethod.getDeclaringClass(), the receiver (this) will be a subtype, causingWrongMethodTypeExceptionat runtime. ReplaceinvokeExactwithinvoke, which allows subtype compatibility.Apply this diff:
if (isPrivate) { // Call private method String descriptor = "(" + Type.getDescriptor(method.getDeclaringClass()) + superDescriptor.substring(1); mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/invoke/MethodHandle", - "invokeExact", + "invoke", descriptor, false);
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java (1)
6-11: instrumentClass override looks correct; consider reusing the helper if desiredThe generic signature and delegation to
LegacyJsonMigrationHelper().instrumentClass(clazz)look correct and type-safe for test purposes.
If this pattern is used heavily across tests and you care about a tiny allocation reduction, you could keep a singleLegacyJsonMigrationHelperinstance as a field and reuse it, but it’s not required here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(9 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNumber.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Z.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_String__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonArray__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest25.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__I.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonObject__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonNull.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ExtendsLegacyClientCallablePrivate__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNull__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__Integer.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_D__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonString.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonValue__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonValue.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonArray.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonString__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonBoolean__V.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/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java
🔇 Additional comments (16)
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_JsonNumber__V.java (1)
1-11: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod with aJsonNumberparameter, following the pattern established for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonObject.java (1)
1-13: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod returningJsonObject, following the pattern established for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_Z__V.java (1)
1-9: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod with a boolean parameter, following the pattern established for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__D.java (1)
1-10: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod returningdouble, following the pattern established for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate_I__V.java (1)
1-9: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod with anintparameter, following the pattern established for testing private callable support.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablePrivate__JsonBoolean.java (1)
1-13: LGTM!This test fixture correctly implements a private
@LegacyClientCallablemethod returningJsonBoolean, following the pattern established for testing private callable support.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (8)
31-33: LGTM!The new imports support the enhanced method aggregation logic in
getAllCallables.
155-165: LGTM!The filter change from
!Modifier.isPrivateto!Modifier.isStaticcorrectly enables private@LegacyClientCallablemethods to be processed, which is the core goal of this PR.
167-184: LGTM!The
getAllCallablesmethod correctly aggregates handler methods across the class hierarchy while preserving subclass overrides and detecting conflicts where the same method name has different parameter types.
270-301: LGTM!The infrastructure for handling private methods is well-designed: collecting private method names, generating static
MethodHandlefields, creating lookup helpers, and initializing handles in<clinit>.
303-361: LGTM!The
createLookupHelpermethod correctly generates bytecode to obtain aMethodHandlefor private methods viaMethodHandles.lookup().unreflect(), with propersetAccessible(true)handling.
377-383: LGTM!Loading the
MethodHandlefrom the static field for private methods is the correct approach for preparing the invocation.
464-498: LGTM!The utility methods
pushIntandloadClassConstantare well-implemented.pushIntuses the most efficient bytecode for each value range, andloadClassConstantcorrectly handles primitive types by loading theirTYPEfields from wrapper classes.
389-409: LGTM!The parameter conversion logic correctly handles
JsonValue-annotated parameters with proper type conversion and casting, while preserving the existing behavior for non-JsonValue parameters.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest.java (1)
1-282: LGTM!This abstract test base provides comprehensive coverage for private callable methods across various parameter and return types. The reflection-based helper methods cleanly abstract the invocation details, and the abstract fixture methods enable version-specific implementations.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesPrivateTest24.java (1)
13-41: JSON creator overrides are consistent and suitable for the testsAll JSON factory overrides correctly use
Json.create*and return values asObject, which matches the likely abstract contract in the base test. The chosen literals (true,42,"test") are fine as stable sample values for assertions.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.