fix: convert JsonValue[] arguments #27
Conversation
WalkthroughImplements support for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 0
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
108-118: Consider explicit null checks for clearer error messages.The documentation states that
NullPointerExceptionis thrown if source or target is null, but relying on implicit NPEs (e.g., fromsource.length) can result in less descriptive error messages. Explicitly validating nulls upfront would improve debuggability.🔎 Apply this diff to add explicit null validation:
public static void convertToJsonValue(Object[] source, JsonValue[] target) { + if (source == null) { + throw new NullPointerException("source array cannot be null"); + } + if (target == null) { + throw new NullPointerException("target array cannot be null"); + } if (source.length != target.length) { throw new IllegalArgumentException( String.format("Array length mismatch: source.length=%d, target.length=%d", source.length, target.length)); } for (int i = 0; i < target.length; i++) { target[i] = convertToJsonValue(source[i]); } }src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.java (1)
22-28: Consider static import forinstanceOfmatcher.The code uses static imports for most Hamcrest matchers (
assertThat,notNullValue,emptyArray,not) but accessesinstanceOfvia theMatchersprefix on line 36. For consistency with the existing import style, consider adding a static import.🔎 View suggested change:
import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.emptyArray; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import com.vaadin.flow.component.ClientCallable; import elemental.json.JsonObject; -import org.hamcrest.Matchers;Then update line 36:
- assertThat(arg, Matchers.instanceOf(JsonObject[].class)); + assertThat(arg, instanceOf(JsonObject[].class));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObjectVarargs__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonString__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObjectVarargs__V.java(1 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java(3 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.544Z
Learnt from: javier-godoy
Repo: FlowingCode/JsonMigrationHelper PR: 11
File: src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java:281-301
Timestamp: 2025-11-25T16:35:42.544Z
Learning: In the JsonMigrationHelper project, code copied from the Vaadin codebase should be kept consistent with the original source unless there's a specific reason to deviate.
Applied to files:
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObjectVarargs__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonString__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java
🔇 Additional comments (14)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (1)
470-489: LGTM! Bytecode generation correctly handles JsonValue[] parameters.The stack manipulation using
DUP_X1properly rearranges the operands so that after callingconvertToJsonValue(Object[], JsonValue[]), the target array remains on the stack for passing to the super method. The component type is correctly extracted and the local variable index is appropriately incremented.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.java (1)
22-37: LGTM! Test assertions improved with Hamcrest matchers.The migration to Hamcrest assertions and the addition of runtime type verification strengthen the test validation. The changes are consistent with the broader test suite updates.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonString__V.java (1)
1-38: LGTM! Well-structured test for JsonString[] array handling.This new test class provides coverage for the JsonString[] array conversion path, complementing the JsonObject[] tests. The assertions are consistent with the project's testing patterns.
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java (2)
50-52: LGTM! Return type made more specific.Changing the return type from
ObjecttoJsonStringimproves type safety without breaking compatibility, sinceJsonStringis assignable to the abstract method'sObjectreturn type.
69-72: LGTM! Array factory method correctly implemented.The
createArrayOfJsonString()implementation properly creates aJsonString[]array with two elements, matching the pattern used forcreateArrayOfJsonObject().src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (2)
82-82: LGTM! Abstract factory method enables version-specific array creation.The abstract method allows test subclasses to provide appropriate JsonString[] implementations for their respective Vaadin versions.
320-327: LGTM! Test correctly exercises JsonString[] parameter conversion.The test properly validates that the instrumented class can handle JsonString[] parameters and that the trace method is invoked, confirming the conversion path works as expected.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObjectVarargs__V.java (1)
33-37: LGTM! Varargs parameter correctly handled as array.The change from
JsonObject[] argtoJsonObject... argtests that varargs work correctly with the instrumentation, since varargs compile to arrays at the bytecode level. The runtime type assertion confirms the parameter is received asJsonObject[].class.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObjectVarargs__V.java (3)
22-27: LGTM: Clean Hamcrest migration.The import additions properly support the Hamcrest-based assertions while maintaining minimal dependencies.
33-36: LGTM: Comprehensive assertions with runtime type validation.The Hamcrest assertions are clear and expressive. The
instanceOfcheck on line 35 is particularly valuable—it confirms the instrumentation produces the correct ElementalJsonObject[]type rather than leaving a Jackson-backed array.
32-32: Varargs parameter handling is incomplete at the bytecode level.The ACC_VARARGS flag (0x0080) at bytecode level indicates a variable arity (varargs) method. However, the instrumentation code at lines 420-422 of
ClassInstrumentationUtil.javamasks modifiers to only preserve visibility flags (ACC_PUBLIC | ACC_PROTECTED | ACC_PRIVATE), which strips theACC_VARARGSflag from generated method overrides. While the code functions correctly becauseJsonMigrationHelper25.javadetects varargs viaMethod.isVarArgs()on the original method rather than the override, the generated bytecode does not properly preserve the varargs metadata on instrumented methods. Consider addingOpcodes.ACC_VARARGSto the modifier mask when the original method has varargs to maintain complete bytecode fidelity.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java (2)
55-57: LGTM: Return type narrowing improves type safety.Changing from
ObjecttoStringNodemakes the contract explicit and provides better compile-time type checking.
74-77: LGTM: Array factory method follows established pattern.The
createArrayOfJsonString()implementation mirrors the existingcreateArrayOfJsonObject()pattern at lines 70-72, providing consistent test data generation for array-of-json scenarios.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.java (1)
34-36: Excellent runtime type validation.The assertions correctly validate the array parameter after bytecode transformation. The addition of the
instanceOfcheck on line 36 is particularly important—it verifies that the instrumented bytecode properly converted the Jackson array to an ElementalJsonObject[]at runtime, which is the core fix for issue #26.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #26
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.