Use modern Java language features #140
Conversation
WalkthroughWalkthroughThe recent changes across the Joda Money codebase primarily focus on modernising the code with the introduction of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MoneyUtils
participant BigMoney
User ->> MoneyUtils: Calls method with Money
MoneyUtils ->> BigMoney: Processes money calculations
MoneyUtils -->> User: Returns the result
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (34)
- src/main/java/org/joda/money/BigMoney.java (96 hunks)
- src/main/java/org/joda/money/BigMoneyProvider.java (1 hunks)
- src/main/java/org/joda/money/CurrencyMismatchException.java (3 hunks)
- src/main/java/org/joda/money/CurrencyUnit.java (24 hunks)
- src/main/java/org/joda/money/CurrencyUnitDataProvider.java (1 hunks)
- src/main/java/org/joda/money/DefaultCurrencyUnitDataProvider.java (6 hunks)
- src/main/java/org/joda/money/IllegalCurrencyException.java (1 hunks)
- src/main/java/org/joda/money/Money.java (70 hunks)
- src/main/java/org/joda/money/MoneyUtils.java (13 hunks)
- src/main/java/org/joda/money/Ser.java (4 hunks)
- src/main/java/org/joda/money/format/AmountPrinterParser.java (7 hunks)
- src/main/java/org/joda/money/format/LiteralPrinterParser.java (1 hunks)
- src/main/java/org/joda/money/format/MoneyAmountStyle.java (28 hunks)
- src/main/java/org/joda/money/format/MoneyFormatException.java (2 hunks)
- src/main/java/org/joda/money/format/MoneyFormatter.java (14 hunks)
- src/main/java/org/joda/money/format/MoneyFormatterBuilder.java (19 hunks)
- src/main/java/org/joda/money/format/MoneyParseContext.java (22 hunks)
- src/main/java/org/joda/money/format/MoneyParser.java (1 hunks)
- src/main/java/org/joda/money/format/MoneyPrintContext.java (3 hunks)
- src/main/java/org/joda/money/format/MoneyPrinter.java (1 hunks)
- src/main/java/org/joda/money/format/MultiPrinterParser.java (2 hunks)
- src/main/java/org/joda/money/format/SignedPrinterParser.java (1 hunks)
- src/test/java/org/joda/money/TestCurrencyMismatchException.java (4 hunks)
- src/test/java/org/joda/money/TestCurrencyUnit.java (36 hunks)
- src/test/java/org/joda/money/TestCurrencyUnitExtension.java (5 hunks)
- src/test/java/org/joda/money/TestIllegalCurrencyException.java (1 hunks)
- src/test/java/org/joda/money/TestMoney.java (81 hunks)
- src/test/java/org/joda/money/TestMoneyUtils_BigMoney.java (2 hunks)
- src/test/java/org/joda/money/TestStringConvert.java (2 hunks)
- src/test/java/org/joda/money/format/TestMoneyAmountStyle.java (23 hunks)
- src/test/java/org/joda/money/format/TestMoneyFormatter.java (15 hunks)
- src/test/java/org/joda/money/format/TestMoneyFormatterBuilder.java (34 hunks)
- src/test/java/org/joda/money/format/TestMoneyFormatterException.java (1 hunks)
- src/test/java/org/joda/money/format/TestMoneyParseContext.java (5 hunks)
Files skipped from review due to trivial changes (29)
- src/main/java/org/joda/money/BigMoney.java
- src/main/java/org/joda/money/BigMoneyProvider.java
- src/main/java/org/joda/money/CurrencyMismatchException.java
- src/main/java/org/joda/money/CurrencyUnit.java
- src/main/java/org/joda/money/CurrencyUnitDataProvider.java
- src/main/java/org/joda/money/DefaultCurrencyUnitDataProvider.java
- src/main/java/org/joda/money/IllegalCurrencyException.java
- src/main/java/org/joda/money/Money.java
- src/main/java/org/joda/money/MoneyUtils.java
- src/main/java/org/joda/money/format/AmountPrinterParser.java
- src/main/java/org/joda/money/format/LiteralPrinterParser.java
- src/main/java/org/joda/money/format/MoneyAmountStyle.java
- src/main/java/org/joda/money/format/MoneyFormatException.java
- src/main/java/org/joda/money/format/MoneyFormatter.java
- src/main/java/org/joda/money/format/MoneyFormatterBuilder.java
- src/main/java/org/joda/money/format/MoneyParseContext.java
- src/main/java/org/joda/money/format/MoneyPrintContext.java
- src/main/java/org/joda/money/format/MoneyPrinter.java
- src/main/java/org/joda/money/format/MultiPrinterParser.java
- src/main/java/org/joda/money/format/SignedPrinterParser.java
- src/test/java/org/joda/money/TestCurrencyMismatchException.java
- src/test/java/org/joda/money/TestCurrencyUnitExtension.java
- src/test/java/org/joda/money/TestIllegalCurrencyException.java
- src/test/java/org/joda/money/TestMoneyUtils_BigMoney.java
- src/test/java/org/joda/money/TestStringConvert.java
- src/test/java/org/joda/money/format/TestMoneyAmountStyle.java
- src/test/java/org/joda/money/format/TestMoneyFormatterBuilder.java
- src/test/java/org/joda/money/format/TestMoneyFormatterException.java
- src/test/java/org/joda/money/format/TestMoneyParseContext.java
Additional context used
ast-grep
src/test/java/org/joda/money/format/TestMoneyFormatter.java
[warning] 89-89: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8src/test/java/org/joda/money/TestCurrencyUnit.java
[warning] 453-453: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
[warning] 466-466: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
[warning] 481-481: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8src/test/java/org/joda/money/TestMoney.java
[warning] 826-826: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
[warning] 840-840: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
[warning] 854-854: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
Additional comments not posted (5)
src/main/java/org/joda/money/format/MoneyParser.java (1)
48-48: Visibility Modifier Clarification.The change to explicitly declare the
parsemethod aspublic abstractenhances clarity and aligns with interface design best practices.src/main/java/org/joda/money/Ser.java (1)
76-88: Modern Java Features Utilised.The use of switch expressions and
varenhances code readability and conciseness. The changes are well-implemented and maintain the original logic.src/test/java/org/joda/money/format/TestMoneyFormatter.java (1)
62-70: Lambda Expressions and Type Inference.The use of lambda expressions and
varfor type inference improves code readability and reduces boilerplate. The changes are appropriate and maintain the test logic.src/test/java/org/joda/money/TestCurrencyUnit.java (1)
Line range hint
47-61: Type Inference withvar.The use of
varfor type inference simplifies the code and enhances readability. The changes maintain the original functionality and logic of the tests.src/test/java/org/joda/money/TestMoney.java (1)
86-94: Type Inference withvar.The use of
varfor type inference enhances readability and reduces verbosity. The changes preserve the logic and functionality of the tests.
| } | ||
|
|
||
| private void writeBigMoney(ObjectOutput out, BigMoney obj) throws IOException { | ||
| writeCurrency(out, obj.getCurrencyUnit()); | ||
| byte[] bytes = obj.getAmount().unscaledValue().toByteArray(); | ||
| var bytes = obj.getAmount().unscaledValue().toByteArray(); |
There was a problem hiding this comment.
Security Concern: ObjectInputStream Usage.
Using ObjectInputStream can lead to security vulnerabilities such as remote code execution if untrusted data is deserialized. Consider alternatives or ensure that data is trusted and validated.
There was a problem hiding this comment.
Using ObjectInputStream is the only way to test code that implements Serializable, removing Serializable would be backwards incompatible. "Security concern" is a bit of an over-reaction to this code.
There was a problem hiding this comment.
@jodastephen, thank you for your insight. You're correct that ObjectInputStream is essential for testing Serializable code and maintaining backward compatibility. The security concern typically applies to scenarios involving untrusted data. In a controlled environment, where the data is trusted, the risk is indeed minimal. Your approach makes sense given the context.
| var a = iPrintTest; | ||
| var baos = new ByteArrayOutputStream(); | ||
| try (var oos = new ObjectOutputStream(baos)) { | ||
| oos.writeObject(a); | ||
| oos.close(); | ||
| ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| MoneyFormatter input = (MoneyFormatter) ois.readObject(); | ||
| Money value = MONEY_GBP_12_34; | ||
| var ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| var input = (MoneyFormatter) ois.readObject(); | ||
| var value = MONEY_GBP_12_34; |
There was a problem hiding this comment.
Security Concern: ObjectInputStream Usage.
Using ObjectInputStream can lead to security vulnerabilities such as remote code execution if untrusted data is deserialized. Consider alternatives or ensure that data is trusted and validated.
Tools
ast-grep
[warning] 89-89: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
| var cu = CurrencyUnit.of("GBP"); | ||
| var baos = new ByteArrayOutputStream(); | ||
| try (var oos = new ObjectOutputStream(baos)) { | ||
| oos.writeObject(cu); | ||
| oos.close(); | ||
| ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| CurrencyUnit input = (CurrencyUnit) ois.readObject(); | ||
| var ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| var input = (CurrencyUnit) ois.readObject(); |
There was a problem hiding this comment.
Security Concern: ObjectInputStream Usage.
Using ObjectInputStream can lead to security vulnerabilities such as remote code execution if untrusted data is deserialized. Consider alternatives or ensure that data is trusted and validated.
Tools
ast-grep
[warning] 453-453: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
| var a = GBP_2_34; | ||
| var baos = new ByteArrayOutputStream(); | ||
| try (var oos = new ObjectOutputStream(baos)) { | ||
| oos.writeObject(a); | ||
| oos.close(); | ||
| ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| Money input = (Money) ois.readObject(); | ||
| var ois = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())); | ||
| var input = (Money) ois.readObject(); |
There was a problem hiding this comment.
Security Concern: ObjectInputStream Usage.
Using ObjectInputStream can lead to security vulnerabilities such as remote code execution if untrusted data is deserialized. Consider alternatives or ensure that data is trusted and validated.
Tools
ast-grep
[warning] 826-826: Avoid using ObjectInputStream, it is insecure and can lead to remote code execution
Context: new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray()))
Note: [CWE-502]: Deserialization of Untrusted Data [OWASP A08:2017]: Insecure Deserialization [OWASP A08:2021]: Software and Data Integrity Failures [REFERENCES]
- https://www.owasp.org/index.php/Deserialization_of_untrusted_data
- https://www.oracle.com/java/technologies/javase/seccodeguide.html#8
Summary by CodeRabbit
New Features
varkeyword for type inference in variable declarations.Bug Fixes
Style
varfor clearer and more concise code.Tests
varfor local variable declarations, maintaining existing logic while improving readability.