feat: add conversion to JsonNode AND feat: add JsonSerializer #11
Conversation
WalkthroughAdds a public API method to convert ClientCallable results via the migration helper, implements that conversion in helper variants, and introduces a new JsonSerializer utility class for bidirectional conversion between Java objects and elemental.json types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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: 4
🧹 Nitpick comments (4)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (3)
227-234: Deprecated API:Class.newInstance()is deprecated since Java 9.Use
getDeclaredConstructor().newInstance()instead for forward compatibility.T instance; try { - instance = type.newInstance(); + instance = type.getDeclaredConstructor().newInstance(); } catch (Exception e) { throw new IllegalArgumentException( "Could not create an instance of type " + type + ". Make sure it contains a default public constructor and the class is accessible.", e); }
411-412: Deprecated API:Class.newInstance()is deprecated.Same issue as in
toObjectmethod.try { - return (Collection<?>) collectionType.newInstance(); + return (Collection<?>) collectionType.getDeclaredConstructor().newInstance(); } catch (Exception e) {
95-97: Commented-out code forJsonSerializablesupport.This commented code appears in both serialization and deserialization paths (lines 95-97 and 237-241). If this is intentional dead code, consider removing it entirely. If it's planned functionality, consider documenting the intent with a TODO comment.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
22-22: Import appears to be only for Javadoc@linkreference.The
ClientCallableimport is only used in the Javadoc at line 68. While valid, some teams prefer using fully qualified names in Javadoc to avoid "unused import" warnings from static analysis tools. This is minor and can be left as-is if the project style permits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)
🔇 Additional comments (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
32-33: Interface method addition looks correct.The new
convertToClientCallableResultmethod appropriately extends the interface contract to support the Vaadin 25 migration path for ClientCallable methods.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
67-77: New public API method for ClientCallable result conversion.The method is well-documented and correctly delegates to the version-specific helper. The Javadoc clearly explains the Vaadin 25 behavior.
src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
42-46: Legacy implementation correctly passes through the object unchanged.For pre-Vaadin 25,
JsonValueis already the expected return type forClientCallablemethods, so returning the object unchanged is the correct behavior.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
20251125
branch
from
d4fccde to
803051e
Compare
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (2)
75-78: Outdated Javadoc: Remove references toJsonSerializable.The Javadoc mentions
JsonSerializable instancebut based on past discussions,JsonSerializablewas removed in Vaadin 25 and the related handling code was removed from this class. The same applies to the Javadoc at lines 185-187./** - * Converts a Java bean, {@link JsonSerializable} instance, String, wrapper + * Converts a Java bean, String, wrapper * of primitive type or enum to a {@link JsonValue}.
223-231: Deprecated API:Class.newInstance()is deprecated since Java 9.
type.newInstance()has been deprecated since Java 9 in favor oftype.getDeclaredConstructor().newInstance(). The same applies to line 404. While this code is copied from Vaadin, the deprecation warning may cause build noise and the API could be removed in future Java versions.try { - instance = type.newInstance(); - } catch (Exception e) { + instance = type.getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { throw new IllegalArgumentException(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.519Z
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.519Z
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/JsonSerializer.java
🔇 Additional comments (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (3)
295-317: LGTM - Collection deserialization handles common cases.The collection deserialization logic correctly handles
ListandSetinterfaces and properly propagates the parameterized type for element deserialization. The error message typo ("Cloud" → "Could") appears to have been fixed.
389-412: LGTM - Collection instantiation with sensible defaults.The method correctly defaults to
ArrayListforListinterface andLinkedHashSetforSetinterface (preserving insertion order). The fallback to concrete class instantiation provides flexibility.Note: The same
newInstance()deprecation at line 404 applies as mentioned earlier.
70-73: LGTM - Proper utility class pattern.Private constructor prevents instantiation, and all methods are static.
javier-godoy
force-pushed
the
20251125
branch
from
e5daa73 to
2d09c6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (1)
223-231: Consider replacing deprecatednewInstance()method.
type.newInstance()has been deprecated since Java 9. Consider usingtype.getDeclaredConstructor().newInstance()instead for better compatibility with newer Java versions.The same issue appears at line 404 in
tryToCreateCollection.- try { - instance = type.newInstance(); - } catch (Exception e) { + try { + instance = type.getDeclaredConstructor().newInstance(); + } catch (Exception e) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.519Z
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.519Z
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/JsonSerializer.java
🔇 Additional comments (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (5)
85-130: Serialization logic is well-structured.The method correctly handles null, collections, arrays, simple types, records, and beans. The use of reflection for both records and JavaBeans is appropriate, and error handling is clear.
233-271: Bean deserialization correctly preserves generic type information.The implementation properly extracts generic parameter types from bean property writers (lines 256-257) and passes them to recursive deserialization calls, enabling correct handling of parameterized properties like
List<String>.
295-317: Collection deserialization logic is correct.The method properly validates the JsonType, infers generic parameterized types, creates appropriate collection instances, and handles empty collections. The typo in the error message (line 302) has been correctly fixed to "Could not infer..."
345-387: Simple type conversions are comprehensive.The method handles all common primitive types, wrapper classes, enums, and JsonValue pass-through appropriately. The use of
Optionalfor the return type is a good pattern for distinguishing "not a simple type" from "null value."
389-412: Collection instantiation handles common interfaces.The method appropriately creates
ArrayListforListinterfaces andLinkedHashSetforSetinterfaces, with clear error messages for unsupported types. Note that line 404 uses the deprecatednewInstance()method, which was flagged earlier (line 225).
| /** | ||
| * General-purpose serializer of Java objects to {@link JsonValue} and | ||
| * deserializer of JsonValue to Java objects. | ||
| * | ||
| * <p> | ||
| * For internal use only. May be renamed or removed in a future release. | ||
| * | ||
| * @since 1.0 | ||
| */ | ||
| public final class JsonSerializer { | ||
|
|
||
| private JsonSerializer() { | ||
| } |
There was a problem hiding this comment.
Documentation contradicts public API status.
Line 66 states "For internal use only. May be renamed or removed in a future release," but the class and its methods are declared public and the PR objectives indicate this is a new public API. Update the documentation to reflect the intended public status or clarify the API stability guarantees.
🤖 Prompt for AI Agents
In src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java around
lines 61 to 73, the Javadoc says "For internal use only. May be renamed or
removed in a future release" which contradicts the class being public and the PR
goal of a public API; update the class-level Javadoc to reflect its public
status and stability guarantees (for example, remove the "internal use only"
sentence and add an explicit statement such as "Public API —
stable/experimental" depending on the intended guarantee), optionally include an
@apiNote or @since tag clarifying version and stability, and ensure the wording
matches the project's API stability policy.
javier-godoy
force-pushed
the
20251125
branch
from
2d09c6c to
b01cc5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (1)
263-307: Record deserialization still drops generic type information for collection components.In
toRecord, collection-typed record components are deserialized via the two-argumenttoObject(componentTypes[i], …), which passesnullasgenericTypeto the private overload. When such a component is a parameterized collection (e.g.List<String>),toCollectionthen seesgenericType == nulland throws the “Could not infer the generic parameterized type…”IllegalArgumentException, so records with collection components aren’t handled as expected.You can preserve the element type for nested collections by threading the component’s generic type through to the private overload:
- for (int i = 0; i < components.length; i++) { - componentTypes[i] = components[i].getType(); - values[i] = toObject(componentTypes[i], - ((JsonObject) json).get(components[i].getName())); - } + for (int i = 0; i < components.length; i++) { + componentTypes[i] = components[i].getType(); + values[i] = toObject(componentTypes[i], + components[i].getGenericType(), + ((JsonObject) json).get(components[i].getName())); + }This leverages the existing
toObject(Class<T> type, Type genericType, JsonValue json)helper and enables records likerecord Foo(List<Bar> bars) {}to deserialize properly.Based on learnings, this does deviate slightly from the Vaadin 24 implementation, so only adopt it if you’re comfortable diverging from upstream behavior.
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
32-33: NewconvertToClientCallableResultmethod looks consistentThe added method fits the existing interface style (Object in/out, conversion‑oriented naming) and its placement next to
convertToJsonValue/invokekeeps the contract coherent. No issues from this file’s perspective. If this interface mirrors Vaadin code, just ensure any intentional divergence from the upstream definition is documented.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (1)
127-136: Tighten up Javadoc wording and fix small typos in collection description.The Javadoc has a couple of minor issues (“collection of object”, “collectios”, “collections is”). You can clean this up without affecting behavior:
- * Converts a collection of object into a {@link JsonArray}, converting each + * Converts a collection of objects into a {@link JsonArray}, converting each @@ - * @return the json representation of the objects in the collectios. An - * empty array is returned if the input collections is + * @return the json representation of the objects in the collection. An + * empty array is returned if the input collection isIf strict 1:1 alignment with the original Vaadin Javadoc is required, feel free to skip this.
Based on learnings, this would slightly diverge from the upstream Vaadin text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java
- src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T16:35:42.519Z
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.519Z
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/JsonMigrationHelper.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java
🔇 Additional comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonSerializer.java (1)
61-80: Class-level Javadoc and public API surface look coherent.The class description,
@sincetag, and the exposed static methods now line up with the intent of providing a general-purpose, public Json (de)serializer, without leftover “internal use only” orJsonSerializablereferences. No changes needed here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.