Add unit tests and fixes to class instrumentation #17
Conversation
WalkthroughConvert class instrumentation to an instance-based, version-aware utility with dynamic Jackson descriptor lookup; add ClassInstrumentationJacksonHelper; adjust Elemental* toJson behaviors and UnsupportedJsonValueImpl; switch migration helpers to use per-version instrumentation; and add extensive Vaadin 24/25 client-callable test harnesses and fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
46-48: Incorrect type name in error message.The error message references
JsonObjectbut the method handles generalJsonValuetypes.- object.getClass().getName() + " cannot be converted to elemental.json.JsonObject"); + object.getClass().getName() + " cannot be converted to elemental.json.JsonValue");
🧹 Nitpick comments (10)
src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
48-51: Implementation relies on all implementers extendingJsonNode.The cast
(JsonNode) thisassumes every class implementingUnsupportedJsonValueImplalso extends a JacksonJsonNodetype. This is currently valid (e.g.,ElementalNullNode extends NullNode,ElementalNumberNode extends NumericNode), but consider adding a comment documenting this constraint for future maintainers.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_Z__V.java (1)
13-14: Remove trailing blank lines.Apply this diff:
- -src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java (1)
21-22: Consider migrating from deprecated ExpectedException.
ExpectedExceptionhas been deprecated since JUnit 4.13 in favor ofassertThrows. If the project uses JUnit 4.13+ or JUnit 5, consider refactoring to the modern API for clearer test intent.Example refactor using
assertThrows(JUnit 4.13+):- @Rule - public ExpectedException thrown = ExpectedException.none(); - @Override protected <T extends Component> Class<? extends T> instrumentClass(Class<T> clazz) { + boolean expectException = false; for (Class<?> arg : getClientCallableTestMethod(clazz).getParameterTypes()) { if (JsonValue.class.isAssignableFrom(arg)) { - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage(containsString(ERRMSG)); + expectException = true; break; } } + if (expectException) { + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> new JsonMigrationHelper25().instrumentClass(clazz)); + assertTrue(exception.getMessage().contains(ERRMSG)); + return clazz; + } return new JsonMigrationHelper25().instrumentClass(clazz); }src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java (2)
16-17: Consider migrating from deprecated ExpectedException.
ExpectedExceptionhas been deprecated since JUnit 4.13. Consider refactoring toassertThrowsfor clearer test intent, similar to the suggestion for ClientCallablesTest25.
28-30: Clarify the assertion intent.The assertion
assertEquals(clazz, instrumentedClazz)suggests thatLegacyJsonMigrationHelperreturns the original class unchanged for Vaadin 24. If this is the expected behavior, consider adding a comment explaining why instrumentation is skipped.Example:
+ // For Vaadin 24, LegacyJsonMigrationHelper returns the original class without instrumentation Class<? extends T> instrumentedClazz = new LegacyJsonMigrationHelper().instrumentClass(clazz); assertEquals(clazz, instrumentedClazz);src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java (2)
24-31: Consider making test method discovery more robust.The current implementation relies on the method being named exactly "test". This is fragile and could lead to false negatives if the naming convention changes.
Consider either:
- Adding validation that exactly one @ClientCallable method exists
- Making the method name a parameter or using a more flexible search
protected static Method getClientCallableTestMethod(Class<? extends Component> clazz) { + Method found = null; for (Method method : clazz.getDeclaredMethods()) { if ("test".equals(method.getName()) && method.isAnnotationPresent(ClientCallable.class)) { - return method; + if (found != null) { + throw new IllegalStateException("Multiple test methods found in " + clazz); + } + found = method; } } - return null; + return found; }
43-44: Use assertNotNull for clearer assertion.Using
assertTrue(MESSAGE, testMethod != null)is less clear thanassertNotNull(MESSAGE, testMethod).- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (2)
24-31: Inconsistent visibility with ClientCallablesTest.In
ClientCallablesTest,getClientCallableTestMethodisprotected static(allowing subclass access), but here it'sprivate static. Consider making itprotected staticfor consistency, especially if subclasses might need to reference it.- private static Method getClientCallableTestMethod(BaseClientCallable c) { + protected static Method getClientCallableTestMethod(BaseClientCallable c) {
43-43: Use assertNotNull for clearer assertion.Using
assertTrue(MESSAGE, testMethod != null)is less clear thanassertNotNull(MESSAGE, testMethod).- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
375-386: Handle ClassNotFoundException explicitly for clarity.The reflection-based lazy loading of
ClassInstrumentationJacksonHelperuses@SneakyThrows, which suppresses checked exceptions. If the helper class is missing, the error will be less clear.Consider explicit exception handling with a descriptive message:
- @SneakyThrows private String getConvertedTypeDescriptor(Class<?> type) { if (getConvertedTypeDescriptor == null) { + try { Class<?> helper = Class.forName("com.flowingcode.vaadin.jsonmigration.ClassInstrumentationJacksonHelper"); MethodHandles.Lookup lookup = MethodHandles.lookup(); MethodType methodType = MethodType.methodType(String.class, Class.class); getConvertedTypeDescriptor = lookup.findStatic(helper, "getConvertedTypeDescriptor", methodType); + } catch (ClassNotFoundException e) { + throw new IllegalStateException( + "ClassInstrumentationJacksonHelper not found. Ensure Jackson databind is on the classpath.", e); + } catch (Exception e) { + throw new IllegalStateException("Failed to initialize type descriptor conversion", e); + } } + try { return (String) getConvertedTypeDescriptor.invokeExact(type); + } catch (Throwable e) { + throw new IllegalStateException("Failed to convert type descriptor for " + type, e); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(8 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/BaseClientCallable.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/ClientCallable_JsonString__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonArray.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonString__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNull__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonObject.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonValue__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNumber__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Z.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonValue.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonString.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonBoolean__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonValue__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNull__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonBoolean__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_Z__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java
🔇 Additional comments (54)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
31-34: Verify that returning Javanullis intentional.Returning Java
nullfromtoJson()differs from the typical JSON serialization expectation where a JSON null value would serialize to the string"null". If the intent is to produce valid JSON output, this should return"null"instead. However, if this is intentional behavior for a specific use case (e.g., signaling "no value" to callers), please confirm.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonArray__V.java (1)
8-14: LGTM!The test class follows the established naming convention and correctly implements a
@ClientCallablemethod with aJsonArrayparameter for instrumentation testing.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.java (1)
8-14: LGTM!The test class is consistent with the established pattern for
@ClientCallableparameter type testing.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonBoolean.java (1)
9-16: LGTM!The test class correctly implements a
@ClientCallablemethod returningJsonBooleanand follows the established naming convention (double underscore indicating return type).src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
45-49: LGTM! Instance-based instrumentation correctly implemented.The refactoring from static utility to instance-based, version-aware instrumentation (version 25) is correctly implemented. The
ClassInstrumentationUtilinstance is properly initialized and used.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__D.java (1)
5-5: No action needed. The@LegacyClientCallableannotation is correctly accessible.The annotation is defined in
src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable.javaand the test file is in the same package (com.flowingcode.vaadin.jsonmigration). In Java, types within the same package are directly accessible without explicit import statements. The code compiles and functions correctly.Likely an incorrect or invalid review comment.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Z.java (1)
6-12: LGTM! Test stub follows expected pattern.The test stub correctly extends
BaseClientCallableand implements a client-callable method that traces execution and returns a boolean value.Note: The AI summary incorrectly states
test(boolean arg), but the actual implementation is parameterlesstest()returning boolean.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__V.java (1)
6-11: LGTM! Test stub follows expected pattern.The void-returning client-callable test stub is correctly implemented.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_D__V.java (1)
6-11: LGTM! Test stub follows expected pattern.The test stub correctly accepts a double parameter and traces execution.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Integer.java (1)
3-9: LGTM! Legacy test stub follows expected pattern.The legacy client-callable test stub correctly returns an Integer value after tracing.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonString__V.java (1)
5-10: LGTM! Legacy test stub follows expected pattern.The legacy test stub correctly accepts a JsonString parameter and traces execution.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNumber.java (1)
9-15: LGTM! Test stub follows expected pattern.The test stub correctly returns a JsonNumber value after tracing.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNull__V.java (1)
8-13: LGTM! Test stub follows expected pattern.The test stub correctly accepts a JsonNull parameter and traces execution.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__I.java (1)
6-12: LGTM! Test stub follows expected pattern.The test stub correctly returns a primitive int value after tracing.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonValue__V.java (1)
8-14: LGTM!The test helper class correctly implements the expected pattern for ClientCallable parameter testing.
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
31-39: LGTM!The conditional formatting logic correctly handles integral double values by formatting them without decimal points, while delegating non-integral and edge cases (NaN, Infinity, overflow) to the parent implementation.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__I.java (1)
3-10: LGTM!The test helper correctly implements the legacy client-callable pattern with primitive int return type.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonValue.java (1)
9-16: LGTM!The test helper correctly returns a JsonValue (JsonObject is a valid JsonValue implementation) and follows the expected pattern.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_D__V.java (1)
3-9: LGTM!The test helper correctly implements the legacy client-callable pattern with a double parameter.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonArray.java (1)
9-16: LGTM!The test helper correctly returns a JsonArray and follows the expected pattern for testing JsonArray return types.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Integer.java (1)
6-13: LGTM!The test helper correctly returns an Integer (autoboxed from primitive 0) and follows the expected pattern for testing wrapper type returns.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonObject.java (1)
9-16: LGTM!The test helper correctly returns a JsonObject and follows the expected pattern for testing JsonObject return types.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNull.java (1)
1-16: JsonNull client-callable test looks correctImports, annotation usage, trace call, and
Json.createNull()return type are all consistent with the rest of the suite.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__D.java (1)
1-13: Primitive double client-callable scaffold is fineThe annotation, return type, and tracing pattern are straightforward and consistent with the other helper classes.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Z.java (1)
1-10: Legacy boolean client-callable helper is consistentThe use of
@LegacyClientCallable, boolean return, and tracing pattern aligns with the overall legacy test suite conventions.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonArray__V.java (1)
1-11: Legacy JsonArray-argument callable is well-formedSignature, annotation, and body (trace-only) are appropriate for the instrumentation tests and match the established naming scheme.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_String__V.java (1)
1-12: String-parameter client-callable helper looks goodModern
@ClientCallableusage with a single String argument and trace-only body fits the existing test pattern.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNull__V.java (1)
1-11: Legacy JsonNull-argument callable matches suite conventionsThe parameter type, void return, and
@LegacyClientCallableusage align cleanly with the other JSON legacy helper classes.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.java (1)
1-11: Legacy JsonObject-argument callable is correctly structuredThis is a straightforward trace-only legacy endpoint with the expected JsonObject parameter and naming.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonObject.java (1)
1-13: Legacy JsonObject-returning callable implementation is appropriateThe method traces and returns a freshly created JsonObject via
Json.createObject(), matching the intended legacy test behavior.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.java (1)
1-13: LGTM! Simple test fixture is correct.The test class correctly follows the pattern for JSON return-type fixtures: extends
BaseClientCallable, callstrace(), and returns an appropriateJsonNumberinstance.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.java (1)
1-13: LGTM! Simple test fixture is correct.The test class correctly follows the pattern: extends
BaseClientCallable, callstrace(), and returnsJsonNullviaJson.createNull().src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonValue__V.java (1)
1-11: LGTM! Simple test fixture is correct.The test class correctly follows the parameter-variant pattern (indicated by
__Vsuffix): extendsBaseClientCallable, accepts aJsonValueparameter, and callstrace().src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.java (1)
1-13: LGTM! Simple test fixture is correct.The test class correctly returns a
JsonObject(viaJson.createObject()) asJsonValue, which is valid sinceJsonObjectimplements theJsonValueinterface.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonArray.java (1)
1-13: LGTM! Simple test fixture is correct.The test class correctly follows the pattern: extends
BaseClientCallable, callstrace(), and returns an emptyJsonArrayviaJson.createArray().src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_Z__V.java (1)
1-12: LGTM! Test fixture is correct.The test class correctly uses
@ClientCallable(not@LegacyClientCallable) and follows the parameter-variant pattern for boolean arguments.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_Z__V.java (1)
1-9: LGTM! Simple test fixture is correct.The test class correctly follows the parameter-variant pattern for boolean arguments with the
@LegacyClientCallableannotation.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_I__V.java (1)
1-9: LGTM! Simple test fixture is correct.The test class correctly follows the parameter-variant pattern for int arguments with the
@LegacyClientCallableannotation.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObject__V.java (1)
1-14: LGTM!The test class follows the established pattern for client-callable test scaffolding. It correctly extends
BaseClientCallable, uses the@ClientCallableannotation, and invokestrace()as expected.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNumber__V.java (1)
1-11: LGTM!The test class follows the established pattern for legacy client-callable test scaffolding with proper annotation and trace invocation.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.java (1)
1-13: LGTM!The test class correctly implements the legacy client-callable pattern with a
JsonStringreturn type. The naming convention (__JsonStringindicating a return type with no parameters) is consistent with the project's test scaffolding patterns.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonBoolean__V.java (1)
1-11: LGTM!The test class follows the established pattern for legacy client-callable test scaffolding with
JsonBooleanparameter type.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonBoolean.java (1)
1-13: LGTM!The test class correctly implements the legacy client-callable pattern with a
JsonBooleanreturn type, usingJson.create(true)to construct the return value.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java (2)
19-34: Well-structured type mapping logic.The type conversion logic correctly handles:
- Specific type mappings before the generic
JsonValuefallback- Uses identity comparison (
==) for concrete types, which is appropriate forClassobjects- Falls back to
JsonNodefor anyJsonValue-assignable type not explicitly mapped
10-15: Jackson package path is correct and consistent across the codebase.The project uses Jackson 3.0.0 (as declared in pom.xml with
tools.jackson.core:jackson-databind:3.0.0), which correctly uses thetools.jackson.databindpackage. All imports throughout the codebase consistently use this namespace. No issues found.src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
34-39: Clean delegation to instance-based instrumentation.The refactoring to use
ClassInstrumentationUtilwith version24for Vaadin 24 legacy support is well-structured. The return typeClass<? extends T>correctly reflects that instrumentation may produce a subclass.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_String__V.java (1)
1-9: LGTM!This test helper class correctly implements the expected pattern for testing legacy client callable methods with String parameters.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java (1)
1-44: LGTM!The implementation correctly provides Vaadin 24-specific instrumentation and JSON fixtures using elemental.json for legacy client callable tests.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java (1)
12-13: Note about test execution requirements.The comment indicates this test requires Vaadin 25 in the classpath. Ensure test execution is properly configured to skip or conditionally run this test when Vaadin 25 is not available, to avoid build failures.
Consider adding JUnit assumptions or conditional test execution:
@Before public void checkVaadinVersion() { Assume.assumeTrue("Vaadin 25 required", isVaadin25Available()); }src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (5)
57-63: LGTM! Instance-based design enables version-aware instrumentation.The refactoring from static utility to instance-based architecture with a version field is well-designed, allowing different instrumentation strategies for Vaadin 24 vs 25+.
171-176: Excellent validation! Prevents incorrect annotation usage.This validation correctly prevents the use of
@ClientCallablewithJsonValueparameters, enforcing the requirement to use@LegacyClientCallableinstead. The error message is clear and actionable.
217-217: Good refactoring: non-static inner class enables access to version.Making
InstrumentedClassLoadera non-static inner class allows it to access the outer class'sversionfield (used inhasLegacyVaadin()), which is essential for version-aware instrumentation.
322-336: LGTM! Primitive return handling is correct.The bytecode generation correctly handles different primitive return types with appropriate opcodes (LRETURN, FRETURN, DRETURN, IRETURN for primitives, ARETURN for objects).
148-150: The version boundary for legacy Vaadin support is correct.The method uses
version <= 24to determine legacy Vaadin support, which aligns with Vaadin 25 being the version that removes elemental.json and migrates exclusively to Jackson JsonNode/ObjectNode/ArrayNode as the standard JSON API. Vaadin 25 is a breaking change release that dropped support for elemental.json types, confirming that versions 24 and below are legacy and versions 25+ use the modern JSON handling approach.
src/test/java/com/flowingcode/vaadin/jsonmigration/BaseClientCallable.java
Outdated
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__V.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
20251130
branch
from
caf64ec to
954619b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__D.java (1)
6-6: Consider adding class-level documentation.The class correctly extends
BaseClientCallableand follows the established naming pattern for testing double return types. However, class-level Javadoc would help clarify its purpose within the test suite.Example:
+/** + * Test fixture for @ClientCallable methods with double return type and no parameters. + */ public class ClientCallable__D extends BaseClientCallable {src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
24-25: Clarify contract that allUnsupportedJsonValueImplimplementors must also beJsonNode
toJson()now assumesthisis atools.jackson.databind.JsonNode. That’s fine for the current Jackson-backed implementations, but any future implementor that isn’t aJsonNodewill hit aClassCastException.Consider documenting this requirement in the interface Javadoc (or renaming the interface to make the Jackson coupling explicit) so the constraint is clear to future contributors.
Also applies to: 48-51
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
375-386: Consider renaming thegetConvertedTypeDescriptorfield for clarityUsing the same identifier for both the
MethodHandlefield and thegetConvertedTypeDescriptor(Class<?>)method is legal but confusing at a glance.Renaming the field (e.g. to
convertedTypeDescriptorHandle) would make it clearer when code is referring to the cached handle vs. calling the helper method, and reduce the chance of accidental misuse during future edits.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java (1)
21-22: Consider migrating fromExpectedExceptiontoassertThrows.
ExpectedExceptionrule is deprecated in JUnit 4.13+. While functional, consider usingassertThrowsfor new code:assertThrows(IllegalArgumentException.class, () -> { new JsonMigrationHelper25().instrumentClass(clazz); });This is optional since the current approach works correctly.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (1)
41-45: Consider usingassertNotNullfor null checks.Using
assertNotNullis more semantic thanassertTrue(x != null):- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);This is a minor readability improvement.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java (2)
41-46: Consider usingassertNotNullfor null checks.Same suggestion as in
LegacyClientCallablesTest:- assertTrue(MESSAGE, testMethod != null); + assertNotNull(MESSAGE, testMethod);
11-58: Consider consolidating shared test logic withLegacyClientCallablesTest.Both
ClientCallablesTestandLegacyClientCallablesTestshare nearly identical structure and test methods. If future maintenance becomes burdensome, consider extracting a common abstract base that both can extend. However, the current separation is acceptable if the two test suites are expected to diverge in behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(8 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/BaseClientCallable.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_D__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_I__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonArray__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonBoolean__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNull__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNumber__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonValue__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_String__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_Z__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__D.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__I.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Integer.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonArray.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonBoolean.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonObject.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Z.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (39)
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonString__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNull__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonObject.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/BaseClientCallable.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonBoolean__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonBoolean.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_Z__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonObject.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObject__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonValue.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_I__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__D.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_D__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonString__V.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__I.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_String__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonArray.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonString.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Z.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonArray__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonValue__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__I.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNull__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonBoolean.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Z.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_D__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNull.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonArray__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonValue__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_I__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonNumber__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__Integer.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__Integer.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/ClientCallablesTest25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonBoolean__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.java
🔇 Additional comments (19)
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_Z__V.java (1)
1-12: ClientCallable boolean fixture looks consistent and correctClass setup, annotation, and
trace()invocation match the pattern of otherClientCallable_*__Vtest helpers and correctly exercise aboolean(Z) parameter signature for instrumentation tests. No changes needed.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonBoolean__V.java (1)
1-16: LGTM! Test fixture follows the expected pattern.The test fixture correctly implements the client-callable pattern for
JsonBooleantype verification, consistent with the broader test suite. The unused parameter is intentional for type compatibility testing.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObject__V.java (1)
7-10: LGTM! Test fixture correctly implements the pattern.The method serves as a test fixture to verify that
@LegacyClientCallableinstrumentation works correctly withJsonObjectparameters. The unused parameter is intentional, as the fixture tests parameter type handling rather than parameter usage.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNull.java (1)
1-13: LGTM! Consistent test fixture for JsonNull.The test class follows the established pattern for LegacyClientCallable test fixtures. The implementation correctly returns a JsonNull instance and maintains consistency with other JSON type test classes.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonNumber.java (1)
1-13: LGTM! Consistent test fixture for JsonNumber.The test class follows the established pattern for LegacyClientCallable test fixtures. The implementation correctly returns a JsonNumber instance and maintains consistency with other JSON type test classes.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__D.java (2)
1-5: LGTM: Package and imports are correct.The package declaration and import statement are properly structured for this test fixture.
8-12: LGTM: Test method correctly implements the fixture pattern.The
@ClientCallablemethod correctly:
- Calls
trace()to record invocation- Returns a double value (
0.0)- Follows the established pattern for testing client-callable methods with double return types
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
45-50: Instance-based instrumentation wiring for Vaadin 25 looks correctUsing a shared
ClassInstrumentationUtil(25)instance and delegatinginstrumentClassthrough it is consistent with the new version-aware design and should behave deterministically for all 25+ usages.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
31-39:toJson()formatting for integral values is reasonableThe new
toJson()logic for emitting integral values without a trailing “.0” and delegating to the default implementation for non-integrals is straightforward and should work well for typical JSON numeric usage.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonValue.java (1)
1-13: Legacy JsonValue client-callable fixture looks consistentThis test helper cleanly mirrors the pattern of the other
LegacyClientCallable__*fixtures (trace + return aJsonValue) and should integrate well with the existing client-callables test harness.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonArray.java (1)
1-16: JsonArray client-callable test helper is consistent with the suiteThe fixture cleanly follows the established
BaseClientCallable+@ClientCallable test()pattern and should integrate without surprises into the existing client-callables tests.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable__JsonNumber.java (1)
1-16: JsonNumber client-callable fixture looks goodThis class mirrors the existing client-callable fixtures and provides the expected JsonNumber-returning endpoint for instrumentation tests.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonNumber__V.java (1)
1-14: JsonNumber-parameter client-callable fixture is well-formedThe void-return
@ClientCallablewith aJsonNumberargument andtrace()call fits the existing*_JsonX__Vpattern and should give good coverage for JsonValue-parameter instrumentation.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_String__V.java (1)
1-12: LGTM!This test fixture correctly follows the established pattern for ClientCallable test variants. The implementation is clean and consistent with other similar test classes in the PR.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable__JsonString.java (1)
1-13: LGTM!The test fixture correctly implements the LegacyClientCallable pattern for JsonString return type, consistent with other JSON-returning test variants in this PR.
src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
34-39: LGTM!The instrumentation delegation pattern with version-specific
ClassInstrumentationUtil(24)is consistent with the approach used inJsonMigrationHelper25. The return type change toClass<? extends T>correctly reflects that the instrumented class may be a subclass.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java (1)
1-44: LGTM!The implementation correctly provides Vaadin 24-specific instrumentation via
LegacyJsonMigrationHelperand useselemental.json.Jsonfactory methods appropriate for the legacy JSON API.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (1)
59-265: LGTM!The test methods comprehensively cover all ClientCallable variants (primitives, wrappers, String, void, and JsonValue types). The pattern of comparing instrumented vs non-instrumented behavior is effective for validating correctness.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java (1)
60-265: LGTM!The test methods provide comprehensive coverage for all ClientCallable variants. The structure is consistent with the parallel
LegacyClientCallablesTestbase, ensuring uniform testing across both annotation types.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
265-266: Consider usinghasLegacyVaadin()method for consistency.The inline negation
!hasLegacyVaadin()is used here, but the methodhasLegacyVaadin()exists at lines 148-150. Using the method directly would improve readability and maintainability.Apply this diff for consistency:
- boolean hasJsonValueReturn = !hasLegacyVaadin() && JsonValue.class.isAssignableFrom(method.getReturnType()); - boolean hasJsonValueParams = !hasLegacyVaadin() && hasJsonValueParameters(method); + boolean isLegacy = hasLegacyVaadin(); + boolean hasJsonValueReturn = !isLegacy && JsonValue.class.isAssignableFrom(method.getReturnType()); + boolean hasJsonValueParams = !isLegacy && hasJsonValueParameters(method);
📜 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(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (7)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (7)
59-59: Instance-scoped cache correctly fixes the version isolation issue.The change from
staticto instance-scopedclassLoaderCache(line 59), combined with the non-staticInstrumentedClassLoaderinner class (line 217), properly resolves the issue flagged in the previous review. EachClassInstrumentationUtilinstance now maintains its own cache and loader instances, ensuring that the capturedversionmatches the util instance that created the loader.Also applies to: 217-217
164-194: Version-aware method filtering logic is correct.The filtering logic properly handles version-specific instrumentation requirements:
- Legacy mode (≤24): instruments only
@LegacyClientCallablemethods- Modern mode: instruments
@ClientCallablemethods withJsonValuereturn or@LegacyClientCallablemethods- Clear validation prevents
@ClientCallablewithJsonValueparameters (lines 171-176)The implementation aligns with the version-aware design.
321-336: Primitive return type handling is correct.The bytecode generation properly handles all JVM return instructions:
RETURNfor voidLRETURN/FRETURN/DRETURNfor long/float/doubleIRETURNfor int/short/byte/boolean/charARETURNfor reference typesThis implementation is complete and correct.
25-27: New imports and constructor support the version-aware design.The added imports are appropriate:
MethodHandle/MethodHandles/MethodTypefor dynamic descriptor resolutionStream/Collectorsfor functional method filtering- Constructor properly initializes version-specific state
Also applies to: 35-37, 61-63
152-162: Method candidate filtering logic is correct.The
getDeclaredCallablesmethod properly filters for non-static, non-private methods annotated with either@ClientCallableor@LegacyClientCallable. This provides a clean separation of concerns as the first stage of the filtering pipeline.
144-146: ConvertingneedsInstrumentationto instance method is correct.This change is necessary to support the instance-based
getInstrumentableMethodswhich requires version-specific context. The logic remains sound.
148-150: Version check helper is clear and well-encapsulated.The
hasLegacyVaadin()method provides a clear, named abstraction for the version threshold logic.
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.