Refactor simple JSON serialization #421
Conversation
* Use a ClassValue map to speed up serialization * Remove "clever but not faster" int handling in `JsonOutput`
📝 WalkthroughWalkthroughThe pull request introduces several updates across multiple files, primarily focusing on enhancements to the serialization process in the Joda-Beans library. Key changes include the addition of a new method in the Changes
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: 8
🧹 Outside diff range and nitpick comments (5)
src/test/java/org/joda/beans/TestResolvedType.java (2)
Line range hint
221-263: Consider improving test readability with helper methodsThe test method effectively covers various scenarios, but its structure could be more maintainable. Consider extracting the argument count validation into separate helper methods.
- void test_queries( - ResolvedType test, - Class<?> expectedRawType, - List<ResolvedType> expectedArgTypes, - String expectedToString) { - if (expectedArgTypes.isEmpty()) { - assertThatIllegalArgumentException() - .isThrownBy(() -> test.getArgument(0)) - .withMessage("Unexpected generic type access for " + expectedToString + ", index 0 is invalid"); - assertThat(test.getArgumentOrDefault(0)).isEqualTo(ResolvedType.OBJECT); - } else if (expectedArgTypes.size() == 1) { + void test_queries( + ResolvedType test, + Class<?> expectedRawType, + List<ResolvedType> expectedArgTypes, + String expectedToString) { + validateArgumentAccess(test, expectedArgTypes, expectedToString); + validateArrayType(test, expectedRawType, expectedToString); + assertThat(test.toArrayType().toComponentType()).isEqualTo(test); + } + + private void validateArgumentAccess(ResolvedType test, List<ResolvedType> expectedArgTypes, String expectedToString) { + switch (expectedArgTypes.size()) { + case 0 -> validateNoArguments(test, expectedToString); + case 1 -> validateSingleArgument(test, expectedArgTypes, expectedToString); + case 2 -> validateDoubleArguments(test, expectedArgTypes, expectedToString); + } + }
Line range hint
265-277: Consider adding error case tests for string conversionThe test effectively validates successful conversions, but consider adding test cases for malformed strings and edge cases.
Add test cases for:
- Empty string
- Null input
- Malformed generic type strings
- Special characters in class names
Example:
@Test void test_jodaConvertEdgeCases() { assertThatIllegalArgumentException() .isThrownBy(() -> StringConvert.INSTANCE.convertFromString(ResolvedType.class, "")); assertThatIllegalArgumentException() .isThrownBy(() -> StringConvert.INSTANCE.convertFromString(ResolvedType.class, null)); }src/main/java/org/joda/beans/ser/SerIteratorFactory.java (1)
Line range hint
207-230: Consider enhancing error handlingThe current error handling wraps ClassNotFoundException in a RuntimeException without preserving the context. Consider providing more detailed error information.
- } catch (ClassNotFoundException ex) { - throw new RuntimeException(ex); + } catch (ClassNotFoundException ex) { + throw new RuntimeException("Failed to load array component type: " + clsStr, ex);src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.java (2)
108-110: Adjust constructor access modifier to match class visibilityThe class
JodaBeanSimpleJsonWalkerhas package-private visibility, but its constructor is declaredpublic. Since the class is notpublic, the constructor should not bepubliceither.Change the constructor to have package-private access:
-public JodaBeanSimpleJsonWalker(JodaBeanSer settings) { +JodaBeanSimpleJsonWalker(JodaBeanSer settings) { this.settings = settings; }
208-210: Place ternary operator separators at the end of linesAccording to the coding standard, in multi-line expressions, ternary operators
?and:should be at the end of lines. Currently, the:is at the beginning of line 210.Adjust the formatting as follows:
var keyConverterOpt = settings.getConverter().converterFor(keyType.getRawType()); ToStringConverter<Object> keyConverter = keyConverterOpt.isPresent() ? - keyConverterOpt.get().withoutGenerics() - : + keyConverterOpt.get().withoutGenerics() : key -> settings.getConverter().convertToString(key);This places the
:at the end of line 209, adhering to the coding guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
src/changes/changes.xml(1 hunks)src/main/java/org/joda/beans/ResolvedType.java(1 hunks)src/main/java/org/joda/beans/ser/SerIteratorFactory.java(4 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.java(1 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java(3 hunks)src/main/java/org/joda/beans/ser/json/JsonOutput.java(2 hunks)src/test/java/org/joda/beans/TestResolvedType.java(3 hunks)src/test/java/org/joda/beans/ser/json/TestSerializeJsonSimple.java(2 hunks)src/test/resources/org/joda/beans/ser/ImmAddress.simplejson(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/main/java/org/joda/beans/ResolvedType.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/main/java/org/joda/beans/ser/SerIteratorFactory.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/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.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/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.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/main/java/org/joda/beans/ser/json/JsonOutput.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/TestResolvedType.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
src/test/java/org/joda/beans/ser/json/TestSerializeJsonSimple.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 (10)
src/main/java/org/joda/beans/ser/json/JsonOutput.java (2)
30-31: Well-documented performance consideration
The added comments effectively explain the performance trade-off regarding character replacement handling. This documentation helps future maintainers understand why characters 32-126 are handled differently.
133-133: Improved code clarity without performance penalty
The simplification to use Integer.toString() directly makes the code more maintainable whilst retaining good performance. This change aligns well with the PR's objective to remove the previous "clever but not faster" integer handling.
src/test/java/org/joda/beans/ser/json/TestSerializeJsonSimple.java (2)
21-21: LGTM: Import addition aligns with new assertion requirements
The addition of assertThatNullPointerException import is appropriate for the updated null-checking assertions.
230-250: Verify the architectural decision for exception handling
The switch from IllegalArgumentException to NullPointerException for writer methods whilst retaining IllegalArgumentException for reader methods suggests an intentional distinction in error handling approaches. This change aligns with standard Java practices where null parameters typically throw NullPointerException.
However, we should verify this architectural decision:
src/main/java/org/joda/beans/ResolvedType.java (1)
441-452: LGTM! New method enhances API with safe default handling.
The implementation is correct and follows the existing patterns in the codebase. The method provides a safer alternative to getArgument(int) by returning OBJECT instead of throwing an exception for invalid indices.
Changes to the public Java API in this PR:
- Added: New method
ResolvedType.getArgumentOrDefault(int)that provides a safe way to access type arguments with a default fallback toObject.
src/main/java/org/joda/beans/ser/SerIteratorFactory.java (3)
123-133: LGTM: Appropriate use of type inference
The changes effectively utilise Java's type inference capabilities whilst maintaining code clarity and type safety.
Line range hint 153-171: LGTM: Well-structured type inference implementation
The changes appropriately leverage type inference whilst maintaining the method's logical structure and type safety.
244-244: LGTM: Consistent use of type inference
The change maintains consistency with the codebase's approach to type inference.
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java (1)
Line range hint 69-71: Appropriate use of the var keyword
Good use of the var keyword in var buf = new StringBuilder(1024);, adhering to Java 21 standards and the project's coding guidelines favouring var for type declarations.
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.java (1)
1-480: No changes to the public Java API in this PR
After analysing the code, there are no changes to the public Java API that will affect end-users. All added classes and methods are package-private and not part of the public Java API.
| @@ -101,5 +101,5 @@ | |||
| }]], | |||
| "beanBeanMap": {}, | |||
| "doubleVector": [1.1, 2.2, 3.3], | |||
| "matrix": ["1.1,2.2", "3.2"] | |||
| "matrix": [[1.1, 2.2], [3.2]] | |||
There was a problem hiding this comment.
💡 Codebase verification
Matrix format change requires documentation update
The change to use nested numeric arrays instead of string arrays is a breaking change in the JSON serialisation format. While the change itself improves type safety and follows standard JSON practices, I found:
- The change is part of a broader JSON serialisation refactoring (commit 3ffa25f)
- No documentation of this breaking change in the serialisation guide
- The test class
TestSerializeJsonSimpleverifies the new format but doesn't document the change
Action items:
- Document this breaking change in
userguide-serialization.md - Add a note about migrating from string arrays to nested numeric arrays
- Consider adding a deserialiser to handle the old format for backwards compatibility
🔗 Analysis chain
Matrix representation change looks good!
The new nested array format for the matrix field is a more standard and type-safe representation compared to the previous string-based format. This change improves data accuracy by using proper numeric types instead of parsing strings.
Let's verify if this breaking change is properly documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the matrix format change is documented in release notes or migration guide
# Look for documentation about the breaking change
echo "Checking release notes and documentation..."
rg -i "matrix|array|json.*format|breaking.*change" --type md
rg -i "matrix|array|json.*format|breaking.*change" "changes.xml"
# Look for related test cases
echo "Checking related test cases..."
fd -e java -e scala -x rg -l "matrix.*json|json.*matrix|array.*serializ|serializ.*array" {}
Length of output: 727
Script:
#!/bin/bash
# Let's check for any documentation or test files related to JSON serialization changes
echo "Checking for JSON serialization documentation and changes..."
rg -i "json.*serialization|serialization.*format" --type md
# Look for test files and implementation related to matrix serialization
echo "Checking test files for matrix serialization..."
fd -e java -e scala -x rg -l "matrix.*test|test.*matrix" {}
# Check for any changelog or version history
echo "Looking for changelog or version files..."
fd -g "*change*" -g "*version*" -g "*history*" -g "*release*"
# Check the specific test class that might be using this JSON file
echo "Checking test implementations..."
rg -l "ImmAddress.simplejson"
Length of output: 1127
Script:
#!/bin/bash
# Let's examine the test class and serialization guide
echo "Checking test class implementation..."
cat src/test/java/org/joda/beans/ser/json/TestSerializeJsonSimple.java
echo "Checking serialization user guide..."
cat src/site/markdown/userguide-serialization.md
# Look for any recent commits related to this change
echo "Checking recent commits related to JSON serialization..."
git log --grep="json" --grep="serializ" -i --pretty=format:"%h %s" -n 5
Length of output: 15949
| <action dev="jodastephen" type="update"> | ||
| Incompatible change: | ||
| The simple JSON serialization format has changed slightly. | ||
| Two-dimensional arrays are now written as would be expected - a list of list of doubles. | ||
| </action> |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needs updating for the serialisation format change
The serialisation documentation in userguide-serialization.md lacks specific details about the JSON format, particularly regarding array handling. Given this is an incompatible change, the documentation should be updated to:
- Add details about the two-dimensional array serialisation format in
src/site/markdown/userguide-serialization.md - Consider adding a note in the "Handling change" section about migrating from the old format
- Update examples to demonstrate the new array serialisation format
🔗 Analysis chain
Well-documented breaking change.
The changelog entry properly documents an incompatible change to the JSON serialization format, specifically how two-dimensional arrays are now serialised as a list of list of doubles.
Let's check if there are any related documentation updates:
Let's check the content of the documentation files to verify if they cover the serialization format change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related documentation updates about the serialization format change
# Look for documentation files
echo "Searching for documentation about JSON serialization..."
fd -e md -e html -e txt | xargs rg -l "JSON|serialization|array"
# Look for migration guides
echo "Searching for migration guides..."
fd -e md -e html -e txt | xargs rg -l "migration|upgrade|breaking.?changes"
Length of output: 521
Script:
#!/bin/bash
# Check content of relevant documentation files
echo "=== Checking userguide-serialization.md ==="
cat src/site/markdown/userguide-serialization.md
echo -e "\n=== Checking relevant sections in index.md ==="
rg -A 5 -B 5 "JSON|serialization|breaking.?changes" src/site/markdown/index.md
Length of output: 4996
JsonOutputSummary by CodeRabbit
ResolvedTypeclass.ResolvedTypeand updated assertions in JSON writer tests.