Refactor to remove false positives from Sonar

- change `updateResourceInstancesNames` to account for case where `preparedResource` is null
- change `findInputByName` to return an Either<InputDefinition, RuntimeException> in order to make exceptions explicit
- create `rollbackWithEither` (+ tests) to make exceptions more explicit

Issue-ID: SDC-2992
Signed-off-by: Chris Andre <chris.andre@yoppworks.com>
Change-Id: I487994a3f9e88b0a2b14d2679c3587d85d8aa12d

2 files changed, 111 insertions, 47 deletions

diff --git a/catalog-be/src/main/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogic.java b/catalog-be/src/main/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogic.java
index 114a9c76cc..7eda330fe3 100644
--- a/catalog-be/src/main/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogic.java
+++ b/catalog-be/src/main/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogic.java
@@ -88,6 +88,7 @@ import org.openecomp.sdc.be.config.BeEcompErrorManager.ErrorSeverity;
 import org.openecomp.sdc.be.config.ConfigurationManager;
 import org.openecomp.sdc.be.dao.api.ActionStatus;
 import org.openecomp.sdc.be.dao.janusgraph.JanusGraphOperationStatus;
+import org.openecomp.sdc.be.dao.jsongraph.JanusGraphDao;
 import org.openecomp.sdc.be.datamodel.api.HighestFilterEnum;
 import org.openecomp.sdc.be.datamodel.utils.ArtifactUtils;
 import org.openecomp.sdc.be.datamodel.utils.UiComponentDataConverter;
