From cbadc6e2c7b83ab27d13c9c79477f280edbd64e1 Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Mon, 17 Jun 2024 16:46:26 +0200 Subject: [PATCH] mitigate-hosting-asset-fetching-performance-problems (#60) Co-authored-by: Michael Hoennig Reviewed-on: https://dev.hostsharing.net/hostsharing/hs.hsadmin.ng/pulls/60 Reviewed-by: Marc Sandlus --- .../hs/booking/item/HsBookingItemEntity.java | 5 +- .../hosting/asset/HsHostingAssetEntity.java | 11 ++-- ...HsBookingItemControllerAcceptanceTest.java | 50 +++++++++++-------- ...sBookingItemRepositoryIntegrationTest.java | 4 +- ...sHostingAssetControllerAcceptanceTest.java | 4 +- ...HostingAssetRepositoryIntegrationTest.java | 11 ++-- .../test/ContextBasedTestWithCleanup.java | 41 ++++++++------- 7 files changed, 74 insertions(+), 52 deletions(-) diff --git a/src/main/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemEntity.java b/src/main/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemEntity.java index b820c243..6daa8ba9 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemEntity.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemEntity.java @@ -24,6 +24,7 @@ import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.EnumType; import jakarta.persistence.Enumerated; +import jakarta.persistence.FetchType; import jakarta.persistence.GeneratedValue; import jakarta.persistence.Id; import jakarta.persistence.JoinColumn; @@ -82,11 +83,11 @@ public class HsBookingItemEntity implements Stringifyable, RbacObject { @Version private int version; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "projectuuid") private HsBookingProjectEntity project; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "parentitemuuid") private HsBookingItemEntity parentItem; diff --git a/src/main/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetEntity.java b/src/main/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetEntity.java index 3f8202ef..164e42d0 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetEntity.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetEntity.java @@ -21,6 +21,7 @@ import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.EnumType; import jakarta.persistence.Enumerated; +import jakarta.persistence.FetchType; import jakarta.persistence.GeneratedValue; import jakarta.persistence.Id; import jakarta.persistence.JoinColumn; @@ -77,15 +78,15 @@ public class HsHostingAssetEntity implements Stringifyable, RbacObject { @Version private int version; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "bookingitemuuid") private HsBookingItemEntity bookingItem; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "parentassetuuid") private HsHostingAssetEntity parentAsset; - @ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "assignedtoassetuuid") private HsHostingAssetEntity assignedToAsset; @@ -93,12 +94,12 @@ public class HsHostingAssetEntity implements Stringifyable, RbacObject { @Enumerated(EnumType.STRING) private HsHostingAssetType type; - @OneToMany(cascade = CascadeType.REFRESH, orphanRemoval = true) + @OneToMany(cascade = CascadeType.REFRESH, orphanRemoval = true, fetch = FetchType.LAZY) @JoinColumn(name="parentassetuuid", referencedColumnName="uuid") private List subHostingAssets; @Column(name = "identifier") - private String identifier; // vm1234, xyz00, example.org, xyz00_abc + private String identifier; // e.g. vm1234, xyz00, example.org, xyz00_abc @Column(name = "caption") private String caption; diff --git a/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemControllerAcceptanceTest.java index 2804a758..bf8b4ba9 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemControllerAcceptanceTest.java @@ -4,13 +4,17 @@ import io.hypersistence.utils.hibernate.type.range.Range; import io.restassured.RestAssured; import io.restassured.http.ContentType; import net.hostsharing.hsadminng.HsadminNgApplication; -import net.hostsharing.hsadminng.hs.booking.project.HsBookingProjectEntity; import net.hostsharing.hsadminng.hs.booking.project.HsBookingProjectRepository; import net.hostsharing.hsadminng.hs.office.debitor.HsOfficeDebitorRepository; import net.hostsharing.hsadminng.rbac.test.ContextBasedTestWithCleanup; import net.hostsharing.hsadminng.rbac.test.JpaAttempt; +import org.junit.jupiter.api.ClassOrderer; +import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestClassOrder; +import org.junit.jupiter.api.TestMethodOrder; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.server.LocalServerPort; @@ -33,6 +37,7 @@ import static org.hamcrest.Matchers.matchesRegex; classes = { HsadminNgApplication.class, JpaAttempt.class } ) @Transactional +@TestClassOrder(ClassOrderer.OrderAnnotation.class) // fail early on fetching problems class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup { @LocalServerPort @@ -51,6 +56,7 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup JpaAttempt jpaAttempt; @Nested + @Order(2) class ListBookingItems { @Test @@ -119,6 +125,7 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @Nested + @Order(3) class AddBookingItem { @Test @@ -170,13 +177,16 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @Nested + @Order(1) + @TestMethodOrder(MethodOrderer.OrderAnnotation.class) class GetBookingItem { @Test + @Order(1) void globalAdmin_canGetArbitraryBookingItem() { context.define("superuser-alex@hostsharing.net"); final var givenBookingItemUuid = bookingItemRepo.findByCaption("separate ManagedWebspace").stream() - .filter(bi -> belongsToDebitorWithDefaultPrefix(bi, "fir")) + .filter(bi -> belongsToProject(bi, "D-1000111 default project")) .map(HsBookingItemEntity::getUuid) .findAny().orElseThrow(); @@ -206,10 +216,11 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @Test + @Order(2) void normalUser_canNotGetUnrelatedBookingItem() { context.define("superuser-alex@hostsharing.net"); final var givenBookingItemUuid = bookingItemRepo.findByCaption("separate ManagedServer").stream() - .filter(bi -> belongsToDebitorWithDefaultPrefix(bi, "sec")) + .filter(bi -> belongsToProject(bi, "D-1000212 default project")) .map(HsBookingItemEntity::getUuid) .findAny().orElseThrow(); @@ -224,23 +235,21 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @Test - // TODO.impl: For unknown reason, this test fails in about 50%, not finding the uuid (404), maybe no SELECT permission? + @Order(3) + // TODO.impl: For unknown reason this test fails in about 50%, not finding the uuid (404). void projectAdmin_canGetRelatedBookingItem() { context.define("superuser-alex@hostsharing.net"); - final var givenBookingItemUuid = bookingItemRepo.findByCaption("separate ManagedServer").stream() - .filter(bi -> belongsToDebitorWithDefaultPrefix(bi, "sec")) - .map(HsBookingItemEntity::getUuid) + final var givenBookingItem = bookingItemRepo.findByCaption("separate ManagedServer").stream() + .filter(bi -> belongsToProject(bi, "D-1000313 default project")) .findAny().orElseThrow(); - generateRbacDiagramForObjectPermission(givenBookingItemUuid, "SELECT", "select"); - RestAssured // @formatter:off .given() .header("current-user", "superuser-alex@hostsharing.net") - .header("assumed-roles", "hs_booking_project#D-1000212-D-1000212defaultproject:ADMIN") + .header("assumed-roles", "hs_booking_project#D-1000313-D-1000313defaultproject:ADMIN") .port(port) .when() - .get("http://localhost/api/hs/booking/items/" + givenBookingItemUuid) + .get("http://localhost/api/hs/booking/items/" + givenBookingItem.getUuid()) .then().log().all().assertThat() .statusCode(200) .contentType("application/json") @@ -260,22 +269,22 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup """)); // @formatter:on } - private static boolean belongsToDebitorWithDefaultPrefix(final HsBookingItemEntity bi, final String defaultPrefix) { + private static boolean belongsToProject(final HsBookingItemEntity bi, final String projectCaption) { return ofNullable(bi) .map(HsBookingItemEntity::getProject) - .map(HsBookingProjectEntity::getDebitor) - .filter(bd -> bd.getDefaultPrefix().equals(defaultPrefix)) + .filter(bp -> bp.getCaption().equals(projectCaption)) .isPresent(); } } @Nested + @Order(4) class PatchBookingItem { @Test void globalAdmin_canPatchAllUpdatablePropertiesOfBookingItem() { - final var givenBookingItem = givenSomeBookingItem(1000111, MANAGED_WEBSPACE, + final var givenBookingItem = givenSomeNewBookingItem("D-1000111 default project", MANAGED_WEBSPACE, resource("HDD", 100), resource("SSD", 50), resource("Traffic", 250)); RestAssured // @formatter:off @@ -324,12 +333,13 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @Nested + @Order(5) class DeleteBookingItem { @Test void globalAdmin_canDeleteArbitraryBookingItem() { context.define("superuser-alex@hostsharing.net"); - final var givenBookingItem = givenSomeBookingItem(1000111, MANAGED_WEBSPACE, + final var givenBookingItem = givenSomeNewBookingItem("D-1000111 default project", MANAGED_WEBSPACE, resource("HDD", 100), resource("SSD", 50), resource("Traffic", 250)); RestAssured // @formatter:off @@ -348,7 +358,7 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup @Test void normalUser_canNotDeleteUnrelatedBookingItem() { context.define("superuser-alex@hostsharing.net"); - final var givenBookingItem = givenSomeBookingItem(1000111, MANAGED_WEBSPACE, + final var givenBookingItem = givenSomeNewBookingItem("D-1000111 default project", MANAGED_WEBSPACE, resource("HDD", 100), resource("SSD", 50), resource("Traffic", 250)); RestAssured // @formatter:off @@ -366,13 +376,11 @@ class HsBookingItemControllerAcceptanceTest extends ContextBasedTestWithCleanup } @SafeVarargs - private HsBookingItemEntity givenSomeBookingItem(final int debitorNumber, + private HsBookingItemEntity givenSomeNewBookingItem(final String projectCaption, final HsBookingItemType hsBookingItemType, final Map.Entry... resources) { return jpaAttempt.transacted(() -> { context.define("superuser-alex@hostsharing.net"); - final var givenProject = debitorRepo.findDebitorByDebitorNumber(debitorNumber).stream() - .map(d -> projectRepo.findAllByDebitorUuid(d.getUuid())) - .flatMap(java.util.List::stream) + final var givenProject = projectRepo.findByCaption(projectCaption).stream() .findAny().orElseThrow(); final var newBookingItem = HsBookingItemEntity.builder() .uuid(UUID.randomUUID()) diff --git a/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemRepositoryIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemRepositoryIntegrationTest.java index 028971ee..fcc290ff 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemRepositoryIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/booking/item/HsBookingItemRepositoryIntegrationTest.java @@ -231,7 +231,9 @@ class HsBookingItemRepositoryIntegrationTest extends ContextBasedTestWithCleanup private void assertThatBookingItemActuallyInDatabase(final HsBookingItemEntity saved) { final var found = bookingItemRepo.findByUuid(saved.getUuid()); assertThat(found).isNotEmpty().get().isNotSameAs(saved) - .extracting(Object::toString).isEqualTo(saved.toString()); + .extracting(HsBookingItemEntity::getResources) + .extracting(Object::toString) + .isEqualTo(saved.getResources().toString()); } } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetControllerAcceptanceTest.java index e9f8180d..2ea554c6 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetControllerAcceptanceTest.java @@ -458,8 +458,8 @@ class HsHostingAssetControllerAcceptanceTest extends ContextBasedTestWithCleanup context.define("superuser-alex@hostsharing.net"); assertThat(assetRepo.findByUuid(givenAsset.getUuid())).isPresent().get() .matches(asset -> { - assertThat(asset.toString()).isEqualTo( - "HsHostingAssetEntity(MANAGED_SERVER, vm2001, some test-asset, D-1000111:D-1000111 default project:some ManagedServer, { monit_max_cpu_usage: 90, monit_max_ram_usage: 70, monit_max_ssd_usage: 85, monit_min_free_ssd: 5 })"); + assertThat(asset.getConfig().toString()).isEqualTo( + "{ monit_max_cpu_usage: 90, monit_max_ram_usage: 70, monit_max_ssd_usage: 85, monit_min_free_ssd: 5 }"); return true; }); } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetRepositoryIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetRepositoryIntegrationTest.java index 83560cc9..480f9416 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetRepositoryIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/hosting/asset/HsHostingAssetRepositoryIntegrationTest.java @@ -24,6 +24,8 @@ import jakarta.servlet.http.HttpServletRequest; import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; import static java.util.Map.entry; import static net.hostsharing.hsadminng.hs.hosting.asset.HsHostingAssetType.CLOUD_SERVER; @@ -152,8 +154,11 @@ class HsHostingAssetRepositoryIntegrationTest extends ContextBasedTestWithCleanu } private void assertThatAssetIsPersisted(final HsHostingAssetEntity saved) { - final var found = assetRepo.findByUuid(saved.getUuid()); - assertThat(found).isNotEmpty().map(HsHostingAssetEntity::toString).get().isEqualTo(saved.toString()); + attempt(em, () -> { + context("superuser-alex@hostsharing.net"); + final var found = assetRepo.findByUuid(saved.getUuid()); + assertThat(found).isNotEmpty().map(HsHostingAssetEntity::toString).get().isEqualTo(saved.toString()); + }); } } @@ -240,7 +245,7 @@ class HsHostingAssetRepositoryIntegrationTest extends ContextBasedTestWithCleanu private void assertThatAssetActuallyInDatabase(final HsHostingAssetEntity saved) { final var found = assetRepo.findByUuid(saved.getUuid()); assertThat(found).isNotEmpty().get().isNotSameAs(saved) - .extracting(Object::toString).isEqualTo(saved.toString()); + .extracting(HsHostingAssetEntity::getVersion).isEqualTo(saved.getVersion()); } } diff --git a/src/test/java/net/hostsharing/hsadminng/rbac/test/ContextBasedTestWithCleanup.java b/src/test/java/net/hostsharing/hsadminng/rbac/test/ContextBasedTestWithCleanup.java index 154dbb11..6c9ac849 100644 --- a/src/test/java/net/hostsharing/hsadminng/rbac/test/ContextBasedTestWithCleanup.java +++ b/src/test/java/net/hostsharing/hsadminng/rbac/test/ContextBasedTestWithCleanup.java @@ -63,7 +63,6 @@ public abstract class ContextBasedTestWithCleanup extends ContextBasedTest { return merged; } - // remove HsOfficeCoopAssetsTransactionRawEntity, which is not needed anymore after this change public UUID toCleanup(final Class entityClass, final UUID uuidToCleanup) { out.println("toCleanup(" + entityClass.getSimpleName() + ", " + uuidToCleanup + ")"); entitiesToCleanup.put(uuidToCleanup, entityClass); @@ -176,7 +175,8 @@ public abstract class ContextBasedTestWithCleanup extends ContextBasedTest { } private void cleanupTemporaryTestData() { - jpaAttempt.transacted(() -> { + // For better performance in a single transaction ... + final var exception = jpaAttempt.transacted(() -> { context.define("superuser-alex@hostsharing.net", null); entitiesToCleanup.reversed().forEach((uuid, entityClass) -> { final var rvTableName = entityClass.getAnnotation(Table.class).name(); @@ -188,7 +188,12 @@ public abstract class ContextBasedTestWithCleanup extends ContextBasedTest { .setParameter("uuid", uuid).executeUpdate(); out.println("DELETING temporary " + entityClass.getSimpleName() + "#" + uuid + " deleted " + deletedRows + " rows"); }); - }).assertSuccessful(); + }).caughtException(); + + // ... and in case of foreign key violations, we rely on the RbacObject cleanup. + if (exception != null) { + System.err.println(exception); + } } private long assertNoNewRbacObjectsRolesAndGrantsLeaked() { @@ -214,24 +219,24 @@ public abstract class ContextBasedTestWithCleanup extends ContextBasedTest { } private void deleteLeakedRbacObjects() { - jpaAttempt.transacted(() -> rbacObjectRepo.findAll()).returnedValue().stream() - .filter(o -> o.serialId > latestIntialTestDataSerialId) - .sorted(comparing(o -> o.serialId)) - .forEach(o -> { - final var exception = jpaAttempt.transacted(() -> { - context.define("superuser-alex@hostsharing.net", null); + rbacObjectRepo.findAll().stream() + .filter(o -> o.serialId > latestIntialTestDataSerialId) + .sorted(comparing(o -> o.serialId)) + .forEach(o -> { + final var exception = jpaAttempt.transacted(() -> { + context.define("superuser-alex@hostsharing.net", null); - em.createNativeQuery("DELETE FROM " + o.objectTable + " WHERE uuid=:uuid") - .setParameter("uuid", o.uuid) - .executeUpdate(); + em.createNativeQuery("DELETE FROM " + o.objectTable + " WHERE uuid=:uuid") + .setParameter("uuid", o.uuid) + .executeUpdate(); - out.println("DELETING leaked " + o.objectTable + "#" + o.uuid + " SUCCEEDED"); - }).caughtException(); + out.println("DELETING leaked " + o.objectTable + "#" + o.uuid + " SUCCEEDED"); + }).caughtException(); - if (exception != null) { - out.println("DELETING leaked " + o.objectTable + "#" + o.uuid + " FAILED " + exception); - } - }); + if (exception != null) { + out.println("DELETING leaked " + o.objectTable + "#" + o.uuid + " FAILED " + exception); + } + }); } private void assertEqual(final Set before, final Set after) {