-
Notifications
You must be signed in to change notification settings - Fork 0
fix: return class that implement both JsonNode and JsonValue #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds package-private Elemental*Node wrappers and an UnsupportedJsonValueImpl interface; updates convertToClientCallableResult to a generic signature across JsonMigration, JsonMigrationHelper, JsonMigrationHelper25, and LegacyJsonMigrationHelper; adjusts JsonMigrationHelper25 to map elemental JsonValue types to the new wrapper nodes and makes convertToJsonNode package-visible. 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-25T16:35:42.544ZApplied to files:
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
32-38: Consider removing trailing blank lines.There are multiple trailing blank lines at the end of the file that could be cleaned up.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java (1)
67-77: Javadoc is outdated after signature change.The
@returndocumentation still mentions returning "anObject" but the method now returnsT extends JsonValue. Consider updating the Javadoc to reflect the generic return type.* Converts a given Java object into the return type of a {@link ClientCallable method}. * * In Vaadin 25, this method converts {@code JsonValue} into {@code JsonNode}. * * @param object the object to convert - * @return an {@code Object} suitable to use as the result of a {@code ClientCallable} method. + * @return the converted value suitable to use as the result of a {@code ClientCallable} method */ public static <T extends JsonValue> T convertToClientCallableResult(T object) {src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java (1)
25-61: Consider adding descriptive exception messages for debugging.The design correctly satisfies the JsonValue interface contract while preventing usage of elemental JSON operations in Vaadin 25 contexts. However, adding messages to the exceptions would help developers understand why these operations fail.
Example:
@Override default boolean asBoolean() { - throw new UnsupportedOperationException(); + throw new UnsupportedOperationException("JsonValue operations not supported on Jackson-backed nodes"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java(1 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/ElementalObjectNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.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/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigration.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.javasrc/main/java/com/flowingcode/vaadin/jsonmigration/UnsupportedJsonValueImpl.java
🧬 Code graph analysis (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (5)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(30-52)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(31-54)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(24-31)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (6)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(31-54)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(216-261)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (7)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(30-52)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(31-54)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
SuppressWarnings(60-66)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (6)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(30-52)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(31-54)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(216-261)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (7)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java (1)
SuppressWarnings(30-52)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
SuppressWarnings(31-54)src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
SuppressWarnings(24-31)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(216-261)src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
SuppressWarnings(60-66)
🔇 Additional comments (8)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper.java (1)
32-32: Generic signature aligns well with PR objectives.The change from
Objectreturn type to<T extends JsonValue> Tprovides type safety while ensuring the returned value satisfies Vaadin 24's requirement forJsonValue-assignable return types. This is a clean solution for the cross-version compatibility issue.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalBooleanNode.java (1)
24-29: Implementation is consistent with sibling wrapper classes.The class correctly extends Jackson's
BooleanNodeand implementsUnsupportedJsonValueImplto satisfy both the Jackson serialization requirements (Vaadin 25) and theJsonValueinterface requirement (Vaadin 24).src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNumberNode.java (1)
24-31: Clean wrapper implementation.Using
DoubleNodeas the base class is appropriate sinceelemental.json.JsonNumberinternally represents numeric values asdouble. The implementation is consistent with the otherElemental*Nodewrappers.src/main/java/com/flowingcode/vaadin/jsonmigration/LegacyJsonMigrationHelper.java (1)
43-46: LGTM!The generic signature
<T extends JsonValue> T convertToClientCallableResult(T object)correctly provides type-safe propagation for Vaadin 24, where the method simply returns the input JsonValue unchanged. This aligns with the PR objective of maintaining compatibility across Vaadin versions.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalStringNode.java (1)
24-30: LGTM!Clean implementation following the established wrapper pattern. The class correctly bridges
StringNodewithJsonValueviaUnsupportedJsonValueImpl.src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java (1)
31-54: LGTM!The implementation correctly:
- Uses
LinkedHashMapto preserve key ordering- Optimizes for empty and single-key cases with immutable collections
- Recursively converts nested values via
convertToJsonNodesrc/main/java/com/flowingcode/vaadin/jsonmigration/ElementalNullNode.java (1)
24-31: No serialization concerns with ElementalNullNode — verification confirms safe implementation.The original concern about Jackson's
NullNodesingleton pattern andreadResolve()does not apply in this context. Investigation shows:
- All
Elemental*Nodeclasses are intentionally annotated with@SuppressWarnings("serial"), indicating serialization is not intended- No Java serialization infrastructure exists in the codebase (no
ObjectOutputStream,ObjectInputStream, or custom serialization methods)ElementalNullNodeinstances are temporary wrapper objects created inconvertToClientCallableResult()for Vaadin client callbacks and are never persisted or serialized- The pattern is consistent across all node types and safe for its intended purpose
The implementation is correct and follows the established pattern.
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
174-207: Visibility change forconvertToJsonNodeis appropriate; conversion logic looks correctMaking
static BaseJsonNode convertToJsonNode(JsonValue jsonValue)package‑private so thatElementalArrayNode/ElementalObjectNodecan reuse it is reasonable, and the mapping of eachJsonValuetype to Jackson nodes (ObjectNode,ArrayNode,textNode,numberNode,booleanNode,nullNode) is consistent and exhaustive for the known elementalJsonTypevalues.No functional issues spotted here; this method now forms the common backbone for both argument conversion in
invoke/convertArrayand the new Elemental*Node wrappers.
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalArrayNode.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalObjectNode.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
Show resolved
Hide resolved
59c7555 to
77f97fe
Compare
paodb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Close #12
In Vaadin 24:
convertToClientCallableResultreturns the sameJsonValueobject that it receivedIn Vaadin 25:
convertToClientCallableResultreturns a Jackson node that implementsJsonValue. The interface is implemented just for satisfying the method return type (which must beJsonValue). These little monsters do not support any operation fromJsonValue(which isn't a problem since Vaadin 25 would only use the Jackson part).Summary by CodeRabbit
Improvements
New Features
✏️ Tip: You can customize this high-level summary in your review settings.