Create packed binary format #444
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a comprehensive set of classes for binary serialization in the Joda Beans library, specifically focusing on MsgPack-based serialization. The new classes 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: 1
🧹 Nitpick comments (3)
src/test/java/org/joda/beans/TestClone.java (1)
Line range hint
64-70: Consider enhancing the test robustness and clarityWhile the test correctly verifies the absence of a clone method, consider these improvements:
- The test should also check for inherited methods using
getMethods()instead of justgetDeclaredMethods().- The assertion message could be more descriptive about the test's purpose.
- The test name could better reflect that it's verifying the absence of clone functionality.
Here's a suggested improvement:
- void test_noclone_on_mutable_bean_option() { + void test_clone_method_should_not_exist_on_mutable_bean() { // test that clone() was not code-generated Class<?> c = NoClone.class; - Method[] noCloneMethods = c.getDeclaredMethods(); + Method[] noCloneMethods = c.getMethods(); for (Method method : noCloneMethods) { - assertThat("clone").isNotEqualTo(method.getName()); + assertThat(method.getName()) + .as("Mutable bean NoClone should not have a clone method") + .isNotEqualTo("clone"); } }src/main/java/org/joda/beans/ser/bin/BeanPackOutput.java (2)
93-102: Consider simplifying negative zero handling.
The check Double.compare(value, -0d) != 0 is correct for excluding -0.0. However, you might consider if special handling of negative zero is needed in this serialisation scenario. If not explicitly required, removing the check could simplify the logic.
174-203: Potential duplication with int/long logic.
The logic for serialising longs largely duplicates the int logic. Consider extracting a shared helper function if you need frequent updates or want to reduce code size.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/joda/beans/ser/bin/BeanPack.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackInput.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackOutput.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.java(1 hunks)src/test/java/org/joda/beans/TestClone.java(1 hunks)src/test/java/org/joda/beans/TestMetaBeans.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/joda/beans/ser/bin/BeanPack.java
🧰 Additional context used
📓 Path-based instructions (5)
src/test/java/org/joda/beans/TestClone.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/TestMetaBeans.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/BeanPackInput.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/bin/BeanPackOutput.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/bin/BeanPackVisualizer.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/test/java/org/joda/beans/TestMetaBeans.java (1)
31-34: Excellent change to package-private visibility!
The change aligns with our coding guidelines for test methods. The test is well-structured with:
- Clear naming that describes the test purpose
- Focused assertion that verifies the meta bean lookup behaviour
- Modern testing practices using JUnit Jupiter and AssertJ
src/main/java/org/joda/beans/ser/bin/BeanPackOutput.java (1)
144-165: No major issues spotted.
The int serialisation logic is well structured and accounts for positive/negative ranges. The code is consistent with Java 21 best practices.
src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.java (1)
193-206: Check indexing logic for chunked doubles.
The loop increments i inside a nested block, causing i to increment up to twice per iteration. While this may be intentional for chunking lines of output, ensure it aligns with the intended behaviour. Otherwise, consider a clearer approach to chunk the output.
| private Duration parseDuration() throws IOException { | ||
| var seconds = input.readLong(); | ||
| var nanos = input.readInt(); | ||
| var duration = Duration.ofSeconds(seconds, nanos); | ||
| handleDuration(duration); | ||
| return parseDuration(); | ||
| } |
There was a problem hiding this comment.
Infinite recursion in parseDuration.
The method parseDuration(Duration duration) calls handleDuration(duration) then, on line 312, it returns parseDuration() again instead of returning the duration. This causes endless recursion and will crash at runtime.
Suggested fix:
handleDuration(duration);
- return parseDuration();
+ return duration;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Duration parseDuration() throws IOException { | |
| var seconds = input.readLong(); | |
| var nanos = input.readInt(); | |
| var duration = Duration.ofSeconds(seconds, nanos); | |
| handleDuration(duration); | |
| return parseDuration(); | |
| } | |
| private Duration parseDuration() throws IOException { | |
| var seconds = input.readLong(); | |
| var nanos = input.readInt(); | |
| var duration = Duration.ofSeconds(seconds, nanos); | |
| handleDuration(duration); | |
| return duration; | |
| } |
* Define the input/output classes for the new binary format * Design is inspired by MessagePack, and partly compatible * First commit just introduces the format, without the reader/writer
jodastephen
force-pushed
the
beanpack
branch
from
d49af86 to
1c4a693
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.java (3)
31-36: Consider alternative approach to building output text.
The current approach with a single shared StringBuilder is perfectly fine for moderate usage. However, if you expect large or deeply nested structures in future usage scenarios, a more specialised approach (e.g., streaming data to a file or a custom writer) could be considered for enhanced performance and memory usage.
61-83: Indent manipulation is correct, but watch out for boundary conditions.
Each path appends or trims two characters from “indent”. If an unexpected path changes “indent” by more or fewer than two characters, the substring operation could cause an exception. Currently, the code is consistent, yet it might be safer to guard the substring usage in case of any future code modifications.
Line range hint
331-333: Consider adjusting the parameter type.
The parameter for toHex is an int, immediately narrowed with a cast to byte, which might be slightly misleading. If you really intend each input to be a single 8-bit value, consider changing the signature to “toHex(byte b)” for clarity.src/main/java/org/joda/beans/ser/bin/BeanPack.java (1)
27-333: Potential future refactor to enums or record.
A large set of integer constants is normal practice for a binary format specification. If future evolution demands grouping or variation, reconsider an approach using enumerations or separate record structures to organise them more robustly. For present usage, this approach is clear and straightforward.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/joda/beans/ser/bin/BeanPack.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackInput.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackOutput.java(1 hunks)src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.java(1 hunks)src/test/java/org/joda/beans/TestClone.java(1 hunks)src/test/java/org/joda/beans/TestMetaBeans.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/org/joda/beans/TestClone.java
- src/test/java/org/joda/beans/TestMetaBeans.java
- src/main/java/org/joda/beans/ser/bin/BeanPackInput.java
- src/main/java/org/joda/beans/ser/bin/BeanPackOutput.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.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/bin/BeanPack.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 (4)
src/main/java/org/joda/beans/ser/bin/BeanPackVisualizer.java (3)
27-27: No immediate concerns regarding class declaration and scope.
This class is package-private and final, so it should not leak into the public API. Its usage is presumably limited to internal operations, ensuring stable backwards compatibility.
51-58: Minimal error handling in visualisation.
When an error occurs in reading, you append “!!ERROR!!” to the buffer and return it. This is a straightforward approach, but depending on the usage context, you may want finer-grained exception details, partial results, or structured error reporting.
94-97: Replacing “-” and “=” in the indent could alter the alignment.
This line selectively replaces indentation characters to produce more readable output. This technique is clear, but be aware that if hyphens or equals signs appear in future expansions of the indentation scheme, they may inadvertently be removed.
src/main/java/org/joda/beans/ser/bin/BeanPack.java (1)
26-26: Use of an abstract class for constants is acceptable.
You have placed these constants in an abstract class with package-private scope. This arrangement prevents them from becoming part of the public API, which is advisable for internal usage.
Summary by CodeRabbit
New Features
BeanPack,BeanPackInput,BeanPackOutput, andBeanPackVisualizer.Bug Fixes
Documentation