@@ -742,29 +743,31 @@ public class ResourceBusinessLogic extends ComponentBusinessLogic {
 		boolean isTopologyChanged) {
 		if (oldResource == null || preparedResource == null) {
 			log.debug("Failed to update resource instances names : oldResource or preparedResource is null");
-		} else if (CollectionUtils.isNotEmpty(oldResource.getComponentInstances())) {
-			Map<String, String> oldInstances = oldResource.getComponentInstances()
-				.stream()
-				.collect(toMap(ComponentInstance::getInvariantName, ComponentInstance::getName));
-			List<ComponentInstance> updatedInstances = preparedResource.getComponentInstances()
-				.stream()
-				.filter(i -> oldInstances.containsKey(i.getInvariantName()) && !i.getName()
-					.equals(oldInstances.get(i.getInvariantName())))
-				.collect(toList());
-			if (CollectionUtils.isNotEmpty(updatedInstances)) {
-				if (isTopologyChanged) {
-					updatedInstances.stream().filter(i -> !i.isCreatedFromCsar())
-						.forEach(i -> i.setName(oldInstances.get(i.getInvariantName())));
-				} else {
-					updatedInstances.forEach(i -> i.setName(oldInstances.get(i.getInvariantName())));
+		} else {
+			if (CollectionUtils.isNotEmpty(oldResource.getComponentInstances())) {
+				Map<String, String> oldInstances = oldResource.getComponentInstances()
+					.stream()
+					.collect(toMap(ComponentInstance::getInvariantName, ComponentInstance::getName));
+				List<ComponentInstance> updatedInstances = preparedResource.getComponentInstances()
+					.stream()
+					.filter(i -> oldInstances.containsKey(i.getInvariantName()) && !i.getName()
+						.equals(oldInstances.get(i.getInvariantName())))
+					.collect(toList());
+				if (CollectionUtils.isNotEmpty(updatedInstances)) {
+					if (isTopologyChanged) {
+						updatedInstances.stream().filter(i -> !i.isCreatedFromCsar())
+							.forEach(i -> i.setName(oldInstances.get(i.getInvariantName())));
+					} else {
+						updatedInstances.forEach(i -> i.setName(oldInstances.get(i.getInvariantName())));
+					}
 				}
 			}
 
+			componentInstanceBusinessLogic.updateComponentInstance(ComponentTypeEnum.RESOURCE_PARAM_NAME,
+				null, preparedResource.getUniqueId(), csarInfo.getModifier()
+					.getUserId(),
+				preparedResource.getComponentInstances(), false);
 		}
-		componentInstanceBusinessLogic.updateComponentInstance(ComponentTypeEnum.RESOURCE_PARAM_NAME,
-			null, preparedResource.getUniqueId(), csarInfo.getModifier()
-				.getUserId(),
-			preparedResource.getComponentInstances(), false);
 	}
 
 	private Either<Resource, ResponseFormat> createOrUpdateArtifacts(ArtifactOperationEnum operation,
@@ -1822,36 +1825,62 @@ public class ResourceBusinessLogic extends ComponentBusinessLogic {
 					.listIterator();
 			while (getInputValuesIter.hasNext()) {
 				GetInputValueDataDefinition getInput = getInputValuesIter.next();
-				InputDefinition input = findInputByName(inputs, getInput);
-				getInput.setInputId(input.getUniqueId());
-				if (getInput.getGetInputIndex() != null) {
-					GetInputValueDataDefinition getInputIndex = getInput.getGetInputIndex();
-					input = findInputByName(inputs, getInputIndex);
-					getInputIndex.setInputId(input.getUniqueId());
-					getInputValuesIter.add(getInputIndex);
+				Either<InputDefinition, RuntimeException> inputEither = findInputByName(inputs, getInput);
+				if (inputEither.isRight()) {
+					throw inputEither.right().value();
+				} else {
+					InputDefinition input = inputEither.left().value();
+					getInput.setInputId(input.getUniqueId());
+					if (getInput.getGetInputIndex() != null) {
+						GetInputValueDataDefinition getInputIndex = getInput.getGetInputIndex();
+						Either<InputDefinition, RuntimeException> newInputEither = findInputByName(inputs,
+							getInputIndex);
+						if (newInputEither.isRight()) {
+							throw newInputEither.right().value();
+						} else {
+							InputDefinition newInput = newInputEither.left().value();
+							getInputIndex.setInputId(newInput.getUniqueId());
+						}
+						getInputValuesIter.add(getInputIndex);
+					}
 				}
 			}
 		}
 	}
 
-	private InputDefinition findInputByName(List<InputDefinition> inputs, GetInputValueDataDefinition getInput) {
+	static <T> Either<T, RuntimeException> rollbackWithEither(
+		final JanusGraphDao janusGraphDao,
+		final ActionStatus actionStatus,
+		final String... params) {
+		if (janusGraphDao != null)
+			janusGraphDao.rollback();
+		return Either.right(new ByActionStatusComponentException(actionStatus, params));
+	}
+
+	<T> Either<T, RuntimeException> rollbackWithEither(
+		final ActionStatus actionStatus,
+		final String... params) {
+		return rollbackWithEither(janusGraphDao, actionStatus, params);
+	}
+
+	private Either<InputDefinition, RuntimeException> findInputByName(List<InputDefinition> inputs, GetInputValueDataDefinition getInput) {
 
 		final String inputName = getInput != null ? getInput.getInputName() : "";
 
 		if(inputs == null || inputs.isEmpty()) {
 			log.debug("#findInputByName - Inputs list is empty");
-			rollbackWithException(ActionStatus.INPUTS_NOT_FOUND, inputName);
-		}
-
-		Optional<InputDefinition> inputOpt = inputs.stream()
-				.filter(p -> p.getName()
-						.equals(inputName))
+			return rollbackWithEither(ActionStatus.INPUTS_NOT_FOUND, inputName);
+		} else {
+			Optional<InputDefinition> inputOpt = inputs.stream()
+				.filter(p -> p.getName().equals(inputName))
 				.findFirst();
-		if (!inputOpt.isPresent()) {
-			log.debug("#findInputByName - Failed to find the input {} ", inputName);
-			rollbackWithException(ActionStatus.INPUTS_NOT_FOUND, inputName);
+			if (!inputOpt.isPresent()) {
+				log.debug("#findInputByName - Failed to find the input {} ", inputName);
+				return rollbackWithEither(ActionStatus.INPUTS_NOT_FOUND, inputName);
+			} else {
+				return Either.left(inputOpt.get());
+			}
 		}
-		return inputOpt.get();
 	}
 
 	private void fillGroupsFinalFields(List<GroupDefinition> groupsAsList) {
@@ -3414,18 +3443,28 @@ public class ResourceBusinessLogic extends ComponentBusinessLogic {
 									.collect(toList())
 									.toString());
 						}
-						InputDefinition input = findInputByName(inputs, getInput);
-						getInput.setInputId(input.getUniqueId());
-						getInputValues.add(getInput);
-
-						GetInputValueDataDefinition getInputIndex = getInput.getGetInputIndex();
-						if (getInputIndex != null) {
-							input = findInputByName(inputs, getInputIndex);
-							getInputIndex.setInputId(input.getUniqueId());
-							getInputValues.add(getInputIndex);
+						Either<InputDefinition, RuntimeException> inputEither = findInputByName(inputs, getInput);
+						if (inputEither.isRight()) {
+							throw inputEither.right().value();
+						} else {
+							InputDefinition input = inputEither.left().value();
+							getInput.setInputId(input.getUniqueId());
+							getInputValues.add(getInput);
+
+							GetInputValueDataDefinition getInputIndex = getInput.getGetInputIndex();
+							if (getInputIndex != null) {
+								Either<InputDefinition, RuntimeException> newInputEither = findInputByName(inputs,
+									getInputIndex);
+								if (inputEither.isRight()) {
+									throw newInputEither.right().value();
+								} else {
+									InputDefinition newInput = newInputEither.left().value();
+									getInputIndex.setInputId(newInput.getUniqueId());
+								}
+								getInputValues.add(getInputIndex);
 
+							}
 						}
-
 					}
 					property.setGetInputValues(getInputValues);
 				}
diff --git a/catalog-be/src/test/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogicTest.java b/catalog-be/src/test/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogicTest.java
index c19d997d73..4d773a54da 100644
--- a/catalog-be/src/test/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogicTest.java
+++ b/catalog-be/src/test/java/org/openecomp/sdc/be/components/impl/ResourceBusinessLogicTest.java
@@ -59,6 +59,7 @@ import org.openecomp.sdc.be.components.csar.CsarArtifactsAndGroupsBusinessLogic;
 import org.openecomp.sdc.be.components.csar.CsarBusinessLogic;
 import org.openecomp.sdc.be.components.csar.CsarInfo;
 import org.openecomp.sdc.be.components.impl.ArtifactsBusinessLogic.ArtifactOperationEnum;
+import org.openecomp.sdc.be.components.impl.exceptions.ByActionStatusComponentException;
 import org.openecomp.sdc.be.components.impl.exceptions.ComponentException;
 import org.openecomp.sdc.be.components.impl.generic.GenericTypeBusinessLogic;
 import org.openecomp.sdc.be.components.lifecycle.LifecycleBusinessLogic;
@@ -2221,5 +2222,29 @@ public class ResourceBusinessLogicTest {
         Assert.assertEquals(true,res.isLeft());
     }
 
+    @Test
+	public void rollbackWithEitherAlwaysReturnARuntimeException() {
+		JanusGraphDao janusGraphDao = mockJanusGraphDao;
+		ActionStatus actionStatus = ActionStatus.INPUTS_NOT_FOUND;
+		String params = "testName";
+
+		Either<Object, RuntimeException> result =
+			ResourceBusinessLogic.rollbackWithEither(janusGraphDao, actionStatus, params);
+
+		assertTrue(result.isRight());
+		assertTrue(result.right().value() instanceof ByActionStatusComponentException);
+	}
 
+	@Test
+	public void rollbackWithEitherWorksWithNullJanusGraphDao() {
+		JanusGraphDao janusGraphDao = null;
+		ActionStatus actionStatus = ActionStatus.INPUTS_NOT_FOUND;
+		String params = "testName";
+
+		Either<Object, RuntimeException> result =
+			ResourceBusinessLogic.rollbackWithEither(janusGraphDao, actionStatus, params);
+
+		assertTrue(result.isRight());
+		assertTrue(result.right().value() instanceof ByActionStatusComponentException);
+	}
 }