From 787400c089a7a01a32c1166e781a04671760bc57 Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Wed, 17 Aug 2022 16:38:03 +0200 Subject: [PATCH] only use @Transactional from Spring (not JavaX) and use readOnly=true where possible --- build.gradle | 3 ++- .../hs/hscustomer/CustomerController.java | 4 ++-- .../hs/hspackage/PackageController.java | 4 ++-- .../rbac/rbacgrant/RbacGrantController.java | 7 +++---- .../rbac/rbacrole/RbacRoleController.java | 4 ++-- .../rbac/rbacuser/RbacUserController.java | 6 +++--- .../hsadminng/arch/ArchUnitTest.java | 19 +++++++++++++++++-- .../RbacGrantControllerAcceptanceTest.java | 11 ++++++----- .../RbacUserRepositoryIntegrationTest.java | 2 -- 9 files changed, 37 insertions(+), 23 deletions(-) diff --git a/build.gradle b/build.gradle index 194e415d..164133ea 100644 --- a/build.gradle +++ b/build.gradle @@ -90,7 +90,8 @@ openapiProcessor { } } sourceSets.main.java.srcDir 'build/generated/sources/openapi' -compileJava.dependsOn('processSpring') +project.tasks.processResources.dependsOn('processSpring') +project.tasks.compileJava.dependsOn('processSpring') spotless { java { diff --git a/src/main/java/net/hostsharing/hsadminng/hs/hscustomer/CustomerController.java b/src/main/java/net/hostsharing/hsadminng/hs/hscustomer/CustomerController.java index 693a4939..88699886 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/hscustomer/CustomerController.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/hscustomer/CustomerController.java @@ -5,10 +5,10 @@ import net.hostsharing.hsadminng.generated.api.v1.api.CustomersApi; import net.hostsharing.hsadminng.generated.api.v1.model.CustomerResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder; -import javax.transaction.Transactional; import java.util.List; import java.util.UUID; @@ -26,7 +26,7 @@ public class CustomerController implements CustomersApi { private CustomerRepository customerRepository; @Override - @Transactional + @Transactional(readOnly = true) public ResponseEntity> listCustomers( String userName, String assumedRoles, diff --git a/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java b/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java index 46587bb0..d95b1916 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/hspackage/PackageController.java @@ -7,9 +7,9 @@ import net.hostsharing.hsadminng.generated.api.v1.model.PackageResource; import net.hostsharing.hsadminng.generated.api.v1.model.PackageUpdateResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.RestController; -import javax.transaction.Transactional; import java.util.List; import java.util.UUID; @@ -26,7 +26,7 @@ public class PackageController implements PackagesApi { private PackageRepository packageRepository; @Override - @Transactional + @Transactional(readOnly = true) public ResponseEntity> listPackages( String userName, String assumedRoles, diff --git a/src/main/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantController.java b/src/main/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantController.java index 31f59f6a..9cfd6482 100644 --- a/src/main/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantController.java +++ b/src/main/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantController.java @@ -2,16 +2,15 @@ package net.hostsharing.hsadminng.rbac.rbacgrant; import net.hostsharing.hsadminng.context.Context; import net.hostsharing.hsadminng.generated.api.v1.api.RbacgrantsApi; -import net.hostsharing.hsadminng.generated.api.v1.api.RbacrolesApi; import net.hostsharing.hsadminng.generated.api.v1.model.RbacGrantResource; -import net.hostsharing.hsadminng.generated.api.v1.model.RbacRoleResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder; -import javax.transaction.Transactional; import java.util.List; +import java.util.UUID; import static net.hostsharing.hsadminng.Mapper.map; import static net.hostsharing.hsadminng.Mapper.mapList; @@ -27,7 +26,7 @@ public class RbacGrantController implements RbacgrantsApi { private RbacGrantRepository rbacGrantRepository; @Override - @Transactional + @Transactional(readOnly = true) public ResponseEntity> listUserGrants( final String currentUser, final String assumedRoles) { diff --git a/src/main/java/net/hostsharing/hsadminng/rbac/rbacrole/RbacRoleController.java b/src/main/java/net/hostsharing/hsadminng/rbac/rbacrole/RbacRoleController.java index 86a16aed..4cf34f85 100644 --- a/src/main/java/net/hostsharing/hsadminng/rbac/rbacrole/RbacRoleController.java +++ b/src/main/java/net/hostsharing/hsadminng/rbac/rbacrole/RbacRoleController.java @@ -5,9 +5,9 @@ import net.hostsharing.hsadminng.generated.api.v1.api.RbacrolesApi; import net.hostsharing.hsadminng.generated.api.v1.model.RbacRoleResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.RestController; -import javax.transaction.Transactional; import java.util.List; import static net.hostsharing.hsadminng.Mapper.mapList; @@ -23,7 +23,7 @@ public class RbacRoleController implements RbacrolesApi { private RbacRoleRepository rbacRoleRepository; @Override - @Transactional + @Transactional(readOnly = true) public ResponseEntity> listRoles( final String currentUser, final String assumedRoles) { diff --git a/src/main/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserController.java b/src/main/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserController.java index f92aa9e0..cc8dd3db 100644 --- a/src/main/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserController.java +++ b/src/main/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserController.java @@ -6,11 +6,11 @@ import net.hostsharing.hsadminng.generated.api.v1.model.RbacUserPermissionResour import net.hostsharing.hsadminng.generated.api.v1.model.RbacUserResource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.*; import org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder; import javax.persistence.EntityManager; -import javax.transaction.Transactional; import java.util.List; import java.util.UUID; @@ -48,7 +48,7 @@ public class RbacUserController implements RbacusersApi { } @Override - @Transactional + @Transactional(readOnly=true) public ResponseEntity> listUsers( @RequestHeader(name = "current-user") final String currentUserName, @RequestHeader(name = "assumed-roles", required = false) final String assumedRoles, @@ -62,7 +62,7 @@ public class RbacUserController implements RbacusersApi { } @Override - @Transactional + @Transactional(readOnly=true) public ResponseEntity> listUserPermissions( @RequestHeader(name = "current-user") final String currentUserName, @RequestHeader(name = "assumed-roles", required = false) final String assumedRoles, diff --git a/src/test/java/net/hostsharing/hsadminng/arch/ArchUnitTest.java b/src/test/java/net/hostsharing/hsadminng/arch/ArchUnitTest.java index bff8e68a..ac8ab474 100644 --- a/src/test/java/net/hostsharing/hsadminng/arch/ArchUnitTest.java +++ b/src/test/java/net/hostsharing/hsadminng/arch/ArchUnitTest.java @@ -8,8 +8,7 @@ import org.junit.jupiter.api.Test; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.web.bind.annotation.RestController; -import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; -import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.*; import static com.tngtech.archunit.library.dependencies.SlicesRuleDefinition.slices; @AnalyzeClasses(packages = ArchUnitTest.NET_HOSTSHARING_HSADMINNG) @@ -66,6 +65,22 @@ public class ArchUnitTest { .should().haveSimpleNameEndingWith("AcceptanceTest") .orShould().haveSimpleNameNotContaining("AcceptanceTest$"); + @ArchTest + @SuppressWarnings("unused") + public static final ArchRule doNotUseJavaxTransactionAnnotationAtClassLevel = noClasses() + .should().beAnnotatedWith(javax.transaction.Transactional.class.getName()) + .as("Use @%s instead of @%s." .formatted( + org.springframework.transaction.annotation.Transactional.class.getName(), + javax.transaction.Transactional.class)); + + @ArchTest + @SuppressWarnings("unused") + public static final ArchRule doNotUseJavaxTransactionAnnotationAtMethodLevel = noMethods() + .should().beAnnotatedWith(javax.transaction.Transactional.class) + .as("Use @%s instead of @%s." .formatted( + org.springframework.transaction.annotation.Transactional.class.getName(), + javax.transaction.Transactional.class.getName())); + @Test public void everythingShouldBeFreeOfCycles() { slices().matching("net.hostsharing.hsadminng.(*)..").should().beFreeOfCycles(); diff --git a/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantControllerAcceptanceTest.java index c2135acc..4fcbce75 100644 --- a/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/rbac/rbacgrant/RbacGrantControllerAcceptanceTest.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assumptions.assumeThat; import static org.hamcrest.CoreMatchers.containsString; @SpringBootTest( @@ -31,7 +32,7 @@ import static org.hamcrest.CoreMatchers.containsString; classes = { HsadminNgApplication.class, JpaAttempt.class } ) @Accepts({ "GRT:S(Schema)" }) -@Transactional(propagation = Propagation.NEVER) +@Transactional(readOnly = true, propagation = Propagation.NEVER) class RbacGrantControllerAcceptanceTest { @LocalServerPort @@ -64,8 +65,8 @@ class RbacGrantControllerAcceptanceTest { // given final var givenNewUserName = "test-user-" + RandomStringUtils.randomAlphabetic(8) + "@example.com"; - final String givenCurrentUserPackageAdmin = "aaa00@aaa.example.com"; - final String givenAssumedRole = "package#aaa00.admin"; + final var givenCurrentUserPackageAdmin = "aaa00@aaa.example.com"; + final var givenAssumedRole = "package#aaa00.admin"; final var givenOwnPackageAdminRole = "package#aaa00.admin"; // when @@ -105,8 +106,8 @@ class RbacGrantControllerAcceptanceTest { // given final var givenNewUserName = "test-user-" + RandomStringUtils.randomAlphabetic(8) + "@example.com"; - final String givenCurrentUserPackageAdmin = "aaa00@aaa.example.com"; - final String givenAssumedRole = "package#aaa00.admin"; + final var givenCurrentUserPackageAdmin = "aaa00@aaa.example.com"; + final var givenAssumedRole = "package#aaa00.admin"; final var givenAlienPackageAdminRole = "package#aab00.admin"; // when diff --git a/src/test/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserRepositoryIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserRepositoryIntegrationTest.java index 67b61c66..2a3ac1c9 100644 --- a/src/test/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserRepositoryIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/rbac/rbacuser/RbacUserRepositoryIntegrationTest.java @@ -9,7 +9,6 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; import org.springframework.context.annotation.ComponentScan; import org.springframework.orm.jpa.JpaSystemException; -import org.springframework.test.annotation.Commit; import org.springframework.test.annotation.DirtiesContext; import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; @@ -59,7 +58,6 @@ class RbacUserRepositoryIntegrationTest { } @Test - @Commit @Transactional(propagation = Propagation.NEVER) void anyoneCanCreateTheirOwnUser_committed() {