Fix issues reported by Errorprone static analysis tool#12419
Fix issues reported by Errorprone static analysis tool#12419
Conversation
Pearl1594
requested review from
DaanHoogland,
Copilot,
sureshanaparti and
weizhouapache
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12419 +/- ##
============================================
+ Coverage 16.26% 16.30% +0.03%
- Complexity 13428 13478 +50
============================================
Files 5660 5661 +1
Lines 499907 500617 +710
Branches 60696 60985 +289
============================================
+ Hits 81316 81605 +289
- Misses 409521 409924 +403
- Partials 9070 9088 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR integrates Google Error Prone into the build system to perform compile-time static analysis. Error Prone has identified and the PR fixes multiple categories of issues including:
- Bugs where methods were called but results were ignored
- Incorrect primitive comparisons that should use
.equals() - Malformed logging statements with wrong placeholders
- Dead code and redundant operations
- Missing
hashCode()implementations whenequals()is overridden
Changes:
- Added Error Prone (version 2.24.1) to the Maven compiler plugin configuration
- Fixed 35+ Error Prone violations across multiple modules including server, engine, plugins, and core components
- Applied
@SuppressWarnings("BanJNDI")annotations to LDAP-related code where JNDI usage is intentional
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Added Error Prone plugin configuration to Maven compiler |
| ByteBuffer.java | Fixed missing assignment of Arrays.copyOf result |
| ServerNtlmsspChallenge.java | Fixed array concatenation in error message |
| GlobalLoadBalancingRulesServiceImpl.java | Changed == to .equals() for Long comparison |
| RoutedIpv4ManagerImpl.java | Fixed missing format placeholders in error messages |
| UserVmManagerImpl.java | Fixed config key retrieval and enum comparison |
| VolumeApiServiceImpl.java | Removed unnecessary String.format wrapper |
| ManagementServerImpl.java | Changed == to .equals() for Long comparison |
| IpAddressManagerImpl.java | Fixed missing throw keyword for exception |
| HighAvailabilityManagerImpl.java | Fixed logging format with correct placeholders |
| ConfigurationManagerImpl.java | Changed == to .equals() for Boolean comparison |
| TemplateJoinDaoImpl.java | Used saturated cast to prevent overflow |
| Argument.java | Improved Comparable implementation with proper typing |
| ApiXmlDocWriter.java | Fixed class type checking logic |
| OpenLdapUserManagerImpl.java | Fixed logging and added JNDI suppression |
| ADLdapUserManagerImpl.java | Added JNDI suppression annotation |
| StorPoolDataMotionStrategy.java | Removed unnecessary String.format wrapper |
| StorPoolPrimaryDataStoreDriver.java | Changed == to .equals() for Long comparison |
| NexentaStorAppliance.java | Added missing hashCode() implementations |
| RedfishWrapper.java | Fixed incorrect format placeholder count |
| XenServerGuru.java | Fixed typo and simplified Pair construction |
| MultipathSCSIAdapterBase.java | Converted logging to use placeholders |
| KVMStorageProcessor.java | Fixed missing throw keywords |
| LibvirtComputingResource.java | Fixed logging format placeholders |
| HypervInvestigator.java | Simplified boolean return expression |
| RootCAProvider.java | Removed duplicate method call |
| OnwireClassRegistry.java | Fixed self-assignment bug |
| DefaultEndPointSelector.java | Modernized iterator removal pattern |
| ScaleIOVMSnapshotStrategy.java | Changed == to .equals() for Long comparison |
| Upgrade41500to41510.java | Replaced anonymous HashMap with Map.of() |
| DatabaseAccessObject.java | Fixed logging format mismatch |
| SystemVmTemplateRegistration.java | Replaced anonymous HashMap with Map.of() |
| NetworkOfferingVO.java | Fixed incorrect hardcoded enum value |
| NetworkOrchestrator.java | Removed duplicate condition check |
| VirtualMachineManagerImpl.java | Fixed missing format placeholder |
| DirectAgentAttache.java | Added missing hashCode() implementation |
| AgentAttache.java | Added missing hashCode() implementation |
| DirectDownloadCommand.java | Removed dead assignment |
| RequestWrapper.java | Fixed incorrect .getClass() call |
| HAProxyConfigurator.java | Removed unnecessary toString() call |
| AbstractConfigItemFacade.java | Fixed incorrect .getClass() usage |
| UpdateBackupOfferingCmd.java | Fixed incomplete error message |
| MockVmMgr.java | Fixed modulo operation for bounded random |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("BanJNDI") |
There was a problem hiding this comment.
The @SuppressWarnings(\"BanJNDI\") annotation is being used to suppress Error Prone warnings about JNDI usage. While JNDI usage in LDAP operations is legitimate, ensure that all JNDI contexts are properly secured and validated to prevent LDAP injection attacks. Review the input validation in these methods (getUsersInGroup, getUserForDn, searchUser, searchUsers) to ensure user-controlled data is properly sanitized before being used in LDAP queries.
| private static final String MICROSOFT_AD_MEMBERS_FILTER = "memberOf"; | ||
|
|
||
| @Override | ||
| @SuppressWarnings("BanJNDI") |
There was a problem hiding this comment.
The @SuppressWarnings(\"BanJNDI\") annotation suppresses JNDI-related warnings. Ensure that the groupName parameter and any other user-controlled inputs are properly validated and sanitized before being used in LDAP queries to prevent LDAP injection vulnerabilities.
There was a problem hiding this comment.
we should probably take this one serious. are we blocking jndi somehow?
...nexenta/src/main/java/org/apache/cloudstack/storage/datastore/util/NexentaStorAppliance.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentAttache.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16354 |
DaanHoogland
force-pushed
the
ghi11438-errorprone-fixes
branch
from
86dfab5 to
856a753
Compare
DaanHoogland
force-pushed
the
ghi11438-errorprone-fixes
branch
from
856a753 to
824257d
Compare
…/cloudstack into ghi11438-errorprone-fixes
api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
…up/UpdateBackupOfferingCmd.java Co-authored-by: dahn <daan@onecht.net>
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16718 |
|
4 participants
Description
This PR adds error prone to the build. All issues identified by it have been addressed.
Issue were identified at compile time :
mvn clean compiletemporarily added the following change in pom.xml
The project built successfully:
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?