From 2433e66295c9264bba26f15b5013378d31815841 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 12 Oct 2026 15:35:28 +0100 Subject: [PATCH 01/15] fix: Restoring the built-in sites can leave the main sites with empty pages EXO-91183 A site save replaced its layout one DAO call at a time: a concurrent reader could read the site row while its root container was already deleted and get an empty layout, which the site restore then saved back over the restored one. - @ContainerTransactional(atomic = true) runs the method in one JPA transaction, committed when it returns and rolled back when it throws or when a failed call marked it rollback-only (RollbackException). The kernel EntityManager writes join it; when the method begins it, the Spring transactional methods and Spring Data repositories join it too and their callbacks run with its outcome. SiteStorageImpl create, save and remove use it. - The site, layout, page, navigation and description caches evict their entries again once the transaction completes, after invalidating the loads in flight (TransactionCallbacks.afterCompletion); FutureCache.clearFutures invalidates every load it removes. - A site read with a missing root container is read again once and flagged as unresolved; saving it keeps the stored layout. - The site restore applies the previous site settings to the imported site instead of reading the site again after the import and saving it a second time. Co-Authored-By: Claude Opus 5.5 --- .../portal/config/model/PortalConfig.java | 44 +- .../portal/pom/data/PortalData.java | 6 + .../meeds/common/ContainerTransactional.java | 27 +- .../common/ContainerTransactionalAspect.java | 227 ++++++++- .../io/meeds/common/TransactionCallbacks.java | 81 ++++ .../commons/cache/future/FutureCache.java | 5 +- .../ContainerTransactionalAspectTest.java | 429 ++++++++++++++++++ .../ContainerTransactionalRepositoryTest.java | 155 +++++++ .../common/TransactionCallbacksTest.java | 129 ++++++ .../commons/cache/future/FutureCacheTest.java | 57 +++ .../config/NewPortalConfigListener.java | 27 +- .../config/UserPortalConfigService.java | 32 +- .../exoplatform/portal/mop/dao/SiteDAO.java | 10 + .../portal/mop/dao/SiteDAOImpl.java | 12 + .../portal/mop/storage/SiteStorageImpl.java | 98 ++-- .../cache/CacheDescriptionStorage.java | 24 +- .../mop/storage/cache/CacheLayoutStorage.java | 26 +- .../storage/cache/CacheNavigationStorage.java | 66 ++- .../mop/storage/cache/CachePageStorage.java | 27 +- .../mop/storage/cache/CacheSiteStorage.java | 33 +- .../config/TestUserPortalConfigService.java | 44 ++ .../portal/mop/dao/SiteDAOTest.java | 37 ++ .../portal/mop/dao/mock/InMemorySiteDAO.java | 6 + .../portal/mop/storage/TestDataStorage.java | 421 +++++++++++++++++ 24 files changed, 1914 insertions(+), 109 deletions(-) create mode 100644 component/common/src/main/java/io/meeds/common/TransactionCallbacks.java create mode 100644 component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java create mode 100644 component/common/src/test/java/io/meeds/common/ContainerTransactionalRepositoryTest.java create mode 100644 component/common/src/test/java/io/meeds/common/TransactionCallbacksTest.java create mode 100644 component/common/src/test/java/org/exoplatform/commons/cache/future/FutureCacheTest.java diff --git a/component/api/src/main/java/org/exoplatform/portal/config/model/PortalConfig.java b/component/api/src/main/java/org/exoplatform/portal/config/model/PortalConfig.java index 13462c5b93..647df8bfbb 100644 --- a/component/api/src/main/java/org/exoplatform/portal/config/model/PortalConfig.java +++ b/component/api/src/main/java/org/exoplatform/portal/config/model/PortalConfig.java @@ -30,8 +30,10 @@ import org.exoplatform.portal.mop.SiteType; import org.exoplatform.portal.pom.data.PortalData; +import lombok.AccessLevel; import lombok.Data; import lombok.EqualsAndHashCode; +import lombok.Setter; @Data @EqualsAndHashCode(callSuper = true) @@ -88,6 +90,14 @@ public class PortalConfig extends ModelObject implements Cloneable { private long bannerFileId; + /** + * True when the stored layout of the site couldn't be read because its root + * container is missing: a save keeps the stored layout, unless a layout is + * set with {@link #setPortalLayout(Container)}. + */ + @Setter(AccessLevel.NONE) + private boolean layoutUnresolved; + public PortalConfig() { this(PORTAL_TYPE); } @@ -127,6 +137,7 @@ public PortalConfig(PortalData data) { this.displayed = data.isDisplayed(); this.displayOrder = data.getDisplayOrder(); this.bannerFileId = data.getBannerFileId(); + this.layoutUnresolved = data.isLayoutUnresolved(); } public long getId() { @@ -139,6 +150,7 @@ public long getId() { public void setPortalLayout(Container container) { portalLayout = container == null ? new SiteLayout() : new SiteLayout(container); + layoutUnresolved = false; } public String getProperty(String name) { @@ -216,21 +228,23 @@ private static Container initDefaultLayout() { } public PortalData build() { - return new PortalData(storageId, - name, - type, - locale, - label, - description, - accessPermissions == null ? Collections.emptyList() : Arrays.asList(accessPermissions), - editPermission, - properties == null ? Collections.emptyMap() : new Properties(properties), - skin, - portalLayout.build(), - defaultLayout, - displayed, - displayOrder, - bannerFileId); + PortalData portalData = new PortalData(storageId, + name, + type, + locale, + label, + description, + accessPermissions == null ? Collections.emptyList() : Arrays.asList(accessPermissions), + editPermission, + properties == null ? Collections.emptyMap() : new Properties(properties), + skin, + portalLayout.build(), + defaultLayout, + displayed, + displayOrder, + bannerFileId); + portalData.setLayoutUnresolved(layoutUnresolved); + return portalData; } @Override diff --git a/component/api/src/main/java/org/exoplatform/portal/pom/data/PortalData.java b/component/api/src/main/java/org/exoplatform/portal/pom/data/PortalData.java index 3e1b9cc897..9e44da4bd8 100644 --- a/component/api/src/main/java/org/exoplatform/portal/pom/data/PortalData.java +++ b/component/api/src/main/java/org/exoplatform/portal/pom/data/PortalData.java @@ -69,6 +69,12 @@ public class PortalData extends ModelData { private long bannerFileId; + /** + * True when the stored layout of the site couldn't be read because its root + * container is missing: a save of this data keeps the stored layout. + */ + private boolean layoutUnresolved; + private PortalData() { super(null, null); this.key = null; diff --git a/component/common/src/main/java/io/meeds/common/ContainerTransactional.java b/component/common/src/main/java/io/meeds/common/ContainerTransactional.java index c562d8b0b0..f30a324997 100644 --- a/component/common/src/main/java/io/meeds/common/ContainerTransactional.java +++ b/component/common/src/main/java/io/meeds/common/ContainerTransactional.java @@ -26,11 +26,34 @@ /** * Describe a method as transactional. This annotation will allow to: * - Set current container to PortalContainer - * - Start a transaction for IDM and JPA sessions + * - Start the request lifecycle of the container: the IDM session transaction + * and the JPA EntityManager. The JPA writes made through kernel DAOs commit + * one DAO call at a time, unless {@link #atomic()} is set. * - * This annotation is handled in io.meeds.commons.persistence.impl.ContainerTransactionalAspect. + * This annotation is handled in io.meeds.common.ContainerTransactionalAspect. */ @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.METHOD) public @interface ContainerTransactional { + + /** + * @return true to run the method in one JPA transaction, committed when it + * returns and rolled back when it throws. The writes made through + * the kernel EntityManager (kernel DAOs, ExoTransactional methods) + * join it. When the method begins the transaction, the Spring + * transactional methods, Spring Data repositories included, join it + * too, unless a Spring transaction of its own is active on the + * thread, and the callbacks they register run once it is committed + * or rolled back, unless a Spring transaction synchronization is + * already active on the thread: their callbacks are then left to + * that synchronization. A Spring cache eviction (CacheEvict) made + * inside the method takes effect at once, before the commit. When a + * call that failed marked the transaction + * rollback-only, the method throws a RollbackException after the + * rollback, even if it returned. When a JPA transaction is already + * active on the thread, the method joins it and marks it + * rollback-only when it throws. + */ + boolean atomic() default false; + } diff --git a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java index ec5e0a5115..668d61fbf6 100644 --- a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java +++ b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java @@ -18,24 +18,42 @@ */ package io.meeds.common; +import java.util.List; + import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; +import org.aspectj.lang.reflect.MethodSignature; +import org.springframework.orm.jpa.EntityManagerHolder; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; +import org.springframework.transaction.support.TransactionSynchronizationUtils; +import org.exoplatform.commons.persistence.impl.EntityManagerService; import org.exoplatform.container.ExoContainer; import org.exoplatform.container.ExoContainerContext; import org.exoplatform.container.PortalContainer; import org.exoplatform.container.RootContainer; import org.exoplatform.container.component.RequestLifeCycle; +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; +import jakarta.persistence.EntityTransaction; +import jakarta.persistence.RollbackException; + /** * An aspect processing point that will allow to encapsulate two behavior: * - Set current container to PortalContainer - * - Start a transaction for IDM and JPA sessions + * - Start the request lifecycle of the container, and a JPA transaction when + * the annotation is {@link ContainerTransactional#atomic()} */ @Aspect public class ContainerTransactionalAspect { + /** + * The advice keeps this signature: the classes an addon wove at its own build + * call it as is. + */ @Around("execution(* *(..)) && @annotation(io.meeds.common.ContainerTransactional)") public Object around(ProceedingJoinPoint point) throws Throwable { ExoContainer container = ExoContainerContext.getCurrentContainer(); @@ -46,7 +64,11 @@ public Object around(ProceedingJoinPoint point) throws Throwable { } RequestLifeCycle.begin(transactionContainer); try { - return point.proceed(); + if (isAtomic(point)) { + return proceedAtomically(point, transactionContainer.getComponentInstanceOfType(EntityManagerService.class)); + } else { + return point.proceed(); + } } finally { RequestLifeCycle.end(); if (transactionContainer != container) { @@ -55,4 +77,205 @@ public Object around(ProceedingJoinPoint point) throws Throwable { } } + /** + * Runs the advised method in the active JPA transaction of the thread, or in + * a new one that is committed when the method returns and rolled back when it + * throws. The kernel DAOs annotated with ExoTransactional join it without + * committing it. When the method begins the transaction, the Spring + * transactional methods (Spring Data repositories included) join it too, + * unless a Spring transaction of its own is active on the thread. + */ + Object proceedAtomically(ProceedingJoinPoint point, EntityManagerService entityManagerService) throws Throwable { + if (entityManagerService == null) { + // No JPA persistence in the container: there is no JPA write to group + return point.proceed(); + } + EntityManager entityManager = entityManagerService.getEntityManager(); + if (entityManager == null) { + throw new IllegalStateException("No JPA EntityManager is started on the current thread: the method can't run atomically"); + } + EntityTransaction transaction = entityManager.getTransaction(); + if (transaction.isActive()) { + return proceedInActiveTransaction(point, transaction); + } else { + return proceedInNewTransaction(point, + transaction, + SpringTransactionScope.open(entityManagerService.getEntityManagerFactory(), entityManager)); + } + } + + private Object proceedInActiveTransaction(ProceedingJoinPoint point, EntityTransaction transaction) throws Throwable { + try { + return point.proceed(); + } catch (Throwable e) { // NOSONAR the enclosing transaction can't be committed whatever the error + if (transaction.isActive()) { + transaction.setRollbackOnly(); + } + throw e; + } + } + + private Object proceedInNewTransaction(ProceedingJoinPoint point, + EntityTransaction transaction, + SpringTransactionScope springScope) throws Throwable { + int status = TransactionSynchronization.STATUS_UNKNOWN; + boolean committing = false; + try { + transaction.begin(); + Object result = point.proceed(); + if (transaction.getRollbackOnly()) { + springScope.beforeCompletion(); + springScope.close(); + transaction.rollback(); + status = TransactionSynchronization.STATUS_ROLLED_BACK; + throw new RollbackException("The transaction of " + point.getSignature() + + " was marked rollback-only by a call that failed: none of its writes is committed"); + } + springScope.beforeCommit(); + springScope.close(); + committing = true; + transaction.commit(); + status = TransactionSynchronization.STATUS_COMMITTED; + springScope.afterCommit(); + return result; + } catch (Throwable e) { // NOSONAR none of the method writes is committed whatever the error + if (transaction.isActive()) { + springScope.beforeCompletion(); + springScope.close(); + transaction.rollback(); + status = TransactionSynchronization.STATUS_ROLLED_BACK; + } else if (committing && status == TransactionSynchronization.STATUS_UNKNOWN && e instanceof RollbackException) { + // The JPA provider rolls the transaction back before throwing + status = TransactionSynchronization.STATUS_ROLLED_BACK; + } + throw e; + } finally { + springScope.end(status); + } + } + + boolean isAtomic(ProceedingJoinPoint point) { + if (point.getSignature() instanceof MethodSignature methodSignature) { + ContainerTransactional containerTransactional = methodSignature.getMethod().getAnnotation(ContainerTransactional.class); + return containerTransactional != null && containerTransactional.atomic(); + } else { + return false; + } + } + + /** + * Spring transactional methods, Spring Data repositories included, write + * through the EntityManager that Spring's transaction synchronization holds + * for the factory. The scope holds the thread's EntityManager there, as a + * transaction already active, so that they join the transaction instead of + * committing their own. When no Spring transaction synchronization is + * active on the thread, the scope opens one and collects the callbacks they + * register (a TransactionalEventListener event) so that they run with the + * outcome of the transaction. The before-commit and before-completion + * callbacks run inside the transaction. The EntityManager is then released + * and the synchronization closed, before the transaction is committed or + * rolled back: a Spring transactional method called by a callback of the + * commit or of the rollback (a TransactionCallbacks callback), or by an + * after-commit or after-completion callback, runs in a Spring transaction of + * its own, with callbacks of its own, and one called while the commit + * flushes the writes (an entity listener) doesn't join the transaction. When a + * synchronization is already active, as in a Spring transaction without an + * EntityManager of its own, the scope collects none of their callbacks: they + * are left to that synchronization, which runs them with its own outcome and, + * without an actual transaction, skips a TransactionalEventListener event + * unless its fallback execution runs it at once. + */ + private static final class SpringTransactionScope { + + private static final SpringTransactionScope NONE = new SpringTransactionScope(null, false); + + private final EntityManagerFactory boundFactory; + + private final boolean synchronizing; + + private boolean completing; + + private List synchronizations; + + private SpringTransactionScope(EntityManagerFactory boundFactory, boolean synchronizing) { + this.boundFactory = boundFactory; + this.synchronizing = synchronizing; + } + + static SpringTransactionScope open(EntityManagerFactory entityManagerFactory, EntityManager entityManager) { + if (entityManagerFactory == null || TransactionSynchronizationManager.hasResource(entityManagerFactory)) { + // A Spring transaction of its own holds an EntityManager for the + // factory on the thread: the Spring writes stay in it + return NONE; + } + TransactionSynchronizationManager.bindResource(entityManagerFactory, new ThreadEntityManagerHolder(entityManager)); + boolean synchronizing = !TransactionSynchronizationManager.isSynchronizationActive(); + if (synchronizing) { + TransactionSynchronizationManager.initSynchronization(); + TransactionSynchronizationManager.setActualTransactionActive(true); + } + return new SpringTransactionScope(entityManagerFactory, synchronizing); + } + + void beforeCommit() { + if (synchronizing) { + TransactionSynchronizationUtils.triggerBeforeCommit(false); + beforeCompletion(); + } + } + + void beforeCompletion() { + if (synchronizing && !completing) { + completing = true; + TransactionSynchronizationUtils.triggerBeforeCompletion(); + } + } + + void close() { + release(); + if (synchronizing && synchronizations == null) { + synchronizations = TransactionSynchronizationManager.getSynchronizations(); + TransactionSynchronizationManager.clearSynchronization(); + TransactionSynchronizationManager.setActualTransactionActive(false); + } + } + + void afterCommit() { + close(); + if (synchronizing) { + TransactionSynchronizationUtils.invokeAfterCommit(synchronizations); + } + } + + void end(int status) { + close(); + if (synchronizing) { + TransactionSynchronizationUtils.invokeAfterCompletion(synchronizations, status); + } + } + + private void release() { + if (boundFactory != null) { + TransactionSynchronizationManager.unbindResourceIfPossible(boundFactory); + } + } + + } + + private static class ThreadEntityManagerHolder extends EntityManagerHolder { + + ThreadEntityManagerHolder(EntityManager entityManager) { + super(entityManager); + // The EntityManager is given to the repositories as is, without joining + // it to a transaction nor closing it with one + setSynchronizedWithTransaction(true); + } + + @Override + protected boolean isTransactionActive() { + return getEntityManager().getTransaction().isActive(); + } + + } + } diff --git a/component/common/src/main/java/io/meeds/common/TransactionCallbacks.java b/component/common/src/main/java/io/meeds/common/TransactionCallbacks.java new file mode 100644 index 0000000000..c35f6e3a83 --- /dev/null +++ b/component/common/src/main/java/io/meeds/common/TransactionCallbacks.java @@ -0,0 +1,81 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import org.hibernate.Transaction; + +import org.exoplatform.commons.persistence.impl.EntityManagerService; +import org.exoplatform.container.ExoContainerContext; +import org.exoplatform.services.log.ExoLogger; +import org.exoplatform.services.log.Log; + +import jakarta.persistence.EntityManager; +import jakarta.transaction.Synchronization; + +/** + * Callbacks run once the JPA transaction of the current thread completes + */ +public class TransactionCallbacks { + + private static final Log LOG = ExoLogger.getLogger(TransactionCallbacks.class); + + private TransactionCallbacks() { + // Utility class + } + + /** + * Registers a callback run once the JPA transaction active on the current + * thread completes, whether it's committed or rolled back. A cache eviction + * made inside a transaction is repeated this way: a concurrent read made + * before the commit caches the previous state again. + * + * @param callback the callback to run, its errors are logged + * @return true when the callback is registered, false when no JPA + * transaction is active on the thread: the callback isn't run then + */ + public static boolean afterCompletion(Runnable callback) { + return afterCompletion(ExoContainerContext.getService(EntityManagerService.class), callback); + } + + static boolean afterCompletion(EntityManagerService entityManagerService, Runnable callback) { + EntityManager entityManager = entityManagerService == null ? null : entityManagerService.getEntityManager(); + if (entityManager == null + || !(entityManager.getTransaction() instanceof Transaction transaction) + || !transaction.isActive()) { + return false; + } + transaction.registerSynchronization(new Synchronization() { + @Override + public void beforeCompletion() { + // Nothing to run before the transaction completes + } + + @Override + public void afterCompletion(int status) { + try { + callback.run(); + } catch (RuntimeException e) { + LOG.warn("Error while running a callback after the completion of a transaction", e); + } + } + }); + return true; + } + +} diff --git a/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java b/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java index 1742aa811c..3ae2738822 100644 --- a/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java +++ b/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java @@ -92,8 +92,9 @@ public void removeFuture(K key) { * {@link #removeFuture(Object)} applied to a whole-cache clear. */ public void clearFutures() { - futureEntries.values().forEach(Retrieval::invalidate); - futureEntries.clear(); + // Each load leaves the map with its invalidation, so that a load + // registered meanwhile is never dropped from the map without it + futureEntries.keySet().forEach(this::removeFuture); } /** diff --git a/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java b/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java new file mode 100644 index 0000000000..8aa498b251 --- /dev/null +++ b/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java @@ -0,0 +1,429 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.util.List; + +import org.aspectj.lang.ProceedingJoinPoint; +import org.aspectj.lang.Signature; +import org.aspectj.lang.reflect.MethodSignature; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InOrder; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.orm.jpa.EntityManagerHolder; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; + +import org.exoplatform.commons.persistence.impl.EntityManagerService; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; +import jakarta.persistence.EntityTransaction; +import jakarta.persistence.RollbackException; + +@ExtendWith(MockitoExtension.class) +class ContainerTransactionalAspectTest { + + private static final Object RESULT = new Object(); + + @Mock + private ProceedingJoinPoint point; + + @Mock + private EntityManagerService entityManagerService; + + @Mock + private EntityManagerFactory entityManagerFactory; + + @Mock + private EntityManager entityManager; + + @Mock + private EntityTransaction transaction; + + private ContainerTransactionalAspect aspect = new ContainerTransactionalAspect(); + + @BeforeEach + void setUp() { + lenient().when(entityManagerService.getEntityManager()).thenReturn(entityManager); + lenient().when(entityManagerService.getEntityManagerFactory()).thenReturn(entityManagerFactory); + lenient().when(entityManager.getTransaction()).thenReturn(transaction); + } + + private boolean isSpringScopeClosed() { + return !TransactionSynchronizationManager.hasResource(entityManagerFactory) + && !TransactionSynchronizationManager.isSynchronizationActive() + && !TransactionSynchronizationManager.isActualTransactionActive(); + } + + @AfterEach + void tearDown() { + TransactionSynchronizationManager.unbindResourceIfPossible(entityManagerFactory); + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.clearSynchronization(); + } + TransactionSynchronizationManager.setActualTransactionActive(false); + } + + @Test + void testCommitWhenTheMethodReturns() throws Throwable { + when(point.proceed()).thenReturn(RESULT); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + InOrder order = inOrder(transaction, point); + order.verify(transaction).begin(); + order.verify(point).proceed(); + order.verify(transaction).commit(); + verify(transaction, never()).rollback(); + } + + @Test + void testRollbackWhenTheMethodThrows() throws Throwable { + IllegalStateException error = new IllegalStateException("write failed"); + when(point.proceed()).thenAnswer(invocation -> { + when(transaction.isActive()).thenReturn(true); + throw error; + }); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService))); + + verify(transaction).begin(); + verify(transaction).rollback(); + verify(transaction, never()).commit(); + } + + @Test + void testRollbackAndThrowWhenTheTransactionIsMarkedRollbackOnly() throws Throwable { + when(point.proceed()).thenAnswer(invocation -> { + when(transaction.isActive()).thenReturn(true); + when(transaction.getRollbackOnly()).thenReturn(true); + return RESULT; + }); + boolean[] closedBeforeRollback = new boolean[1]; + doAnswer(invocation -> { + closedBeforeRollback[0] = isSpringScopeClosed(); + return when(transaction.isActive()).thenReturn(false); + }).when(transaction).rollback(); + + assertThrows(RollbackException.class, () -> aspect.proceedAtomically(point, entityManagerService)); + + verify(transaction).rollback(); + verify(transaction, never()).commit(); + assertTrue(closedBeforeRollback[0]); + } + + @Test + void testJoinTheActiveTransaction() throws Throwable { + when(transaction.isActive()).thenReturn(true); + when(point.proceed()).thenReturn(RESULT); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + verify(transaction, never()).begin(); + verify(transaction, never()).commit(); + verify(transaction, never()).rollback(); + verify(transaction, never()).setRollbackOnly(); + } + + @Test + void testMarkTheJoinedTransactionRollbackOnlyWhenTheMethodThrows() throws Throwable { + IllegalStateException error = new IllegalStateException("write failed"); + when(transaction.isActive()).thenReturn(true); + when(point.proceed()).thenThrow(error); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService))); + + verify(transaction).setRollbackOnly(); + verify(transaction, never()).begin(); + verify(transaction, never()).commit(); + verify(transaction, never()).rollback(); + } + + @Test + void testHoldTheEntityManagerForSpringRepositoriesDuringTheMethod() throws Throwable { + when(point.proceed()).thenAnswer(invocation -> { + EntityManagerHolder holder = (EntityManagerHolder) TransactionSynchronizationManager.getResource(entityManagerFactory); + assertSame(entityManager, holder.getEntityManager()); + assertTrue(holder.isSynchronizedWithTransaction()); + throw new IllegalStateException("write failed"); + }); + + assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService)); + + assertFalse(TransactionSynchronizationManager.hasResource(entityManagerFactory)); + } + + @Test + void testKeepTheEntityManagerOfASpringTransaction() throws Throwable { + EntityManagerHolder springHolder = new EntityManagerHolder(mock(EntityManager.class)); + TransactionSynchronizationManager.bindResource(entityManagerFactory, springHolder); + when(point.proceed()).thenAnswer(invocation -> { + assertSame(springHolder, TransactionSynchronizationManager.getResource(entityManagerFactory)); + return RESULT; + }); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + assertSame(springHolder, TransactionSynchronizationManager.getResource(entityManagerFactory)); + } + + @Test + void testRunTheSpringCallbacksOnceTheTransactionIsCommitted() throws Throwable { + TransactionSynchronization synchronization = mock(TransactionSynchronization.class); + when(point.proceed()).thenAnswer(invocation -> { + assertTrue(TransactionSynchronizationManager.isActualTransactionActive()); + TransactionSynchronizationManager.registerSynchronization(synchronization); + return RESULT; + }); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + InOrder order = inOrder(synchronization, transaction); + order.verify(synchronization).beforeCommit(false); + order.verify(synchronization).beforeCompletion(); + order.verify(transaction).commit(); + order.verify(synchronization).afterCommit(); + order.verify(synchronization).afterCompletion(TransactionSynchronization.STATUS_COMMITTED); + assertFalse(TransactionSynchronizationManager.isSynchronizationActive()); + assertFalse(TransactionSynchronizationManager.isActualTransactionActive()); + } + + @Test + void testRunTheSpringCallbacksOnceTheTransactionIsRolledBack() throws Throwable { + TransactionSynchronization synchronization = mock(TransactionSynchronization.class); + IllegalStateException error = new IllegalStateException("write failed"); + when(point.proceed()).thenAnswer(invocation -> { + when(transaction.isActive()).thenReturn(true); + TransactionSynchronizationManager.registerSynchronization(synchronization); + throw error; + }); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService))); + + InOrder order = inOrder(synchronization, transaction); + order.verify(synchronization).beforeCompletion(); + order.verify(transaction).rollback(); + order.verify(synchronization).afterCompletion(TransactionSynchronization.STATUS_ROLLED_BACK); + verify(synchronization, never()).afterCommit(); + assertFalse(TransactionSynchronizationManager.isSynchronizationActive()); + } + + @Test + void testReleaseTheEntityManagerBeforeTheSpringCallbacks() throws Throwable { + boolean[] releasedBeforeAfterCommit = new boolean[1]; + boolean[] scopeClosedBeforeAfterCommit = new boolean[1]; + boolean[] scopeClosedBeforeAfterCompletion = new boolean[1]; + when(point.proceed()).thenAnswer(invocation -> { + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + @Override + public void afterCommit() { + releasedBeforeAfterCommit[0] = !TransactionSynchronizationManager.hasResource(entityManagerFactory); + scopeClosedBeforeAfterCommit[0] = !TransactionSynchronizationManager.isSynchronizationActive() + && !TransactionSynchronizationManager.isActualTransactionActive(); + } + + @Override + public void afterCompletion(int status) { + scopeClosedBeforeAfterCompletion[0] = !TransactionSynchronizationManager.isSynchronizationActive(); + } + }); + return RESULT; + }); + + aspect.proceedAtomically(point, entityManagerService); + + assertTrue(releasedBeforeAfterCommit[0]); + assertTrue(scopeClosedBeforeAfterCommit[0], "A Spring transactional call of an after-commit callback has callbacks of its own"); + assertTrue(scopeClosedBeforeAfterCompletion[0]); + } + + @Test + void testReleaseTheEntityManagerBeforeTheSpringCallbacksOfARollback() throws Throwable { + boolean[] releasedBeforeAfterCompletion = new boolean[1]; + boolean[] scopeClosedBeforeAfterCompletion = new boolean[1]; + when(point.proceed()).thenAnswer(invocation -> { + when(transaction.isActive()).thenReturn(true); + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + @Override + public void afterCompletion(int status) { + releasedBeforeAfterCompletion[0] = !TransactionSynchronizationManager.hasResource(entityManagerFactory); + scopeClosedBeforeAfterCompletion[0] = !TransactionSynchronizationManager.isSynchronizationActive() + && !TransactionSynchronizationManager.isActualTransactionActive(); + } + }); + throw new IllegalStateException("write failed"); + }); + + assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService)); + + assertTrue(releasedBeforeAfterCompletion[0]); + assertTrue(scopeClosedBeforeAfterCompletion[0], "A Spring transactional call of an after-completion callback has callbacks of its own"); + } + + @Test + void testLeaveTheActiveSpringSynchronizationToItsTransaction() throws Throwable { + TransactionSynchronization outerSynchronization = mock(TransactionSynchronization.class); + TransactionSynchronizationManager.initSynchronization(); + TransactionSynchronizationManager.registerSynchronization(outerSynchronization); + when(point.proceed()).thenAnswer(invocation -> { + EntityManagerHolder holder = (EntityManagerHolder) TransactionSynchronizationManager.getResource(entityManagerFactory); + assertSame(entityManager, holder.getEntityManager()); + return RESULT; + }); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + verify(transaction).commit(); + assertFalse(TransactionSynchronizationManager.hasResource(entityManagerFactory)); + assertTrue(TransactionSynchronizationManager.isSynchronizationActive()); + assertEquals(List.of(outerSynchronization), TransactionSynchronizationManager.getSynchronizations()); + verifyNoInteractions(outerSynchronization); + } + + @Test + void testCloseTheSpringScopeBeforeTheCommit() throws Throwable { + boolean[] closedBeforeCommit = new boolean[1]; + when(point.proceed()).thenReturn(RESULT); + doAnswer(invocation -> closedBeforeCommit[0] = isSpringScopeClosed()).when(transaction).commit(); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + + assertTrue(closedBeforeCommit[0], "A Spring transactional call of a callback of the commit runs in a Spring transaction of its own"); + } + + @Test + void testCloseTheSpringScopeBeforeTheRollback() throws Throwable { + boolean[] closedBeforeRollback = new boolean[1]; + when(point.proceed()).thenAnswer(invocation -> { + when(transaction.isActive()).thenReturn(true); + throw new IllegalStateException("write failed"); + }); + doAnswer(invocation -> closedBeforeRollback[0] = isSpringScopeClosed()).when(transaction).rollback(); + + assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService)); + + assertTrue(closedBeforeRollback[0], "A Spring transactional call of a callback of the rollback runs in a Spring transaction of its own"); + } + + @Test + void testReportARollbackWhenTheCommitFails() throws Throwable { + TransactionSynchronization synchronization = mock(TransactionSynchronization.class); + RollbackException error = new RollbackException("flush failed"); + when(point.proceed()).thenAnswer(invocation -> { + TransactionSynchronizationManager.registerSynchronization(synchronization); + return RESULT; + }); + doThrow(error).when(transaction).commit(); + + assertSame(error, assertThrows(RollbackException.class, () -> aspect.proceedAtomically(point, entityManagerService))); + + verify(synchronization).afterCompletion(TransactionSynchronization.STATUS_ROLLED_BACK); + verify(synchronization, never()).afterCommit(); + } + + @Test + void testOpenNoSpringScopeInAJoinedTransaction() throws Throwable { + when(transaction.isActive()).thenReturn(true); + when(point.proceed()).thenAnswer(invocation -> { + assertFalse(TransactionSynchronizationManager.hasResource(entityManagerFactory)); + assertFalse(TransactionSynchronizationManager.isSynchronizationActive()); + return RESULT; + }); + + assertSame(RESULT, aspect.proceedAtomically(point, entityManagerService)); + } + + @Test + void testProceedWithoutJpaPersistence() throws Throwable { + when(point.proceed()).thenReturn(RESULT); + + assertSame(RESULT, aspect.proceedAtomically(point, null)); + } + + @Test + void testRefuseToRunWithoutEntityManager() throws Throwable { + when(entityManagerService.getEntityManager()).thenReturn(null); + + assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService)); + + verify(point, never()).proceed(); + } + + @Test + void testIsAtomic() throws Exception { + assertTrue(aspect.isAtomic(pointOf("atomicMethod"))); + assertFalse(aspect.isAtomic(pointOf("containerMethod"))); + assertFalse(aspect.isAtomic(pointOf("plainMethod"))); + ProceedingJoinPoint constructorPoint = mock(ProceedingJoinPoint.class); + when(constructorPoint.getSignature()).thenReturn(mock(Signature.class)); + assertFalse(aspect.isAtomic(constructorPoint)); + } + + @Test + void testAtomicIsOptIn() throws Exception { + assertEquals(Boolean.FALSE, ContainerTransactional.class.getMethod("atomic").getDefaultValue()); + } + + private ProceedingJoinPoint pointOf(String methodName) throws NoSuchMethodException { + MethodSignature signature = mock(MethodSignature.class); + when(signature.getMethod()).thenReturn(AnnotatedMethods.class.getDeclaredMethod(methodName)); + ProceedingJoinPoint methodPoint = mock(ProceedingJoinPoint.class); + when(methodPoint.getSignature()).thenReturn(signature); + return methodPoint; + } + + @SuppressWarnings("unused") + private static class AnnotatedMethods { + + @ContainerTransactional(atomic = true) + void atomicMethod() { + // Annotation holder + } + + @ContainerTransactional + void containerMethod() { + // Annotation holder + } + + void plainMethod() { + // Annotation holder + } + } + +} diff --git a/component/common/src/test/java/io/meeds/common/ContainerTransactionalRepositoryTest.java b/component/common/src/test/java/io/meeds/common/ContainerTransactionalRepositoryTest.java new file mode 100644 index 0000000000..940931adb7 --- /dev/null +++ b/component/common/src/test/java/io/meeds/common/ContainerTransactionalRepositoryTest.java @@ -0,0 +1,155 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.List; + +import org.aspectj.lang.ProceedingJoinPoint; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; + +import org.exoplatform.commons.persistence.impl.EntityManagerService; +import org.exoplatform.container.component.RequestLifeCycle; +import org.exoplatform.jpa.CommonsDAOJPAImplTest; +import org.exoplatform.settings.jpa.dao.SettingContextDAO; +import org.exoplatform.settings.jpa.entity.ContextEntity; + +import io.meeds.spring.module.dao.TestJpaDao; +import io.meeds.spring.module.entity.TestJpaEntity; + +import jakarta.persistence.EntityManager; + +@SpringJUnitConfig(CommonsDAOJPAImplTest.class) +public class ContainerTransactionalRepositoryTest extends CommonsDAOJPAImplTest { // NOSONAR + + @Autowired + private TestJpaDao testJpaDao; + + @Test + public void repositoryWritesRollBackWithTheAtomicMethod() throws Throwable { + long entitiesCount = testJpaDao.count(); + IllegalStateException error = new IllegalStateException("Failure after the repository write"); + ProceedingJoinPoint point = mock(ProceedingJoinPoint.class); + when(point.proceed()).thenAnswer(invocation -> { + testJpaDao.save(new TestJpaEntity(null, "rolled back")); + throw error; + }); + + assertSame(error, assertThrows(IllegalStateException.class, () -> proceedAtomically(point))); + + assertEquals(entitiesCount, testJpaDao.count()); + } + + @Test + public void repositoryWritesCommitWithTheAtomicMethod() throws Throwable { + long entitiesCount = testJpaDao.count(); + ProceedingJoinPoint point = mock(ProceedingJoinPoint.class); + when(point.proceed()).thenAnswer(invocation -> testJpaDao.save(new TestJpaEntity(null, "committed"))); + + proceedAtomically(point); + + assertEquals(entitiesCount + 1, testJpaDao.count()); + } + + @Test + public void kernelWritesAfterARepositoryWriteCommitWithTheAtomicMethod() throws Throwable { + SettingContextDAO settingContextDAO = getService(SettingContextDAO.class); + long entitiesCount = testJpaDao.count(); + long contextsCount = countContexts(settingContextDAO); + ProceedingJoinPoint point = mock(ProceedingJoinPoint.class); + when(point.proceed()).thenAnswer(invocation -> { + EntityManager entityManager = getService(EntityManagerService.class).getEntityManager(); + TestJpaEntity entity = testJpaDao.save(new TestJpaEntity(null, "committed with a kernel write")); + assertTrue(entityManager.isOpen()); + assertTrue(entityManager.getTransaction().isActive()); + ContextEntity context = settingContextDAO.create(new ContextEntity().setType("atomic").setName("committed")); + return List.of(entity, context); + }); + + List written = (List) proceedAtomically(point); + + assertEquals(entitiesCount + 1, testJpaDao.count()); + assertEquals(contextsCount + 1, countContexts(settingContextDAO)); + testJpaDao.delete((TestJpaEntity) written.get(0)); + RequestLifeCycle.begin(getContainer()); + try { + settingContextDAO.delete((ContextEntity) written.get(1)); + } finally { + RequestLifeCycle.end(); + } + } + + @Test + public void kernelWritesAfterARepositoryWriteRollBackWithTheAtomicMethod() throws Throwable { + SettingContextDAO settingContextDAO = getService(SettingContextDAO.class); + long entitiesCount = testJpaDao.count(); + long contextsCount = countContexts(settingContextDAO); + IllegalStateException error = new IllegalStateException("Failure after the kernel write"); + ProceedingJoinPoint point = mock(ProceedingJoinPoint.class); + when(point.proceed()).thenAnswer(invocation -> { + testJpaDao.save(new TestJpaEntity(null, "rolled back with a kernel write")); + settingContextDAO.create(new ContextEntity().setType("atomic").setName("rolled back")); + throw error; + }); + + assertSame(error, assertThrows(IllegalStateException.class, () -> proceedAtomically(point))); + + assertEquals(entitiesCount, testJpaDao.count()); + assertEquals(contextsCount, countContexts(settingContextDAO)); + } + + @Test + public void repositoryWritesOfATransactionCallbackCommitInATransactionOfTheirOwn() throws Throwable { + long entitiesCount = testJpaDao.count(); + ProceedingJoinPoint point = mock(ProceedingJoinPoint.class); + when(point.proceed()).thenAnswer(invocation -> TransactionCallbacks.afterCompletion(getService(EntityManagerService.class), + () -> testJpaDao.save(new TestJpaEntity(null, + "saved by a callback")))); + + assertEquals(Boolean.TRUE, proceedAtomically(point)); + + assertEquals(entitiesCount + 1, testJpaDao.count()); + } + + private long countContexts(SettingContextDAO settingContextDAO) { + RequestLifeCycle.begin(getContainer()); + try { + return settingContextDAO.count(); + } finally { + RequestLifeCycle.end(); + } + } + + private Object proceedAtomically(ProceedingJoinPoint point) throws Throwable { + RequestLifeCycle.begin(getContainer()); + try { + return new ContainerTransactionalAspect().proceedAtomically(point, getService(EntityManagerService.class)); + } finally { + RequestLifeCycle.end(); + } + } + +} diff --git a/component/common/src/test/java/io/meeds/common/TransactionCallbacksTest.java b/component/common/src/test/java/io/meeds/common/TransactionCallbacksTest.java new file mode 100644 index 0000000000..463dc241f1 --- /dev/null +++ b/component/common/src/test/java/io/meeds/common/TransactionCallbacksTest.java @@ -0,0 +1,129 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.hibernate.Transaction; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import org.exoplatform.commons.persistence.impl.EntityManagerService; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityTransaction; +import jakarta.transaction.Status; +import jakarta.transaction.Synchronization; + +@ExtendWith(MockitoExtension.class) +class TransactionCallbacksTest { + + @Mock + private EntityManagerService entityManagerService; + + @Mock + private EntityManager entityManager; + + @Mock + private Transaction transaction; + + private AtomicInteger callbackRuns = new AtomicInteger(); + + private Runnable callback = callbackRuns::incrementAndGet; + + @BeforeEach + void setUp() { + lenient().when(entityManagerService.getEntityManager()).thenReturn(entityManager); + lenient().when(entityManager.getTransaction()).thenReturn(transaction); + } + + @Test + void testRunTheCallbackOnceTheActiveTransactionCompletes() { + when(transaction.isActive()).thenReturn(true); + + assertTrue(TransactionCallbacks.afterCompletion(entityManagerService, callback)); + assertEquals(0, callbackRuns.get(), "The callback runs once the transaction completes"); + + ArgumentCaptor synchronization = ArgumentCaptor.forClass(Synchronization.class); + verify(transaction).registerSynchronization(synchronization.capture()); + synchronization.getValue().beforeCompletion(); + assertEquals(0, callbackRuns.get()); + synchronization.getValue().afterCompletion(Status.STATUS_COMMITTED); + assertEquals(1, callbackRuns.get()); + synchronization.getValue().afterCompletion(Status.STATUS_ROLLEDBACK); + assertEquals(2, callbackRuns.get()); + } + + @Test + void testLogTheCallbackErrors() { + when(transaction.isActive()).thenReturn(true); + + assertTrue(TransactionCallbacks.afterCompletion(entityManagerService, () -> { + throw new IllegalStateException("eviction failed"); + })); + + ArgumentCaptor synchronization = ArgumentCaptor.forClass(Synchronization.class); + verify(transaction).registerSynchronization(synchronization.capture()); + synchronization.getValue().afterCompletion(Status.STATUS_COMMITTED); + } + + @Test + void testNoCallbackWithoutActiveTransaction() { + when(transaction.isActive()).thenReturn(false); + + assertFalse(TransactionCallbacks.afterCompletion(entityManagerService, callback)); + + verify(transaction, never()).registerSynchronization(any()); + assertEquals(0, callbackRuns.get()); + } + + @Test + void testNoCallbackWithoutEntityManager() { + when(entityManagerService.getEntityManager()).thenReturn(null); + + assertFalse(TransactionCallbacks.afterCompletion(entityManagerService, callback)); + assertFalse(TransactionCallbacks.afterCompletion(null, callback)); + assertEquals(0, callbackRuns.get()); + } + + @Test + void testNoCallbackWithAnotherTransactionImplementation() { + EntityTransaction jpaTransaction = mock(EntityTransaction.class); + lenient().when(jpaTransaction.isActive()).thenReturn(true); + when(entityManager.getTransaction()).thenReturn(jpaTransaction); + + assertFalse(TransactionCallbacks.afterCompletion(entityManagerService, callback)); + assertEquals(0, callbackRuns.get()); + } + +} diff --git a/component/common/src/test/java/org/exoplatform/commons/cache/future/FutureCacheTest.java b/component/common/src/test/java/org/exoplatform/commons/cache/future/FutureCacheTest.java new file mode 100644 index 0000000000..5320ae9cf5 --- /dev/null +++ b/component/common/src/test/java/org/exoplatform/commons/cache/future/FutureCacheTest.java @@ -0,0 +1,57 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.commons.cache.future; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Test; + +class FutureCacheTest { + + @Test + void testClearFuturesInvalidatesTheLoadsInFlight() throws Exception { + CountDownLatch loading = new CountDownLatch(1); + CountDownLatch cleared = new CountDownLatch(1); + FutureMap futureCache = new FutureMap<>((context, key) -> { + loading.countDown(); + cleared.await(10, TimeUnit.SECONDS); + return "previous value"; + }); + AtomicReference loadedValue = new AtomicReference<>(); + Thread reader = new Thread(() -> loadedValue.set(futureCache.get(null, "key"))); + reader.start(); + try { + assertTrue(loading.await(10, TimeUnit.SECONDS)); + futureCache.clearFutures(); + } finally { + cleared.countDown(); + reader.join(10000); + } + + assertEquals("previous value", loadedValue.get(), "The load in flight returns its value to its reader"); + assertNull(futureCache.get("key"), "The load in flight caches its value after the clear"); + } + +} diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index 05355e4a40..3429aa3b99 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -27,6 +27,7 @@ import java.util.Locale; import java.util.Objects; import java.util.Set; +import java.util.function.Consumer; import java.util.regex.Pattern; import org.apache.commons.collections4.CollectionUtils; @@ -307,10 +308,21 @@ public void mergePlugin(NewPortalConfigListener other) { } public void initPortalConfigDB(NewPortalConfig ele, boolean prepareImport) { + initPortalConfigDB(ele, prepareImport, null); + } + + /** + * @param ele the sites configuration of an extension + * @param prepareImport true to import the sites even when the configuration + * doesn't override them + * @param beforeImport applied to each site configuration before it's + * imported, null to import it as is + */ + public void initPortalConfigDB(NewPortalConfig ele, boolean prepareImport, Consumer beforeImport) { RequestLifeCycle.begin(PortalContainer.getInstance()); try { if (ele.getOverrideMode() || prepareImport) { - initPortalConfigDB(ele); + initPortalConfigDB(ele, beforeImport); } } catch (Exception e) { log.error("NewPortalConfig error: " + e.getMessage(), e); @@ -320,8 +332,12 @@ public void initPortalConfigDB(NewPortalConfig ele, boolean prepareImport) { } public void initPortalConfigDB(NewPortalConfig config) { + initPortalConfigDB(config, (Consumer) null); + } + + private void initPortalConfigDB(NewPortalConfig config, Consumer beforeImport) { for (String owner : config.getPredefinedOwner()) { - if (createPortalConfig(config, owner)) { + if (createPortalConfig(config, owner, beforeImport)) { LOG.info("Importing Site of type '{}' with name '{}' from location '{}' wth mode '{}'", config.getOwnerType(), owner, @@ -373,6 +389,10 @@ public void initPageNavigationDB(NewPortalConfig config) { } public boolean createPortalConfig(NewPortalConfig config, String owner) { + return createPortalConfig(config, owner, null); + } + + private boolean createPortalConfig(NewPortalConfig config, String owner, Consumer beforeImport) { String type = config.getOwnerType(); String fixedOwnerName = fixOwnerName(type, owner); @@ -416,6 +436,9 @@ public boolean createPortalConfig(NewPortalConfig config, String owner) { // If XML configuration of PortalConfig exists or PortalConfig doesn't exist // in storage, import PortalConfig switch default ImportMode + if (beforeImport != null) { + beforeImport.accept(pConfig); + } ImportMode importMode = getRightMode(config.getImportMode()); PortalConfigImporter portalImporter = new PortalConfigImporter(importMode, pConfig, layoutService); try { diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java index 6a8764c27b..144dd13738 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java @@ -173,6 +173,7 @@ public boolean restoreSite(String type, if (!canRestore(type, name)) { return false; } + PortalConfig previousSite = restoreSiteLayout ? layoutService.getPortalConfig(type, name) : null; List configs = newPortalConfigListener.getPortalConfigs(type, name); for(NewPortalConfig config : configs) { config = config.clone(); @@ -182,21 +183,9 @@ public boolean restoreSite(String type, ownerName.add(name); config.setPredefinedOwner(ownerName); if (restoreSiteLayout) { - PortalConfig previousSite = layoutService.getPortalConfig(type, name); - newPortalConfigListener.initPortalConfigDB(config, true); - PortalConfig site = layoutService.getPortalConfig(type, name); - if (previousSite != null) { - // Preserve previous version or properties - site.setLabel(previousSite.getLabel()); - site.setDescription(previousSite.getDescription()); - site.setAccessPermissions(previousSite.getAccessPermissions()); - site.setEditPermission(previousSite.getEditPermission()); - site.setDisplayed(previousSite.isDisplayed()); - site.setDisplayOrder(previousSite.getDisplayOrder()); - site.setIcon(previousSite.getIcon()); - site.setProperties(previousSite.getProperties()); - layoutService.save(site); - } + // The imported site carries the settings of the site it replaces, so + // that it is saved once, without being read again + newPortalConfigListener.initPortalConfigDB(config, true, importedSite -> keepSiteSettings(importedSite, previousSite)); } if (restorePages) { newPortalConfigListener.initPageDB(config, true); @@ -713,6 +702,19 @@ public void setMetaPortalConfig(PortalConfig metaPortalConfig) { this.metaPortalConfig = metaPortalConfig; } + private void keepSiteSettings(PortalConfig importedSite, PortalConfig previousSite) { + if (previousSite != null) { + importedSite.setLabel(previousSite.getLabel()); + importedSite.setDescription(previousSite.getDescription()); + importedSite.setAccessPermissions(previousSite.getAccessPermissions()); + importedSite.setEditPermission(previousSite.getEditPermission()); + importedSite.setDisplayed(previousSite.isDisplayed()); + importedSite.setDisplayOrder(previousSite.getDisplayOrder()); + importedSite.setIcon(previousSite.getIcon()); + importedSite.setProperties(previousSite.getProperties()); + } + } + private PageContext getPage(PageKey pageRef, Identity userAclIdentity) { if (pageRef == null) { return null; diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAO.java b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAO.java index cd1f124f30..dfc88f2824 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAO.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAO.java @@ -30,6 +30,16 @@ public interface SiteDAO extends GenericDAO { SiteEntity findByKey(SiteKey siteKey); + /** + * Reads the site row again from the database, bypassing the state already + * loaded in the current persistence context + * + * @param entity a site entity loaded by this DAO, detached from the + * persistence context by this call + * @return the entity read again, null when the site was removed meanwhile + */ + SiteEntity refresh(SiteEntity entity); + List findByType(SiteType siteType); List findSiteKey(SiteType siteType); diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAOImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAOImpl.java index c8f4fa3656..00fded6e11 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAOImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/SiteDAOImpl.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; +import jakarta.persistence.EntityManager; import jakarta.persistence.NoResultException; import jakarta.persistence.TypedQuery; @@ -98,6 +99,17 @@ public SiteEntity findByKey(SiteKey siteKey) { } } + @Override + public SiteEntity refresh(SiteEntity entity) { + EntityManager entityManager = getEntityManager(); + if (entityManager.contains(entity)) { + // The site row is then read from the database instead of the + // persistence context + entityManager.detach(entity); + } + return find(entity.getId()); + } + @Override public List findByType(SiteType siteType) { TypedQuery query = getEntityManager().createNamedQuery("SiteEntity.findByType", SiteEntity.class); diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java index 60fbf22b3d..15eb824c96 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java @@ -72,6 +72,8 @@ import org.exoplatform.upload.UploadResource; import org.exoplatform.upload.UploadService; +import io.meeds.common.ContainerTransactional; + public class SiteStorageImpl implements SiteStorage { private static final String DEFAULT_SHAREDLAYOUT_PATH = "war:/conf/portal/portal/sharedlayout.xml"; // NOSONAR @@ -126,9 +128,10 @@ public void create(PortalConfig config) { } @Override + @ContainerTransactional(atomic = true) public void create(PortalData config) { SiteEntity entity = new SiteEntity(); - buildSiteEntity(entity, config); + buildSiteEntity(entity, config, false); entity = siteDAO.create(entity); savePermissions(entity.getId(), config); } @@ -142,14 +145,23 @@ public void save(PortalConfig config) { save(config.build()); } + /** + * Replaces the site layout and its attributes in one transaction, so that a + * concurrent reader never reads a site whose layout containers are deleted + * while its new ones aren't committed yet. + */ @Override + @ContainerTransactional(atomic = true) public void save(PortalData config) { SiteKey siteKey = new SiteKey(config.getKey().getType(), config.getKey().getId()); SiteEntity entity = siteDAO.findByKey(siteKey); if (entity == null) { throw new IllegalStateException("Cannot update portal " + config.getName() + " that does not exist"); } - buildSiteEntity(entity, config); + if (config.isLayoutUnresolved()) { + LOG.warn("The layout of site {} wasn't resolved when the site was read, the stored layout is kept", siteKey); + } + buildSiteEntity(entity, config, config.isLayoutUnresolved()); entity = siteDAO.update(entity); savePermissions(entity.getId(), config); } @@ -165,7 +177,13 @@ public void remove(PortalData config) { remove(siteKey); } + /** + * Removes the site layout, pages, navigation and row in one transaction, so + * that a concurrent reader never reads a site whose layout containers are + * already deleted. + */ @Override + @ContainerTransactional(atomic = true) public void remove(SiteKey siteKey) { PortalData config = getPortalConfig(siteKey); if (config != null && config.isRemovable()) { @@ -199,18 +217,12 @@ public PortalData getPortalConfig(PortalKey key) { @Override public PortalData getPortalConfig(SiteKey siteKey) { - SiteEntity entity = siteDAO.findByKey(siteKey); - if (entity != null) { - return buildPortalData(entity); - } else { - return null; - } + return buildResolvedPortalData(siteDAO.findByKey(siteKey)); } @Override public PortalData getPortalConfig(long siteId) { - SiteEntity entity = siteDAO.find(siteId); - return buildPortalData(entity); + return buildResolvedPortalData(siteDAO.find(siteId)); } @Override @@ -284,7 +296,7 @@ public List getSitesKeys(SiteFilter siteFilter) { } - private void buildSiteEntity(SiteEntity entity, PortalData config) { + private void buildSiteEntity(SiteEntity entity, PortalData config, boolean keepStoredLayout) { entity.setDescription(config.getDescription()); entity.setLabel(config.getLabel()); entity.setLocale(config.getLocale()); @@ -299,18 +311,38 @@ private void buildSiteEntity(SiteEntity entity, PortalData config) { } entity.setProperties(propertiesString); - List children = new ArrayList<>(); - children.add(config.getPortalLayout()); + if (!keepStoredLayout) { + List children = new ArrayList<>(); + children.add(config.getPortalLayout()); - JSONArray siteBody = parseJsonArray(entity.getSiteBody()); - List newSiteBody = layoutStorage.saveChildren(siteBody, children); - entity.setChildren(newSiteBody); - entity.setSiteBody(((JSONArray) entity.toJSON().get("children")).toJSONString()); + JSONArray siteBody = parseJsonArray(entity.getSiteBody()); + List newSiteBody = layoutStorage.saveChildren(siteBody, children); + entity.setChildren(newSiteBody); + entity.setSiteBody(((JSONArray) entity.toJSON().get("children")).toJSONString()); + } entity.setDisplayed(config.isDisplayed()); entity.setDisplayOrder(config.getDisplayOrder()); entity.setBannerFileId(config.getBannerFileId()); } + /** + * A site row read while a concurrent save replaces its layout can reference + * root containers that this save already deleted: the row is then read again + * to get the committed layout. + */ + private PortalData buildResolvedPortalData(SiteEntity entity) { + PortalData portalData = buildPortalData(entity); + if (portalData != null && portalData.isLayoutUnresolved()) { + portalData = buildPortalData(siteDAO.refresh(entity)); + if (portalData != null && portalData.isLayoutUnresolved()) { + LOG.warn("The root container of the layout of site {}:{} doesn't exist, the site is read with an empty layout", + portalData.getType(), + portalData.getName()); + } + } + return portalData; + } + @SuppressWarnings("unchecked") private PortalData buildPortalData(SiteEntity entity) { if (entity == null) { @@ -341,21 +373,23 @@ private PortalData buildPortalData(SiteEntity entity) { }); } - return new PortalData("site_" + entity.getId(), - entity.getName(), - entity.getSiteType().getName(), - entity.getLocale(), - entity.getLabel(), - entity.getDescription(), - access, - editPermission, - Collections.unmodifiableMap(properties), - entity.getSkin(), - rootContainer, - entity.isDefaultSiteBody(), - entity.isDisplayed(), - entity.getDisplayOrder(), - entity.getBannerFileId()); + PortalData portalData = new PortalData("site_" + entity.getId(), + entity.getName(), + entity.getSiteType().getName(), + entity.getLocale(), + entity.getLabel(), + entity.getDescription(), + access, + editPermission, + Collections.unmodifiableMap(properties), + entity.getSkin(), + rootContainer, + entity.isDefaultSiteBody(), + entity.isDisplayed(), + entity.getDisplayOrder(), + entity.getBannerFileId()); + portalData.setLayoutUnresolved(rootContainer == null && siteBody != null && !siteBody.isEmpty()); + return portalData; } private void savePermissions(Long id, PortalData config) { diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheDescriptionStorage.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheDescriptionStorage.java index f0fd3676d4..c5915e42d1 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheDescriptionStorage.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheDescriptionStorage.java @@ -41,6 +41,8 @@ import io.meeds.portal.mop.storage.cache.model.DescriptionLocaleListCache; +import io.meeds.common.TransactionCallbacks; + public class CacheDescriptionStorage extends DescriptionStorageImpl { private static final Log LOG = @@ -91,7 +93,7 @@ public void setDescription(String id, Locale locale, State description) { try { super.setDescription(id, locale, description); } finally { - clearCacheEntries(id); + removeFromCache(id); } } @@ -100,7 +102,7 @@ public void setDescription(String id, State description) { try { super.setDescription(id, description); } finally { - clearCacheEntries(id); + removeFromCache(id); } } @@ -109,7 +111,7 @@ public void setDescriptions(String id, Map descriptions) { try { super.setDescriptions(id, descriptions); } finally { - clearCacheEntries(id); + removeFromCache(id); } } @@ -147,4 +149,20 @@ private void clearCacheEntries(String id) { } } + /** + * Inside a transaction, the cache entries are cleared again once the + * transaction completes: a concurrent read made before the commit caches the + * previous descriptions again. The loads in flight are invalidated first so + * that they don't cache them either, unless one is already writing its + * result: the cache expiry is then the backstop. + */ + private void removeFromCache(String id) { + Runnable eviction = () -> { + descriptionFutureCache.clearFutures(); + descriptionLocaleListFutureCache.clearFutures(); + clearCacheEntries(id); + }; + eviction.run(); + TransactionCallbacks.afterCompletion(eviction); + } } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheLayoutStorage.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheLayoutStorage.java index 3e4a591877..c05c2346c1 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheLayoutStorage.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheLayoutStorage.java @@ -30,6 +30,8 @@ import org.exoplatform.services.cache.CacheService; import org.exoplatform.services.cache.ExoCache; +import io.meeds.common.TransactionCallbacks; + public class CacheLayoutStorage extends LayoutStorage { public static final String PORTLET_PREFERENCES_CACHE_NAME = "portal.PortletPreferences"; @@ -74,7 +76,7 @@ public WindowData retrieve(Object context, Long siteKey) throws Exception { protected WindowEntity createWindow(WindowEntity dstW) { WindowEntity window = super.createWindow(dstW); if (window != null && window.getId() != null) { - portletPreferencesCache.remove(window.getId()); + removeFromCache(window.getId()); } return window; } @@ -84,7 +86,7 @@ protected WindowEntity updateWindow(WindowEntity window) { try { return super.updateWindow(window); } finally { - portletPreferencesCache.remove(window.getId()); + removeFromCache(window.getId()); } } @@ -99,7 +101,7 @@ protected void deleteWindow(WindowEntity window) { try { super.deleteWindow(window); } finally { - portletPreferencesCache.remove(window.getId()); + removeFromCache(window.getId()); } } @@ -108,7 +110,7 @@ protected void deleteWindowById(Long id) { try { super.deleteWindowById(id); } finally { - portletPreferencesCache.remove(id); + removeFromCache(id); } } @@ -130,4 +132,20 @@ private WindowEntity toEntity(WindowData window) { windowEntity.setCustomization(window.getCustomization()); return windowEntity; } + + /** + * Inside a transaction, the window is removed from the cache again once the + * transaction completes: a concurrent read made before the commit caches the + * previous state of the window again. The load in flight is invalidated + * first so that it doesn't cache it either, unless it's already writing its + * result: the cache expiry is then the backstop. + */ + private void removeFromCache(Long windowId) { + Runnable eviction = () -> { + portletPreferencesFutureCache.removeFuture(windowId); + portletPreferencesCache.remove(windowId); + }; + eviction.run(); + TransactionCallbacks.afterCompletion(eviction); + } } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheNavigationStorage.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheNavigationStorage.java index 37180e2879..740599dad8 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheNavigationStorage.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheNavigationStorage.java @@ -38,6 +38,8 @@ import org.exoplatform.services.log.ExoLogger; import org.exoplatform.services.log.Log; +import io.meeds.common.TransactionCallbacks; + public class CacheNavigationStorage extends NavigationStorageImpl { private static final Log LOG = @@ -85,7 +87,7 @@ public NodeData[] createNode(Long parentId, Long previousId, String name, NodeSt try { return super.createNode(parentId, previousId, name, state, index); } finally { - this.nodeFutureCache.remove(parentId); + evict(() -> this.nodeFutureCache.remove(parentId)); } } @@ -95,7 +97,7 @@ public NodeData destroyNode(Long targetId) { try { return super.destroyNode(targetId); } finally { - clearNodeCache(nodeData); + evict(() -> clearNodeCache(nodeData)); } } @@ -112,9 +114,11 @@ public NodeData[] moveNode(Long targetId, Long fromId, Long toId, Long previousI try { return super.moveNode(targetId, fromId, toId, previousId); } finally { - clearNodeCache(nodeData); - clearNodeCache(fromNodeData); - clearNodeCache(toNodeData); + evict(() -> { + clearNodeCache(nodeData); + clearNodeCache(fromNodeData); + clearNodeCache(toNodeData); + }); } } @@ -125,8 +129,10 @@ public NodeData[] renameNode(Long targetId, Long parentId, String name) { try { return super.renameNode(targetId, parentId, name); } finally { - clearNodeCache(nodeData); - clearNodeCache(parentNodeData); + evict(() -> { + clearNodeCache(nodeData); + clearNodeCache(parentNodeData); + }); } } @@ -136,7 +142,7 @@ public NodeData updateNode(Long targetId, NodeState state) { try { return super.updateNode(targetId, state); } finally { - clearNodeCache(nodeData); + evict(() -> clearNodeCache(nodeData)); } } @@ -151,8 +157,10 @@ public void saveNavigation(SiteKey key, NavigationState state) { try { super.saveNavigation(key, state); } finally { - clearNavigationByKey(key); - clearNodeCache(key); + evict(() -> { + clearNavigationByKey(key); + clearNodeCache(key); + }); } } @@ -161,8 +169,10 @@ public boolean destroyNavigation(SiteKey siteKey) { try { return super.destroyNavigation(siteKey); } finally { - clearNavigationByKey(siteKey); - clearNodeCache(siteKey); + evict(() -> { + clearNavigationByKey(siteKey); + clearNodeCache(siteKey); + }); } } @@ -171,14 +181,16 @@ public boolean destroyNavigation(NavigationData data) { try { return super.destroyNavigation(data); } finally { - if (data != null && data.getSiteKey() != null) { - SiteKey siteKey = data.getSiteKey(); - clearNavigationByKey(siteKey); - clearNodeCache(siteKey); - if (data.getRootId() != null) { - clearNodeCache(Long.parseLong(data.getRootId())); + evict(() -> { + if (data != null && data.getSiteKey() != null) { + SiteKey siteKey = data.getSiteKey(); + clearNavigationByKey(siteKey); + clearNodeCache(siteKey); + if (data.getRootId() != null) { + clearNodeCache(Long.parseLong(data.getRootId())); + } } - } + }); } } @@ -226,4 +238,20 @@ public void clearNodeCache(NodeData nodeData) { } } + /** + * Inside a transaction, the eviction is run again once the transaction + * completes: a concurrent read made before the commit caches the previous + * navigation state again. The loads in flight are invalidated first so that + * they don't cache it either, unless one is already writing its result: the + * cache expiry is then the backstop. + */ + private void evict(Runnable eviction) { + Runnable invalidatingEviction = () -> { + navigationFutureCache.clearFutures(); + nodeFutureCache.clearFutures(); + eviction.run(); + }; + invalidatingEviction.run(); + TransactionCallbacks.afterCompletion(invalidatingEviction); + } } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CachePageStorage.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CachePageStorage.java index 50046cd45f..da162f7f19 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CachePageStorage.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CachePageStorage.java @@ -31,6 +31,8 @@ import org.exoplatform.services.cache.ExoCache; import org.exoplatform.services.listener.ListenerService; +import io.meeds.common.TransactionCallbacks; + public class CachePageStorage extends PageStorageImpl { public static final String PAGE_CACHE_NAME = "portal.PageService"; @@ -62,8 +64,8 @@ public PageContext clone(PageKey srcPageKey, PageKey dstPageKey) { try { return super.clone(srcPageKey, dstPageKey); } finally { - pageFutureCache.remove(srcPageKey); - pageFutureCache.remove(dstPageKey); + removeFromCache(srcPageKey); + removeFromCache(dstPageKey); } } @@ -72,7 +74,7 @@ public boolean savePage(PageContext page) { try { return super.savePage(page); } finally { - pageFutureCache.remove(page.getKey()); + removeFromCache(page.getKey()); } } @@ -81,7 +83,7 @@ public void save(PageData page) { try { super.save(page); } finally { - pageFutureCache.remove(page.getKey().toMopPageKey()); + removeFromCache(page.getKey().toMopPageKey()); } } @@ -90,7 +92,7 @@ public boolean destroyPage(PageKey key) { try { return super.destroyPage(key); } finally { - pageFutureCache.remove(key); + removeFromCache(key); } } @@ -110,4 +112,19 @@ protected PageKey getPageKey(long id) { return pageKey; } + /** + * Inside a transaction, the page is removed from the cache again once the + * transaction completes: a concurrent read made before the commit caches the + * previous state of the page again. The load in flight is invalidated first + * so that it doesn't cache it either, unless it's already writing its + * result: the cache expiry is then the backstop. + */ + private void removeFromCache(PageKey pageKey) { + Runnable eviction = () -> { + pageFutureCache.removeFuture(pageKey); + pageFutureCache.remove(pageKey); + }; + eviction.run(); + TransactionCallbacks.afterCompletion(eviction); + } } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheSiteStorage.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheSiteStorage.java index bcf747e6cb..6bf6b42399 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheSiteStorage.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/cache/CacheSiteStorage.java @@ -38,6 +38,8 @@ import java.util.List; +import io.meeds.common.TransactionCallbacks; + public class CacheSiteStorage extends SiteStorageImpl { public static final String SITE_CACHE_NAME = "portal.SiteService"; @@ -105,8 +107,7 @@ public void create(PortalData config) { try { super.create(config); } finally { - siteFutureCache.remove(getSiteKey(config.getKey())); - clearSiteKeysByFilterFutureCache(); + removeFromCache(getSiteKey(config.getKey())); } } @@ -115,18 +116,16 @@ public void save(PortalData config) { try { super.save(config); } finally { - siteFutureCache.remove(getSiteKey(config.getKey())); - clearSiteKeysByFilterFutureCache(); + removeFromCache(getSiteKey(config.getKey())); } } @Override - public void remove(PortalData config) { + public void remove(SiteKey siteKey) { try { - super.remove(config); + super.remove(siteKey); } finally { - siteFutureCache.remove(getSiteKey(config.getKey())); - clearSiteKeysByFilterFutureCache(); + removeFromCache(siteKey); } } @@ -152,6 +151,24 @@ private SiteKey getSiteKey(PortalKey portalKey) { return new SiteKey(portalKey.getType(), portalKey.getId()); } + /** + * Inside a transaction, the site is removed from the cache again once the + * transaction completes: a concurrent read made before the commit caches the + * previous state of the site again. The loads in flight are invalidated + * first so that they don't cache it either, unless one is already writing + * its result: the cache expiry is then the backstop. + */ + private void removeFromCache(SiteKey siteKey) { + Runnable eviction = () -> { + siteFutureCache.removeFuture(siteKey); + siteFutureCache.remove(siteKey); + siteKeysByFilterFutureCache.clearFutures(); + clearSiteKeysByFilterFutureCache(); + }; + eviction.run(); + TransactionCallbacks.afterCompletion(eviction); + } + private void clearSiteKeysByFilterFutureCache() { this.siteKeysByFilterFutureCache.clear(); } diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestUserPortalConfigService.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestUserPortalConfigService.java index 0ae7f0bed9..03fad9b368 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestUserPortalConfigService.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestUserPortalConfigService.java @@ -37,9 +37,11 @@ import org.exoplatform.container.configuration.ConfigurationManager; import org.exoplatform.container.xml.InitParams; import org.exoplatform.portal.config.model.Application; +import org.exoplatform.portal.config.model.Container; import org.exoplatform.portal.config.model.Page; import org.exoplatform.portal.config.model.PageBody; import org.exoplatform.portal.config.model.PortalConfig; +import org.exoplatform.portal.config.model.Properties; import org.exoplatform.portal.mop.EventType; import org.exoplatform.portal.mop.SiteKey; import org.exoplatform.portal.mop.SiteType; @@ -673,6 +675,48 @@ public void testRestore() { assertFalse(userPortalConfigService.restoreSite("portal", customSiteName, ImportMode.OVERWRITE, false, true, false)); } + public void testRestoreSiteLayoutKeepsTheSiteSettings() { + PortalConfig originalSite = layoutService.getPortalConfig(PortalConfig.PORTAL_TYPE, "classic"); + List> siteUpdates = new ArrayList<>(); + listenerService.addListener(LayoutService.PORTAL_CONFIG_UPDATED, new Listener() { + @Override + public void onEvent(Event event) { + siteUpdates.add(event); + } + }); + try { + PortalConfig site = layoutService.getPortalConfig(PortalConfig.PORTAL_TYPE, "classic"); + site.setLabel("Customized label"); + site.setDescription("Customized description"); + site.setDisplayed(false); + site.setDisplayOrder(7); + site.setProperties(new Properties(Map.of("customized", "true"))); + site.setPortalLayout(new Container()); + layoutService.save(site); + siteUpdates.clear(); + + assertTrue(userPortalConfigService.restoreSite("portal", "classic", ImportMode.OVERWRITE, true, false, false)); + + assertEquals("The restored site is saved once, with the settings it replaces", 1, siteUpdates.size()); + PortalConfig restoredSite = layoutService.getPortalConfig(PortalConfig.PORTAL_TYPE, "classic"); + assertEquals("Customized label", restoredSite.getLabel()); + assertEquals("Customized description", restoredSite.getDescription()); + assertFalse(restoredSite.isDisplayed()); + assertEquals(7, restoredSite.getDisplayOrder()); + assertEquals("true", restoredSite.getProperty("customized")); + assertEquals(originalSite.getPortalLayout().getChildren().size(), restoredSite.getPortalLayout().getChildren().size()); + assertFalse(restoredSite.getPortalLayout().getChildren().isEmpty()); + } finally { + PortalConfig site = layoutService.getPortalConfig(PortalConfig.PORTAL_TYPE, "classic"); + site.setLabel(originalSite.getLabel()); + site.setDescription(originalSite.getDescription()); + site.setDisplayed(originalSite.isDisplayed()); + site.setDisplayOrder(originalSite.getDisplayOrder()); + site.setProperties(originalSite.getProperties()); + layoutService.save(site); + } + } + public void testRootGetPage() { new UnitTest() { public void execute() throws Exception { diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/SiteDAOTest.java b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/SiteDAOTest.java index 33800fa8a4..fc835883af 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/SiteDAOTest.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/SiteDAOTest.java @@ -20,6 +20,9 @@ import java.util.List; +import jakarta.persistence.EntityManager; + +import org.exoplatform.commons.persistence.impl.EntityManagerService; import org.exoplatform.portal.jdbc.entity.SiteEntity; import org.exoplatform.portal.mop.SiteFilter; import org.exoplatform.portal.mop.SiteKey; @@ -152,6 +155,40 @@ public void testFind() { siteDAO.delete(siteEntity2); } + public void testRefreshReadsTheCommittedRow() { + SiteEntity siteEntity = createSiteEntity("refreshed", SiteType.PORTAL); + siteDAO.create(siteEntity); + restartTransaction(); + + SiteEntity loadedEntity = siteDAO.find(siteEntity.getId()); + // A bulk update bypasses the persistence context, as a concurrent writer + // does: the entity loaded on this thread keeps the previous body + executeInTransaction("UPDATE GateInSite s SET s.siteBody = 'committedBody' WHERE s.id = " + siteEntity.getId()); + assertEquals("refreshedBody", siteDAO.find(siteEntity.getId()).getSiteBody()); + + SiteEntity refreshedEntity = siteDAO.refresh(loadedEntity); + assertNotNull(refreshedEntity); + assertEquals("committedBody", refreshedEntity.getSiteBody()); + } + + public void testRefreshRemovedSite() { + SiteEntity siteEntity = createSiteEntity("removed", SiteType.PORTAL); + siteDAO.create(siteEntity); + restartTransaction(); + + SiteEntity loadedEntity = siteDAO.find(siteEntity.getId()); + executeInTransaction("DELETE FROM GateInSite s WHERE s.id = " + siteEntity.getId()); + + assertNull(siteDAO.refresh(loadedEntity)); + } + + private void executeInTransaction(String statement) { + EntityManager entityManager = getContainer().getComponentInstanceOfType(EntityManagerService.class).getEntityManager(); + entityManager.getTransaction().begin(); + entityManager.createQuery(statement).executeUpdate(); + entityManager.getTransaction().commit(); + } + private void assertSite(SiteEntity expected, SiteEntity siteEntity) { assertEquals(expected.getDescription(), siteEntity.getDescription()); assertEquals(expected.getId(), siteEntity.getId()); diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemorySiteDAO.java b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemorySiteDAO.java index 83930bf9b7..43312a07c5 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemorySiteDAO.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemorySiteDAO.java @@ -32,6 +32,12 @@ public class InMemorySiteDAO extends AbstractInMemoryDAO implements SiteDAO { + @Override + public SiteEntity refresh(SiteEntity entity) { + // No persistence context in memory: the stored entity is the current one + return find(entity.getId()); + } + @Override public SiteEntity findByKey(SiteKey siteKey) { return entities.values() diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index fb42397285..11249e852c 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -20,17 +20,35 @@ import static org.junit.Assert.assertThrows; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; import java.util.ArrayList; import java.util.Collections; import java.util.LinkedList; +import java.util.List; import java.util.Random; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.json.simple.JSONArray; +import org.json.simple.JSONObject; +import org.json.simple.parser.JSONParser; + +import org.exoplatform.commons.api.settings.SettingService; import org.exoplatform.commons.exception.ObjectNotFoundException; +import org.exoplatform.commons.file.services.FileService; +import org.exoplatform.commons.persistence.impl.EntityManagerService; import org.exoplatform.component.test.AbstractKernelTest; import org.exoplatform.component.test.ConfigurationUnit; import org.exoplatform.component.test.ConfiguredBy; import org.exoplatform.component.test.ContainerScope; +import org.exoplatform.container.ExoContainerContext; import org.exoplatform.container.PortalContainer; +import org.exoplatform.container.component.RequestLifeCycle; +import org.exoplatform.container.configuration.ConfigurationManager; +import org.exoplatform.portal.config.StaleModelException; import org.exoplatform.portal.config.UserACL; import org.exoplatform.portal.config.model.Application; import org.exoplatform.portal.config.model.ApplicationState; @@ -40,10 +58,13 @@ import org.exoplatform.portal.config.model.PersistentApplicationState; import org.exoplatform.portal.config.model.PortalConfig; import org.exoplatform.portal.config.model.TransientApplicationState; +import org.exoplatform.portal.jdbc.entity.SiteEntity; import org.exoplatform.portal.mop.EventType; import org.exoplatform.portal.mop.QueryResult; import org.exoplatform.portal.mop.SiteKey; import org.exoplatform.portal.mop.SiteType; +import org.exoplatform.portal.mop.dao.ContainerDAO; +import org.exoplatform.portal.mop.dao.SiteDAO; import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NavigationState; import org.exoplatform.portal.mop.navigation.NodeContext; @@ -54,16 +75,27 @@ import org.exoplatform.portal.mop.page.PageState; import org.exoplatform.portal.mop.service.LayoutService; import org.exoplatform.portal.mop.service.NavigationService; +import org.exoplatform.portal.mop.storage.cache.CacheLayoutStorage; +import org.exoplatform.portal.mop.storage.cache.CacheNavigationStorage; +import org.exoplatform.portal.mop.storage.cache.CachePageStorage; +import org.exoplatform.portal.mop.storage.cache.CacheSiteStorage; +import org.exoplatform.portal.pom.data.ComponentData; import org.exoplatform.portal.pom.data.ContainerData; import org.exoplatform.portal.pom.data.PortalData; import org.exoplatform.portal.pom.spi.portlet.Portlet; import org.exoplatform.portal.pom.spi.portlet.PortletBuilder; +import org.exoplatform.services.cache.CacheService; +import org.exoplatform.services.cache.ExoCache; import org.exoplatform.services.listener.Event; import org.exoplatform.services.listener.Listener; import org.exoplatform.services.listener.ListenerService; import org.exoplatform.services.organization.Group; import org.exoplatform.services.organization.GroupHandler; import org.exoplatform.services.organization.OrganizationService; +import org.exoplatform.upload.UploadService; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityTransaction; /** * Created by The eXo Platform SARL Author : Tung Pham thanhtungty@gmail.com Nov @@ -76,6 +108,8 @@ }) public class TestDataStorage extends AbstractKernelTest { + private static final String MISSING_ROOT_CONTAINER_BODY = "[{\"type\":\"CONTAINER\",\"id\":" + Long.MAX_VALUE + "}]"; + /** . */ private LayoutService storage_; @@ -782,6 +816,393 @@ public void testGroupLayout() throws Exception { assertNull(group); } + public void testSaveSiteKeepsTheStoredLayoutWhenTheNewLayoutFails() throws Exception { + String siteName = "failingLayoutSave"; + createSite(SiteType.PORTAL, siteName); + PortalData site = modelStorage.getPortalConfig(SiteKey.portal(siteName)); + String rootContainerId = site.getPortalLayout().getStorageId(); + + PortalData failingSite = new PortalData(site.getStorageId(), + siteName, + site.getType(), + site.getLocale(), + "Changed label", + site.getDescription(), + site.getAccessPermissions(), + site.getEditPermission(), + site.getProperties(), + site.getSkin(), + newContainer(siteName, List.of(new UnsupportedComponentData())), + site.isDisplayed(), + site.getDisplayOrder(), + site.getBannerFileId()); + assertThrows(StaleModelException.class, () -> modelStorage.save(failingSite)); + restartTransaction(); + + PortalData storedSite = modelStorage.getPortalConfig(SiteKey.portal(siteName)); + assertFalse(storedSite.isLayoutUnresolved()); + assertEquals(rootContainerId, storedSite.getPortalLayout().getStorageId()); + assertEquals(site.getLabel(), storedSite.getLabel()); + } + + public void testCreateSiteStoresNothingWhenTheLayoutFails() throws Exception { + String siteName = "failingLayoutCreate"; + ContainerDAO containerDAO = PortalContainer.getInstance().getComponentInstanceOfType(ContainerDAO.class); + long containersCount = containerDAO.count(); + + PortalData failingSite = new PortalData(null, + siteName, + SiteType.PORTAL.getName(), + null, + null, + null, + new ArrayList<>(), + null, + null, + null, + newContainer(siteName, List.of(new UnsupportedComponentData())), + true, + 5, + 0); + assertThrows(StaleModelException.class, () -> modelStorage.create(failingSite)); + restartTransaction(); + + assertNull(modelStorage.getPortalConfig(SiteKey.portal(siteName))); + assertEquals(containersCount, containerDAO.count().longValue()); + } + + public void testSaveSiteReadWithAMissingRootContainerKeepsTheStoredLayout() throws Exception { + String siteName = "unresolvedLayout"; + createSite(SiteType.PORTAL, siteName); + SiteDAO siteDAO = PortalContainer.getInstance().getComponentInstanceOfType(SiteDAO.class); + SiteEntity siteEntity = siteDAO.findByKey(SiteKey.portal(siteName)); + String siteBody = siteEntity.getSiteBody(); + String rootContainerId = getRootContainerId(siteBody); + + // The site row references a root container that a concurrent save deleted + // and replaced without having committed yet + siteEntity.setSiteBody(MISSING_ROOT_CONTAINER_BODY); + siteDAO.update(siteEntity); + restartTransaction(); + + PortalData unresolvedSite = modelStorage.getPortalConfig(SiteKey.portal(siteName)); + assertTrue(unresolvedSite.isLayoutUnresolved()); + + // The concurrent save commits the site row + siteEntity = siteDAO.findByKey(SiteKey.portal(siteName)); + siteEntity.setSiteBody(siteBody); + siteDAO.update(siteEntity); + restartTransaction(); + + PortalConfig unresolvedConfig = new PortalConfig(unresolvedSite); + unresolvedConfig.setLabel("Changed label"); + storage_.save(unresolvedConfig); + restartTransaction(); + + PortalData savedSite = modelStorage.getPortalConfig(SiteKey.portal(siteName)); + assertEquals("Changed label", savedSite.getLabel()); + assertFalse(savedSite.isLayoutUnresolved()); + assertEquals(rootContainerId, savedSite.getPortalLayout().getStorageId()); + } + + public void testReadSiteAgainWhenItsRootContainerIsMissing() throws Exception { + String siteName = "reReadLayout"; + createSite(SiteType.PORTAL, siteName); + PortalContainer container = PortalContainer.getInstance(); + SiteDAO siteDAO = container.getComponentInstanceOfType(SiteDAO.class); + SiteEntity siteEntity = siteDAO.findByKey(SiteKey.portal(siteName)); + String siteBody = siteEntity.getSiteBody(); + String rootContainerId = getRootContainerId(siteBody); + siteEntity.setSiteBody(MISSING_ROOT_CONTAINER_BODY); + siteDAO.update(siteEntity); + restartTransaction(); + + // The concurrent save commits the site row right after this thread read it + SiteDAO concurrentlySavedSiteDAO = (SiteDAO) Proxy.newProxyInstance(SiteDAO.class.getClassLoader(), + new Class[] { SiteDAO.class }, + (proxy, method, args) -> { + Object result = invoke(siteDAO, method, args); + if ("findByKey".equals(method.getName())) { + executeInTransaction("UPDATE GateInSite s SET s.siteBody = '" + + siteBody + "' WHERE s.id = " + + ((SiteEntity) result).getId()); + } + return result; + }); + SiteStorageImpl siteStorage = newSiteStorage(pageService, concurrentlySavedSiteDAO); + + PortalData site = siteStorage.getPortalConfig(SiteKey.portal(siteName)); + assertFalse(site.isLayoutUnresolved()); + assertEquals(rootContainerId, site.getPortalLayout().getStorageId()); + } + + public void testRemoveSiteKeepsTheSiteWhenTheRemovalFails() throws Exception { + String siteName = "failingRemoval"; + createSite(SiteType.PORTAL, siteName); + String rootContainerId = modelStorage.getPortalConfig(SiteKey.portal(siteName)).getPortalLayout().getStorageId(); + + PageStorage failingPageStorage = (PageStorage) Proxy.newProxyInstance(PageStorage.class.getClassLoader(), + new Class[] { PageStorage.class }, + (proxy, method, args) -> { + if ("destroyPages".equals(method.getName())) { + throw new IllegalStateException("Pages removal failure"); + } + return invoke(pageService, method, args); + }); + SiteStorageImpl siteStorage = newSiteStorage(failingPageStorage, + PortalContainer.getInstance().getComponentInstanceOfType(SiteDAO.class)); + assertThrows(IllegalStateException.class, () -> siteStorage.remove(SiteKey.portal(siteName))); + restartTransaction(); + + PortalData storedSite = siteStorage.getPortalConfig(SiteKey.portal(siteName)); + assertNotNull(storedSite); + assertFalse(storedSite.isLayoutUnresolved()); + assertEquals(rootContainerId, storedSite.getPortalLayout().getStorageId()); + assertNotNull(navService.loadNavigation(SiteKey.portal(siteName))); + } + + public void testCachesAreClearedAgainOnceTheTransactionCompletes() throws Exception { + PortalContainer container = PortalContainer.getInstance(); + CacheService cacheService = container.getComponentInstanceOfType(CacheService.class); + ExoCache windowCache = cacheService.getCacheInstance(CacheLayoutStorage.PORTLET_PREFERENCES_CACHE_NAME); + ExoCache siteCache = cacheService.getCacheInstance(CacheSiteStorage.SITE_CACHE_NAME); + SiteKey siteKey = SiteKey.portal("evictedCaches"); + PortalConfig newSite = new PortalConfig(siteKey.getTypeName(), siteKey.getName()); + newSite.setAccessPermissions(new String[] { UserACL.EVERYONE }); + Application newWindow = new Application(); + newWindow.setState(new TransientApplicationState("test/test", null)); + Container layout = new Container(); + layout.getChildren().add(newWindow); + newSite.setPortalLayout(layout); + storage_.create(newSite); + restartTransaction(); + + PortalConfig site = storage_.getPortalConfig(siteKey.getTypeName(), siteKey.getName()); + Application window = getFirstApplication(site.getPortalLayout()); + assertNotNull(window); + Long windowId = Long.valueOf(window.getStorageId()); + container.getComponentInstanceOfType(LayoutStorage.class).findWindow(windowId); + Object cachedWindow = windowCache.get(windowId); + Object cachedSite = siteCache.get(siteKey); + assertNotNull(cachedWindow); + assertNotNull(cachedSite); + + EntityTransaction transaction = beginTransaction(); + try { + storage_.save(site); + // Reads made by other threads before the commit cache the previous + // states again + windowCache.put(windowId, cachedWindow); + siteCache.put(siteKey, cachedSite); + transaction.commit(); + } finally { + if (transaction.isActive()) { + transaction.rollback(); + } + } + + assertNull(windowCache.get(windowId)); + assertNull(siteCache.get(siteKey)); + } + + public void testRemovedSiteCachesAreClearedAgainOnceTheTransactionCompletes() throws Exception { + String siteName = "removedSiteCaches"; + createSite(SiteType.PORTAL, siteName); + SiteKey siteKey = SiteKey.portal(siteName); + PageKey pageKey = siteKey.page("removedPage"); + pageService.savePage(new PageContext(pageKey, null)); + restartTransaction(); + + PortalContainer container = PortalContainer.getInstance(); + CacheService cacheService = container.getComponentInstanceOfType(CacheService.class); + ExoCache pageCache = cacheService.getCacheInstance(CachePageStorage.PAGE_CACHE_NAME); + ExoCache navigationCache = cacheService.getCacheInstance(CacheNavigationStorage.NAVIGATION_CACHE_NAME); + ExoCache siteCache = cacheService.getCacheInstance(CacheSiteStorage.SITE_CACHE_NAME); + assertNotNull(pageService.loadPage(pageKey)); + assertNotNull(navService.loadNavigation(siteKey)); + assertNotNull(modelStorage.getPortalConfig(siteKey)); + Object cachedPage = pageCache.get(pageKey); + Object cachedNavigation = navigationCache.get(siteKey); + Object cachedSite = siteCache.get(siteKey); + assertNotNull(cachedPage); + assertNotNull(cachedNavigation); + assertNotNull(cachedSite); + + EntityTransaction transaction = beginTransaction(); + try { + modelStorage.remove(siteKey); + // Reads made by other threads before the commit cache the removed site, + // page and navigation again + pageCache.put(pageKey, cachedPage); + navigationCache.put(siteKey, cachedNavigation); + siteCache.put(siteKey, cachedSite); + transaction.commit(); + } finally { + if (transaction.isActive()) { + transaction.rollback(); + } + } + + assertNull(pageCache.get(pageKey)); + assertNull(navigationCache.get(siteKey)); + assertNull(siteCache.get(siteKey)); + } + + public void testSiteLoadInFlightDuringASaveIsNotCached() throws Exception { + String siteName = "loadInFlight"; + createSite(SiteType.PORTAL, siteName); + SiteKey siteKey = SiteKey.portal(siteName); + PortalContainer container = PortalContainer.getInstance(); + CacheService cacheService = container.getComponentInstanceOfType(CacheService.class); + ExoCache siteCache = cacheService.getCacheInstance(CacheSiteStorage.SITE_CACHE_NAME); + PortalConfig savedSite = new PortalConfig(modelStorage.getPortalConfig(siteKey)); + siteCache.remove(siteKey); + + // The reader thread reads the site row, then waits until the save commits + // before building and caching the site + SiteDAO siteDAO = container.getComponentInstanceOfType(SiteDAO.class); + Thread testThread = Thread.currentThread(); + CountDownLatch siteRowRead = new CountDownLatch(1); + CountDownLatch siteSaved = new CountDownLatch(1); + SiteDAO slowReaderSiteDAO = (SiteDAO) Proxy.newProxyInstance(SiteDAO.class.getClassLoader(), + new Class[] { SiteDAO.class }, + (proxy, method, args) -> { + Object result = invoke(siteDAO, method, args); + if ("findByKey".equals(method.getName()) + && Thread.currentThread() != testThread) { + siteRowRead.countDown(); + siteSaved.await(10, TimeUnit.SECONDS); + } + return result; + }); + CacheSiteStorage siteStorage = new CacheSiteStorage(cacheService, + container.getComponentInstanceOfType(SettingService.class), + container.getComponentInstanceOfType(ConfigurationManager.class), + container.getComponentInstanceOfType(NavigationStorage.class), + pageService, + container.getComponentInstanceOfType(LayoutStorage.class), + slowReaderSiteDAO, + container.getComponentInstanceOfType(UploadService.class), + container.getComponentInstanceOfType(FileService.class)); + AtomicReference readSite = new AtomicReference<>(); + Thread reader = new Thread(() -> { + ExoContainerContext.setCurrentContainer(container); + RequestLifeCycle.begin(container); + try { + readSite.set(siteStorage.getPortalConfig(siteKey)); + } finally { + RequestLifeCycle.end(); + } + }); + reader.start(); + try { + assertTrue(siteRowRead.await(10, TimeUnit.SECONDS)); + savedSite.setLabel("Saved label"); + siteStorage.save(savedSite); + } finally { + siteSaved.countDown(); + reader.join(10000); + } + + assertNotNull(readSite.get()); + assertNull("The site read before the save is cached", siteCache.get(siteKey)); + assertEquals("Saved label", siteStorage.getPortalConfig(siteKey).getLabel()); + } + + private SiteStorageImpl newSiteStorage(PageStorage pageStorage, SiteDAO siteDAO) { + PortalContainer container = PortalContainer.getInstance(); + return new SiteStorageImpl(container.getComponentInstanceOfType(SettingService.class), + container.getComponentInstanceOfType(ConfigurationManager.class), + container.getComponentInstanceOfType(NavigationStorage.class), + pageStorage, + container.getComponentInstanceOfType(LayoutStorage.class), + siteDAO, + container.getComponentInstanceOfType(UploadService.class), + container.getComponentInstanceOfType(FileService.class)); + } + + private Application getFirstApplication(Container container) { + for (ModelObject child : container.getChildren()) { + if (child instanceof Application application) { + return application; + } else if (child instanceof Container childContainer) { + Application application = getFirstApplication(childContainer); + if (application != null) { + return application; + } + } + } + return null; + } + + private static Object invoke(Object target, Method method, Object[] args) throws Throwable { + try { + return method.invoke(target, args); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } + + private void executeInTransaction(String statement) { + EntityManager entityManager = getEntityManager(); + EntityTransaction transaction = entityManager.getTransaction(); + boolean begun = !transaction.isActive(); + if (begun) { + transaction.begin(); + } + entityManager.createQuery(statement).executeUpdate(); + if (begun) { + transaction.commit(); + } + } + + /** + * The test drives the transaction boundary: the transaction that the test + * class may have begun is committed first + */ + private EntityTransaction beginTransaction() { + EntityTransaction transaction = getEntityManager().getTransaction(); + if (transaction.isActive()) { + transaction.commit(); + } + transaction.begin(); + return transaction; + } + + private EntityManager getEntityManager() { + return PortalContainer.getInstance().getComponentInstanceOfType(EntityManagerService.class).getEntityManager(); + } + + private String getRootContainerId(String siteBody) throws Exception { + JSONArray children = (JSONArray) new JSONParser().parse(siteBody); + return String.valueOf(((JSONObject) children.get(0)).get("id")); + } + + private ContainerData newContainer(String siteName, List children) { + return new ContainerData(null, + "testcontainer_" + siteName, + "", + "", + "", + "", + "", + "", + "", + "", + "", + "", + null, + null, + Collections.emptyList(), + children); + } + + private static class UnsupportedComponentData extends ComponentData { + + private static final long serialVersionUID = 1L; + + } + protected void createSite(SiteType type, String siteName) throws Exception { ContainerData container = new ContainerData(null, "testcontainer_" + siteName, From 32c86aae4bcb14e4ea34fe8cc10c774d270d50d2 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 11:47:51 +0100 Subject: [PATCH 02/15] fix: A site restore keeps the pages and navigation nodes of every configuration of the site EXO-91183 A site restore imported each configuration of the site in turn with the requested mode. In overwrite mode, each configuration deleted the pages the previous ones had just imported, and the navigation nodes they had imported under the nodes it defines or attaches a fragment to. In restore-defaults mode, a configuration deleted the nested nodes that a previous configuration had imported under a node they both define, unless it defined them too: the nodes a restore kept depended on the order of the configurations. An overwrite also deleted and re-created every page of the site, so a navigation node of another site that references one of them lost its page: an overwrite restore of the global site left the People and Spaces nodes of the dw site without a page (reproduced on a local server). An overwrite restore of the global site logged, for every second of its configurations (reproduced on a local server): NewPortalConfig error: Error while committing the transaction [org.hibernate.TransientPropertyValueException: Persistent instance of 'org.exoplatform.portal.jdbc.entity.NodeEntity' references an unsaved transient instance of 'org.exoplatform.portal.jdbc.entity.PageEntity' (persist the transient instance before flushing) [org.exoplatform.portal.jdbc.entity.NodeEntity.page -> org.exoplatform.portal.jdbc.entity.PageEntity]] Can't find Window with ids: ... The page deletion was refused at commit, while the page windows were already deleted in their own transactions: the page kept a layout whose windows no longer exist, and the pages of that configuration weren't imported. - An overwrite deletes the site pages that none of the configurations defines, then imports the pages of each configuration in place, so that they keep the nodes of other sites that reference them. Nothing is deleted when the pages of a configuration can't be read. - The navigation of each configuration is imported without deleting any node, then NavigationOrphanRemover deletes the nodes that none of the configurations defines, as the requested mode does: every such node in overwrite mode, the nodes nested in a default node in restore-defaults mode. As the import of each fragment did, a node directly under a fragment attachment point, or under one of the fragment top-level nodes, is a top-level section that restore-defaults mode keeps, and a node only in the path of an attachment point keeps its child nodes. Nothing is deleted when the navigation of a configuration can't be read. - PageStorageImpl deletes a page, and replaces a page layout, in one transaction, and broadcasts the page event once that write returns: after its commit, unless the write joins an active transaction, as a site removal does. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 72 +++++ .../config/UserPortalConfigService.java | 18 ++ .../mop/importer/DefinedNavigationNode.java | 86 ++++++ .../mop/importer/NavigationOrphanRemover.java | 132 ++++++++++ .../portal/mop/storage/PageStorageImpl.java | 76 ++++-- .../config/TestRestoreSiteConfigurations.java | 249 ++++++++++++++++++ .../portal/mop/storage/TestDataStorage.java | 63 +++++ .../portal/test/InitContainer10TestSuite.java | 33 +++ .../portal/config/restore/configuration.xml | 141 ++++++++++ .../portal/contributed/pages.xml | 32 +++ .../portal/contributed/portal.xml | 32 +++ .../portal/contributed/navigation.xml | 41 +++ .../portal/contributed/pages.xml | 32 +++ .../portal/contributed/navigation.xml | 54 ++++ .../portal/contributed/pages.xml | 32 +++ .../portal/partial/navigation.xml | 32 +++ .../contribution4/portal/partial/pages.xml | 32 +++ .../contribution4/portal/partial/portal.xml | 32 +++ .../portal/partial/navigation.xml | 25 ++ .../contribution5/portal/partial/pages.xml | 24 ++ .../portal/attached/navigation.xml | 33 +++ .../contribution6/portal/attached/portal.xml | 32 +++ 22 files changed, 1275 insertions(+), 28 deletions(-) create mode 100644 component/portal/src/main/java/org/exoplatform/portal/mop/importer/DefinedNavigationNode.java create mode 100644 component/portal/src/main/java/org/exoplatform/portal/mop/importer/NavigationOrphanRemover.java create mode 100644 component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java create mode 100644 component/portal/src/test/java/org/exoplatform/portal/test/InitContainer10TestSuite.java create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/portal.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/portal.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/portal.xml diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index 3429aa3b99..ec2cad07ef 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -54,9 +54,11 @@ import org.exoplatform.portal.mop.SiteType; import org.exoplatform.portal.mop.importer.ImportMode; import org.exoplatform.portal.mop.importer.NavigationImporter; +import org.exoplatform.portal.mop.importer.NavigationOrphanRemover; import org.exoplatform.portal.mop.importer.PageImporter; import org.exoplatform.portal.mop.importer.PortalConfigImporter; import org.exoplatform.portal.mop.importer.Status; +import org.exoplatform.portal.mop.page.PageContext; import org.exoplatform.portal.mop.service.LayoutService; import org.exoplatform.portal.mop.service.NavigationService; import org.exoplatform.portal.mop.storage.DescriptionStorage; @@ -388,6 +390,76 @@ public void initPageNavigationDB(NewPortalConfig config) { } } + /** + * Removes the navigation nodes of a site that none of its configurations + * defines, as the import mode removes orphan nodes, once each configuration + * navigation is imported without removing any node. Nothing is removed when + * the navigation of a configuration can't be read, since the nodes it defines + * can't be told apart from the orphan ones then. + * + * @param configs the configurations of the site + * @param owner the site name + * @param importMode the import mode requested for the site navigation + */ + public void removeNavigationOrphans(List configs, String owner, ImportMode importMode) { + RequestLifeCycle.begin(PortalContainer.getInstance()); + try { + List navigations = new ArrayList<>(); + for (NewPortalConfig config : configs) { + UnmarshalledObject navigation = getConfig(config, owner, "navigation", PageNavigation.class); + if (navigation != null) { + navigations.add(navigation.getObject()); + } + } + new NavigationOrphanRemover(navigationService, navigations, importMode).perform(); + } catch (Exception e) { + log.error("Error while removing the orphan navigation nodes of site {}, no node is removed", owner, e); + } finally { + RequestLifeCycle.end(); + } + } + + /** + * Removes the pages of a site that none of its configurations defines, as an + * overwrite of the site pages does, so that the pages of each configuration + * can then be imported in place and keep the navigation nodes of other sites + * that reference them. Nothing is removed when the pages of a configuration + * can't be read, since the pages it defines can't be told apart from the + * orphan ones then, nor from a user site, whose pages an import doesn't + * remove. + * + * @param configs the configurations of the site + * @param owner the site name + */ + public void removePageOrphans(List configs, String owner) { + if (configs.isEmpty()) { + return; + } + SiteKey siteKey = new SiteKey(configs.get(0).getOwnerType(), owner); + if (siteKey.getType().equals(SiteType.USER)) { + return; + } + RequestLifeCycle.begin(PortalContainer.getInstance()); + try { + Set pageNames = new HashSet<>(); + for (NewPortalConfig config : configs) { + UnmarshalledObject pageSet = getConfig(config, owner, "pages", PageSet.class); + if (pageSet != null) { + pageSet.getObject().getPages().forEach(page -> pageNames.add(page.getName())); + } + } + for (PageContext page : layoutService.findPages(siteKey)) { + if (!pageNames.contains(page.getKey().getName())) { + layoutService.remove(page.getKey()); + } + } + } catch (Exception e) { + log.error("Error while removing the orphan pages of site {}", owner, e); + } finally { + RequestLifeCycle.end(); + } + } + public boolean createPortalConfig(NewPortalConfig config, String owner) { return createPortalConfig(config, owner, null); } diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java index 144dd13738..6382a62184 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java @@ -53,6 +53,7 @@ import org.exoplatform.portal.mop.SiteType; import org.exoplatform.portal.mop.Visibility; import org.exoplatform.portal.mop.importer.ImportMode; +import org.exoplatform.portal.mop.importer.NavigationOrphanRemover; import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NodeContext; import org.exoplatform.portal.mop.page.PageContext; @@ -175,6 +176,18 @@ public boolean restoreSite(String type, } PortalConfig previousSite = restoreSiteLayout ? layoutService.getPortalConfig(type, name) : null; List configs = newPortalConfigListener.getPortalConfigs(type, name); + // An overwrite removes the pages that no configuration defines, then the + // pages of each configuration are imported in place, so that they keep the + // navigation nodes of other sites that reference them. The navigation of + // each configuration is imported without removing any node, then the nodes + // that no configuration defines are removed as the import mode does + boolean removePageOrphans = restorePages && importMode == ImportMode.OVERWRITE; + ImportMode pagesImportMode = removePageOrphans ? ImportMode.MERGE : importMode; + boolean removeNavigationOrphans = restoreNavigationTree && NavigationOrphanRemover.removesOrphans(importMode); + ImportMode navigationImportMode = removeNavigationOrphans ? ImportMode.MERGE : importMode; + if (removePageOrphans) { + newPortalConfigListener.removePageOrphans(configs, name); + } for(NewPortalConfig config : configs) { config = config.clone(); config.setImportMode(importMode.name()); @@ -188,12 +201,17 @@ public boolean restoreSite(String type, newPortalConfigListener.initPortalConfigDB(config, true, importedSite -> keepSiteSettings(importedSite, previousSite)); } if (restorePages) { + config.setImportMode(pagesImportMode.name()); newPortalConfigListener.initPageDB(config, true); } if (restoreNavigationTree) { + config.setImportMode(navigationImportMode.name()); newPortalConfigListener.initPageNavigationDB(config, true); } } + if (removeNavigationOrphans) { + newPortalConfigListener.removeNavigationOrphans(configs, name, importMode); + } return true; } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/importer/DefinedNavigationNode.java b/component/portal/src/main/java/org/exoplatform/portal/mop/importer/DefinedNavigationNode.java new file mode 100644 index 0000000000..44e5b10a5b --- /dev/null +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/importer/DefinedNavigationNode.java @@ -0,0 +1,86 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.portal.mop.importer; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.exoplatform.portal.config.model.PageNode; + +/** + * A navigation node that the configurations of a site define, attach a + * navigation fragment to, or name in the path of a fragment attachment point, + * with the child nodes the configurations define under it. + */ +class DefinedNavigationNode { + + private final Map children = new HashMap<>(); + + /** + * True when the configurations define the node children: the node is + * defined by a configuration, or is the attachment point of a fragment. The + * children of a node that is only in the path of an attachment point are + * not defined by the configurations. + */ + private boolean childrenDefined; + + /** + * True when the node is the attachment point of a fragment, or a top-level + * node of a fragment: a child node that no configuration defines under it is + * a top-level section, not a node nested in a default node. + */ + private boolean topLevelParent; + + DefinedNavigationNode getChild(String name) { + return children.get(name); + } + + DefinedNavigationNode getOrAddPathNode(String name) { + return children.computeIfAbsent(name, key -> new DefinedNavigationNode()); + } + + void setAttachmentPoint() { + childrenDefined = true; + topLevelParent = true; + } + + void addTopLevelNode(PageNode node) { + addNode(node).topLevelParent = true; + } + + boolean isChildrenDefined() { + return childrenDefined; + } + + boolean isTopLevelParent() { + return topLevelParent; + } + + private DefinedNavigationNode addNode(PageNode node) { + DefinedNavigationNode definedNode = getOrAddPathNode(node.getName()); + definedNode.childrenDefined = true; + List nodes = node.getNodes(); + if (nodes != null) { + nodes.forEach(definedNode::addNode); + } + return definedNode; + } + +} diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/importer/NavigationOrphanRemover.java b/component/portal/src/main/java/org/exoplatform/portal/mop/importer/NavigationOrphanRemover.java new file mode 100644 index 0000000000..d4160649e2 --- /dev/null +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/importer/NavigationOrphanRemover.java @@ -0,0 +1,132 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.portal.mop.importer; + +import java.util.List; + +import org.exoplatform.portal.config.model.NavigationFragment; +import org.exoplatform.portal.config.model.PageNavigation; +import org.exoplatform.portal.mop.SiteKey; +import org.exoplatform.portal.mop.navigation.NavigationContext; +import org.exoplatform.portal.mop.navigation.NodeContext; +import org.exoplatform.portal.mop.navigation.NodeModel; +import org.exoplatform.portal.mop.navigation.Scope; +import org.exoplatform.portal.mop.service.NavigationService; + +/** + * Removes the nodes of a site navigation that none of the navigation + * configurations of the site defines, as the import mode removes orphan nodes: + * every node with {@link ImportMode#OVERWRITE}, the nodes nested in a default + * node with {@link ImportMode#RESTORE_DEFAULTS}. The configurations are + * imported first without removing any node, so that a configuration doesn't + * remove the nodes another configuration of the site defines. As the import + * of a fragment, the removal keeps the child nodes of a node that is only in + * the path of a fragment attachment point. + */ +public class NavigationOrphanRemover { + + private final NavigationService navigationService; + + private final List navigations; + + private final ImportMode importMode; + + /** + * @param navigationService the navigation service + * @param navigations the navigation configurations of one site + * @param importMode the import mode requested for the site + */ + public NavigationOrphanRemover(NavigationService navigationService, List navigations, ImportMode importMode) { + this.navigationService = navigationService; + this.navigations = navigations; + this.importMode = importMode; + } + + /** + * @param importMode an import mode + * @return true when the import mode removes the orphan nodes of a navigation + */ + public static boolean removesOrphans(ImportMode importMode) { + return importMode.config.destroyOrphan || importMode.config.destroyNestedOrphan; + } + + public void perform() { + if (navigations.isEmpty() || !removesOrphans(importMode)) { + return; + } + PageNavigation siteNavigation = navigations.get(0); + NavigationContext navigation = navigationService.loadNavigation(new SiteKey(siteNavigation.getOwnerType(), + siteNavigation.getOwnerId())); + if (navigation == null) { + return; + } + NodeContext rootNode = navigationService.loadNode(NodeModel.SELF_MODEL, navigation, Scope.ALL, null); + if (removeOrphans(rootNode, getDefinedRootNode())) { + navigationService.saveNode(rootNode, null); + } + } + + private DefinedNavigationNode getDefinedRootNode() { + DefinedNavigationNode rootNode = new DefinedNavigationNode(); + for (PageNavigation navigation : navigations) { + if (navigation.getFragments() != null) { + for (NavigationFragment fragment : navigation.getFragments()) { + DefinedNavigationNode attachmentNode = getAttachmentNode(rootNode, fragment.getParentURI()); + attachmentNode.setAttachmentPoint(); + if (fragment.getNodes() != null) { + fragment.getNodes().forEach(attachmentNode::addTopLevelNode); + } + } + } + } + return rootNode; + } + + private DefinedNavigationNode getAttachmentNode(DefinedNavigationNode rootNode, String parentURI) { + DefinedNavigationNode attachmentNode = rootNode; + if (parentURI != null) { + for (String name : parentURI.split("/")) { + if (!name.isEmpty()) { + attachmentNode = attachmentNode.getOrAddPathNode(name); + } + } + } + return attachmentNode; + } + + private boolean removeOrphans(NodeContext node, DefinedNavigationNode definedNode) { + boolean removed = false; + NodeContext child = node.getFirst(); + while (child != null) { + NodeContext nextChild = child.getNext(); + DefinedNavigationNode definedChild = definedNode.getChild(child.getName()); + if (definedChild != null) { + removed |= removeOrphans(child, definedChild); + } else if (definedNode.isChildrenDefined() + && (importMode.config.destroyOrphan + || (importMode.config.destroyNestedOrphan && !definedNode.isTopLevelParent()))) { + child.removeNode(); + removed = true; + } + child = nextChild; + } + return removed; + } + +} diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java index 91ed6bbe43..ab8e133acf 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java @@ -53,6 +53,8 @@ import org.exoplatform.services.log.ExoLogger; import org.exoplatform.services.log.Log; +import io.meeds.common.ContainerTransactional; + public class PageStorageImpl implements PageStorage { private static final Log LOG = ExoLogger.getLogger(PageStorageImpl.class); @@ -173,19 +175,11 @@ public boolean destroyPage(PageKey key) { if (key == null) { throw new IllegalArgumentException("PageKey is mandatory"); } - - PageEntity page = pageDAO.findByKey(key); - if (page != null) { - String pageBody = page.getPageBody(); - JSONArray children = parseJsonArray(pageBody); - layoutStorage.deleteChildren(children); - layoutStorage.deletePermissions(PageEntity.class.getName(), page.getId()); - pageDAO.delete(page); - + boolean destroyed = deletePage(key); + if (destroyed) { broadcastEvent(EventType.PAGE_DESTROYED, key); - return true; } - return false; + return destroyed; } @Override @@ -262,23 +256,7 @@ public QueryResult findPages(int from, @Override public void save(PageData page) { - SiteKey siteKey = new SiteKey(page.getKey().getType(), page.getKey().getId()); - org.exoplatform.portal.mop.page.PageKey mopKey = - new org.exoplatform.portal.mop.page.PageKey(siteKey, page.getKey().getName()); - - PageEntity dst = pageDAO.findByKey(mopKey); - if (dst == null) { - throw new NoSuchDataException("The page " + page.getKey() + " not found"); - } - List children = page.getChildren(); - - JSONArray pageBodyJson = parseJsonArray(dst.getPageBody()); - List newPageBody = layoutStorage.saveChildren(pageBodyJson, children); - dst.setChildren(newPageBody); - dst.setPageBody(((JSONArray) dst.toJSON().get("children")).toJSONString()); - - pageDAO.update(dst); - broadcastEvent(EventType.PAGE_UPDATED, mopKey); + broadcastEvent(EventType.PAGE_UPDATED, savePageLayout(page)); } @Override @@ -328,6 +306,48 @@ protected PageData buildPageData(PageEntity entity) { entity.getLink()); } + /** + * Removes the page layout, permissions and row in one transaction, so that a + * page whose removal fails keeps its layout. + */ + @ContainerTransactional(atomic = true) + private boolean deletePage(PageKey key) { + PageEntity page = pageDAO.findByKey(key); + if (page == null) { + return false; + } + layoutStorage.deleteChildren(parseJsonArray(page.getPageBody())); + layoutStorage.deletePermissions(PageEntity.class.getName(), page.getId()); + pageDAO.delete(page); + return true; + } + + /** + * Replaces the page layout in one transaction, so that a concurrent reader + * never reads a page whose removed windows are deleted while its new layout + * isn't committed yet, and a page whose update fails keeps its layout. + */ + @ContainerTransactional(atomic = true) + private org.exoplatform.portal.mop.page.PageKey savePageLayout(PageData page) { + SiteKey siteKey = new SiteKey(page.getKey().getType(), page.getKey().getId()); + org.exoplatform.portal.mop.page.PageKey mopKey = + new org.exoplatform.portal.mop.page.PageKey(siteKey, page.getKey().getName()); + + PageEntity dst = pageDAO.findByKey(mopKey); + if (dst == null) { + throw new NoSuchDataException("The page " + page.getKey() + " not found"); + } + List children = page.getChildren(); + + JSONArray pageBodyJson = parseJsonArray(dst.getPageBody()); + List newPageBody = layoutStorage.saveChildren(pageBodyJson, children); + dst.setChildren(newPageBody); + dst.setPageBody(((JSONArray) dst.toJSON().get("children")).toJSONString()); + + pageDAO.update(dst); + return mopKey; + } + protected void broadcastEvent(String eventName, org.exoplatform.portal.mop.page.PageKey pageKey) { try { listenerService.broadcast(eventName, this, pageKey); diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java new file mode 100644 index 0000000000..8b448b1ed1 --- /dev/null +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -0,0 +1,249 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.portal.config; + +import java.util.Set; +import java.util.stream.Collectors; + +import org.exoplatform.component.test.ConfigurationUnit; +import org.exoplatform.component.test.ConfiguredBy; +import org.exoplatform.component.test.ContainerScope; +import org.exoplatform.container.PortalContainer; +import org.exoplatform.portal.config.model.PortalConfig; +import org.exoplatform.portal.mop.SiteKey; +import org.exoplatform.portal.mop.importer.ImportMode; +import org.exoplatform.portal.mop.navigation.NavigationContext; +import org.exoplatform.portal.mop.navigation.NodeContext; +import org.exoplatform.portal.mop.navigation.NodeModel; +import org.exoplatform.portal.mop.navigation.Scope; +import org.exoplatform.portal.mop.page.PageContext; +import org.exoplatform.portal.mop.page.PageKey; +import org.exoplatform.portal.mop.service.NavigationService; +import org.exoplatform.portal.mop.storage.PageStorage; + +/** + * Restores the site "contributed", defined by three configurations: the first + * one holds pages and no navigation, the two others hold pages and the nodes + * home > section > node2 and home > section > node3, and the third one attaches + * home > attached3 > leaf3 to the home node. The site "partial" is defined by a + * configuration and by another one whose pages and navigation can't be read. + * The site "attached" is defined by a configuration that attaches section6 > + * node6 to the node section6, which no configuration defines. + */ +@ConfiguredBy({ + @ConfigurationUnit(scope = ContainerScope.PORTAL, path = "conf/portal/configuration.xml"), + @ConfigurationUnit(scope = ContainerScope.PORTAL, path = "conf/exo.portal.component.portal-configuration-local.xml"), + @ConfigurationUnit(scope = ContainerScope.PORTAL, path = "org/exoplatform/portal/config/conf/configuration.xml"), + @ConfigurationUnit(scope = ContainerScope.PORTAL, path = "org/exoplatform/portal/config/restore/configuration.xml") }) +public class TestRestoreSiteConfigurations extends AbstractConfigTest { + + private static final String SITE_NAME = "contributed"; + + private static final SiteKey SITE_KEY = SiteKey.portal(SITE_NAME); + + private static final String PARTIAL_SITE_NAME = "partial"; + + private static final String ATTACHED_SITE_NAME = "attached"; + + private static final String CUSTOM_NAME = "custom"; + + private static final String HOME = "home"; + + private static final String SECTION = "section"; + + private static final String ATTACHED = "attached3"; + + private UserPortalConfigService userPortalConfigService; + + private NavigationService navigationService; + + private PageStorage pageStorage; + + @Override + protected void setUp() throws Exception { + super.setUp(); + PortalContainer container = getContainer(); + userPortalConfigService = container.getComponentInstanceOfType(UserPortalConfigService.class); + navigationService = container.getComponentInstanceOfType(NavigationService.class); + pageStorage = container.getComponentInstanceOfType(PageStorage.class); + } + + public void testOverwriteRestoresThePagesAndNavigationOfEveryConfiguration() { + addCustomPageAndNodes(); + pageStorage.destroyPage(new PageKey(SITE_KEY, "page3")); + SiteKey attachedSiteKey = SiteKey.portal(ATTACHED_SITE_NAME); + addCustomNode(getRootNode(attachedSiteKey), new PageKey(SITE_KEY, "page1")); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, SITE_NAME, ImportMode.OVERWRITE, true, true, true)); + restartTransaction(); + + assertEquals(Set.of("page1", "page2", "page3"), getPageNames(SITE_KEY)); + NodeContext rootNode = getRootNode(SITE_KEY); + assertEquals(Set.of(HOME), getNodeNames(rootNode)); + assertEquals(Set.of(SECTION, ATTACHED), getNodeNames(rootNode.get(HOME))); + NodeContext sectionNode = rootNode.get(HOME).get(SECTION); + assertEquals(Set.of("node2", "node3"), getNodeNames(sectionNode)); + assertEquals(new PageKey(SITE_KEY, "page2"), sectionNode.get("node2").getState().getPageRef()); + assertEquals("A node references the removed default page that the restore creates again", + new PageKey(SITE_KEY, "page3"), + sectionNode.get("node3").getState().getPageRef()); + NodeContext attachedNode = rootNode.get(HOME).get(ATTACHED); + assertEquals(Set.of("leaf3"), getNodeNames(attachedNode)); + assertEquals(new PageKey(SITE_KEY, "page3"), attachedNode.get("leaf3").getState().getPageRef()); + assertEquals("A node of another site keeps the restored page it references", + new PageKey(SITE_KEY, "page1"), + getRootNode(attachedSiteKey).get(CUSTOM_NAME).getState().getPageRef()); + } finally { + removeCustomNode(attachedSiteKey); + } + } + + public void testRestoreDefaultsKeepsTheDefaultNodesOfEveryConfiguration() { + addCustomPageAndNodes(); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, + SITE_NAME, + ImportMode.RESTORE_DEFAULTS, + true, + true, + true)); + restartTransaction(); + + assertEquals("A restore of the default pages keeps the custom pages", + Set.of("page1", "page2", "page3", CUSTOM_NAME), + getPageNames(SITE_KEY)); + NodeContext rootNode = getRootNode(SITE_KEY); + assertEquals("A custom node of a fragment attachment point is kept", + Set.of(HOME, CUSTOM_NAME), + getNodeNames(rootNode)); + assertEquals("A custom node of a top-level node of a fragment is kept", + Set.of(SECTION, ATTACHED, CUSTOM_NAME), + getNodeNames(rootNode.get(HOME))); + assertEquals("The nodes of each configuration are kept and the custom nested node is removed", + Set.of("node2", "node3"), + getNodeNames(rootNode.get(HOME).get(SECTION))); + assertEquals("A custom node of a top-level node of a fragment attached to another node is kept", + Set.of("leaf3", CUSTOM_NAME), + getNodeNames(rootNode.get(HOME).get(ATTACHED))); + } finally { + userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, SITE_NAME, ImportMode.OVERWRITE, false, true, true); + restartTransaction(); + } + } + + public void testOverwriteKeepsTheNodesWhenANavigationConfigurationCantBeRead() { + SiteKey partialSiteKey = SiteKey.portal(PARTIAL_SITE_NAME); + addCustomNode(getRootNode(partialSiteKey), null); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, + PARTIAL_SITE_NAME, + ImportMode.OVERWRITE, + false, + false, + true)); + restartTransaction(); + + assertEquals(Set.of("node4", CUSTOM_NAME), getNodeNames(getRootNode(partialSiteKey))); + } finally { + removeCustomNode(partialSiteKey); + } + } + + public void testOverwriteKeepsThePagesWhenAPagesConfigurationCantBeRead() { + PageKey customPageKey = new PageKey(SiteKey.portal(PARTIAL_SITE_NAME), CUSTOM_NAME); + pageStorage.savePage(new PageContext(customPageKey, null)); + restartTransaction(); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, + PARTIAL_SITE_NAME, + ImportMode.OVERWRITE, + false, + true, + false)); + restartTransaction(); + + assertEquals(Set.of("page4", CUSTOM_NAME), getPageNames(customPageKey.getSite())); + } finally { + pageStorage.destroyPage(customPageKey); + restartTransaction(); + } + } + + public void testOverwriteKeepsTheNodesOutsideTheFragmentAttachmentPoints() { + SiteKey attachedSiteKey = SiteKey.portal(ATTACHED_SITE_NAME); + addCustomNode(getRootNode(attachedSiteKey), null); + addCustomNode(getRootNode(attachedSiteKey).get("section6"), null); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, + ATTACHED_SITE_NAME, + ImportMode.OVERWRITE, + false, + false, + true)); + restartTransaction(); + + NodeContext rootNode = getRootNode(attachedSiteKey); + assertEquals("A node outside the fragment attachment point is kept", + Set.of("section6", CUSTOM_NAME), + getNodeNames(rootNode)); + assertEquals(Set.of("node6"), getNodeNames(rootNode.get("section6"))); + } finally { + removeCustomNode(attachedSiteKey); + } + } + + private void addCustomPageAndNodes() { + PageKey customPageKey = new PageKey(SITE_KEY, CUSTOM_NAME); + pageStorage.savePage(new PageContext(customPageKey, null)); + addCustomNode(getRootNode(SITE_KEY), customPageKey); + addCustomNode(getRootNode(SITE_KEY).get(HOME), customPageKey); + addCustomNode(getRootNode(SITE_KEY).get(HOME).get(SECTION), customPageKey); + addCustomNode(getRootNode(SITE_KEY).get(HOME).get(ATTACHED), customPageKey); + } + + private void addCustomNode(NodeContext parentNode, PageKey pageKey) { + NodeContext customNode = parentNode.add(null, CUSTOM_NAME); + customNode.setState(customNode.getState().builder().pageRef(pageKey).build()); + navigationService.saveNode(parentNode, null); + restartTransaction(); + } + + private void removeCustomNode(SiteKey siteKey) { + NodeContext rootNode = getRootNode(siteKey); + if (rootNode.get(CUSTOM_NAME) != null) { + rootNode.get(CUSTOM_NAME).removeNode(); + navigationService.saveNode(rootNode, null); + } + restartTransaction(); + } + + private NodeContext getRootNode(SiteKey siteKey) { + NavigationContext navigation = navigationService.loadNavigation(siteKey); + return navigationService.loadNode(NodeModel.SELF_MODEL, navigation, Scope.ALL, null); + } + + private Set getNodeNames(NodeContext parentNode) { + return parentNode.getNodes().stream().map(node -> ((NodeContext) node).getName()).collect(Collectors.toSet()); + } + + private Set getPageNames(SiteKey siteKey) { + return pageStorage.loadPages(siteKey).stream().map(page -> page.getKey().getName()).collect(Collectors.toSet()); + } + +} diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index 11249e852c..649f794b5d 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -64,6 +64,7 @@ import org.exoplatform.portal.mop.SiteKey; import org.exoplatform.portal.mop.SiteType; import org.exoplatform.portal.mop.dao.ContainerDAO; +import org.exoplatform.portal.mop.dao.PageDAO; import org.exoplatform.portal.mop.dao.SiteDAO; import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NavigationState; @@ -961,6 +962,33 @@ public void testRemoveSiteKeepsTheSiteWhenTheRemovalFails() throws Exception { assertNotNull(navService.loadNavigation(SiteKey.portal(siteName))); } + public void testDestroyPageKeepsThePageLayoutWhenThePageRemovalFails() throws Exception { + PageKey pageKey = createPageWithWindow("failingPageRemoval"); + PageStorageImpl pageStorage = newPageStorageFailingOn("delete"); + + assertThrows(IllegalStateException.class, () -> pageStorage.destroyPage(pageKey)); + restartTransaction(); + + Page storedPage = pageStorage.getPage(pageKey); + assertNotNull(storedPage); + assertEquals(1, storedPage.getChildren().size()); + assertEquals(1, ((Container) storedPage.getChildren().get(0)).getChildren().size()); + } + + public void testSavePageKeepsThePageLayoutWhenThePageUpdateFails() throws Exception { + PageKey pageKey = createPageWithWindow("failingPageUpdate"); + PageStorageImpl pageStorage = newPageStorageFailingOn("update"); + Page page = pageStorage.getPage(pageKey); + ((Container) page.getChildren().get(0)).setChildren(new ArrayList<>()); + + assertThrows(IllegalStateException.class, () -> pageStorage.save(page.build())); + restartTransaction(); + + Page storedPage = pageStorage.getPage(pageKey); + assertEquals(1, storedPage.getChildren().size()); + assertEquals(1, ((Container) storedPage.getChildren().get(0)).getChildren().size()); + } + public void testCachesAreClearedAgainOnceTheTransactionCompletes() throws Exception { PortalContainer container = PortalContainer.getInstance(); CacheService cacheService = container.getComponentInstanceOfType(CacheService.class); @@ -1109,6 +1137,41 @@ public void testSiteLoadInFlightDuringASaveIsNotCached() throws Exception { assertEquals("Saved label", siteStorage.getPortalConfig(siteKey).getLabel()); } + private PageKey createPageWithWindow(String siteName) throws Exception { + createSite(SiteType.PORTAL, siteName); + Application application = new Application(); + application.setState(new TransientApplicationState("test/test", null)); + Container container = new Container(); + container.setTemplate("test"); + container.setChildren(new ArrayList<>(Collections.singletonList(application))); + Page page = new Page(); + page.setOwnerType(PortalConfig.PORTAL_TYPE); + page.setOwnerId(siteName); + page.setName("foo"); + page.setChildren(new ArrayList<>(Collections.singletonList(container))); + pageService.savePage(new PageContext(page.getPageKey(), null)); + pageService.save(page.build()); + restartTransaction(); + return page.getPageKey(); + } + + private PageStorageImpl newPageStorageFailingOn(String pageDAOMethodName) { + PortalContainer container = PortalContainer.getInstance(); + PageDAO pageDAO = container.getComponentInstanceOfType(PageDAO.class); + PageDAO failingPageDAO = (PageDAO) Proxy.newProxyInstance(PageDAO.class.getClassLoader(), + new Class[] { PageDAO.class }, + (proxy, method, args) -> { + if (pageDAOMethodName.equals(method.getName())) { + throw new IllegalStateException("Page DAO failure"); + } + return invoke(pageDAO, method, args); + }); + return new PageStorageImpl(listenerService, + container.getComponentInstanceOfType(LayoutStorage.class), + container.getComponentInstanceOfType(SiteDAO.class), + failingPageDAO); + } + private SiteStorageImpl newSiteStorage(PageStorage pageStorage, SiteDAO siteDAO) { PortalContainer container = PortalContainer.getInstance(); return new SiteStorageImpl(container.getComponentInstanceOfType(SettingService.class), diff --git a/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer10TestSuite.java b/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer10TestSuite.java new file mode 100644 index 0000000000..d6c9fe9538 --- /dev/null +++ b/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer10TestSuite.java @@ -0,0 +1,33 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.portal.test; + +import org.junit.runner.RunWith; +import org.junit.runners.Suite; +import org.junit.runners.Suite.SuiteClasses; + +import org.exoplatform.portal.config.TestRestoreSiteConfigurations; + +@RunWith(Suite.class) +@SuiteClasses({ + TestRestoreSiteConfigurations.class, +}) +public class InitContainer10TestSuite { + +} diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml new file mode 100644 index 0000000000..8e81a5eafd --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml @@ -0,0 +1,141 @@ + + + + + org.exoplatform.portal.config.UserPortalConfigService + + new.portal.config.user.listener.contributed + initListener + org.exoplatform.portal.config.NewPortalConfigListener + + + contribution1 + + + + + contributed + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution1 + + + + + contribution2 + + + + + contributed + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution2 + + + + + contribution3 + + + + + contributed + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution3 + + + + + contribution4 + + + + + partial + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution4 + + + + + contribution5 + + + + + partial + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution5 + + + + + contribution6 + + + + + attached + + + + + portal + + + classpath:/org/exoplatform/portal/config/restore/contribution6 + + + + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/pages.xml new file mode 100644 index 0000000000..ebcbf4285f --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/pages.xml @@ -0,0 +1,32 @@ + + + + + + page1 + Page 1 + Everyone + *:/platform/administrators + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/portal.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/portal.xml new file mode 100644 index 0000000000..564c85fa70 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution1/portal/contributed/portal.xml @@ -0,0 +1,32 @@ + + + + contributed + + en + Everyone + *:/platform/administrators + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml new file mode 100644 index 0000000000..4a56405b44 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml @@ -0,0 +1,41 @@ + + + + 1 + + + + home + + + section + + + node2 + + portal::contributed::page2 + + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/pages.xml new file mode 100644 index 0000000000..e3730a1297 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/pages.xml @@ -0,0 +1,32 @@ + + + + + + page2 + Page 2 + Everyone + *:/platform/administrators + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/navigation.xml new file mode 100644 index 0000000000..1c43ada28c --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/navigation.xml @@ -0,0 +1,54 @@ + + + + 1 + + + + home + + + section + + + node3 + + portal::contributed::page3 + + + + + + + home + + attached3 + + + leaf3 + + portal::contributed::page3 + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/pages.xml new file mode 100644 index 0000000000..9bf6239e3c --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution3/portal/contributed/pages.xml @@ -0,0 +1,32 @@ + + + + + + page3 + Page 3 + Everyone + *:/platform/administrators + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/navigation.xml new file mode 100644 index 0000000000..85fdd60192 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/navigation.xml @@ -0,0 +1,32 @@ + + + + 1 + + + + node4 + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/pages.xml new file mode 100644 index 0000000000..7811073292 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/pages.xml @@ -0,0 +1,32 @@ + + + + + + page4 + Page 4 + Everyone + *:/platform/administrators + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/portal.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/portal.xml new file mode 100644 index 0000000000..1bf0cd2686 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution4/portal/partial/portal.xml @@ -0,0 +1,32 @@ + + + + partial + + en + Everyone + *:/platform/administrators + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/navigation.xml new file mode 100644 index 0000000000..236e15aa90 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/navigation.xml @@ -0,0 +1,25 @@ + + + + + + + node5 + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/pages.xml new file mode 100644 index 0000000000..caacccd2f0 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution5/portal/partial/pages.xml @@ -0,0 +1,24 @@ + + + + + + page5 + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/navigation.xml new file mode 100644 index 0000000000..adc6e3b448 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/navigation.xml @@ -0,0 +1,33 @@ + + + + 1 + + + section6 + + node6 + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/portal.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/portal.xml new file mode 100644 index 0000000000..5fe269c0f4 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution6/portal/attached/portal.xml @@ -0,0 +1,32 @@ + + + + attached + + en + Everyone + *:/platform/administrators + + + + From 0364c31806a763742345c8902f281a9d3bca45a0 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 15:05:50 +0100 Subject: [PATCH 03/15] fix: A site that a configuration overwrites at startup is imported from all its configurations EXO-91183 At startup, each configuration of a site was imported with its own mode, one after the other. A configuration in overwrite mode (a deployer setting such as exo.agenda.portalConfig.metadata.importmode=overwrite, or an extension) deleted every page of the site, and the navigation nodes the other configurations imported under the nodes it defines, at every start: the pages and nodes of the configurations imported before it were lost, and the pages it imported again got new identifiers, so the nodes of other sites that reference them lost their page. - A site that one of its configurations overwrites is imported from all its configurations, whatever their own mode, as an overwrite restore of the site does: the pages that no configuration defines are deleted, the pages of every configuration are imported in place, the navigation of every configuration is imported without deleting any node, then the nodes that no configuration defines are deleted. The pages are imported before any navigation, as before. - NewPortalConfigListener#importSitePages and #importSiteNavigation hold that sequence for the startup import and for UserPortalConfigService#restoreSite. The startup import keeps importing the site layout of each configuration with its own mode. - A restore imports the site layout of every configuration (NewPortalConfigListener#importSiteLayout), then their pages, then their navigation: a node that references a page of a later configuration of the site is now linked to it. An extension that overwrites a site no longer deletes the pages and nodes that the other configurations of the site define: they are kept, and the extension's own pages and nodes are merged over them. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 136 ++++++++++++++++-- .../config/UserPortalConfigService.java | 50 ++----- .../config/TestImportNavigationOverwrite.java | 12 +- .../config/TestRestoreSiteConfigurations.java | 50 ++++++- .../config/TestSiteDataImportOverwrite.java | 3 +- .../portal/config/restore/configuration.xml | 45 ++++++ .../portal/contributed/navigation.xml | 5 + .../portal/startup/navigation.xml | 33 +++++ .../contribution7/portal/startup/pages.xml | 32 +++++ .../contribution7/portal/startup/portal.xml | 32 +++++ .../portal/startup/navigation.xml | 33 +++++ .../contribution8/portal/startup/pages.xml | 32 +++++ 12 files changed, 405 insertions(+), 58 deletions(-) create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/pages.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/portal.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/navigation.xml create mode 100644 component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/pages.xml diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index ec2cad07ef..36c8362e5c 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -230,17 +230,62 @@ public void run() throws Exception { for (NewPortalConfig ele : configs) { initPortalConfigDB(ele, prepareImport); } + // A site that one of its configurations overwrites is imported from all + // its configurations, as an overwrite restore of the site does, so that a + // configuration doesn't remove the pages and nodes of the others + Set overwrittenSites = getOverwrittenSites(prepareImport); for (NewPortalConfig ele : configs) { - initPageDB(ele, prepareImport); + initPageDB(withoutSites(ele, overwrittenSites), prepareImport); + } + for (SiteKey site : overwrittenSites) { + importSitePages(getPortalConfigs(site.getTypeName(), site.getName()), site.getName(), ImportMode.OVERWRITE); } for (NewPortalConfig ele : configs) { - initPageNavigationDB(ele, prepareImport); + initPageNavigationDB(withoutSites(ele, overwrittenSites), prepareImport); + } + for (SiteKey site : overwrittenSites) { + importSiteNavigation(getPortalConfigs(site.getTypeName(), site.getName()), site.getName(), ImportMode.OVERWRITE); } // touchImport(); } + private Set getOverwrittenSites(boolean prepareImport) { + Set sites = new HashSet<>(); + for (NewPortalConfig config : configs) { + if ((config.getOverrideMode() || prepareImport) && isOverwrite(config)) { + config.getPredefinedOwner() + .stream() + .filter(createdOwners::contains) + .forEach(owner -> sites.add(new SiteKey(config.getOwnerType(), owner))); + } + } + return sites; + } + + private boolean isOverwrite(NewPortalConfig config) { + try { + return getRightMode(config.getImportMode()) == ImportMode.OVERWRITE; + } catch (IllegalArgumentException e) { + // The import of the configuration logs its unknown import mode + return false; + } + } + + /** + * @return the configuration without the owners of the given sites + */ + private NewPortalConfig withoutSites(NewPortalConfig config, Set sites) { + HashSet owners = new HashSet<>(config.getPredefinedOwner()); + if (owners.removeIf(owner -> sites.contains(new SiteKey(config.getOwnerType(), owner)))) { + NewPortalConfig otherSitesConfig = config.clone(); + otherSitesConfig.setPredefinedOwner(owners); + return otherSitesConfig; + } + return config; + } + public NewPortalConfig getPortalConfig(String type, String name) { return CollectionUtils.isNotEmpty(configs) ? configs.stream() .filter(c -> StringUtils.equalsIgnoreCase(c.getOwnerType(), type) @@ -390,10 +435,78 @@ public void initPageNavigationDB(NewPortalConfig config) { } } + /** + * Imports the site layout of each configuration of a site with the given + * import mode. + * + * @param configs the configurations of the site + * @param owner the site name + * @param importMode the import mode + * @param beforeImport applied to each site configuration before it's + * imported, null to import it as is + */ + public void importSiteLayout(List configs, + String owner, + ImportMode importMode, + Consumer beforeImport) { + for (NewPortalConfig config : configs) { + initPortalConfigDB(getSiteConfig(config, owner, importMode), true, beforeImport); + } + } + + /** + * Imports the pages of each configuration of a site with the given import + * mode. An overwrite removes the pages that no configuration defines first, + * then the pages of each configuration are imported in place, so that a + * configuration doesn't remove the pages of the others, and the pages keep + * the navigation nodes of other sites that reference them. + * + * @param configs the configurations of the site + * @param owner the site name + * @param importMode the import mode + */ + public void importSitePages(List configs, String owner, ImportMode importMode) { + boolean overwrite = importMode == ImportMode.OVERWRITE; + if (overwrite) { + removePageOrphans(configs, owner); + } + for (NewPortalConfig config : configs) { + initPageDB(getSiteConfig(config, owner, overwrite ? ImportMode.MERGE : importMode), true); + } + } + + /** + * Imports the navigation of each configuration of a site with the given + * import mode. The navigation of each configuration is imported without + * removing any node, then the nodes that no configuration defines are + * removed as the import mode does, so that a configuration doesn't remove + * the nodes of the others. + * + * @param configs the configurations of the site + * @param owner the site name + * @param importMode the import mode + */ + public void importSiteNavigation(List configs, String owner, ImportMode importMode) { + boolean removeOrphans = NavigationOrphanRemover.removesOrphans(importMode); + for (NewPortalConfig config : configs) { + initPageNavigationDB(getSiteConfig(config, owner, removeOrphans ? ImportMode.MERGE : importMode), true); + } + if (removeOrphans) { + removeNavigationOrphans(configs, owner, importMode); + } + } + + private NewPortalConfig getSiteConfig(NewPortalConfig config, String owner, ImportMode importMode) { + NewPortalConfig siteConfig = config.clone(); + siteConfig.setImportMode(importMode.name()); + siteConfig.setOverrideMode(true); + siteConfig.setPredefinedOwner(new HashSet<>(Set.of(owner))); + return siteConfig; + } + /** * Removes the navigation nodes of a site that none of its configurations - * defines, as the import mode removes orphan nodes, once each configuration - * navigation is imported without removing any node. Nothing is removed when + * defines, as the import mode removes orphan nodes. Nothing is removed when * the navigation of a configuration can't be read, since the nodes it defines * can't be told apart from the orphan ones then. * @@ -401,7 +514,7 @@ public void initPageNavigationDB(NewPortalConfig config) { * @param owner the site name * @param importMode the import mode requested for the site navigation */ - public void removeNavigationOrphans(List configs, String owner, ImportMode importMode) { + private void removeNavigationOrphans(List configs, String owner, ImportMode importMode) { RequestLifeCycle.begin(PortalContainer.getInstance()); try { List navigations = new ArrayList<>(); @@ -420,18 +533,15 @@ public void removeNavigationOrphans(List configs, String owner, } /** - * Removes the pages of a site that none of its configurations defines, as an - * overwrite of the site pages does, so that the pages of each configuration - * can then be imported in place and keep the navigation nodes of other sites - * that reference them. Nothing is removed when the pages of a configuration - * can't be read, since the pages it defines can't be told apart from the - * orphan ones then, nor from a user site, whose pages an import doesn't - * remove. + * Removes the pages of a site that none of its configurations defines. + * Nothing is removed when the pages of a configuration can't be read, since + * the pages it defines can't be told apart from the orphan ones then, nor + * from a user site, whose pages an import doesn't remove. * * @param configs the configurations of the site * @param owner the site name */ - public void removePageOrphans(List configs, String owner) { + private void removePageOrphans(List configs, String owner) { if (configs.isEmpty()) { return; } diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java index 6382a62184..39f3ca3054 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java @@ -21,7 +21,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -53,7 +52,6 @@ import org.exoplatform.portal.mop.SiteType; import org.exoplatform.portal.mop.Visibility; import org.exoplatform.portal.mop.importer.ImportMode; -import org.exoplatform.portal.mop.importer.NavigationOrphanRemover; import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NodeContext; import org.exoplatform.portal.mop.page.PageContext; @@ -176,41 +174,19 @@ public boolean restoreSite(String type, } PortalConfig previousSite = restoreSiteLayout ? layoutService.getPortalConfig(type, name) : null; List configs = newPortalConfigListener.getPortalConfigs(type, name); - // An overwrite removes the pages that no configuration defines, then the - // pages of each configuration are imported in place, so that they keep the - // navigation nodes of other sites that reference them. The navigation of - // each configuration is imported without removing any node, then the nodes - // that no configuration defines are removed as the import mode does - boolean removePageOrphans = restorePages && importMode == ImportMode.OVERWRITE; - ImportMode pagesImportMode = removePageOrphans ? ImportMode.MERGE : importMode; - boolean removeNavigationOrphans = restoreNavigationTree && NavigationOrphanRemover.removesOrphans(importMode); - ImportMode navigationImportMode = removeNavigationOrphans ? ImportMode.MERGE : importMode; - if (removePageOrphans) { - newPortalConfigListener.removePageOrphans(configs, name); - } - for(NewPortalConfig config : configs) { - config = config.clone(); - config.setImportMode(importMode.name()); - config.setOverrideMode(true); - HashSet ownerName = new HashSet<>(); - ownerName.add(name); - config.setPredefinedOwner(ownerName); - if (restoreSiteLayout) { - // The imported site carries the settings of the site it replaces, so - // that it is saved once, without being read again - newPortalConfigListener.initPortalConfigDB(config, true, importedSite -> keepSiteSettings(importedSite, previousSite)); - } - if (restorePages) { - config.setImportMode(pagesImportMode.name()); - newPortalConfigListener.initPageDB(config, true); - } - if (restoreNavigationTree) { - config.setImportMode(navigationImportMode.name()); - newPortalConfigListener.initPageNavigationDB(config, true); - } - } - if (removeNavigationOrphans) { - newPortalConfigListener.removeNavigationOrphans(configs, name, importMode); + if (restoreSiteLayout) { + // The imported site carries the settings of the site it replaces, so + // that it is saved once, without being read again + newPortalConfigListener.importSiteLayout(configs, + name, + importMode, + importedSite -> keepSiteSettings(importedSite, previousSite)); + } + if (restorePages) { + newPortalConfigListener.importSitePages(configs, name, importMode); + } + if (restoreNavigationTree) { + newPortalConfigListener.importSiteNavigation(configs, name, importMode); } return true; } diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestImportNavigationOverwrite.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestImportNavigationOverwrite.java index ba71462a5e..d44e4ea0f6 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestImportNavigationOverwrite.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestImportNavigationOverwrite.java @@ -30,9 +30,17 @@ protected ImportMode getMode() { @Override protected final void afterOnePhaseBoot(NodeContext root) { - assertState(root); - assertEquals(2, root.get("baz").getIndex()); + // A configuration that overwrites the site keeps the nodes of the other + // configurations of the site + assertEquals(4, root.getNodeCount()); + NodeContext foo = root.get("foo"); + assertEquals("foo_icon_2", foo.getState().getIcon()); + assertEquals(1, foo.getNodeCount()); + assertEquals("bar_icon", root.get("bar").getState().getIcon()); + assertEquals("daa_icon", root.get("daa").getState().getIcon()); assertEquals(0, root.get("foo").getIndex()); + assertEquals(2, root.get("baz").getIndex()); + assertEquals(3, root.get("daa").getIndex()); } @Override diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java index 8b448b1ed1..8370f2cf2d 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -40,11 +40,14 @@ /** * Restores the site "contributed", defined by three configurations: the first * one holds pages and no navigation, the two others hold pages and the nodes - * home > section > node2 and home > section > node3, and the third one attaches - * home > attached3 > leaf3 to the home node. The site "partial" is defined by a + * home > section > node2 and home > section > node3, the second one also holds + * home > section > node23, which references a page of the third one, and the + * third one attaches home > attached3 > leaf3 to the home node. The site "partial" is defined by a * configuration and by another one whose pages and navigation can't be read. * The site "attached" is defined by a configuration that attaches section6 > - * node6 to the node section6, which no configuration defines. + * node6 to the node section6, which no configuration defines. The site + * "startup" is defined by a configuration imported in insert mode and by + * another one that overwrites the site at each startup. */ @ConfiguredBy({ @ConfigurationUnit(scope = ContainerScope.PORTAL, path = "conf/portal/configuration.xml"), @@ -61,6 +64,8 @@ public class TestRestoreSiteConfigurations extends AbstractConfigTest { private static final String ATTACHED_SITE_NAME = "attached"; + private static final String STARTUP_SITE_NAME = "startup"; + private static final String CUSTOM_NAME = "custom"; private static final String HOME = "home"; @@ -98,8 +103,11 @@ public void testOverwriteRestoresThePagesAndNavigationOfEveryConfiguration() { assertEquals(Set.of(HOME), getNodeNames(rootNode)); assertEquals(Set.of(SECTION, ATTACHED), getNodeNames(rootNode.get(HOME))); NodeContext sectionNode = rootNode.get(HOME).get(SECTION); - assertEquals(Set.of("node2", "node3"), getNodeNames(sectionNode)); + assertEquals(Set.of("node2", "node23", "node3"), getNodeNames(sectionNode)); assertEquals(new PageKey(SITE_KEY, "page2"), sectionNode.get("node2").getState().getPageRef()); + assertEquals("A node references the page of a later configuration, imported before any navigation", + new PageKey(SITE_KEY, "page3"), + sectionNode.get("node23").getState().getPageRef()); assertEquals("A node references the removed default page that the restore creates again", new PageKey(SITE_KEY, "page3"), sectionNode.get("node3").getState().getPageRef()); @@ -136,7 +144,7 @@ public void testRestoreDefaultsKeepsTheDefaultNodesOfEveryConfiguration() { Set.of(SECTION, ATTACHED, CUSTOM_NAME), getNodeNames(rootNode.get(HOME))); assertEquals("The nodes of each configuration are kept and the custom nested node is removed", - Set.of("node2", "node3"), + Set.of("node2", "node23", "node3"), getNodeNames(rootNode.get(HOME).get(SECTION))); assertEquals("A custom node of a top-level node of a fragment attached to another node is kept", Set.of("leaf3", CUSTOM_NAME), @@ -185,6 +193,38 @@ public void testOverwriteKeepsThePagesWhenAPagesConfigurationCantBeRead() { } } + public void testStartupOverwriteImportsTheSiteFromEveryConfiguration() { + SiteKey startupSiteKey = SiteKey.portal(STARTUP_SITE_NAME); + assertEquals("An overwrite at startup keeps the pages of the other configurations", + Set.of("pageA", "pageB"), + getPageNames(startupSiteKey)); + assertEquals("An overwrite at startup keeps the nodes of the other configurations", + Set.of("nodeA", "nodeB"), + getNodeNames(getRootNode(startupSiteKey))); + + PageKey customPageKey = new PageKey(startupSiteKey, CUSTOM_NAME); + pageStorage.savePage(new PageContext(customPageKey, null)); + addCustomNode(getRootNode(startupSiteKey), customPageKey); + SiteKey attachedSiteKey = SiteKey.portal(ATTACHED_SITE_NAME); + addCustomNode(getRootNode(attachedSiteKey), new PageKey(startupSiteKey, "pageB")); + try { + userPortalConfigService.start(); + restartTransaction(); + + assertEquals("An overwrite at startup removes the pages that no configuration defines only", + Set.of("pageA", "pageB"), + getPageNames(startupSiteKey)); + assertEquals("An overwrite at startup removes the nodes that no configuration defines only", + Set.of("nodeA", "nodeB"), + getNodeNames(getRootNode(startupSiteKey))); + assertEquals("A node of another site keeps the page that an overwrite at startup imports", + new PageKey(startupSiteKey, "pageB"), + getRootNode(attachedSiteKey).get(CUSTOM_NAME).getState().getPageRef()); + } finally { + removeCustomNode(attachedSiteKey); + } + } + public void testOverwriteKeepsTheNodesOutsideTheFragmentAttachmentPoints() { SiteKey attachedSiteKey = SiteKey.portal(ATTACHED_SITE_NAME); addCustomNode(getRootNode(attachedSiteKey), null); diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestSiteDataImportOverwrite.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestSiteDataImportOverwrite.java index 68581fda84..a70f088c43 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestSiteDataImportOverwrite.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestSiteDataImportOverwrite.java @@ -46,7 +46,8 @@ protected void afterOneBootWithExtention(PortalContainer container) throws Excep Application layoutPortlet = (Application) layout.getChildren().get(0); assertEquals("site2/layout", layoutService.getId(layoutPortlet.getState())); PageContext page = pageStorage.loadPage(PageKey.parse("portal::classic::home")); - assertNull(page); + assertNotNull(page); + assertEquals("site 1", page.getState().getDisplayName()); page = pageStorage.loadPage(PageKey.parse("portal::classic::page1")); assertNotNull(page); assertEquals("site 2", page.getState().getDisplayName()); diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml index 8e81a5eafd..24f80258fb 100644 --- a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml @@ -135,6 +135,51 @@ + + contribution7 + + + + + startup + + + + + portal + + + insert + + + classpath:/org/exoplatform/portal/config/restore/contribution7 + + + + + contribution8 + + + + + startup + + + + + portal + + + overwrite + + + true + + + classpath:/org/exoplatform/portal/config/restore/contribution8 + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml index 4a56405b44..8b37c47467 100644 --- a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution2/portal/contributed/navigation.xml @@ -35,6 +35,11 @@ portal::contributed::page2 + + node23 + + portal::contributed::page3 + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/navigation.xml new file mode 100644 index 0000000000..b4adacef22 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/navigation.xml @@ -0,0 +1,33 @@ + + + + 1 + + + + nodeA + + portal::startup::pageA + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/pages.xml new file mode 100644 index 0000000000..fbbe547394 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/pages.xml @@ -0,0 +1,32 @@ + + + + + + pageA + Page A + Everyone + *:/platform/administrators + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/portal.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/portal.xml new file mode 100644 index 0000000000..bee7c85623 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution7/portal/startup/portal.xml @@ -0,0 +1,32 @@ + + + + startup + + en + Everyone + *:/platform/administrators + + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/navigation.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/navigation.xml new file mode 100644 index 0000000000..b5584637e1 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/navigation.xml @@ -0,0 +1,33 @@ + + + + 1 + + + + nodeB + + portal::startup::pageB + + + diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/pages.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/pages.xml new file mode 100644 index 0000000000..fd58fbe979 --- /dev/null +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/contribution8/portal/startup/pages.xml @@ -0,0 +1,32 @@ + + + + + + pageB + Page B + Everyone + *:/platform/administrators + + + From f985294c10b450e784e7b2f3475f39ff1337ffc9 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 14:50:50 +0100 Subject: [PATCH 04/15] fix: A failing atomic call logs that the transaction it joined is rolled back EXO-91183 A @ContainerTransactional(atomic = true) method that fails inside an active kernel transaction marks it rollback-only, and ExoTransactionalAspect rolls a rollback-only transaction back without throwing: an @ExoTransactional caller that catches the failure and goes on, such as social's LayoutUpgradePlugin#updateLayout, lost all its writes with no trace. - The aspect logs a warning naming the method and the failure when it marks the joined transaction rollback-only. - SpringTransactionScope and ThreadEntityManagerHolder are top-level, package-private classes. - The Spring scope is closed when the transaction can't begin. Co-Authored-By: Claude Opus 5.5 --- .../common/ContainerTransactionalAspect.java | 130 ++---------------- .../meeds/common/SpringTransactionScope.java | 127 +++++++++++++++++ .../common/ThreadEntityManagerHolder.java | 39 ++++++ .../ContainerTransactionalAspectTest.java | 15 ++ 4 files changed, 190 insertions(+), 121 deletions(-) create mode 100644 component/common/src/main/java/io/meeds/common/SpringTransactionScope.java create mode 100644 component/common/src/main/java/io/meeds/common/ThreadEntityManagerHolder.java diff --git a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java index 668d61fbf6..03243e6c56 100644 --- a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java +++ b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java @@ -18,16 +18,11 @@ */ package io.meeds.common; -import java.util.List; - import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; import org.aspectj.lang.reflect.MethodSignature; -import org.springframework.orm.jpa.EntityManagerHolder; import org.springframework.transaction.support.TransactionSynchronization; -import org.springframework.transaction.support.TransactionSynchronizationManager; -import org.springframework.transaction.support.TransactionSynchronizationUtils; import org.exoplatform.commons.persistence.impl.EntityManagerService; import org.exoplatform.container.ExoContainer; @@ -35,9 +30,10 @@ import org.exoplatform.container.PortalContainer; import org.exoplatform.container.RootContainer; import org.exoplatform.container.component.RequestLifeCycle; +import org.exoplatform.services.log.ExoLogger; +import org.exoplatform.services.log.Log; import jakarta.persistence.EntityManager; -import jakarta.persistence.EntityManagerFactory; import jakarta.persistence.EntityTransaction; import jakarta.persistence.RollbackException; @@ -50,6 +46,8 @@ @Aspect public class ContainerTransactionalAspect { + private static final Log LOG = ExoLogger.getLogger(ContainerTransactionalAspect.class); + /** * The advice keeps this signature: the classes an addon wove at its own build * call it as is. @@ -110,6 +108,11 @@ private Object proceedInActiveTransaction(ProceedingJoinPoint point, EntityTrans } catch (Throwable e) { // NOSONAR the enclosing transaction can't be committed whatever the error if (transaction.isActive()) { transaction.setRollbackOnly(); + // The kernel rolls a transaction marked rollback-only back without + // throwing: a caller that catches the error loses its writes silently + LOG.warn("{} failed in a transaction it joined, which is marked rollback-only: none of its writes is committed. Cause: {}", + point.getSignature(), + e.toString()); } throw e; } @@ -163,119 +166,4 @@ boolean isAtomic(ProceedingJoinPoint point) { } } - /** - * Spring transactional methods, Spring Data repositories included, write - * through the EntityManager that Spring's transaction synchronization holds - * for the factory. The scope holds the thread's EntityManager there, as a - * transaction already active, so that they join the transaction instead of - * committing their own. When no Spring transaction synchronization is - * active on the thread, the scope opens one and collects the callbacks they - * register (a TransactionalEventListener event) so that they run with the - * outcome of the transaction. The before-commit and before-completion - * callbacks run inside the transaction. The EntityManager is then released - * and the synchronization closed, before the transaction is committed or - * rolled back: a Spring transactional method called by a callback of the - * commit or of the rollback (a TransactionCallbacks callback), or by an - * after-commit or after-completion callback, runs in a Spring transaction of - * its own, with callbacks of its own, and one called while the commit - * flushes the writes (an entity listener) doesn't join the transaction. When a - * synchronization is already active, as in a Spring transaction without an - * EntityManager of its own, the scope collects none of their callbacks: they - * are left to that synchronization, which runs them with its own outcome and, - * without an actual transaction, skips a TransactionalEventListener event - * unless its fallback execution runs it at once. - */ - private static final class SpringTransactionScope { - - private static final SpringTransactionScope NONE = new SpringTransactionScope(null, false); - - private final EntityManagerFactory boundFactory; - - private final boolean synchronizing; - - private boolean completing; - - private List synchronizations; - - private SpringTransactionScope(EntityManagerFactory boundFactory, boolean synchronizing) { - this.boundFactory = boundFactory; - this.synchronizing = synchronizing; - } - - static SpringTransactionScope open(EntityManagerFactory entityManagerFactory, EntityManager entityManager) { - if (entityManagerFactory == null || TransactionSynchronizationManager.hasResource(entityManagerFactory)) { - // A Spring transaction of its own holds an EntityManager for the - // factory on the thread: the Spring writes stay in it - return NONE; - } - TransactionSynchronizationManager.bindResource(entityManagerFactory, new ThreadEntityManagerHolder(entityManager)); - boolean synchronizing = !TransactionSynchronizationManager.isSynchronizationActive(); - if (synchronizing) { - TransactionSynchronizationManager.initSynchronization(); - TransactionSynchronizationManager.setActualTransactionActive(true); - } - return new SpringTransactionScope(entityManagerFactory, synchronizing); - } - - void beforeCommit() { - if (synchronizing) { - TransactionSynchronizationUtils.triggerBeforeCommit(false); - beforeCompletion(); - } - } - - void beforeCompletion() { - if (synchronizing && !completing) { - completing = true; - TransactionSynchronizationUtils.triggerBeforeCompletion(); - } - } - - void close() { - release(); - if (synchronizing && synchronizations == null) { - synchronizations = TransactionSynchronizationManager.getSynchronizations(); - TransactionSynchronizationManager.clearSynchronization(); - TransactionSynchronizationManager.setActualTransactionActive(false); - } - } - - void afterCommit() { - close(); - if (synchronizing) { - TransactionSynchronizationUtils.invokeAfterCommit(synchronizations); - } - } - - void end(int status) { - close(); - if (synchronizing) { - TransactionSynchronizationUtils.invokeAfterCompletion(synchronizations, status); - } - } - - private void release() { - if (boundFactory != null) { - TransactionSynchronizationManager.unbindResourceIfPossible(boundFactory); - } - } - - } - - private static class ThreadEntityManagerHolder extends EntityManagerHolder { - - ThreadEntityManagerHolder(EntityManager entityManager) { - super(entityManager); - // The EntityManager is given to the repositories as is, without joining - // it to a transaction nor closing it with one - setSynchronizedWithTransaction(true); - } - - @Override - protected boolean isTransactionActive() { - return getEntityManager().getTransaction().isActive(); - } - - } - } diff --git a/component/common/src/main/java/io/meeds/common/SpringTransactionScope.java b/component/common/src/main/java/io/meeds/common/SpringTransactionScope.java new file mode 100644 index 0000000000..4e2df52155 --- /dev/null +++ b/component/common/src/main/java/io/meeds/common/SpringTransactionScope.java @@ -0,0 +1,127 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import java.util.List; + +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; +import org.springframework.transaction.support.TransactionSynchronizationUtils; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; + +/** + * Spring transactional methods, Spring Data repositories included, write + * through the EntityManager that Spring's transaction synchronization holds + * for the factory. The scope holds the thread's EntityManager there, as a + * transaction already active, so that they join the transaction instead of + * committing their own. When no Spring transaction synchronization is + * active on the thread, the scope opens one and collects the callbacks they + * register (a TransactionalEventListener event) so that they run with the + * outcome of the transaction. The before-commit and before-completion + * callbacks run inside the transaction. The EntityManager is then released + * and the synchronization closed, before the transaction is committed or + * rolled back: a Spring transactional method called by a callback of the + * commit or of the rollback (a TransactionCallbacks callback), or by an + * after-commit or after-completion callback, runs in a Spring transaction of + * its own, with callbacks of its own, and one called while the commit + * flushes the writes (an entity listener) doesn't join the transaction. When a + * synchronization is already active, as in a Spring transaction without an + * EntityManager of its own, the scope collects none of their callbacks: they + * are left to that synchronization, which runs them with its own outcome and, + * without an actual transaction, skips a TransactionalEventListener event + * unless its fallback execution runs it at once. + */ +final class SpringTransactionScope { + + private static final SpringTransactionScope NONE = new SpringTransactionScope(null, false); + + private final EntityManagerFactory boundFactory; + + private final boolean synchronizing; + + private boolean completing; + + private List synchronizations; + + private SpringTransactionScope(EntityManagerFactory boundFactory, boolean synchronizing) { + this.boundFactory = boundFactory; + this.synchronizing = synchronizing; + } + + static SpringTransactionScope open(EntityManagerFactory entityManagerFactory, EntityManager entityManager) { + if (entityManagerFactory == null || TransactionSynchronizationManager.hasResource(entityManagerFactory)) { + // A Spring transaction of its own holds an EntityManager for the + // factory on the thread: the Spring writes stay in it + return NONE; + } + TransactionSynchronizationManager.bindResource(entityManagerFactory, new ThreadEntityManagerHolder(entityManager)); + boolean synchronizing = !TransactionSynchronizationManager.isSynchronizationActive(); + if (synchronizing) { + TransactionSynchronizationManager.initSynchronization(); + TransactionSynchronizationManager.setActualTransactionActive(true); + } + return new SpringTransactionScope(entityManagerFactory, synchronizing); + } + + void beforeCommit() { + if (synchronizing) { + TransactionSynchronizationUtils.triggerBeforeCommit(false); + beforeCompletion(); + } + } + + void beforeCompletion() { + if (synchronizing && !completing) { + completing = true; + TransactionSynchronizationUtils.triggerBeforeCompletion(); + } + } + + void close() { + release(); + if (synchronizing && synchronizations == null) { + synchronizations = TransactionSynchronizationManager.getSynchronizations(); + TransactionSynchronizationManager.clearSynchronization(); + TransactionSynchronizationManager.setActualTransactionActive(false); + } + } + + void afterCommit() { + close(); + if (synchronizing) { + TransactionSynchronizationUtils.invokeAfterCommit(synchronizations); + } + } + + void end(int status) { + close(); + if (synchronizing) { + TransactionSynchronizationUtils.invokeAfterCompletion(synchronizations, status); + } + } + + private void release() { + if (boundFactory != null) { + TransactionSynchronizationManager.unbindResourceIfPossible(boundFactory); + } + } + +} diff --git a/component/common/src/main/java/io/meeds/common/ThreadEntityManagerHolder.java b/component/common/src/main/java/io/meeds/common/ThreadEntityManagerHolder.java new file mode 100644 index 0000000000..7591fe2466 --- /dev/null +++ b/component/common/src/main/java/io/meeds/common/ThreadEntityManagerHolder.java @@ -0,0 +1,39 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package io.meeds.common; + +import org.springframework.orm.jpa.EntityManagerHolder; + +import jakarta.persistence.EntityManager; + +class ThreadEntityManagerHolder extends EntityManagerHolder { + + ThreadEntityManagerHolder(EntityManager entityManager) { + super(entityManager); + // The EntityManager is given to the repositories as is, without joining + // it to a transaction nor closing it with one + setSynchronizedWithTransaction(true); + } + + @Override + protected boolean isTransactionActive() { + return getEntityManager().getTransaction().isActive(); + } + +} diff --git a/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java b/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java index 8aa498b251..f56447a04f 100644 --- a/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java +++ b/component/common/src/test/java/io/meeds/common/ContainerTransactionalAspectTest.java @@ -111,6 +111,7 @@ void testCommitWhenTheMethodReturns() throws Throwable { order.verify(point).proceed(); order.verify(transaction).commit(); verify(transaction, never()).rollback(); + assertTrue(isSpringScopeClosed()); } @Test @@ -126,6 +127,20 @@ void testRollbackWhenTheMethodThrows() throws Throwable { verify(transaction).begin(); verify(transaction).rollback(); verify(transaction, never()).commit(); + assertTrue(isSpringScopeClosed()); + } + + @Test + void testCloseTheSpringScopeWhenTheTransactionCantBegin() throws Throwable { + IllegalStateException error = new IllegalStateException("begin failed"); + doThrow(error).when(transaction).begin(); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceedAtomically(point, entityManagerService))); + + verify(point, never()).proceed(); + verify(transaction, never()).commit(); + verify(transaction, never()).rollback(); + assertTrue(isSpringScopeClosed()); } @Test From 223ed9abb5556dea814f4a4605eff4967846c007 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 14:50:51 +0100 Subject: [PATCH 05/15] test: Pin the node cache eviction after the commit and run the fragment restore test EXO-91183 - A node update evicts the node cache again once the transaction completes, so that a read made by another thread before the commit doesn't keep the previous node state. - TestNavigationFragmentImporterRestoreDefaults runs in InitContainer1TestSuite: it was in no suite, so the build never ran it. - The startup import of a site that a configuration overwrites links its nodes to its pages. Co-Authored-By: Claude Opus 5.5 --- .../config/TestRestoreSiteConfigurations.java | 5 ++- .../portal/mop/storage/TestDataStorage.java | 34 +++++++++++++++++++ .../portal/test/InitContainer1TestSuite.java | 2 ++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java index 8370f2cf2d..1d8d1887e3 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -198,9 +198,12 @@ public void testStartupOverwriteImportsTheSiteFromEveryConfiguration() { assertEquals("An overwrite at startup keeps the pages of the other configurations", Set.of("pageA", "pageB"), getPageNames(startupSiteKey)); + NodeContext startupRootNode = getRootNode(startupSiteKey); assertEquals("An overwrite at startup keeps the nodes of the other configurations", Set.of("nodeA", "nodeB"), - getNodeNames(getRootNode(startupSiteKey))); + getNodeNames(startupRootNode)); + assertEquals(new PageKey(startupSiteKey, "pageA"), startupRootNode.get("nodeA").getState().getPageRef()); + assertEquals(new PageKey(startupSiteKey, "pageB"), startupRootNode.get("nodeB").getState().getPageRef()); PageKey customPageKey = new PageKey(startupSiteKey, CUSTOM_NAME); pageStorage.savePage(new PageContext(customPageKey, null)); diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index 649f794b5d..5819096aaa 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -69,6 +69,7 @@ import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NavigationState; import org.exoplatform.portal.mop.navigation.NodeContext; +import org.exoplatform.portal.mop.navigation.NodeData; import org.exoplatform.portal.mop.navigation.NodeModel; import org.exoplatform.portal.mop.navigation.Scope; import org.exoplatform.portal.mop.page.PageContext; @@ -1076,6 +1077,39 @@ public void testRemovedSiteCachesAreClearedAgainOnceTheTransactionCompletes() th assertNull(siteCache.get(siteKey)); } + public void testUpdatedNodeCacheIsClearedAgainOnceTheTransactionCompletes() throws Exception { + String siteName = "updatedNodeCache"; + createSite(SiteType.PORTAL, siteName); + NavigationContext navigation = navService.loadNavigation(SiteKey.portal(siteName)); + NodeContext rootNode = navService.loadNode(NodeModel.SELF_MODEL, navigation, Scope.CHILDREN, null); + rootNode.add(null, "updatedNode"); + navService.saveNode(rootNode, null); + restartTransaction(); + + PortalContainer container = PortalContainer.getInstance(); + NavigationStorage navigationStorage = container.getComponentInstanceOfType(NavigationStorage.class); + ExoCache nodeCache = container.getComponentInstanceOfType(CacheService.class) + .getCacheInstance(CacheNavigationStorage.NODE_CACHE_NAME); + Long nodeId = Long.valueOf(rootNode.get("updatedNode").getId()); + NodeData nodeData = navigationStorage.loadNode(nodeId); + Object cachedNode = nodeCache.get(nodeId); + assertNotNull(cachedNode); + + EntityTransaction transaction = beginTransaction(); + try { + navigationStorage.updateNode(nodeId, nodeData.getState().builder().label("Updated").build()); + // A read made by another thread before the commit caches the node again + nodeCache.put(nodeId, cachedNode); + transaction.commit(); + } finally { + if (transaction.isActive()) { + transaction.rollback(); + } + } + + assertNull(nodeCache.get(nodeId)); + } + public void testSiteLoadInFlightDuringASaveIsNotCached() throws Exception { String siteName = "loadInFlight"; createSite(SiteType.PORTAL, siteName); diff --git a/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer1TestSuite.java b/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer1TestSuite.java index 14ff4a1885..10efc03688 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer1TestSuite.java +++ b/component/portal/src/test/java/org/exoplatform/portal/test/InitContainer1TestSuite.java @@ -47,6 +47,7 @@ import org.exoplatform.portal.mop.dao.PermissionDAOTest; import org.exoplatform.portal.mop.dao.SiteDAOTest; import org.exoplatform.portal.mop.dao.WindowDAOTest; +import org.exoplatform.portal.mop.importer.TestNavigationFragmentImporterRestoreDefaults; import org.exoplatform.portal.mop.rest.NavigationRestTest; import org.exoplatform.portal.mop.storage.LayoutStorageTest; import org.exoplatform.portal.mop.storage.TestDescriptionService; @@ -100,6 +101,7 @@ TestDescriptionService.class, TestNavigationServiceRebase.class, TestNavigationService.class, + TestNavigationFragmentImporterRestoreDefaults.class, TestNavigationInsertAtIndex.class, TestNavigationServiceSave.class, TestNavigationServiceUpdate.class, From 53bfd23e18063bef1ca5df8337affebe4717357c Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 15:42:21 +0100 Subject: [PATCH 06/15] fix: A method whose transaction was marked rollback-only fails instead of returning EXO-91183 A call that fails inside a transaction it joined can mark that transaction rollback-only: an atomic @ContainerTransactional call does, and so does a failing JPA operation. The @ExoTransactional method that began the transaction then returned normally while none of its writes was committed, so its caller took the write for done. - ExoTransactionalAspect rolls the transaction back and throws a RollbackException when the method it began a transaction for returns over a rollback-only transaction, as ContainerTransactionalAspect does for the transactions it begins. A method that throws keeps its error, unless ending the transaction fails, as before. - The EntityManager the aspect started is closed whatever the method's outcome. - @ExoTransactional states that outcome, as ContainerTransactional#atomic does. - The comment on the log of a failing atomic call no longer says that the rollback is silent. Co-Authored-By: Claude Opus 5.5 --- .../api/persistence/ExoTransactional.java | 6 + .../common/ContainerTransactionalAspect.java | 4 +- .../impl/ExoTransactionalAspect.java | 83 +++++++---- .../impl/ExoTransactionalAspectTest.java | 139 ++++++++++++++++++ 4 files changed, 204 insertions(+), 28 deletions(-) create mode 100644 component/common/src/test/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspectTest.java diff --git a/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java b/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java index e43135e597..9f623d6a23 100644 --- a/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java +++ b/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java @@ -28,6 +28,12 @@ * REQUIRED. Support a current transaction, create a new one if none exists. * Analogous to EJB or Spring transaction attribute of the same name. * + * When a call that failed marked the transaction that the method began + * rollback-only, the transaction is rolled back and the method throws a + * jakarta.persistence.RollbackException, even if it returned. A method that + * joined an active transaction never throws it: the method that began the + * transaction does. + * * This annotation is handled in org.exoplatform.commons.persistence.impl.ExoTransactionalAspect. * */ diff --git a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java index 03243e6c56..9e41d726cc 100644 --- a/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java +++ b/component/common/src/main/java/io/meeds/common/ContainerTransactionalAspect.java @@ -108,8 +108,8 @@ private Object proceedInActiveTransaction(ProceedingJoinPoint point, EntityTrans } catch (Throwable e) { // NOSONAR the enclosing transaction can't be committed whatever the error if (transaction.isActive()) { transaction.setRollbackOnly(); - // The kernel rolls a transaction marked rollback-only back without - // throwing: a caller that catches the error loses its writes silently + // The method that began the transaction fails with a RollbackException + // once it returns over it: this log names the call that marked it LOG.warn("{} failed in a transaction it joined, which is marked rollback-only: none of its writes is committed. Cause: {}", point.getSignature(), e.toString()); diff --git a/component/common/src/main/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspect.java b/component/common/src/main/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspect.java index 4b47c1b557..6da4c57684 100644 --- a/component/common/src/main/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspect.java +++ b/component/common/src/main/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspect.java @@ -21,6 +21,7 @@ import jakarta.persistence.EntityManager; import jakarta.persistence.EntityTransaction; import jakarta.persistence.PersistenceException; +import jakarta.persistence.RollbackException; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; @@ -45,12 +46,22 @@ public class ExoTransactionalAspect { @Around("execution(* *(..)) && @annotation(org.exoplatform.commons.api.persistence.ExoTransactional)") public Object around(ProceedingJoinPoint point) throws Throwable { + return proceed(point, getService()); + } + + /** + * Runs the advised method in the active transaction of the thread, or in a + * new one that is committed once the method returns or throws, unless a call + * that failed marked it rollback-only: it is then rolled back, and a method + * that returned throws a RollbackException, so that the writes it lost don't + * go unnoticed. + */ + Object proceed(ProceedingJoinPoint point, EntityManagerService service) throws Throwable { EntityManager entityManager; boolean begunTx = false; boolean emStarted = false; // Do we need to start EntityManager ? - EntityManagerService service = getService(); entityManager = service.getEntityManager(); if (entityManager == null) { LOG.debug("Injecting new PersistenceContext"); @@ -81,36 +92,56 @@ public Object around(ProceedingJoinPoint point) throws Throwable { } begunTx = true; } + Object result; + try { + result = point.proceed(); + } catch (Throwable e) { // NOSONAR the transaction ends whatever the error + endTransaction(service, entityManager, emStarted, tx, begunTx); + throw e; + } + if (endTransaction(service, entityManager, emStarted, tx, begunTx)) { + throw new RollbackException("The transaction of " + point.getSignature() + + " was marked rollback-only by a call that failed: none of its writes is committed"); + } + return result; + } + + /** + * @return true when the transaction that the advice began was marked + * rollback-only, and is rolled back + */ + private boolean endTransaction(EntityManagerService service, + EntityManager entityManager, + boolean emStarted, + EntityTransaction tx, + boolean begunTx) { try { - return point.proceed(); - } finally { // Do we need to end Transaction ? - try { - if ((begunTx) && (tx.isActive())) { - if (!tx.getRollbackOnly()) { - LOG.debug("Committing current transaction"); - tx.commit(); - } else { - LOG.debug("Rollback current transaction set as RollbackOnly"); - tx.rollback(); - } + if ((begunTx) && (tx.isActive())) { + if (!tx.getRollbackOnly()) { + LOG.debug("Committing current transaction"); + tx.commit(); + } else { + LOG.debug("Rollback current transaction set as RollbackOnly"); + tx.rollback(); + return true; } - } catch (RuntimeException ex) { - try { - if (tx != null && tx.isActive()) { - tx.rollback(); - } - } catch (RuntimeException rbEx) { - LOG.error("Could not roll back transaction", rbEx); - } - throw ex; - } finally { - // Do we need to close EntityManager ? - if (emStarted && (entityManager != null)) { - service.closeEntityManager(); + } + return false; + } catch (RuntimeException ex) { + try { + if (tx != null && tx.isActive()) { + tx.rollback(); } + } catch (RuntimeException rbEx) { + LOG.error("Could not roll back transaction", rbEx); + } + throw ex; + } finally { + // Do we need to close EntityManager ? + if (emStarted && (entityManager != null)) { + service.closeEntityManager(); } - } } diff --git a/component/common/src/test/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspectTest.java b/component/common/src/test/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspectTest.java new file mode 100644 index 0000000000..dcce129d2e --- /dev/null +++ b/component/common/src/test/java/org/exoplatform/commons/persistence/impl/ExoTransactionalAspectTest.java @@ -0,0 +1,139 @@ +/** + * This file is part of the Meeds project (https://meeds.io/). + * + * Copyright (C) 2026 Meeds Association contact@meeds.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.exoplatform.commons.persistence.impl; + +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.aspectj.lang.ProceedingJoinPoint; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityTransaction; +import jakarta.persistence.RollbackException; + +@ExtendWith(MockitoExtension.class) +class ExoTransactionalAspectTest { + + private static final Object RESULT = new Object(); + + @Mock + private ProceedingJoinPoint point; + + @Mock + private EntityManagerService entityManagerService; + + @Mock + private EntityManager entityManager; + + @Mock + private EntityTransaction transaction; + + private ExoTransactionalAspect aspect = new ExoTransactionalAspect(); + + @BeforeEach + void setUp() { + lenient().when(entityManagerService.getEntityManager()).thenReturn(entityManager); + lenient().when(entityManager.getTransaction()).thenReturn(transaction); + } + + @Test + void testCommitWhenTheMethodReturns() throws Throwable { + when(transaction.isActive()).thenReturn(false, true); + when(point.proceed()).thenReturn(RESULT); + + assertSame(RESULT, aspect.proceed(point, entityManagerService)); + + verify(transaction).begin(); + verify(transaction).commit(); + verify(transaction, never()).rollback(); + } + + @Test + void testCommitWhenTheMethodThrows() throws Throwable { + IllegalStateException error = new IllegalStateException("failure"); + when(transaction.isActive()).thenReturn(false, true); + when(point.proceed()).thenThrow(error); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceed(point, entityManagerService))); + + verify(transaction).commit(); + verify(transaction, never()).rollback(); + } + + @Test + void testRollbackAndThrowWhenTheMethodReturnsAndTheTransactionIsMarkedRollbackOnly() throws Throwable { + when(transaction.isActive()).thenReturn(false, true); + when(transaction.getRollbackOnly()).thenReturn(true); + when(point.proceed()).thenReturn(RESULT); + + assertThrows(RollbackException.class, () -> aspect.proceed(point, entityManagerService)); + + verify(transaction).rollback(); + verify(transaction, never()).commit(); + } + + @Test + void testRollbackAndKeepTheErrorWhenTheMethodThrowsAndTheTransactionIsMarkedRollbackOnly() throws Throwable { + IllegalStateException error = new IllegalStateException("failure"); + when(transaction.isActive()).thenReturn(false, true); + when(transaction.getRollbackOnly()).thenReturn(true); + when(point.proceed()).thenThrow(error); + + assertSame(error, assertThrows(IllegalStateException.class, () -> aspect.proceed(point, entityManagerService))); + + verify(transaction).rollback(); + verify(transaction, never()).commit(); + } + + @Test + void testLeaveAJoinedTransactionToTheMethodThatBeganIt() throws Throwable { + when(transaction.isActive()).thenReturn(true); + lenient().when(transaction.getRollbackOnly()).thenReturn(true); + when(point.proceed()).thenReturn(RESULT); + + assertSame(RESULT, aspect.proceed(point, entityManagerService)); + + verify(transaction, never()).begin(); + verify(transaction, never()).commit(); + verify(transaction, never()).rollback(); + } + + @Test + void testCloseTheEntityManagerItStartedWhenTheTransactionIsRolledBack() throws Throwable { + when(entityManagerService.getEntityManager()).thenReturn(null); + when(entityManagerService.createEntityManager()).thenReturn(entityManager); + when(transaction.isActive()).thenReturn(false, true); + when(transaction.getRollbackOnly()).thenReturn(true); + when(point.proceed()).thenReturn(RESULT); + + assertThrows(RollbackException.class, () -> aspect.proceed(point, entityManagerService)); + + verify(entityManagerService).closeEntityManager(); + } + +} From 942212174cb8aa31eb650cf80570823f326ffdb8 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 15:42:22 +0100 Subject: [PATCH 07/15] fix: A site configuration of an unknown type doesn't stop the startup import of the others EXO-91183 The startup import of an overwritten site builds the key of each site a configuration declares. A configuration of an unknown site type made that key throw, and the pages and navigation of every site failed to import. - A configuration whose import mode or site type is unknown is left out of the overwritten sites, and its own import logs the error as before. - Removing the overwritten sites from the other configurations compares their type and name, without building a key from those configurations. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 32 ++++++++++--------- .../portal/config/restore/configuration.xml | 22 +++++++++++++ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index 36c8362e5c..ff2122f763 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -254,31 +254,33 @@ public void run() throws Exception { private Set getOverwrittenSites(boolean prepareImport) { Set sites = new HashSet<>(); for (NewPortalConfig config : configs) { - if ((config.getOverrideMode() || prepareImport) && isOverwrite(config)) { - config.getPredefinedOwner() - .stream() - .filter(createdOwners::contains) - .forEach(owner -> sites.add(new SiteKey(config.getOwnerType(), owner))); + try { + if ((config.getOverrideMode() || prepareImport) && getRightMode(config.getImportMode()) == ImportMode.OVERWRITE) { + for (String owner : config.getPredefinedOwner()) { + if (createdOwners.contains(owner)) { + sites.add(new SiteKey(config.getOwnerType(), owner)); + } + } + } + } catch (RuntimeException e) { + // The import of the configuration logs its unknown import mode or site + // type + LOG.debug("The configuration of sites {} is imported on its own: its import mode or site type is unknown", + config.getPredefinedOwner(), + e); } } return sites; } - private boolean isOverwrite(NewPortalConfig config) { - try { - return getRightMode(config.getImportMode()) == ImportMode.OVERWRITE; - } catch (IllegalArgumentException e) { - // The import of the configuration logs its unknown import mode - return false; - } - } - /** * @return the configuration without the owners of the given sites */ private NewPortalConfig withoutSites(NewPortalConfig config, Set sites) { HashSet owners = new HashSet<>(config.getPredefinedOwner()); - if (owners.removeIf(owner -> sites.contains(new SiteKey(config.getOwnerType(), owner)))) { + if (owners.removeIf(owner -> sites.stream() + .anyMatch(site -> site.getTypeName().equalsIgnoreCase(config.getOwnerType()) + && site.getName().equals(owner)))) { NewPortalConfig otherSitesConfig = config.clone(); otherSitesConfig.setPredefinedOwner(owners); return otherSitesConfig; diff --git a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml index 24f80258fb..3722a0779e 100644 --- a/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml +++ b/component/portal/src/test/resources/org/exoplatform/portal/config/restore/configuration.xml @@ -180,6 +180,28 @@ + + contribution9 + A site type that doesn't exist, overwriting a site of the same name: the other sites are imported all the same + + + + + startup + + + + + unknown + + + overwrite + + + classpath:/org/exoplatform/portal/config/restore/contribution4 + + + From 809c5a0471f6000725d067503ff31cc15df58156 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 17:33:15 +0100 Subject: [PATCH 08/15] fix: Restoring a site's pages imports them even when the startup didn't import the site EXO-91183 Restoring the pages of a site imported the pages of a configuration only when the startup had imported that site, so after a restart "Restore pages" restored the navigation of a site that no configuration overrides, and none of its pages. - A restore imports the pages of every configuration of the site, as it already imports their navigation. An error in a configuration is logged and the next configuration is imported, as before. - The startup import of the pages is unchanged. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 9 +++++-- .../config/UserPortalConfigService.java | 4 +++ .../config/TestRestoreSiteConfigurations.java | 25 +++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index ff2122f763..6c4f30402c 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -461,7 +461,8 @@ public void importSiteLayout(List configs, * mode. An overwrite removes the pages that no configuration defines first, * then the pages of each configuration are imported in place, so that a * configuration doesn't remove the pages of the others, and the pages keep - * the navigation nodes of other sites that reference them. + * the navigation nodes of other sites that reference them. The pages are + * imported whether or not the site was imported at startup. * * @param configs the configurations of the site * @param owner the site name @@ -473,7 +474,11 @@ public void importSitePages(List configs, String owner, ImportM removePageOrphans(configs, owner); } for (NewPortalConfig config : configs) { - initPageDB(getSiteConfig(config, owner, overwrite ? ImportMode.MERGE : importMode), true); + try { + createPage(getSiteConfig(config, owner, overwrite ? ImportMode.MERGE : importMode), owner); + } catch (Exception e) { + log.error("NewPortalConfig error: " + e.getMessage(), e); + } } } diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java index 39f3ca3054..0ba158f553 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/UserPortalConfigService.java @@ -574,6 +574,10 @@ public UserNode getSiteNodeOrGlobalNode(String portalType, // NOSONAR } } + NewPortalConfigListener getNewPortalConfigListener() { + return newPortalConfigListener; + } + @Synchronized public void initListener(ComponentPlugin listener) { if (listener instanceof NewPortalConfigListener portalConfigListener) { diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java index 1d8d1887e3..1d6a795b7e 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -18,6 +18,7 @@ */ package org.exoplatform.portal.config; +import java.util.HashSet; import java.util.Set; import java.util.stream.Collectors; @@ -122,6 +123,30 @@ public void testOverwriteRestoresThePagesAndNavigationOfEveryConfiguration() { } } + public void testRestorePagesImportsThePagesOfASiteThatTheStartupDidntImport() { + PageKey pageKey = new PageKey(SITE_KEY, "page3"); + pageStorage.destroyPage(pageKey); + restartTransaction(); + // A startup that imports no configuration of the site, as a restart does + // when none of them overrides it + Set createdOwners = userPortalConfigService.getNewPortalConfigListener().createdOwners; + Set startupOwners = new HashSet<>(createdOwners); + createdOwners.clear(); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, + SITE_NAME, + ImportMode.RESTORE_DEFAULTS, + false, + true, + true)); + restartTransaction(); + + assertNotNull(pageStorage.getPage(pageKey)); + } finally { + createdOwners.addAll(startupOwners); + } + } + public void testRestoreDefaultsKeepsTheDefaultNodesOfEveryConfiguration() { addCustomPageAndNodes(); try { From 96d2d0beccaa62ecad1ae89763c7929248aa045c Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 17:38:28 +0100 Subject: [PATCH 09/15] fix: State that a method that threw keeps its exception when its transaction is rolled back EXO-91183 The Javadoc of ExoTransactional and of ContainerTransactional#atomic said that a method whose transaction was marked rollback-only throws a RollbackException even if it returned, which read as if a method that threw got a RollbackException too. - A method that returned throws a RollbackException after the rollback. - A method that threw keeps its exception, unless ending the transaction fails, as before. Co-Authored-By: Claude Opus 5.5 --- .../commons/api/persistence/ExoTransactional.java | 5 +++-- .../main/java/io/meeds/common/ContainerTransactional.java | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java b/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java index 9f623d6a23..161d50134e 100644 --- a/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java +++ b/component/api/src/main/java/org/exoplatform/commons/api/persistence/ExoTransactional.java @@ -29,8 +29,9 @@ * Analogous to EJB or Spring transaction attribute of the same name. * * When a call that failed marked the transaction that the method began - * rollback-only, the transaction is rolled back and the method throws a - * jakarta.persistence.RollbackException, even if it returned. A method that + * rollback-only, the transaction is rolled back: a method that returned then + * throws a jakarta.persistence.RollbackException, and a method that threw + * keeps its exception, unless ending the transaction fails. A method that * joined an active transaction never throws it: the method that began the * transaction does. * diff --git a/component/common/src/main/java/io/meeds/common/ContainerTransactional.java b/component/common/src/main/java/io/meeds/common/ContainerTransactional.java index f30a324997..d6e2c353ff 100644 --- a/component/common/src/main/java/io/meeds/common/ContainerTransactional.java +++ b/component/common/src/main/java/io/meeds/common/ContainerTransactional.java @@ -48,9 +48,10 @@ * already active on the thread: their callbacks are then left to * that synchronization. A Spring cache eviction (CacheEvict) made * inside the method takes effect at once, before the commit. When a - * call that failed marked the transaction - * rollback-only, the method throws a RollbackException after the - * rollback, even if it returned. When a JPA transaction is already + * call that failed marked the transaction rollback-only, a method + * that returned throws a RollbackException after the rollback, and + * a method that threw keeps its exception, unless the rollback + * fails. When a JPA transaction is already * active on the thread, the method joins it and marks it * rollback-only when it throws. */ From 49977861165f3844f7e281ebe8ea3b823785c08b Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Mon, 19 Oct 2026 17:38:59 +0100 Subject: [PATCH 10/15] fix: State that a configuration file that can't be parsed stops the orphan removal, not one that can't be opened EXO-91183 The Javadoc of the orphan page and node removal said that nothing is removed when a configuration can't be read. Only a file that is found and can't be parsed stops the removal: a file that can't be opened, as when a configuration ships no pages or no navigation, counts as defining none. Co-Authored-By: Claude Opus 5.5 --- .../portal/config/NewPortalConfigListener.java | 12 +++++++----- .../portal/config/TestRestoreSiteConfigurations.java | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index 6c4f30402c..daaf5282dd 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -514,8 +514,9 @@ private NewPortalConfig getSiteConfig(NewPortalConfig config, String owner, Impo /** * Removes the navigation nodes of a site that none of its configurations * defines, as the import mode removes orphan nodes. Nothing is removed when - * the navigation of a configuration can't be read, since the nodes it defines - * can't be told apart from the orphan ones then. + * the navigation file of a configuration can't be parsed, since the nodes it + * defines can't be told apart from the orphan ones then. A configuration + * whose navigation file can't be opened defines no node. * * @param configs the configurations of the site * @param owner the site name @@ -541,9 +542,10 @@ private void removeNavigationOrphans(List configs, String owner /** * Removes the pages of a site that none of its configurations defines. - * Nothing is removed when the pages of a configuration can't be read, since - * the pages it defines can't be told apart from the orphan ones then, nor - * from a user site, whose pages an import doesn't remove. + * Nothing is removed when the pages file of a configuration can't be parsed, + * since the pages it defines can't be told apart from the orphan ones then, + * nor from a user site, whose pages an import doesn't remove. A + * configuration whose pages file can't be opened defines no page. * * @param configs the configurations of the site * @param owner the site name diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java index 1d6a795b7e..6b59ada9a9 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -44,7 +44,7 @@ * home > section > node2 and home > section > node3, the second one also holds * home > section > node23, which references a page of the third one, and the * third one attaches home > attached3 > leaf3 to the home node. The site "partial" is defined by a - * configuration and by another one whose pages and navigation can't be read. + * configuration and by another one whose pages and navigation can't be parsed. * The site "attached" is defined by a configuration that attaches section6 > * node6 to the node section6, which no configuration defines. The site * "startup" is defined by a configuration imported in insert mode and by From 4a86fed8977c73a749f6431afcecb46e8262ed49 Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Tue, 20 Oct 2026 07:09:18 +0100 Subject: [PATCH 11/15] fix: An orphan page that can't be removed doesn't stop the removal of the other orphan pages EXO-91183 The orphan pages of a site are selected once, then removed. A page removed in between made its removal fail, and the remaining orphan pages were kept, on a restore and at each startup of a site that a configuration overwrites. - Each orphan page is removed on its own: a page that can't be removed is logged (WARN), and the next ones are removed. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 8 ++++- .../config/NewPortalConfigListenerTest.java | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index daaf5282dd..970f728871 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -569,7 +569,13 @@ private void removePageOrphans(List configs, String owner) { } for (PageContext page : layoutService.findPages(siteKey)) { if (!pageNames.contains(page.getKey().getName())) { - layoutService.remove(page.getKey()); + try { + layoutService.remove(page.getKey()); + } catch (RuntimeException e) { + // A page removed since the orphans were selected, for instance, + // doesn't stop the removal of the others + log.warn("Error while removing the orphan page {}", page.getKey(), e); + } } } } catch (Exception e) { diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/NewPortalConfigListenerTest.java b/component/portal/src/test/java/org/exoplatform/portal/config/NewPortalConfigListenerTest.java index 58cb86bae9..ff9ebf0ff8 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/NewPortalConfigListenerTest.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/NewPortalConfigListenerTest.java @@ -23,7 +23,9 @@ import org.exoplatform.container.xml.ValueParam; import org.exoplatform.portal.config.model.PortalConfig; import org.exoplatform.portal.mop.SiteKey; +import org.exoplatform.portal.mop.importer.ImportMode; import org.exoplatform.portal.mop.page.PageContext; +import org.exoplatform.portal.mop.page.PageKey; import org.exoplatform.portal.mop.service.LayoutService; import org.exoplatform.portal.mop.service.NavigationService; import org.exoplatform.portal.mop.storage.DescriptionStorage; @@ -112,4 +114,34 @@ public void testInitPageDB() { verify(layoutService, times(0)).removePages(any()); } + + @Test + public void testOverwriteRemovesTheOtherOrphanPagesWhenOneCantBeRemoved() { + ValueParam valueParam = new ValueParam(); + valueParam.setName("meta.portal"); + valueParam.setValue("classic"); + when(initParams.getValueParam("meta.portal")).thenReturn(valueParam); + NewPortalConfigListener newPortalConfigListener = new NewPortalConfigListener(owner, + layoutService, + configurationManager, + initParams, + navigationService, + descriptionStorage, + userACL, + localeConfigService); + NewPortalConfig newPortalConfig = mock(NewPortalConfig.class); + when(newPortalConfig.getOwnerType()).thenReturn(PortalConfig.PORTAL_TYPE); + when(newPortalConfig.clone()).thenReturn(newPortalConfig); + SiteKey siteKey = SiteKey.portal("global"); + PageKey removedPageKey = new PageKey(siteKey, "removed"); + PageKey orphanPageKey = new PageKey(siteKey, "orphan"); + when(layoutService.findPages(siteKey)).thenReturn(List.of(new PageContext(removedPageKey, null), + new PageContext(orphanPageKey, null))); + // The page was removed since the orphans were selected + doThrow(NullPointerException.class).when(layoutService).remove(removedPageKey); + + newPortalConfigListener.importSitePages(List.of(newPortalConfig), "global", ImportMode.OVERWRITE); + + verify(layoutService).remove(orphanPageKey); + } } From 2d20d0749030548c2ce1198e330206e6722d486c Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Tue, 20 Oct 2026 18:25:10 +0100 Subject: [PATCH 12/15] fix: A page delete removes the layout of the page as stored EXO-91183 PageStorageImpl#deletePage walked the body of the page entity that PageDAO#findByKey returned. When the request had already read that page, for instance through a node load that reads the pages its nodes reference, the query read the row, but Hibernate kept the state of the instance it already managed: the containers and windows another request had saved on the page since stayed in storage, unreferenced. A site restore reads the site's navigation before removing its orphan pages, so a layout saved on an orphan during the restore outlived the page. PageDAO#refresh reads the page row again, bypassing the persistence context, as SiteDAO#refresh does for a site, and deletePage walks the layout of that row. The row is read without a lock: a layout committed between that read and the deletion is still missed. Co-Authored-By: Claude Opus 5.5 --- .../config/NewPortalConfigListener.java | 5 +- .../exoplatform/portal/mop/dao/PageDAO.java | 10 +++ .../portal/mop/dao/PageDAOImpl.java | 11 +++ .../portal/mop/storage/PageStorageImpl.java | 9 +- .../config/TestRestoreSiteConfigurations.java | 89 +++++++++++++++++++ .../portal/mop/dao/mock/InMemoryPageDAO.java | 6 ++ 6 files changed, 128 insertions(+), 2 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java index 970f728871..867de2bc06 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java +++ b/component/portal/src/main/java/org/exoplatform/portal/config/NewPortalConfigListener.java @@ -545,7 +545,10 @@ private void removeNavigationOrphans(List configs, String owner * Nothing is removed when the pages file of a configuration can't be parsed, * since the pages it defines can't be told apart from the orphan ones then, * nor from a user site, whose pages an import doesn't remove. A - * configuration whose pages file can't be opened defines no page. + * configuration whose pages file can't be opened defines no page. The + * orphans are selected once, then removed one by one, each page row read + * again at its removal: a page removed since the selection is skipped, and a + * page created since then is kept, unless it is named after an orphan. * * @param configs the configurations of the site * @param owner the site name diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAO.java b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAO.java index 1942c402c6..f19879f9cf 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAO.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAO.java @@ -28,6 +28,16 @@ public interface PageDAO extends GenericDAO { PageEntity findByKey(PageKey pageKey); + /** + * Reads the page row again from the database, bypassing the state already + * loaded in the current persistence context + * + * @param entity a page entity loaded by this DAO, detached from the + * persistence context by this call + * @return the entity read again, null when the page was removed meanwhile + */ + PageEntity refresh(PageEntity entity); + ListAccess findByQuery(SiteType siteType, String siteName, String name, int offset, int limit); void deleteByOwner(long id); diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAOImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAOImpl.java index ccef7ca6eb..55bf993a9c 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAOImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/dao/PageDAOImpl.java @@ -67,6 +67,17 @@ public PageEntity findByKey(PageKey pageKey) { } } + @Override + public PageEntity refresh(PageEntity entity) { + EntityManager entityManager = getEntityManager(); + if (entityManager.contains(entity)) { + // The page row is then read from the database instead of the + // persistence context + entityManager.detach(entity); + } + return find(entity.getId()); + } + @Override @ExoTransactional public void deleteByOwner(long id) { diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java index ab8e133acf..1d5b87307b 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java @@ -308,11 +308,18 @@ protected PageData buildPageData(PageEntity entity) { /** * Removes the page layout, permissions and row in one transaction, so that a - * page whose removal fails keeps its layout. + * page whose removal fails keeps its layout. The layout removed is the one of + * the page row as stored, not as the current persistence context may hold it + * since an earlier read of the same request. That row is read without a lock: + * a layout another request commits on the page between that read and the + * deletion isn't removed. */ @ContainerTransactional(atomic = true) private boolean deletePage(PageKey key) { PageEntity page = pageDAO.findByKey(key); + if (page != null) { + page = pageDAO.refresh(page); + } if (page == null) { return false; } diff --git a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java index 6b59ada9a9..ad353b1fa0 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java +++ b/component/portal/src/test/java/org/exoplatform/portal/config/TestRestoreSiteConfigurations.java @@ -18,16 +18,29 @@ */ package org.exoplatform.portal.config; +import java.util.ArrayList; import java.util.HashSet; +import java.util.List; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import org.exoplatform.component.test.ConfigurationUnit; import org.exoplatform.component.test.ConfiguredBy; import org.exoplatform.component.test.ContainerScope; +import org.exoplatform.container.ExoContainerContext; import org.exoplatform.container.PortalContainer; +import org.exoplatform.container.component.RequestLifeCycle; +import org.exoplatform.portal.config.model.Container; +import org.exoplatform.portal.config.model.Page; import org.exoplatform.portal.config.model.PortalConfig; import org.exoplatform.portal.mop.SiteKey; +import org.exoplatform.portal.mop.dao.ContainerDAO; +import org.exoplatform.portal.mop.dao.PageDAO; import org.exoplatform.portal.mop.importer.ImportMode; import org.exoplatform.portal.mop.navigation.NavigationContext; import org.exoplatform.portal.mop.navigation.NodeContext; @@ -35,8 +48,12 @@ import org.exoplatform.portal.mop.navigation.Scope; import org.exoplatform.portal.mop.page.PageContext; import org.exoplatform.portal.mop.page.PageKey; +import org.exoplatform.portal.mop.service.LayoutService; import org.exoplatform.portal.mop.service.NavigationService; import org.exoplatform.portal.mop.storage.PageStorage; +import org.exoplatform.services.listener.Event; +import org.exoplatform.services.listener.Listener; +import org.exoplatform.services.listener.ListenerService; /** * Restores the site "contributed", defined by three configurations: the first @@ -81,6 +98,12 @@ public class TestRestoreSiteConfigurations extends AbstractConfigTest { private PageStorage pageStorage; + private ContainerDAO containerDAO; + + private PageDAO pageDAO; + + private ListenerService listenerService; + @Override protected void setUp() throws Exception { super.setUp(); @@ -88,6 +111,9 @@ protected void setUp() throws Exception { userPortalConfigService = container.getComponentInstanceOfType(UserPortalConfigService.class); navigationService = container.getComponentInstanceOfType(NavigationService.class); pageStorage = container.getComponentInstanceOfType(PageStorage.class); + containerDAO = container.getComponentInstanceOfType(ContainerDAO.class); + pageDAO = container.getComponentInstanceOfType(PageDAO.class); + listenerService = container.getComponentInstanceOfType(ListenerService.class); } public void testOverwriteRestoresThePagesAndNavigationOfEveryConfiguration() { @@ -218,6 +244,47 @@ public void testOverwriteKeepsThePagesWhenAPagesConfigurationCantBeRead() { } } + public void testOverwriteRemovesTheLayoutSavedOnAnOrphanPageSinceTheRequestReadIt() throws Exception { + PageKey firstOrphanKey = new PageKey(SITE_KEY, "orphan1"); + PageKey secondOrphanKey = new PageKey(SITE_KEY, "orphan2"); + pageStorage.savePage(new PageContext(firstOrphanKey, null)); + pageStorage.savePage(new PageContext(secondOrphanKey, null)); + restartTransaction(); + // The request reads the orphan pages before the restore, as a node load + // reads the pages its nodes reference + pageDAO.findByKey(firstOrphanKey); + pageDAO.findByKey(secondOrphanKey); + AtomicBoolean saveOnRemoval = new AtomicBoolean(true); + AtomicReference savedContainerId = new AtomicReference<>(); + listenerService.addListener(LayoutService.PAGE_REMOVED, new Listener() { + @Override + public void onEvent(Event event) throws Exception { + PageKey removedKey = event.getData().getPageKey(); + if ((removedKey.equals(firstOrphanKey) || removedKey.equals(secondOrphanKey)) + && saveOnRemoval.compareAndSet(true, false)) { + // The other orphan is saved by another request between two removals + savedContainerId.set(saveContainerInAnotherRequest(removedKey.equals(firstOrphanKey) ? secondOrphanKey : + firstOrphanKey)); + } + } + }); + try { + assertTrue(userPortalConfigService.restoreSite(PortalConfig.PORTAL_TYPE, SITE_NAME, ImportMode.OVERWRITE, false, true, false)); + restartTransaction(); + + assertNotNull(savedContainerId.get()); + assertNull(pageStorage.loadPage(firstOrphanKey)); + assertNull(pageStorage.loadPage(secondOrphanKey)); + assertNull("The container saved on an orphan page since the request read it is removed with the page", + containerDAO.find(Long.parseLong(savedContainerId.get()))); + } finally { + saveOnRemoval.set(false); + pageStorage.destroyPage(firstOrphanKey); + pageStorage.destroyPage(secondOrphanKey); + restartTransaction(); + } + } + public void testStartupOverwriteImportsTheSiteFromEveryConfiguration() { SiteKey startupSiteKey = SiteKey.portal(STARTUP_SITE_NAME); assertEquals("An overwrite at startup keeps the pages of the other configurations", @@ -285,6 +352,28 @@ private void addCustomPageAndNodes() { addCustomNode(getRootNode(SITE_KEY).get(HOME).get(ATTACHED), customPageKey); } + private String saveContainerInAnotherRequest(PageKey pageKey) throws Exception { + PortalContainer container = getContainer(); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + return executor.submit(() -> { + ExoContainerContext.setCurrentContainer(container); + RequestLifeCycle.begin(container); + try { + Page page = pageStorage.getPage(pageKey); + page.setChildren(new ArrayList<>(List.of(new Container()))); + pageStorage.save(page.build()); + return pageStorage.getPage(pageKey).getChildren().get(0).getStorageId(); + } finally { + RequestLifeCycle.end(); + ExoContainerContext.setCurrentContainer(null); + } + }).get(30, TimeUnit.SECONDS); + } finally { + executor.shutdownNow(); + } + } + private void addCustomNode(NodeContext parentNode, PageKey pageKey) { NodeContext customNode = parentNode.add(null, CUSTOM_NAME); customNode.setState(customNode.getState().builder().pageRef(pageKey).build()); diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemoryPageDAO.java b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemoryPageDAO.java index 05e5aceddd..ae957cf7f3 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemoryPageDAO.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/dao/mock/InMemoryPageDAO.java @@ -44,6 +44,12 @@ public PageEntity findByKey(PageKey pageKey) { .orElse(null); } + @Override + public PageEntity refresh(PageEntity entity) { + // No persistence context in memory: the stored entity is the current one + return find(entity.getId()); + } + @Override public InMemoryListAccess findByQuery(SiteType siteType, String siteName, From a2991682afd7b2887ef7fca4a498820e706adaae Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Wed, 21 Oct 2026 08:00:46 +0100 Subject: [PATCH 13/15] fix: Page and site writes work from the stored layout EXO-91183 A page layout save, a page properties save, a page clone, a site save and a site removal read their entity through findByKey. When the request had already read that page or site, for instance through a node load that reads the pages its nodes reference, the query read the row, but Hibernate kept the state of the instance it already managed. Each write then worked from that earlier state: - a page layout save, a site save and a site removal left the containers and windows another request had saved since in storage, unreferenced; - a page properties save wrote the earlier page body back, every column being written, over the layout saved since; - a page clone copied the earlier layout. Each of these writes now reads its row again (PageDAO#refresh, SiteDAO#refresh), as a page delete does. The row is read without a lock: a layout committed between that read and the write is still missed, and a properties save still writes the body it read back over it. Co-Authored-By: Claude Opus 5.5 --- .../portal/mop/storage/PageStorageImpl.java | 15 +- .../portal/mop/storage/SiteStorageImpl.java | 17 ++- .../portal/mop/storage/TestDataStorage.java | 132 ++++++++++++++++++ 3 files changed, 161 insertions(+), 3 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java index 1d5b87307b..2b075b0485 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java @@ -143,6 +143,11 @@ public boolean savePage(PageContext page) { } PageEntity entity = pageDAO.findByKey(page.getKey()); + if (entity != null) { + // The update writes every column, the page body included: it's read + // from the row, not as an earlier read of this request left it + entity = pageDAO.refresh(entity); + } boolean created = false; if (entity == null) { entity = new PageEntity(); @@ -199,6 +204,9 @@ public PageContext clone(PageKey srcPageKey, PageKey dstPageKey) { } PageEntity pageSrc = pageDAO.findByKey(srcPageKey); + if (pageSrc != null) { + pageSrc = pageDAO.refresh(pageSrc); + } if (pageSrc == null) { throw new PageServiceException(PageError.CLONE_NO_SRC_PAGE, String.format("Could not clone non existing page %s from site of type %s with id %s", @@ -332,7 +340,9 @@ private boolean deletePage(PageKey key) { /** * Replaces the page layout in one transaction, so that a concurrent reader * never reads a page whose removed windows are deleted while its new layout - * isn't committed yet, and a page whose update fails keeps its layout. + * isn't committed yet, and a page whose update fails keeps its layout. The + * layout replaced is the one of the page row as stored, not as the current + * persistence context may hold it since an earlier read of the same request. */ @ContainerTransactional(atomic = true) private org.exoplatform.portal.mop.page.PageKey savePageLayout(PageData page) { @@ -341,6 +351,9 @@ private org.exoplatform.portal.mop.page.PageKey savePageLayout(PageData page) { new org.exoplatform.portal.mop.page.PageKey(siteKey, page.getKey().getName()); PageEntity dst = pageDAO.findByKey(mopKey); + if (dst != null) { + dst = pageDAO.refresh(dst); + } if (dst == null) { throw new NoSuchDataException("The page " + page.getKey() + " not found"); } diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java index 15eb824c96..e2a935b811 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/SiteStorageImpl.java @@ -148,13 +148,18 @@ public void save(PortalConfig config) { /** * Replaces the site layout and its attributes in one transaction, so that a * concurrent reader never reads a site whose layout containers are deleted - * while its new ones aren't committed yet. + * while its new ones aren't committed yet. The layout replaced is the one of + * the site row as stored, not as the current persistence context may hold it + * since an earlier read of the same request. */ @Override @ContainerTransactional(atomic = true) public void save(PortalData config) { SiteKey siteKey = new SiteKey(config.getKey().getType(), config.getKey().getId()); SiteEntity entity = siteDAO.findByKey(siteKey); + if (entity != null) { + entity = siteDAO.refresh(entity); + } if (entity == null) { throw new IllegalStateException("Cannot update portal " + config.getName() + " that does not exist"); } @@ -180,7 +185,9 @@ public void remove(PortalData config) { /** * Removes the site layout, pages, navigation and row in one transaction, so * that a concurrent reader never reads a site whose layout containers are - * already deleted. + * already deleted. The layout removed is the one of the site row as stored, + * not as the current persistence context may hold it since an earlier read + * of the same request. */ @Override @ContainerTransactional(atomic = true) @@ -188,6 +195,12 @@ public void remove(SiteKey siteKey) { PortalData config = getPortalConfig(siteKey); if (config != null && config.isRemovable()) { SiteEntity entity = siteDAO.findByKey(siteKey); + if (entity != null) { + entity = siteDAO.refresh(entity); + } + if (entity == null) { + throw new NoSuchDataException("Could not remove non existing portal " + siteKey); + } String siteBody = entity.getSiteBody(); JSONArray children = parseJsonArray(siteBody); layoutStorage.deleteChildren(children); diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index 5819096aaa..6fb5cf11f2 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -29,6 +29,9 @@ import java.util.List; import java.util.Random; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -1171,6 +1174,135 @@ public void testSiteLoadInFlightDuringASaveIsNotCached() throws Exception { assertEquals("Saved label", siteStorage.getPortalConfig(siteKey).getLabel()); } + public void testSavePageLayoutRemovesTheLayoutSavedSinceTheRequestReadThePage() throws Exception { + PageKey pageKey = createPageWithWindow("staleLayoutSave"); + getComponent(PageDAO.class).findByKey(pageKey); + String savedContainerId = addPageContainerInAnotherRequest(pageKey); + + Page page = new Page(); + page.setOwnerType(PortalConfig.PORTAL_TYPE); + page.setOwnerId(pageKey.getSite().getName()); + page.setName(pageKey.getName()); + page.setChildren(new ArrayList<>(List.of(new Container()))); + pageService.save(page.build()); + restartTransaction(); + + assertNull("The container saved since the request read the page is replaced with the rest of its layout", + getComponent(ContainerDAO.class).find(Long.parseLong(savedContainerId))); + } + + public void testSavePagePropertiesKeepsTheLayoutSavedSinceTheRequestReadThePage() throws Exception { + PageKey pageKey = createPageWithWindow("stalePropertiesSave"); + getComponent(PageDAO.class).findByKey(pageKey); + String savedContainerId = addPageContainerInAnotherRequest(pageKey); + + PageContext pageContext = pageService.loadPage(pageKey); + pageService.savePage(new PageContext(pageKey, pageContext.getState().builder().displayName("Changed").build())); + restartTransaction(); + + Page savedPage = pageService.getPage(pageKey); + assertEquals("Changed", pageService.loadPage(pageKey).getState().getDisplayName()); + assertEquals("The properties save keeps the layout saved since the request read the page", + savedContainerId, + savedPage.getChildren().get(savedPage.getChildren().size() - 1).getStorageId()); + } + + public void testClonePageCopiesTheLayoutSavedSinceTheRequestReadThePage() throws Exception { + PageKey pageKey = createPageWithWindow("staleClone"); + getComponent(PageDAO.class).findByKey(pageKey); + addPageContainerInAnotherRequest(pageKey); + + PageKey cloneKey = new PageKey(pageKey.getSite(), "fooClone"); + pageService.clone(pageKey, cloneKey); + restartTransaction(); + + assertEquals("The clone copies the layout saved since the request read the page", + 2, + pageService.getPage(cloneKey).getChildren().size()); + } + + public void testSaveSiteRemovesTheLayoutSavedSinceTheRequestReadTheSite() throws Exception { + SiteKey siteKey = SiteKey.portal("staleSiteSave"); + createSite(SiteType.PORTAL, siteKey.getName()); + restartTransaction(); + getComponent(SiteDAO.class).findByKey(siteKey); + String savedContainerId = addSiteContainerInAnotherRequest(siteKey); + + PortalConfig site = new PortalConfig(modelStorage.getPortalConfig(siteKey)); + site.getPortalLayout().setChildren(new ArrayList<>()); + storage_.save(site); + restartTransaction(); + + assertNull("The container saved since the request read the site is replaced with the rest of its layout", + getComponent(ContainerDAO.class).find(Long.parseLong(savedContainerId))); + } + + public void testRemoveSiteRemovesTheLayoutSavedSinceTheRequestReadTheSite() throws Exception { + SiteKey siteKey = SiteKey.portal("staleSiteRemove"); + createSite(SiteType.PORTAL, siteKey.getName()); + restartTransaction(); + getComponent(SiteDAO.class).findByKey(siteKey); + String savedContainerId = addSiteContainerInAnotherRequest(siteKey); + + modelStorage.remove(siteKey); + restartTransaction(); + + assertNull(modelStorage.getPortalConfig(siteKey)); + assertNull("The container saved since the request read the site is removed with the site", + getComponent(ContainerDAO.class).find(Long.parseLong(savedContainerId))); + } + + private String addPageContainerInAnotherRequest(PageKey pageKey) throws Exception { + return inAnotherRequest(() -> { + Page page = pageService.getPage(pageKey); + ArrayList children = new ArrayList<>(page.getChildren()); + children.add(new Container()); + page.setChildren(children); + pageService.save(page.build()); + List savedChildren = pageService.getPage(pageKey).getChildren(); + assertEquals("The other request's layout is stored", children.size(), savedChildren.size()); + return savedChildren.get(savedChildren.size() - 1).getStorageId(); + }); + } + + private String addSiteContainerInAnotherRequest(SiteKey siteKey) throws Exception { + return inAnotherRequest(() -> { + PortalConfig site = new PortalConfig(modelStorage.getPortalConfig(siteKey)); + ArrayList children = site.getPortalLayout().getChildren() == null ? new ArrayList<>() : + new ArrayList<>(site.getPortalLayout() + .getChildren()); + children.add(new Container()); + site.getPortalLayout().setChildren(children); + storage_.save(site); + List savedChildren = new PortalConfig(modelStorage.getPortalConfig(siteKey)).getPortalLayout().getChildren(); + assertEquals("The other request's layout is stored", children.size(), savedChildren.size()); + return savedChildren.get(savedChildren.size() - 1).getStorageId(); + }); + } + + private T inAnotherRequest(Callable task) throws Exception { + PortalContainer container = PortalContainer.getInstance(); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + return executor.submit(() -> { + ExoContainerContext.setCurrentContainer(container); + RequestLifeCycle.begin(container); + try { + return task.call(); + } finally { + RequestLifeCycle.end(); + ExoContainerContext.setCurrentContainer(null); + } + }).get(30, TimeUnit.SECONDS); + } finally { + executor.shutdownNow(); + } + } + + private T getComponent(Class type) { + return PortalContainer.getInstance().getComponentInstanceOfType(type); + } + private PageKey createPageWithWindow(String siteName) throws Exception { createSite(SiteType.PORTAL, siteName); Application application = new Application(); From 3b43334f475180af3e78d34812998e1f09d6617b Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Wed, 21 Oct 2026 10:14:38 +0100 Subject: [PATCH 14/15] fix: A page properties save writes no layout column EXO-91183 A page row has two writers, its properties save and its layout save, and PageEntity carries no version. Hibernate wrote every column of the row on each update, so a properties save wrote back the page body it had read, over a layout another request had committed since. PageEntity is @DynamicUpdate: an update writes only the columns it changed. Co-Authored-By: Claude Opus 5.5 --- .../portal/jdbc/entity/PageEntity.java | 6 ++++++ .../portal/mop/storage/PageStorageImpl.java | 5 +++-- .../portal/mop/storage/TestDataStorage.java | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/component/portal/src/main/java/org/exoplatform/portal/jdbc/entity/PageEntity.java b/component/portal/src/main/java/org/exoplatform/portal/jdbc/entity/PageEntity.java index b24e83d335..426f962c6a 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/jdbc/entity/PageEntity.java +++ b/component/portal/src/main/java/org/exoplatform/portal/jdbc/entity/PageEntity.java @@ -22,6 +22,7 @@ import java.util.LinkedList; import java.util.List; +import org.hibernate.annotations.DynamicUpdate; import org.json.simple.JSONArray; import org.json.simple.JSONObject; @@ -44,8 +45,13 @@ import lombok.Getter; import lombok.Setter; +/** + * A page row has two writers, its properties save and its layout save: each + * writes only the columns it changed, so neither writes back the other's. + */ @Entity(name = "GateInPage") @Table(name = "PORTAL_PAGES") +@DynamicUpdate @NamedQuery(name = "PageEntity.deleteByOwner", query = "DELETE GateInPage p WHERE p.owner.id = :ownerId") @NamedQuery(name = "PageEntity.findByKey", query = "SELECT p FROM GateInPage p WHERE p.name = :name AND p.owner.siteType = :ownerType AND p.owner.name = :ownerId") public class PageEntity extends ComponentEntity implements Serializable { diff --git a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java index 2b075b0485..92cb772e38 100644 --- a/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java +++ b/component/portal/src/main/java/org/exoplatform/portal/mop/storage/PageStorageImpl.java @@ -144,8 +144,9 @@ public boolean savePage(PageContext page) { PageEntity entity = pageDAO.findByKey(page.getKey()); if (entity != null) { - // The update writes every column, the page body included: it's read - // from the row, not as an earlier read of this request left it + // The update writes the columns that differ from the loaded state: they're + // compared with the row as stored, not as an earlier read of this request + // left it entity = pageDAO.refresh(entity); } boolean created = false; diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index 6fb5cf11f2..10e5c53bfe 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -61,6 +61,7 @@ import org.exoplatform.portal.config.model.PersistentApplicationState; import org.exoplatform.portal.config.model.PortalConfig; import org.exoplatform.portal.config.model.TransientApplicationState; +import org.exoplatform.portal.jdbc.entity.PageEntity; import org.exoplatform.portal.jdbc.entity.SiteEntity; import org.exoplatform.portal.mop.EventType; import org.exoplatform.portal.mop.QueryResult; @@ -1207,6 +1208,24 @@ public void testSavePagePropertiesKeepsTheLayoutSavedSinceTheRequestReadThePage( savedPage.getChildren().get(savedPage.getChildren().size() - 1).getStorageId()); } + public void testPageRowUpdateWritesOnlyTheColumnsItChanged() throws Exception { + PageKey pageKey = createPageWithWindow("dynamicUpdate"); + PageDAO pageDAO = getComponent(PageDAO.class); + PageEntity page = pageDAO.findByKey(pageKey); + String savedContainerId = addPageContainerInAnotherRequest(pageKey); + + page.setDisplayName("Changed"); + pageDAO.update(page); + restartTransaction(); + + PageEntity storedPage = pageDAO.findByKey(pageKey); + assertEquals("Changed", storedPage.getDisplayName()); + JSONArray storedBody = (JSONArray) new JSONParser().parse(storedPage.getPageBody()); + assertEquals("An update of the page properties writes no layout column", + savedContainerId, + String.valueOf(((JSONObject) storedBody.get(storedBody.size() - 1)).get("id"))); + } + public void testClonePageCopiesTheLayoutSavedSinceTheRequestReadThePage() throws Exception { PageKey pageKey = createPageWithWindow("staleClone"); getComponent(PageDAO.class).findByKey(pageKey); From 65f8e9a240be44669d0b1fb0a1fadeefc9001abc Mon Sep 17 00:00:00 2001 From: Boubaker Khanfir Date: Wed, 21 Oct 2026 11:01:21 +0100 Subject: [PATCH 15/15] test: Pin the properties save's re-read of a property changed meanwhile EXO-91183 With PageEntity's dynamic update, the properties save's re-read no longer guards the page body: it makes each property be compared with the row as stored. A property another request changed since the request read the page, saved back to the value the request first read, is written only when the save compares it with the stored row. Co-Authored-By: Claude Opus 5.5 --- .../portal/mop/storage/TestDataStorage.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java index 10e5c53bfe..9f77824846 100644 --- a/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java +++ b/component/portal/src/test/java/org/exoplatform/portal/mop/storage/TestDataStorage.java @@ -1208,6 +1208,28 @@ public void testSavePagePropertiesKeepsTheLayoutSavedSinceTheRequestReadThePage( savedPage.getChildren().get(savedPage.getChildren().size() - 1).getStorageId()); } + public void testSavePagePropertiesWritesBackAPropertyChangedSinceTheRequestReadThePage() throws Exception { + PageKey pageKey = createPageWithWindow("stalePropertyValue"); + PageState originalState = pageService.loadPage(pageKey).getState().builder().displayName("Original").build(); + pageService.savePage(new PageContext(pageKey, originalState)); + restartTransaction(); + PageDAO pageDAO = getComponent(PageDAO.class); + pageDAO.findByKey(pageKey); + inAnotherRequest(() -> { + PageState state = pageService.loadPage(pageKey).getState(); + pageService.savePage(new PageContext(pageKey, state.builder().displayName("Other").build())); + assertEquals("The other request's property is stored", "Other", pageDAO.findByKey(pageKey).getDisplayName()); + return null; + }); + + pageService.savePage(new PageContext(pageKey, originalState)); + restartTransaction(); + + assertEquals("A property saved back to the value the request first read is written", + "Original", + pageDAO.findByKey(pageKey).getDisplayName()); + } + public void testPageRowUpdateWritesOnlyTheColumnsItChanged() throws Exception { PageKey pageKey = createPageWithWindow("dynamicUpdate"); PageDAO pageDAO = getComponent(PageDAO.class);