fix: instrument JsonValue[] arguments #24
Conversation
WalkthroughAdds support for instrumenting JsonValue array parameters by mapping Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 4
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/ClassInstrumentationUtil.java (1)
452-473: Array parameters are not converted back from Jackson types, causing runtimeClassCastException.When
hasJsonValueParamsis true and a parameter isJsonValue[](e.g.,JsonObject[]), the override method signature correctly converts the parameter type to the corresponding Jackson array type (e.g.,ObjectNode[]). However, the parameter conversion loop at line 452 only checksJsonValue.class.isAssignableFrom(paramType), which returns false for array types. This causes array parameters to skip conversion and fall through toloadParameter(), passing the Jackson array directly to the super method that expects the originalJsonValuearray type.The proposed fix in the review comment to add
JsonValue[].class.isAssignableFrom(paramType)to the condition is incomplete. CallingJsonMigration.convertToJsonValue(Object)on an entire array won't work because that method expects single objects. Instead, array parameters require element-by-element conversion, similar to theconvertArray()method inJsonMigrationHelper25(lines 125-155), which iterates through array elements and converts each one individually.Update the parameter conversion logic to detect array types and implement proper element-by-element conversion, or reuse the existing
convertArray()pattern via bytecode generation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(2 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/ClientCallablesTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java(3 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__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(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.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/ClientCallable_JsonObjectVarargs__V.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObjectVarargs__V.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java
🔇 Additional comments (17)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java (1)
51-52: LGTM! Array handling looks correct.The logic correctly maps
JsonValue[](and its subtypes likeJsonObject[]) to the corresponding Jackson array descriptors by recursively converting the component type. This handles the use case from issue #23.Note: Multi-dimensional arrays (e.g.,
JsonValue[][]) are not handled and will fall through to the default descriptor. This seems acceptable given the scope of issue #23.src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (2)
240-241: LGTM! Extended parameter detection for arrays.The addition of
JsonValue[].class.isAssignableFrom(paramType)correctly extends the detection logic to handle array parameters, addressing issue #23.
586-586: Necessary change to support array parameter conversion.Changing from checking individual
JsonValueparameters to always callinggetConvertedTypeDescriptorwhenconvertJsonValueParamsis true is the correct approach. This is because:
- The old code only checked
JsonValue.class.isAssignableFrom(paramType), which returnsfalseforJsonValue[]arraysgetConvertedTypeDescriptorhandles both scalar and array cases internally, falling back toType.getDescriptorfor non-JsonValue types- This change enables proper descriptor generation for array parameters while maintaining backward compatibility
src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java (1)
59-66: LGTM! Test infrastructure improvements.The return type change for
createJsonObject()improves type safety, and the newcreateArrayOfJsonObject()method provides test data for the array parameter test cases.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java (2)
45-45: LGTM! Extended validation for array parameters.Correctly adds
JsonValue[].class.isAssignableFrom(arg)to detect array parameters that should trigger the error message about using@LegacyClientCallable.
80-87: LGTM! Test infrastructure improvements.The return type refinement and new
createArrayOfJsonObject()method align with the test patterns in this PR.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java (2)
65-67: LGTM! Return type refinement improves type safety.The change from
ObjecttoObjectNodeprovides better type safety while remaining compatible with the abstract method signature from the parent class.
69-72: LGTM! Array factory method correctly implemented.The method properly implements the abstract factory pattern, creating a two-element
ObjectNode[]array suitable for testing array parameter instrumentation.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.java (3)
80-80: LGTM! Abstract factory method follows established pattern.The method signature is consistent with other abstract factory methods in the test class, allowing subclasses to provide appropriate test data for array parameters.
288-296: LGTM! Array parameter test follows established patterns.The test properly validates that
JsonObject[]array parameters are correctly instrumented and traced. The implementation is consistent with existing test methods.
299-307: LGTM! Varargs test complements array parameter coverage.The test validates that varargs
JsonObject...parameters work correctly with instrumentation. UsingcreateArrayOfJsonObject()is appropriate since arrays can be passed to varargs parameters.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java (3)
41-41: LGTM! Validates that JsonValue[] is rejected for legacy callables.The check correctly ensures that
JsonValue[]parameters are not allowed in legacy client callables, as these require the new instrumentation logic introduced for Vaadin 25.
78-80: LGTM! Return type refinement enhances code clarity.The change from
ObjecttoJsonObjectmakes the method's contract more explicit while maintaining compatibility with the abstract method signature.
82-85: LGTM! Array factory implementation is correct.The method properly creates a
JsonObject[]array for testing, consistent with the implementation pattern used in other test classes.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (3)
80-80: LGTM! Abstract method maintains test hierarchy consistency.The method signature mirrors the pattern established in
ClientCallablesTest.java, ensuring consistent test structure across the legacy and modern test suites.
318-326: LGTM! Legacy array parameter test properly structured.The test validates that
JsonObject[]arrays work correctly with legacy client callable instrumentation, following the established test pattern.
328-336: LGTM! Varargs test completes legacy parameter coverage.The test ensures that varargs
JsonObject...parameters are correctly handled in legacy client callable instrumentation, providing comprehensive test coverage alongside the array parameter test.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObjectVarargs__V.java
Show resolved
Hide resolved
...est/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.java
Show resolved
Hide resolved
...est/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObjectVarargs__V.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
20251217-02
branch
from
9c6b7d8 to
4d86e85
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_ArrayOfJsonObject__V.java (1)
30-35: Test should access array elements to verify instrumentation.While the assertions for null and non-empty are improvements, the test still doesn't access the array elements themselves. Since this PR fixes JsonValue[] instrumentation (issue #23), the test must verify that individual elements are properly converted and usable.
Apply this diff to access and verify array elements:
@ClientCallable public void test(JsonObject[] arg) { Assert.assertNotNull(arg); Assert.assertThat(arg, not(emptyArray())); + // Verify array elements are properly instrumented and accessible + JsonObject first = arg[0]; + Assert.assertNotNull("First element should not be null", first); trace(); }Alternatively, if the test framework supports passing data to
trace(), consider:@ClientCallable public void test(JsonObject[] arg) { Assert.assertNotNull(arg); Assert.assertThat(arg, not(emptyArray())); - trace(); + trace(arg[0] != null); }
🧹 Nitpick comments (3)
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallable_JsonObjectVarargs__V.java (1)
30-35: Test improvement: consider accessing individual array elements.The test now validates that the array is not null and not empty, which is an improvement over the previous version. However, to fully exercise the runtime conversion logic for
JsonObject[]parameters, consider accessing individual array elements to ensure proper type conversion.For example:
@ClientCallable public void test(JsonObject[] arg) { Assert.assertNotNull(arg); Assert.assertThat(arg, not(emptyArray())); + // Access elements to exercise conversion + for (JsonObject obj : arg) { + Assert.assertNotNull(obj); + } trace(); }src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.java (1)
29-34: Test improvement: consider accessing individual array elements.Similar to the
ClientCallablevariant, this test validates the array structure but doesn't access individualJsonObjectelements. Consider iterating through the array to fully exercise the conversion logic.Example enhancement:
@LegacyClientCallable public void test(JsonObject[] arg) { Assert.assertNotNull(arg); Assert.assertThat(arg, not(emptyArray())); + // Verify elements are properly converted + for (JsonObject obj : arg) { + Assert.assertNotNull(obj); + } trace(); }src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java (1)
45-45: Consider a more explicit array type check for clarity.The current check
JsonValue[].class.isAssignableFrom(arg)is correct and leverages Java's array covariance, but a more explicit alternative might improve readability:- if (JsonValue.class.isAssignableFrom(arg) || JsonValue[].class.isAssignableFrom(arg)) { + if (JsonValue.class.isAssignableFrom(arg) || + (arg.isArray() && JsonValue.class.isAssignableFrom(arg.getComponentType()))) {This makes it clearer that you're checking for array types and inspecting the component type, though the current implementation is functionally equivalent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java(2 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/ClientCallablesTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java(3 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__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(2 hunks)src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_JsonObjectVarargs__V.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest24.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationJacksonHelper.java
- src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest.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/ClientCallablesTest24.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.javasrc/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallable_ArrayOfJsonObject__V.java
🔇 Additional comments (9)
src/main/java/com/flowingcode/vaadin/jsonmigration/ClassInstrumentationUtil.java (2)
240-241: LGTM! Array parameter detection added.The extension to check for
JsonValue[]arrays correctly implements the PR objective to instrument JsonValue array arguments.
586-587: No issue found with parameter descriptor handling.The implementation correctly handles all parameter types. The
getConvertedTypeDescriptormethod returns converted Jackson type descriptors for JsonValue parameters but returns the original type descriptor for all other parameters (line 54 in ClassInstrumentationJacksonHelper). This matches the actual bytecode conversion logic which only converts JsonValue-assignable parameters, ensuring descriptor and conversion behavior are consistent.src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest25.java (1)
65-72: LGTM! Test data providers updated.The changes align with the test suite pattern, providing both single
ObjectNodeinstances and arrays for comprehensive testing of JsonValue array instrumentation.src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest24.java (2)
41-41: LGTM! Array type validation added.Consistent with the other test variants, this properly validates that array parameters require
@LegacyClientCallableannotation.
78-85: LGTM! Test data providers for Vaadin 24.The return type refinement and new array method provide appropriate test data using the Vaadin 24 API (
Json.createObject()).src/test/java/com/flowingcode/vaadin/jsonmigration/LegacyClientCallablesTest.java (2)
80-80: LGTM! Abstract method added for array test data.The new abstract method properly extends the test framework to support
JsonObject[]parameter testing across different Vaadin versions.
318-336: LGTM! Comprehensive array parameter tests added.Both test methods follow the established pattern and provide coverage for:
- Explicit array parameters (
JsonObject[])- Varargs parameters (also
JsonObject[]at runtime)The tests properly verify that instrumented methods with array parameters are correctly annotated and traced.
src/test/java/com/flowingcode/vaadin/jsonmigration/ClientCallablesTest25.java (2)
80-82: LGTM! More specific return type improves clarity.Changing the return type from
ObjecttoObjectNodemakes the method signature more precise and improves type safety without affecting functionality.
84-87: LGTM! Correctly implements array creation for JsonValue[] testing.The new method properly creates an array of
ObjectNodeinstances to support testing JsonValue[] argument instrumentation, directly addressing the PR objective from issue #23.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #23
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.