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.23% 16.25% +0.01%
- Complexity 13382 13390 +8
============================================
Files 5657 5657
Lines 498999 499060 +61
Branches 60566 60613 +47
============================================
+ Hits 81035 81112 +77
+ Misses 408928 408904 -24
- Partials 9036 9044 +8
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?
| public int hashCode() { | ||
| return 1; |
There was a problem hiding this comment.
The hashCode() implementation for LuParams returns a constant value of 1, which violates the hashCode contract. Since the equals() method considers all instances of LuParams to be equal (line 255), this constant hashCode is technically correct but indicates a potential design issue. If all instances are equal, consider whether this class should be used as a key in hash-based collections, or if the equality logic needs to be refined.
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 |
|
@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. |
Pearl1594
changed the title
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16362 |
|
| //@Test(expected = InvalidParameterValueException.class) | ||
| @Test |
There was a problem hiding this comment.
| //@Test(expected = InvalidParameterValueException.class) | |
| @Test | |
| @Test(expected = InvalidParameterValueException.class) |
There was a problem hiding this comment.
true for the below as well. or-else we could either fix or clean up this test file and keep only sensible tests.
|
|
||
| } | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
Do we need @Test annotation here?
There was a problem hiding this comment.
this was never run in this test file, but called in other tests that have the appropriate annotation
|
|
||
| final Boolean isRangeForSystemVM = checkIfVlanRangeIsForSystemVM(id); | ||
| if (forSystemVms != null && isRangeForSystemVM != forSystemVms) { | ||
| if (forSystemVms != null && !isRangeForSystemVM.equals(forSystemVms)) { |
There was a problem hiding this comment.
| if (forSystemVms != null && !isRangeForSystemVM.equals(forSystemVms)) { | |
| if (forSystemVms != null && !forSystemVms.equals(isRangeForSystemVM.)) { |
to prevent NPE if isRangeForSystemVM is null.
There was a problem hiding this comment.
checkIfVlanRangeIsForSystemVM returns boolean so in all likelihood this cant be null though assigned to a Boolean.
| for (Pair<Long, Long> innerLoopZoneId : gslbSiteIds) { | ||
| SiteLoadBalancerConfig siteLb = zoneSiteLoadbalancerMap.get(innerLoopZoneId.first()); | ||
| siteLb.setLocal(zoneId.first() == innerLoopZoneId.first()); | ||
| siteLb.setLocal(zoneId.first().equals(innerLoopZoneId.first())); |
There was a problem hiding this comment.
I just have one concern about changes like this is that this could cause NPE if if zoneId.first() is null for whatever reason.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| equivalant | ||
| erro | ||
| erronous | ||
| errorprone |
There was a problem hiding this comment.
The codespell exclusion entry "errorprone" should be capitalized as "ErrorProne" to match the actual tool name, or removed entirely as it's a valid technical term that doesn't need to be in the codespell ignore list.
There was a problem hiding this comment.
can we try this? it makes sense.
There was a problem hiding this comment.
codespell matches words case insensitively, so this shouldn't matter
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| @SuppressWarnings({"unused", "EqualsHashCode", "EqualsAndHashCode", "java:S2160", "errorprone"}) |
There was a problem hiding this comment.
The suppression annotation includes multiple different annotation names for the same issue. The list contains "EqualsHashCode", "EqualsAndHashCode", "java:S2160", and "errorprone" which are redundant. Consider standardizing to use only the ErrorProne annotation "EqualsHashCode" or the SonarQube annotation "java:S2160".
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(_id, _uuid, _name); |
There was a problem hiding this comment.
The hashCode implementation uses _id, _uuid, and _name, but the equals method in AgentAttache only uses _id for comparison. The hashCode implementation should be consistent with equals and only use _id to maintain the equals-hashCode contract.
| return Objects.hash(_id, _uuid, _name); | |
| return Objects.hash(_id); |
There was a problem hiding this comment.
or add _uuid and _name comparison to equals()? actually only _uuid and _name makes more sense.
| throw new InvalidParameterValueException(String.format("Can't update Backup Offering [id: %s] because there are no parameters to be updated, at least one of the " + | ||
| "following should be informed: name, description or allowUserDrivenBackups.", id)); |
There was a problem hiding this comment.
| throw new InvalidParameterValueException(String.format("Can't update Backup Offering [id: %s] because there are no parameters to be updated, at least one of the " + | |
| "following should be informed: name, description or allowUserDrivenBackups.", id)); | |
| throw new InvalidParameterValueException(String.format( | |
| "Can't update Backup Offering [id: %s] because there are no parameters to be updated,” + | |
| " at least one of the following should be informed: name, description or allowUserDrivenBackups.", | |
| id)); |
(just readability??)
| equivalant | ||
| erro | ||
| erronous | ||
| errorprone |
There was a problem hiding this comment.
can we try this? it makes sense.
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(_id, _uuid, _name); |
There was a problem hiding this comment.
or add _uuid and _name comparison to equals()? actually only _uuid and _name makes more sense.
| throw new AgentUnavailableException(String.format("Unable to stop vm [%s] because the operation to stop timed out", vmUuid), e.getAgentId(), e); | ||
| } catch (final ConcurrentOperationException e) { | ||
| throw new CloudRuntimeException(String.format("Unable to stop vm because of a concurrent operation", vmUuid), e); | ||
| throw new CloudRuntimeException(String.format("Unable to stop vm: %s because of a concurrent operation", vmUuid), e); |
There was a problem hiding this comment.
| throw new CloudRuntimeException(String.format("Unable to stop vm: %s because of a concurrent operation", vmUuid), e); | |
| throw new CloudRuntimeException(String.format("Unable to stop vm [%s] because of a concurrent operation", vmUuid), e); |
| } | ||
| } catch (SQLException e) { | ||
| logger.debug(String.format("Index %s doesn't exist, ignoring exception:", indexName, e.getMessage())); | ||
| logger.debug(String.format("Index %s doesn't exist, ignoring exception:", indexName), e.getMessage()); |
There was a problem hiding this comment.
| logger.debug(String.format("Index %s doesn't exist, ignoring exception:", indexName), e.getMessage()); | |
| logger.debug("Index {} doesn't exist, ignoring exception: {}", indexName, e.getMessage()); |
| //@Test(expected = InvalidParameterValueException.class) | ||
| @Test |
There was a problem hiding this comment.
true for the below as well. or-else we could either fix or clean up this test file and keep only sensible tests.
| if (logger.isTraceEnabled()) { | ||
| logger.trace(String.format("XenServer StrorageSubSystemCommand re always executed in sequence (command of type %s to host %l).", cmd.getClass(), hostId)); | ||
| logger.trace(String.format("XenServer StrorageSubSystemCommand is always executed in sequence (command of type %s to host %s).", cmd.getClass(), hostId)); | ||
| } |
There was a problem hiding this comment.
| if (logger.isTraceEnabled()) { | |
| logger.trace(String.format("XenServer StrorageSubSystemCommand re always executed in sequence (command of type %s to host %l).", cmd.getClass(), hostId)); | |
| logger.trace(String.format("XenServer StrorageSubSystemCommand is always executed in sequence (command of type %s to host %s).", cmd.getClass(), hostId)); | |
| } | |
| logger.trace("XenServer StrorageSubSystemCommand is always executed in sequence (command of type {} to host {}).", cmd.getClass(), hostId); |
| private static final String MICROSOFT_AD_MEMBERS_FILTER = "memberOf"; | ||
|
|
||
| @Override | ||
| @SuppressWarnings("BanJNDI") |
There was a problem hiding this comment.
we should probably take this one serious. are we blocking jndi somehow?
| if (logger.isDebugEnabled()) { | ||
| logger.debug(String.format("VM high availability manager is disabled, rescheduling the HA work %s, for the VM %s (id) to retry later in case VM high availability manager is enabled on retry attempt", work, vm.getName(), vm.getId())); | ||
| logger.debug("VM high availability manager is disabled, rescheduling the HA work {}, for the VM {} (id: {}) to retry later in case VM high availability manager is enabled on retry attempt", work, vm.getName(), vm.getId()); | ||
| } | ||
| long nextTime = getRescheduleTime(wt); |
There was a problem hiding this comment.
| if (logger.isDebugEnabled()) { | |
| logger.debug(String.format("VM high availability manager is disabled, rescheduling the HA work %s, for the VM %s (id) to retry later in case VM high availability manager is enabled on retry attempt", work, vm.getName(), vm.getId())); | |
| logger.debug("VM high availability manager is disabled, rescheduling the HA work {}, for the VM {} (id: {}) to retry later in case VM high availability manager is enabled on retry attempt", work, vm.getName(), vm.getId()); | |
| } | |
| long nextTime = getRescheduleTime(wt); | |
| logger.debug("VM high availability manager is disabled, rescheduling the HA work {}, for the VM {} (id: {}) to retry later in case VM high availability manager is enabled on retry attempt", work, vm.getName(), vm.getId()); | |
| long nextTime = getRescheduleTime(wt); |
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?