From 5dd1de4d58b0f28106c4a209b7c7de6290499e6b Mon Sep 17 00:00:00 2001 From: SAllhusen <70843721+SAllhusen@users.noreply.github.com> Date: Sat, 17 Oct 2020 20:40:18 +0200 Subject: [PATCH] TSK-1414: Fixed most of the new SonaCloud Code Smells since 4.0.2-SNAPSHOT --- .../test/security/JaasExtensionTest.java | 26 ++--- .../impl/ClassificationHistoryQueryImpl.java | 44 +++++---- ...storyEventControllerRestDocumentation.java | 4 +- ...WorkbasketAccessItemControllerIntTest.java | 97 ++++++++----------- 4 files changed, 81 insertions(+), 90 deletions(-) diff --git a/common/taskana-common-test/src/test/java/pro/taskana/common/test/security/JaasExtensionTest.java b/common/taskana-common-test/src/test/java/pro/taskana/common/test/security/JaasExtensionTest.java index 99cbc3eb1..acc7c1b51 100644 --- a/common/taskana-common-test/src/test/java/pro/taskana/common/test/security/JaasExtensionTest.java +++ b/common/taskana-common-test/src/test/java/pro/taskana/common/test/security/JaasExtensionTest.java @@ -35,7 +35,7 @@ class JaasExtensionTest { dynamicTest("dynamic test", () -> assertThat(CURRENT_USER_CONTEXT.getUserid()).isNotNull()); private static final DynamicTest NULL_DYNAMIC_TEST = dynamicTest( - "dynamic test", () -> assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null)); + "dynamic test", () -> assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull()); private static final DynamicTest DYNAMIC_TEST_USER_DYNAMIC_TEST = dynamicTest( "dynamic test", @@ -45,7 +45,7 @@ class JaasExtensionTest { @BeforeAll static void should_NotSetJaasSubject_When_AnnotationIsMissing_On_BeforeAll() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @WithAccessId(user = "beforeall") @@ -58,7 +58,7 @@ class JaasExtensionTest { @WithAccessId(user = "beforeall2") @BeforeAll static void should_NotSetJaasSubject_When_MultipleAnnotationsExist_On_BeforeAll() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } // endregion @@ -67,7 +67,7 @@ class JaasExtensionTest { @BeforeEach void should_NotSetJaasSubject_When_AnnotationIsMissing_On_BeforeEach() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @WithAccessId(user = "beforeeach") @@ -80,7 +80,7 @@ class JaasExtensionTest { @WithAccessId(user = "beforeeach2") @BeforeEach void should_NotSetJaasSubject_When_MultipleAnnotationsExist_On_BeforeEach() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } // endregion @@ -89,7 +89,7 @@ class JaasExtensionTest { @AfterEach void should_NotSetJaasSubject_When_AnnotationIsMissing_On_AfterEach() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @WithAccessId(user = "aftereach") @@ -102,7 +102,7 @@ class JaasExtensionTest { @WithAccessId(user = "afterach2") @AfterEach void should_NotSetJaasSubject_When_MultipleAnnotationsExist_On_AfterEach() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } // endregion @@ -111,7 +111,7 @@ class JaasExtensionTest { @AfterAll static void should_NotSetJaasSubject_When_AnnotationIsMissing_On_AfterAll() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @WithAccessId(user = "afterall") @@ -124,7 +124,7 @@ class JaasExtensionTest { @WithAccessId(user = "afterall2") @AfterAll static void should_NotSetJaasSubject_When_MultipleAnnotationsExist_On_AfterAll() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } // endregion @@ -133,7 +133,7 @@ class JaasExtensionTest { @Test void should_NotSetJaasSubject_When_AnnotationIsMissing_On_Test() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @WithAccessId(user = "user") @@ -164,7 +164,7 @@ class JaasExtensionTest { @Test @Disabled("this can be tested with a org.junit.platform.launcher.TestExecutionListener") void should_ThrowException_When_MultipleAnnotationsExist_On_Test() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } // endregion @@ -173,7 +173,7 @@ class JaasExtensionTest { @TestFactory List should_NotSetJaasSubject_When_AnnotationIsMissing_On_TestFactory() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); return Collections.emptyList(); } @@ -708,7 +708,7 @@ class JaasExtensionTest { @Nested class ConstructorWithoutAccessId { ConstructorWithoutAccessId() { - assertThat(CURRENT_USER_CONTEXT.getUserid()).isEqualTo(null); + assertThat(CURRENT_USER_CONTEXT.getUserid()).isNull(); } @Test diff --git a/history/taskana-simplehistory-provider/src/main/java/pro/taskana/simplehistory/impl/ClassificationHistoryQueryImpl.java b/history/taskana-simplehistory-provider/src/main/java/pro/taskana/simplehistory/impl/ClassificationHistoryQueryImpl.java index f5ac0e1e6..204d4157a 100644 --- a/history/taskana-simplehistory-provider/src/main/java/pro/taskana/simplehistory/impl/ClassificationHistoryQueryImpl.java +++ b/history/taskana-simplehistory-provider/src/main/java/pro/taskana/simplehistory/impl/ClassificationHistoryQueryImpl.java @@ -32,12 +32,11 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer private static final String SQL_EXCEPTION_MESSAGE = "Method openConnection() could not open a connection to the database."; - private TaskanaHistoryEngineImpl taskanaHistoryEngine; + private final TaskanaHistoryEngineImpl taskanaHistoryEngine; + private final List orderBy = new ArrayList<>(); + private final List orderColumns = new ArrayList<>(); private ClassificationHistoryQueryColumnName columnName; - private List orderBy; - private List orderColumns; - private String[] idIn; private String[] eventTypeIn; private TimeInterval[] createdIn; @@ -61,7 +60,6 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer private String[] custom6In; private String[] custom7In; private String[] custom8In; - private String[] eventTypeLike; private String[] userIdLike; private String[] classificationIdLike; @@ -85,8 +83,6 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer public ClassificationHistoryQueryImpl(TaskanaHistoryEngineImpl internalTaskanaHistoryEngine) { this.taskanaHistoryEngine = internalTaskanaHistoryEngine; - this.orderBy = new ArrayList<>(); - this.orderColumns = new ArrayList<>(); } @Override @@ -469,7 +465,7 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer this); List result = new ArrayList<>(); this.columnName = dbColumnName; - List cacheOrderBy = this.orderBy; + List cacheOrderBy = new ArrayList<>(this.orderBy); this.orderBy.clear(); this.addOrderCriteria(columnName.toString(), sortDirection); @@ -481,7 +477,7 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer LOGGER.error(SQL_EXCEPTION_MESSAGE, e.getCause()); return result; } finally { - this.orderBy = cacheOrderBy; + this.orderBy.addAll(cacheOrderBy); this.columnName = null; this.orderColumns.remove(orderColumns.size() - 1); taskanaHistoryEngine.returnConnection(); @@ -532,15 +528,6 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer } } - private ClassificationHistoryQueryImpl addOrderCriteria( - String columnName, SortDirection sortDirection) { - String orderByDirection = - " " + (sortDirection == null ? SortDirection.ASCENDING : sortDirection); - orderBy.add(columnName + orderByDirection); - orderColumns.add(columnName); - return this; - } - public String[] getIdIn() { return idIn; } @@ -712,4 +699,25 @@ public class ClassificationHistoryQueryImpl implements ClassificationHistoryQuer public String[] getCustom8Like() { return custom8Like; } + + public ClassificationHistoryQueryColumnName getColumnName() { + return columnName; + } + + public List getOrderBy() { + return orderBy; + } + + public List getOrderColumns() { + return orderColumns; + } + + private ClassificationHistoryQueryImpl addOrderCriteria( + String columnName, SortDirection sortDirection) { + String orderByDirection = + " " + (sortDirection == null ? SortDirection.ASCENDING : sortDirection); + orderBy.add(columnName + orderByDirection); + orderColumns.add(columnName); + return this; + } } diff --git a/history/taskana-simplehistory-rest-spring/src/test/java/pro/taskana/doc/api/TaskHistoryEventControllerRestDocumentation.java b/history/taskana-simplehistory-rest-spring/src/test/java/pro/taskana/doc/api/TaskHistoryEventControllerRestDocumentation.java index aa299c829..49c8b6b5a 100644 --- a/history/taskana-simplehistory-rest-spring/src/test/java/pro/taskana/doc/api/TaskHistoryEventControllerRestDocumentation.java +++ b/history/taskana-simplehistory-rest-spring/src/test/java/pro/taskana/doc/api/TaskHistoryEventControllerRestDocumentation.java @@ -15,7 +15,7 @@ import org.springframework.test.web.servlet.result.MockMvcResultMatchers; import pro.taskana.common.test.doc.api.BaseRestDocumentation; /** Generate documentation for the history event controller. */ -public class TaskHistoryEventControllerRestDocumentation extends BaseRestDocumentation { +class TaskHistoryEventControllerRestDocumentation extends BaseRestDocumentation { private final HashMap taskHistoryEventFieldDescriptionsMap = new HashMap<>(); @@ -24,7 +24,7 @@ public class TaskHistoryEventControllerRestDocumentation extends BaseRestDocumen private FieldDescriptor[] taskHistoryEventFieldDescriptors; @BeforeEach - public void setUp() { + void setUp() { taskHistoryEventFieldDescriptionsMap.put("taskHistoryId", "Unique ID"); taskHistoryEventFieldDescriptionsMap.put("businessProcessId", "The id of the business process"); taskHistoryEventFieldDescriptionsMap.put( diff --git a/rest/taskana-rest-spring/src/test/java/pro/taskana/workbasket/rest/WorkbasketAccessItemControllerIntTest.java b/rest/taskana-rest-spring/src/test/java/pro/taskana/workbasket/rest/WorkbasketAccessItemControllerIntTest.java index 05c2fe4fa..21af3e26e 100644 --- a/rest/taskana-rest-spring/src/test/java/pro/taskana/workbasket/rest/WorkbasketAccessItemControllerIntTest.java +++ b/rest/taskana-rest-spring/src/test/java/pro/taskana/workbasket/rest/WorkbasketAccessItemControllerIntTest.java @@ -2,12 +2,18 @@ package pro.taskana.workbasket.rest; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static pro.taskana.common.test.rest.RestHelper.TEMPLATE; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Stream; import org.assertj.core.api.ThrowableAssert.ThrowingCallable; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; import org.junit.jupiter.api.TestMethodOrder; +import org.junit.jupiter.api.function.ThrowingConsumer; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.ParameterizedTypeReference; import org.springframework.hateoas.IanaLinkRelations; @@ -15,7 +21,6 @@ import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.client.HttpClientErrorException; -import org.springframework.web.client.RestTemplate; import pro.taskana.common.rest.Mapping; import pro.taskana.common.rest.models.TaskanaPagedModel; @@ -33,18 +38,18 @@ class WorkbasketAccessItemControllerIntTest { WORKBASKET_ACCESS_ITEM_PAGE_MODEL_TYPE = new ParameterizedTypeReference< TaskanaPagedModel>() {}; - private static RestTemplate template; - @Autowired RestHelper restHelper; - @BeforeAll - static void init() { - template = RestHelper.TEMPLATE; + private final RestHelper restHelper; + + @Autowired + WorkbasketAccessItemControllerIntTest(RestHelper restHelper) { + this.restHelper = restHelper; } @Test void testGetAllWorkbasketAccessItems() { ResponseEntity> response = - template.exchange( + TEMPLATE.exchange( restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS), HttpMethod.GET, restHelper.defaultRequest(), @@ -57,7 +62,7 @@ class WorkbasketAccessItemControllerIntTest { void testGetWorkbasketAccessItemsKeepingFilters() { String parameters = "?sort-by=workbasket-key&order=asc&page-size=9&access-ids=user-1-1&page=1"; ResponseEntity> response = - template.exchange( + TEMPLATE.exchange( restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, HttpMethod.GET, restHelper.defaultRequest(), @@ -77,7 +82,7 @@ class WorkbasketAccessItemControllerIntTest { void testThrowsExceptionIfInvalidFilterIsUsed() { ThrowingCallable httpCall = () -> - template.exchange( + TEMPLATE.exchange( restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + "?sort-by=workbasket-key&order=asc&page=1&page-size=9&invalid=user-1-1", HttpMethod.GET, @@ -94,7 +99,7 @@ class WorkbasketAccessItemControllerIntTest { void testGetSecondPageSortedByWorkbasketKey() { String parameters = "?sort-by=workbasket-key&order=asc&page=2&page-size=9&access-ids=user-1-1"; ResponseEntity> response = - template.exchange( + TEMPLATE.exchange( restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, HttpMethod.GET, restHelper.defaultRequest(), @@ -124,7 +129,7 @@ class WorkbasketAccessItemControllerIntTest { String parameters = "?access-id=teamlead-2"; ResponseEntity response = - template.exchange( + TEMPLATE.exchange( restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, HttpMethod.DELETE, restHelper.defaultRequest(), @@ -133,51 +138,29 @@ class WorkbasketAccessItemControllerIntTest { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT); } - @Test - void should_ReturnBadRequest_ifAccessIdIsSubStringOfUser() { - String parameters = "?access-id=user-1"; - ThrowingCallable httpCall = - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, - HttpMethod.DELETE, - restHelper.defaultRequest(), - ParameterizedTypeReference.forType(Void.class)); - assertThatThrownBy(httpCall) - .isInstanceOf(HttpClientErrorException.class) - .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) - .isEqualTo(HttpStatus.BAD_REQUEST); - } + @TestFactory + Stream should_ReturnBadRequest_When_AccessIdIsInvalid() { + List accessIds = + Arrays.asList( + "cn=organisationseinheit ksc,cn=organisation,ou=test,o=taskana", + "cn=monitor-users,cn=groups,ou=test,o=taskana", + "user-1"); - @Test - void should_ReturnBadRequest_ifAccessIdIsGroup() { - String parameters = "?access-id=cn=monitor-users,cn=groups,ou=test,o=taskana"; - ThrowingCallable httpCall = - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, - HttpMethod.DELETE, - restHelper.defaultRequest(), - ParameterizedTypeReference.forType(Void.class)); - assertThatThrownBy(httpCall) - .isInstanceOf(HttpClientErrorException.class) - .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) - .isEqualTo(HttpStatus.BAD_REQUEST); - } - - @Test - void should_ReturnBadRequest_ifAccessIdIsOrganizationalGroup() { - String parameters = "?access-id=cn=organisationseinheit ksc,cn=organisation,ou=test,o=taskana"; - ThrowingCallable httpCall = - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, - HttpMethod.DELETE, - restHelper.defaultRequest(), - ParameterizedTypeReference.forType(Void.class)); - assertThatThrownBy(httpCall) - .isInstanceOf(HttpClientErrorException.class) - .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) - .isEqualTo(HttpStatus.BAD_REQUEST); + ThrowingConsumer test = + accessId -> { + String parameters = "?access-id=" + accessId; + ThrowingCallable httpCall = + () -> + TEMPLATE.exchange( + restHelper.toUrl(Mapping.URL_WORKBASKET_ACCESS_ITEMS) + parameters, + HttpMethod.DELETE, + restHelper.defaultRequest(), + ParameterizedTypeReference.forType(Void.class)); + assertThatThrownBy(httpCall) + .isInstanceOf(HttpClientErrorException.class) + .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) + .isEqualTo(HttpStatus.BAD_REQUEST); + }; + return DynamicTest.stream(accessIds.iterator(), s -> String.format("for user '%s'", s), test); } }