From 3a7e4103fc2ca54e82d597288acd76332b054b98 Mon Sep 17 00:00:00 2001 From: wrodrigues Date: Sat, 15 Feb 2014 13:23:35 +0100 Subject: [PATCH] FindBugs findings: fixing equals() methods in 2 classes; commenting out dead variable in 1 class; adding 5 tests to cover the changes in the equals() methods. Signed-off-by: Hugo Trippaers --- .../com/cloud/agent/manager/AgentAttache.java | 40 ++++----- .../agent/manager/ClusteredAgentAttache.java | 18 ++-- .../agent/manager/ConnectedAgentAttache.java | 33 ++++++-- .../service/api/ProvisioningServiceImpl.java | 34 ++++---- .../manager/ConnectedAgentAttacheTest.java | 82 +++++++++++++++++++ 5 files changed, 155 insertions(+), 52 deletions(-) create mode 100644 engine/orchestration/test/com/cloud/agent/manager/ConnectedAgentAttacheTest.java diff --git a/engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java b/engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java index 3ebaf4aa7ae..fd1531e25a6 100755 --- a/engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java +++ b/engine/orchestration/src/com/cloud/agent/manager/AgentAttache.java @@ -32,9 +32,8 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import org.apache.log4j.Logger; - import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import org.apache.log4j.Logger; import com.cloud.agent.Listener; import com.cloud.agent.api.Answer; @@ -72,7 +71,7 @@ public abstract class AgentAttache { protected static final Comparator s_reqComparator = new Comparator() { @Override - public int compare(Request o1, Request o2) { + public int compare(final Request o1, final Request o2) { long seq1 = o1.getSequence(); long seq2 = o2.getSequence(); if (seq1 < seq2) { @@ -87,7 +86,7 @@ public abstract class AgentAttache { protected static final Comparator s_seqComparator = new Comparator() { @Override - public int compare(Object o1, Object o2) { + public int compare(final Object o1, final Object o2) { long seq1 = ((Request)o1).getSequence(); long seq2 = (Long)o2; if (seq1 < seq2) { @@ -122,7 +121,7 @@ public abstract class AgentAttache { Arrays.sort(s_commandsNotAllowedInConnectingMode); } - protected AgentAttache(AgentManagerImpl agentMgr, final long id, final String name, boolean maintenance) { + protected AgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final boolean maintenance) { _id = id; _name = name; _waitForList = new ConcurrentHashMap(); @@ -180,13 +179,13 @@ public abstract class AgentAttache { } } - protected synchronized void addRequest(Request req) { + protected synchronized void addRequest(final Request req) { int index = findRequest(req); assert (index < 0) : "How can we get index again? " + index + ":" + req.toString(); _requests.add(-index - 1, req); } - protected void cancel(Request req) { + protected void cancel(final Request req) { long seq = req.getSequence(); cancel(seq); } @@ -205,11 +204,11 @@ public abstract class AgentAttache { } } - protected synchronized int findRequest(Request req) { + protected synchronized int findRequest(final Request req) { return Collections.binarySearch(_requests, req, s_reqComparator); } - protected synchronized int findRequest(long seq) { + protected synchronized int findRequest(final long seq) { return Collections.binarySearch(_requests, seq, s_seqComparator); } @@ -331,17 +330,20 @@ public abstract class AgentAttache { } @Override - public boolean equals(Object obj) { - try { - AgentAttache that = (AgentAttache)obj; - return _id == that._id; - } catch (ClassCastException e) { - assert false : "Who's sending an " + obj.getClass().getSimpleName() + " to AgentAttache.equals()? "; + public boolean equals(final Object obj) { + // Return false straight away. + if (obj == null) { return false; } + // No need to handle a ClassCastException. If the classes are different, then equals can return false straight ahead. + if (this.getClass() != obj.getClass()) { + return false; + } + AgentAttache that = (AgentAttache)obj; + return _id == that._id; } - public void send(Request req, final Listener listener) throws AgentUnavailableException { + public void send(final Request req, final Listener listener) throws AgentUnavailableException { checkAvailability(req.getCommands()); long seq = req.getSequence(); @@ -387,7 +389,7 @@ public abstract class AgentAttache { } } - public Answer[] send(Request req, int wait) throws AgentUnavailableException, OperationTimedoutException { + public Answer[] send(final Request req, final int wait) throws AgentUnavailableException, OperationTimedoutException { SynchronousListener sl = new SynchronousListener(null); long seq = req.getSequence(); @@ -478,7 +480,7 @@ public abstract class AgentAttache { _currentSequence = req.getSequence(); } - public void process(Answer[] answers) { + public void process(final Answer[] answers) { //do nothing } @@ -505,7 +507,7 @@ public abstract class AgentAttache { protected class Alarm extends ManagedContextRunnable { long _seq; - public Alarm(long seq) { + public Alarm(final long seq) { _seq = seq; } diff --git a/engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentAttache.java b/engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentAttache.java index 23c3f7694bc..306c47ff60c 100755 --- a/engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentAttache.java +++ b/engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentAttache.java @@ -42,17 +42,17 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout protected final LinkedList _transferRequests; protected boolean _transferMode = false; - static public void initialize(ClusteredAgentManagerImpl agentMgr) { + static public void initialize(final ClusteredAgentManagerImpl agentMgr) { s_clusteredAgentMgr = agentMgr; } - public ClusteredAgentAttache(AgentManagerImpl agentMgr, long id, String name) { + public ClusteredAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name) { super(agentMgr, id, name, null, false); _forward = true; _transferRequests = new LinkedList(); } - public ClusteredAgentAttache(AgentManagerImpl agentMgr, long id, String name, Link link, boolean maintenance) { + public ClusteredAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final Link link, final boolean maintenance) { super(agentMgr, id, name, link, maintenance); _forward = link == null; _transferRequests = new LinkedList(); @@ -84,7 +84,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout } @Override - public void cancel(long seq) { + public void cancel(final long seq) { if (forForward()) { Listener listener = getListener(seq); if (listener != null && listener instanceof SynchronousListener) { @@ -106,7 +106,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout } @Override - public void routeToAgent(byte[] data) throws AgentUnavailableException { + public void routeToAgent(final byte[] data) throws AgentUnavailableException { if (s_logger.isDebugEnabled()) { s_logger.debug(log(Request.getSequence(data), "Routing from " + Request.getManagementServerId(data))); } @@ -136,7 +136,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout } @Override - public void send(Request req, Listener listener) throws AgentUnavailableException { + public void send(final Request req, final Listener listener) throws AgentUnavailableException { if (_link != null) { super.send(req, listener); return; @@ -220,7 +220,7 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout _transferMode = transfer; } - public boolean getTransferMode() { + public synchronized boolean getTransferMode() { return _transferMode; } @@ -232,13 +232,13 @@ public class ClusteredAgentAttache extends ConnectedAgentAttache implements Rout } } - protected synchronized void addRequestToTransfer(Request req) { + protected synchronized void addRequestToTransfer(final Request req) { int index = findTransferRequest(req); assert (index < 0) : "How can we get index again? " + index + ":" + req.toString(); _transferRequests.add(-index - 1, req); } - protected synchronized int findTransferRequest(Request req) { + protected synchronized int findTransferRequest(final Request req) { return Collections.binarySearch(_transferRequests, req, s_reqComparator); } diff --git a/engine/orchestration/src/com/cloud/agent/manager/ConnectedAgentAttache.java b/engine/orchestration/src/com/cloud/agent/manager/ConnectedAgentAttache.java index 00d54bb2450..f11a105b2a1 100755 --- a/engine/orchestration/src/com/cloud/agent/manager/ConnectedAgentAttache.java +++ b/engine/orchestration/src/com/cloud/agent/manager/ConnectedAgentAttache.java @@ -33,13 +33,13 @@ public class ConnectedAgentAttache extends AgentAttache { protected Link _link; - public ConnectedAgentAttache(AgentManagerImpl agentMgr, final long id, final String name, final Link link, boolean maintenance) { + public ConnectedAgentAttache(final AgentManagerImpl agentMgr, final long id, final String name, final Link link, final boolean maintenance) { super(agentMgr, id, name, maintenance); _link = link; } @Override - public synchronized void send(Request req) throws AgentUnavailableException { + public synchronized void send(final Request req) throws AgentUnavailableException { try { _link.send(req.toBytes()); } catch (ClosedChannelException e) { @@ -67,14 +67,31 @@ public class ConnectedAgentAttache extends AgentAttache { } @Override - public boolean equals(Object obj) { - try { - ConnectedAgentAttache that = (ConnectedAgentAttache)obj; - return super.equals(obj) && _link == that._link && _link != null; - } catch (ClassCastException e) { - assert false : "Who's sending an " + obj.getClass().getSimpleName() + " to " + this.getClass().getSimpleName() + ".equals()? "; + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((_link == null) ? 0 : _link.hashCode()); + return result; + } + + @Override + public boolean equals(final Object obj) { + // Return false straight away. + if (obj == null) { return false; } + // No need to handle a ClassCastException. If the classes are different, then equals can return false straight ahead. + if (this.getClass() != obj.getClass()) { + return false; + } + // This should not be part of the equals() method, but I'm keeping it because it is expected behaviour based + // on the previous implementation. The link attribute of the other object should be checked here as well + // to verify if it's not null whilst the this is null. + if (_link == null) { + return false; + } + ConnectedAgentAttache that = (ConnectedAgentAttache)obj; + return super.equals(obj) && _link == that._link; } @Override diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/service/api/ProvisioningServiceImpl.java b/engine/orchestration/src/org/apache/cloudstack/engine/service/api/ProvisioningServiceImpl.java index 86eab58ff0b..51e87663919 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/service/api/ProvisioningServiceImpl.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/service/api/ProvisioningServiceImpl.java @@ -25,9 +25,6 @@ import java.util.Map; import javax.inject.Inject; import javax.ws.rs.Path; -import org.springframework.stereotype.Component; -import org.springframework.stereotype.Service; - import org.apache.cloudstack.engine.datacenter.entity.api.ClusterEntity; import org.apache.cloudstack.engine.datacenter.entity.api.ClusterEntityImpl; import org.apache.cloudstack.engine.datacenter.entity.api.DataCenterResourceManager; @@ -38,6 +35,8 @@ import org.apache.cloudstack.engine.datacenter.entity.api.PodEntityImpl; import org.apache.cloudstack.engine.datacenter.entity.api.StorageEntity; import org.apache.cloudstack.engine.datacenter.entity.api.ZoneEntity; import org.apache.cloudstack.engine.datacenter.entity.api.ZoneEntityImpl; +import org.springframework.stereotype.Component; +import org.springframework.stereotype.Service; import com.cloud.host.Host; import com.cloud.host.Status; @@ -52,13 +51,13 @@ public class ProvisioningServiceImpl implements ProvisioningService { DataCenterResourceManager manager; @Override - public StorageEntity registerStorage(String name, List tags, Map details) { + public StorageEntity registerStorage(final String name, final List tags, final Map details) { // TODO Auto-generated method stub return null; } @Override - public ZoneEntity registerZone(String zoneUuid, String name, String owner, List tags, Map details) { + public ZoneEntity registerZone(final String zoneUuid, final String name, final String owner, final List tags, final Map details) { ZoneEntityImpl zoneEntity = new ZoneEntityImpl(zoneUuid, manager); zoneEntity.setName(name); zoneEntity.setOwner(owner); @@ -68,7 +67,7 @@ public class ProvisioningServiceImpl implements ProvisioningService { } @Override - public PodEntity registerPod(String podUuid, String name, String owner, String zoneUuid, List tags, Map details) { + public PodEntity registerPod(final String podUuid, final String name, final String owner, final String zoneUuid, final List tags, final Map details) { PodEntityImpl podEntity = new PodEntityImpl(podUuid, manager); podEntity.setOwner(owner); podEntity.setName(name); @@ -77,7 +76,7 @@ public class ProvisioningServiceImpl implements ProvisioningService { } @Override - public ClusterEntity registerCluster(String clusterUuid, String name, String owner, List tags, Map details) { + public ClusterEntity registerCluster(final String clusterUuid, final String name, final String owner, final List tags, final Map details) { ClusterEntityImpl clusterEntity = new ClusterEntityImpl(clusterUuid, manager); clusterEntity.setOwner(owner); clusterEntity.setName(name); @@ -86,7 +85,7 @@ public class ProvisioningServiceImpl implements ProvisioningService { } @Override - public HostEntity registerHost(String hostUuid, String name, String owner, List tags, Map details) { + public HostEntity registerHost(final String hostUuid, final String name, final String owner, final List tags, final Map details) { HostEntityImpl hostEntity = new HostEntityImpl(hostUuid, manager); hostEntity.setOwner(owner); hostEntity.setName(name); @@ -97,38 +96,38 @@ public class ProvisioningServiceImpl implements ProvisioningService { } @Override - public void deregisterStorage(String uuid) { + public void deregisterStorage(final String uuid) { // TODO Auto-generated method stub } @Override - public void deregisterZone(String uuid) { + public void deregisterZone(final String uuid) { ZoneEntityImpl zoneEntity = new ZoneEntityImpl(uuid, manager); zoneEntity.disable(); } @Override - public void deregisterPod(String uuid) { + public void deregisterPod(final String uuid) { PodEntityImpl podEntity = new PodEntityImpl(uuid, manager); podEntity.disable(); } @Override - public void deregisterCluster(String uuid) { + public void deregisterCluster(final String uuid) { ClusterEntityImpl clusterEntity = new ClusterEntityImpl(uuid, manager); clusterEntity.disable(); } @Override - public void deregisterHost(String uuid) { + public void deregisterHost(final String uuid) { HostEntityImpl hostEntity = new HostEntityImpl(uuid, manager); hostEntity.disable(); } @Override - public void changeState(String type, String entity, Status state) { + public void changeState(final String type, final String entity, final Status state) { // TODO Auto-generated method stub } @@ -141,7 +140,10 @@ public class ProvisioningServiceImpl implements ProvisioningService { @Override public List listPods() { - List pods = new ArrayList(); + /* + * Not in use now, just commented out. + */ + //List pods = new ArrayList(); //pods.add(new PodEntityImpl("pod-uuid-1", "pod1")); //pods.add(new PodEntityImpl("pod-uuid-2", "pod2")); return null; @@ -162,7 +164,7 @@ public class ProvisioningServiceImpl implements ProvisioningService { } @Override - public ZoneEntity getZone(String uuid) { + public ZoneEntity getZone(final String uuid) { ZoneEntityImpl impl = new ZoneEntityImpl(uuid, manager); return impl; } diff --git a/engine/orchestration/test/com/cloud/agent/manager/ConnectedAgentAttacheTest.java b/engine/orchestration/test/com/cloud/agent/manager/ConnectedAgentAttacheTest.java new file mode 100644 index 00000000000..e0e97c51049 --- /dev/null +++ b/engine/orchestration/test/com/cloud/agent/manager/ConnectedAgentAttacheTest.java @@ -0,0 +1,82 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.agent.manager; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; + +import org.junit.Test; + +import com.cloud.utils.nio.Link; + +public class ConnectedAgentAttacheTest { + + @Test + public void testEquals() throws Exception { + + Link link = mock(Link.class); + + ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link, false); + ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 0, null, link, false); + + assertTrue(agentAttache1.equals(agentAttache2)); + } + + @Test + public void testEqualsFalseNull() throws Exception { + + Link link = mock(Link.class); + + ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link, false); + + assertFalse(agentAttache1.equals(null)); + } + + @Test + public void testEqualsFalseDiffLink() throws Exception { + + Link link1 = mock(Link.class); + Link link2 = mock(Link.class); + + ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 0, null, link1, false); + ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 0, null, link2, false); + + assertFalse(agentAttache1.equals(agentAttache2)); + } + + @Test + public void testEqualsFalseDiffId() throws Exception { + + Link link1 = mock(Link.class); + + ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 1, null, link1, false); + ConnectedAgentAttache agentAttache2 = new ConnectedAgentAttache(null, 2, null, link1, false); + + assertFalse(agentAttache1.equals(agentAttache2)); + } + + @Test + public void testEqualsFalseDiffClass() throws Exception { + + Link link1 = mock(Link.class); + + ConnectedAgentAttache agentAttache1 = new ConnectedAgentAttache(null, 1, null, link1, false); + + assertFalse(agentAttache1.equals("abc")); + } +} \ No newline at end of file