Use ClassValue to simplify JSON #453
Conversation
* As per recent commits, use ClassValue to speed up JSON serialization * Code is structured to allow simple JSON logic to be unified
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Joda-Beans library, focusing on JSON serialization and method placement rules. Key modifications include updates to the JSON serialization format for primitive arrays, improvements in handling collections and map serialization, and new guidelines for positioning manually defined methods. The changes impact how beans are written to JSON, with optimizations in type resolution, error handling, and support for various optional types. Test resources have been updated to reflect these new serialization approaches. Changes
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (18)
src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java (9)
98-100: Expand the comment for clarity on ClassValue usage
The explanatory comment at lines 98-99 is slightly cryptic. Consider elaborating on how ClassValue achieves O(1) lookups and why it is preferred over if/else or switch statements in this context.
225-233: Suggest early exit for null check
When value is null at line 227, consider returning early, instead of using the else block. This can improve readability and reduce nesting.private void writeObject(ResolvedType declaredType, String propertyName, Object value) throws IOException { if (value == null) { + output.writeNull(); + return; } var handler = LOOKUP.get(value.getClass()); handler.handle(this, declaredType, propertyName, value); }
230-232: Use var for local variable
On line 230, consider usingvarfor the variable type.- JsonHandler<Object> handler = LOOKUP.get(value.getClass()); + var handler = LOOKUP.get(value.getClass());
355-356: Consider documenting fallback
When the effective type isObject.class, the fallback logic at lines 355-356 might merit a short comment clarifying that we rely on the real type or the typed converter.
389-402: Clarify exception message
At line 399, the exception message is good but might benefit from specifying the original cause (i.e., detail which converter was used, if known). This can help debugging.
449-466: Check performance of deeply nested arrays
The map-special handling is sound. If the system might handle deeply nested or large arrays, consider the performance implications and whether streaming or chunk-based approaches are beneficial.
495-495: Encourage usage of sealed interfaces
Since Java 21 is used, theMetaTypeHandlerinterface might take advantage of sealed or record references if it’s logically beneficial. This is optional and depends on the design constraints.
555-555: Potential consideration for sealed class
Likewise, this interfaceJsonHandlercould be sealed if all implementations are within this file. Only do so if it aligns with your architectural constraints.
854-862: Consider removing reflection for Grid
IfGridusage is frequent and reflective calls are expensive, investigate potential direct handling. This is a micro-optimisation and may not be critical unless performance is proven to be a bottleneck.src/test/java/org/joda/beans/ser/json/TestSerializeJson.java (3)
65-65: Test name alignment
Using the same naming convention (e.g.test_writeImmAddress2) might help clarify that it outputsImmAddress2.json.
225-234: Check for empty arrays
For string arrays, also consider an empty array test. While likely safe, it can reveal corner-case issues.
Line range hint
180-180: Spelling check
Line 180: “implcit” → “implicit.” Please fix this minor typo if it still exists in the actual code or documentation.src/test/resources/org/joda/beans/ser/Collections2.json (1)
13-16: Clarify the meaning of nested arrays
If users are not deeply familiar with multi-map JSON representation, consider adding comments or documentation clarifying that[["A", "B"], ...]denotes key-value pairs.src/test/resources/org/joda/beans/ser/ImmAddress2.json (2)
10-10: Consider consistency in naming
array2dmight be more consistent if named consistently with other 2D array fields (likematrix). Minor detail, but can help avoid confusion.
146-150: Validate larger grid sizes
If your code eventually handles large or deeply nested grids, confirm memory usage is acceptable.src/changes/changes.xml (3)
17-24: Incompatibility note
The note about manualequals,hashCodeandtoStringis crucial. Updating older code must be done carefully to avoid overshadowing.
41-47: Provide migration guide
The updated JSON format for primitive arrays might impact downstream consumers. Consider adding a short “migration from older array format” section in your docs.
51-54: Typo: 'implcit'
At line 53, ensure to fix the “implcit” → “implicit” if it remains in final docs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/changes/changes.xml(2 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java(5 hunks)src/test/java/org/joda/beans/ser/json/TestSerializeJson.java(6 hunks)src/test/resources/org/joda/beans/ser/Collections2.json(1 hunks)src/test/resources/org/joda/beans/ser/ImmAddress2.json(1 hunks)src/test/resources/org/joda/beans/ser/ImmArrays2.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/org/joda/beans/ser/ImmArrays2.json
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/test/java/org/joda/beans/ser/json/TestSerializeJson.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/test/java/**/*.java: For test code, focus on:
- Correctness of test assertions
- Test coverage of edge cases
- Clear test naming and documentation
- Encourage test methods to be package-scoped where possible
- Be more lenient with code style and minor optimisations
🔇 Additional comments (9)
src/main/java/org/joda/beans/ser/json/JodaBeanJsonWriter.java (2)
183-185: Parameter rename does not break binary compatibility
Renaming the parameter from rootType to includeRootType does not affect the public API signature. However, ensure internal references and documentation are updated consistently across all usage paths to avoid confusion.
280-290: Highlight potential issues with numeric edge cases
The logic here correctly distinguishes between a primitive long and Long. Ensure all edge cases are tested, including minimum/maximum long values and negative numbers, to confirm correct JSON output.
src/test/java/org/joda/beans/ser/json/TestSerializeJson.java (5)
107-107: Keep file naming consistent
Verify that ImmArrays2.json is the correct naming convention for your JSON test resource. Uniformity across different resource files fosters maintainability.
112-113: Backwards compatibility
You are ensuring backward compatibility by parsing the old JSON. Great job!
212-223: Ensure coverage of all object array edge cases
The test checks a mixture of strings and numbers. Consider adding coverage for large numeric boundary values, nulls in the array, or nested arrays to confirm the writer handles them gracefully.
247-256: Add coverage for negative large long values
This test covers a long[], but you may want to verify behaviour for negative or boundary values.
269-269: Sufficient coverage
The existing approach for int[] is clear and likely covers typical scenarios.
src/test/resources/org/joda/beans/ser/Collections2.json (1)
1-1: File name alignment
The new file is named Collections2.json. Ensure it aligns with your naming scheme for resources like ImmArrays2.json and ImmAddress2.json.
src/changes/changes.xml (1)
39-39: Confirm coverage of Iterable
This indicates that standard binary and simple JSON formats now treat Iterable as a collection type. Ensure your test suite includes an Iterable scenario if not already included.
Summary by CodeRabbit
New Features
Iterableand map key serialisationBug Fixes
@metaand@typeannotationsDocumentation