Allon Mureinik has uploaded a new change for review. Change subject: core: findbugs: Organize equals(Object) overrides ......................................................................
core: findbugs: Organize equals(Object) overrides exclude-filters-general.xml excludes all warnings about not overriding equals(Object), which is a dangerous and bug-prone practice. This patch tries to reconcile this approach, and make sense of the exclusions. This patch includes: 1. Removed the equals(Object) and hashCode() from VdcActionParametersBase, as it is never used and (almost) none of the parameter classes override it, meaning that any equality check between parameter classes would be bogus. 2. Removed equals(Object) and hashCode() from RemoveNetworkParameters, as they are useless (see (1) for details), and depend on the VdcActionParametersBase's implementation which was removed anyway. 3. Implement equals(Object) and hashCode() that were missing from ExtendedVmDynamic. This mistake of omission goes to prove how this suppression was a bad idea. 4. Explicitly exclude the missing equals(Object) implementation for PolicyUnitImpl and NetworkPolicyUnit as they are not trivial to implement, and the current code base is proven to work without them. 5. Remove the all-out exclusion of unimplemented equals(Object) methods from exclude-filters-general.xml to avoid bugs like the one described in (3). Change-Id: I3384d0c85f3c8ba91e25b584659e5e5f4617acb4 Signed-off-by: Allon Mureinik <[email protected]> --- M backend/manager/modules/bll/exclude-filters.xml M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveNetworkParameters.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendedVmDynamic.java M exclude-filters-general.xml 5 files changed, 46 insertions(+), 142 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/64/22964/1 diff --git a/backend/manager/modules/bll/exclude-filters.xml b/backend/manager/modules/bll/exclude-filters.xml index 50eca8d..3dfb479 100644 --- a/backend/manager/modules/bll/exclude-filters.xml +++ b/backend/manager/modules/bll/exclude-filters.xml @@ -127,6 +127,25 @@ <Bug code="HE"/> </Match> + <!-- + findbugs complains that class does not override equals(Object) even + though its parent does. + Since this error has been present in the codebase and previously + excluded as part of a wider filter, it's relatively safe to exclude + it now. + + findbugs reason: + Eq: Class doesn't override equals in superclass (EQ_DOESNT_OVERRIDE_EQUALS) + --> + <Match> + <Class name="org.ovirt.engine.core.bll.scheduling.PolicyUnitImpl" /> + <Bug pattern="EQ_DOESNT_OVERRIDE_EQUALS"/> + </Match> + <Match> + <Class name="org.ovirt.engine.core.bll.scheduling.policyunits.NetworkPolicyUnit" /> + <Bug pattern="EQ_DOESNT_OVERRIDE_EQUALS"/> + </Match> + <Match> <Class name="org.ovirt.engine.core.bll.utils.GlusterUtil" /> <Or> diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveNetworkParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveNetworkParameters.java index 51c09d8..3f95eed 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveNetworkParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveNetworkParameters.java @@ -39,32 +39,4 @@ public void setRemoveFromNetworkProvider(boolean removeFromNetworkProvider) { this.removeFromNetworkProvider = removeFromNetworkProvider; } - - @Override - public int hashCode() { - final int prime = 31; - int result = super.hashCode(); - result = prime * result + ((getId() == null) ? 0 : getId().hashCode()); - result = prime * result + (isRemoveFromNetworkProvider() ? 1231 : 1237); - return result; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) - return true; - if (!super.equals(obj)) - return false; - if (getClass() != obj.getClass()) - return false; - RemoveNetworkParameters other = (RemoveNetworkParameters) obj; - if (getId() == null) { - if (other.getId() != null) - return false; - } else if (!getId().equals(other.getId())) - return false; - if (isRemoveFromNetworkProvider() != other.isRemoveFromNetworkProvider()) - return false; - return true; - } } diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java index 79627c7..7f91b68 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java @@ -248,111 +248,6 @@ executionIndex--; } - @Override - public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((imagesParameters == null) ? 0 : imagesParameters.hashCode()); - result = prime * result + (shouldbelogged ? 1231 : 1237); - result = prime * result + ((transctionOption == null) ? 0 : transctionOption.hashCode()); - result = prime * result + ((entityInfo == null) ? 0 : entityInfo.hashCode()); - result = prime * result + (multipleAction ? 1231 : 1237); - result = prime * result + ((parametersCurrentUser == null) ? 0 : parametersCurrentUser.hashCode()); - result = prime * result + ((parentCommand == null) ? 0 : parentCommand.hashCode()); - result = prime * result + ((vdsmTaskIds == null) ? 0 : vdsmTaskIds.hashCode()); - result = prime * result + ((correlationId == null) ? 0 : correlationId.hashCode()); - result = prime * result + executionIndex; - result = prime * result + ((jobId == null) ? 0 : jobId.hashCode()); - result = prime * result + ((stepId == null) ? 0 : stepId.hashCode()); - return result; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null) { - return false; - } - if (getClass() != obj.getClass()) { - return false; - } - VdcActionParametersBase other = (VdcActionParametersBase) obj; - if (imagesParameters == null) { - if (other.imagesParameters != null) { - return false; - } - } else if (!imagesParameters.equals(other.imagesParameters)) { - return false; - } - if (shouldbelogged != other.shouldbelogged) { - return false; - } - if (transctionOption != other.transctionOption) { - return false; - } - if (entityInfo == null) { - if (other.entityInfo != null) { - return false; - } - } else if (!entityInfo.equals(other.entityInfo)) { - return false; - } - if (multipleAction != other.multipleAction) { - return false; - } - if (parametersCurrentUser == null) { - if (other.parametersCurrentUser != null) { - return false; - } - } else if (!parametersCurrentUser.equals(other.parametersCurrentUser)) { - return false; - } - if (parentCommand != other.parentCommand) { - return false; - } - if (vdsmTaskIds == null) { - if (other.vdsmTaskIds != null) { - return false; - } - } else if (!vdsmTaskIds.equals(other.vdsmTaskIds)) { - return false; - } - if (correlationId == null) { - if (other.correlationId != null) { - return false; - } - } else if (!correlationId.equals(other.correlationId)) { - return false; - } - if (executionIndex != other.executionIndex) { - return false; - } - if (jobId == null) { - if (other.jobId != null) { - return false; - } - } else if (!jobId.equals(other.jobId)) { - return false; - } - if (stepId == null) { - if (other.stepId != null) { - return false; - } - } else if (!stepId.equals(other.stepId)) { - return false; - } - if (commandId == null) { - if (other.commandId != null) { - return false; - } - } else if (!commandId.equals(other.commandId)) { - return false; - } - return true; - } - /** * Enum for determining the execution reason of the command. */ diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendedVmDynamic.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendedVmDynamic.java index 30734c2..ee1d901 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendedVmDynamic.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendedVmDynamic.java @@ -2,6 +2,7 @@ import org.ovirt.engine.core.common.businessentities.VDS; import org.ovirt.engine.core.common.businessentities.VmDynamic; +import org.ovirt.engine.core.common.utils.ObjectUtils; public class ExtendedVmDynamic extends VmDynamic { @@ -21,4 +22,30 @@ super.setDisplayIp(value); } } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null) { + return false; + } + if (getClass() != obj.getClass()) { + return false; + } + if (!super.equals(obj)) { + return false; + } + + ExtendedVmDynamic other = (ExtendedVmDynamic) obj; + return ObjectUtils.objectsEqual(host, other.host); + } + + @Override + public int hashCode() { + int result = super.hashCode(); + result = 31 * result + (host != null ? host.hashCode() : 0); + return result; + } } diff --git a/exclude-filters-general.xml b/exclude-filters-general.xml index f0ba0ec..23dfa09 100644 --- a/exclude-filters-general.xml +++ b/exclude-filters-general.xml @@ -36,15 +36,6 @@ </Match> <!-- - TBD: - findbugs reason: - Eq: Class doesnt override equals in superclass (EQ_DOESNT_OVERRIDE_EQUALS) - --> - <Match> - <Bug code="Eq" /> - </Match> - - <!-- Currently Ignoring. findbugs reason: IS: Inconsistent synchronization (IS2_INCONSISTENT_SYNC) -- To view, visit http://gerrit.ovirt.org/22964 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3384d0c85f3c8ba91e25b584659e5e5f4617acb4 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
