Stop using references to collections #458
Conversation
Types with collections are a bit hairy, what if it's first read as Set but later we serialize the same thing as SortedSet? Can't reuse the original value but instead need a copy constructor, and that's a lot of effort to code
📝 WalkthroughWalkthroughThe pull request introduces changes across multiple files in the Joda Beans project, focusing on serialization and reference handling. The modifications include updates to the changes documentation, improvements in binary reference processing, and additions to test coverage for serialization of generic interface collections. The changes primarily target version 3.0.0-RC1 and address compatibility, performance, and serialization mechanisms. 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
|
|
This replaces PR #230 |
![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 (3)
src/main/java/org/joda/beans/ser/bin/BeanReferences.java (2)
137-143: Confirm correctness of the early return path.
The current logic means ifobjects.compute(...)returns a value above 1, the code may skip deeper bean-level checks, but still processes collections when!(base instanceof Bean). Verify that this early return does not miss referencing repeated beans, as it might interfere with future reference resolution in deeply nested structures.
173-173: Clarify the comment or remove if self-explanatory.
The code comment at line 173 (“shouldn't try and reuse references to collections”) duplicates the earlier comment. Consider removing or consolidating for clarity.src/changes/changes.xml (1)
37-41: Confirm wording accuracy.
The statement "The old format cam still be parsed successfully" at line 40 contains a small typo (“cam” should likely be “can”).- The old format cam still be parsed successfully. + The old format can still be parsed successfully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/changes/changes.xml(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanReferences.java(2 hunks)src/test/java/org/joda/beans/ser/SerTestHelper.java(2 hunks)src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.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/SerTestHelper.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/main/java/org/joda/beans/ser/bin/BeanReferences.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.
🔇 Additional comments (3)
src/main/java/org/joda/beans/ser/bin/BeanReferences.java (1)
136-137: Validate potential concurrency usage.
If this code could be called concurrently, objects might need concurrent access handling. Currently, it relies on being called serially, which is acceptable if guaranteed. Otherwise, consider a thread-safe approach.
src/test/java/org/joda/beans/ser/bin/TestSerializeReferencingBin.java (1)
133-143: Test coverage looks good.
This new test covers serialising/deserialising collections of Joda-Convert-based interfaces. It exercises the changes in BeanReferences and ensures references to collections are handled properly. The test appears correct and complete.
src/test/java/org/joda/beans/ser/SerTestHelper.java (1)
261-271: Ensure coverage for nested structures.
The new method testGenericInterfacesCollections covers various collection implementations. Ensure that the returned bean is exercised by multiple tests, especially regarding handling of set/hash collisions and references.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/joda/beans/ser/SerTestHelper.java (1)
261-270: The implementation looks good but could benefit from documentation and additional test cases.The method effectively tests serialisation of various collection types, including both JDK and Guava implementations. However, consider these improvements:
- Add Javadoc explaining the method's purpose and expected usage
- Include test cases for:
- Empty collections
- Collections containing null values
- Edge cases like collections with a single element
Here's a suggested enhancement:
+ /** + * Creates test data for serialising generic collections of various types. + * This includes both JDK collections (ArrayList, HashSet, TreeSet) and + * Guava collections (ImmutableList, ImmutableMap). + * + * @return the test data, not null + */ public static ImmGenericCollections<Object> testGenericInterfacesCollections() { return ImmGenericCollections.builder() .map(ImmutableMap.of( "First", Arrays.asList("A", "B"), "First1", ImmutableList.of("A", "B"), "Third1", new TreeSet<>(ImmutableList.of("A", "B")), "Third", new HashSet<>(Arrays.asList("A", "B")), + "Empty", ImmutableList.of(), + "WithNull", Arrays.asList("A", null, "B"), "Second", testCollections(true))) .build(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/changes/changes.xml(2 hunks)src/main/java/org/joda/beans/ser/bin/BeanReferences.java(2 hunks)src/test/java/org/joda/beans/ser/SerTestHelper.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/joda/beans/ser/bin/BeanReferences.java
- src/changes/changes.xml
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/org/joda/beans/ser/SerTestHelper.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 (1)
src/test/java/org/joda/beans/ser/SerTestHelper.java (1)
22-22: LGTM! The new imports are properly organised.
The added imports for HashSet and TreeSet are correctly placed in alphabetical order and are required for the new test method.
Also applies to: 27-27
Summary by CodeRabbit
Release Notes
New Features
Changes
Compatibility
The release focuses on improving serialization mechanisms and testing coverage for complex collection types.