diff --git a/mycore-base/src/main/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImpl.java b/mycore-base/src/main/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImpl.java index 9bccf12211..0385989e87 100644 --- a/mycore-base/src/main/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImpl.java +++ b/mycore-base/src/main/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImpl.java @@ -32,11 +32,12 @@ import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.SortedSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; @@ -73,13 +74,13 @@ public class MCRCategoryDAOImpl implements MCRCategoryDAO { private static final int LEFT_START_VALUE = 0; - private static long lastModified = System.currentTimeMillis(); + private static final AtomicLong LAST_MODIFIED = new AtomicLong(System.currentTimeMillis()); private static final Logger LOGGER = LogManager.getLogger(); private static final String NAMED_QUERY_NAMESPACE = "MCRCategory."; - private static final Map LAST_MODIFIED_MAP = new HashMap<>(); + private static final Map LAST_MODIFIED_MAP = new ConcurrentHashMap<>(); @Override public MCRCategory addCategory(MCRCategoryID parentID, MCRCategory category) { @@ -575,11 +576,19 @@ public void repairLeftRightValue(String classID) { @Override public long getLastModified() { - return lastModified; + return LAST_MODIFIED.get(); } private static void updateTimeStamp() { - lastModified = System.currentTimeMillis(); + LAST_MODIFIED.accumulateAndGet(System.currentTimeMillis(), MCRCategoryDAOImpl::nextTimestamp); + } + + /** + * Returns the current time, but at least last + 1. Caches like MCRRoleManager compare + * timestamps for equality and would miss a second change within the same millisecond otherwise. + */ + private static long nextTimestamp(long last, long now) { + return Math.max(last + 1, now); } private static MCRCategoryImpl buildCategoryFromPrefetchedList(List list, MCRCategoryID returnID) { @@ -697,8 +706,8 @@ private static void updateLeftRightValue(EntityManager entityManager, String cla * Method updates the last modified timestamp, for the given root id. * */ - protected synchronized void updateLastModified(String root) { - LAST_MODIFIED_MAP.put(root, System.currentTimeMillis()); + protected void updateLastModified(String root) { + LAST_MODIFIED_MAP.merge(root, System.currentTimeMillis(), MCRCategoryDAOImpl::nextTimestamp); } /** @@ -708,11 +717,7 @@ protected synchronized void updateLastModified(String root) { */ @Override public long getLastModified(String root) { - Long long1 = LAST_MODIFIED_MAP.get(root); - if (long1 != null) { - return long1; - } - return -1; + return LAST_MODIFIED_MAP.getOrDefault(root, -1L); } @SuppressWarnings("unchecked") diff --git a/mycore-base/src/main/java/org/mycore/frontend/MCRLayoutUtilities.java b/mycore-base/src/main/java/org/mycore/frontend/MCRLayoutUtilities.java index 970030d21b..58c978a561 100644 --- a/mycore-base/src/main/java/org/mycore/frontend/MCRLayoutUtilities.java +++ b/mycore-base/src/main/java/org/mycore/frontend/MCRLayoutUtilities.java @@ -29,10 +29,10 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; @@ -109,8 +109,6 @@ public class MCRLayoutUtilities { private static final boolean ACCESS_CONTROLL_ON = MCRConfiguration2 .getOrThrow("MCR.Website.ReadAccessVerification", Boolean::parseBoolean); - private static Map itemStore = new HashMap<>(); - private static final LoadingCache NAV_DOCUMENT_CACHE = CacheBuilder.newBuilder() .refreshAfterWrite(STANDARD_CACHE_SECONDS, TimeUnit.SECONDS).build(new CacheLoader<>() { @@ -119,12 +117,7 @@ public class MCRLayoutUtilities { @Override public DocumentHolder load(String key) throws Exception { URL url = MCRResourceHelper.getWebResourceUrl(key); - try { - return new DocumentHolder(url); - } finally { - itemStore.clear(); - } - + return new DocumentHolder(url); } @Override @@ -284,11 +277,15 @@ public static boolean getAccess(String webpageID, String permission, int strateg * Returns a Element presentation of an item[@href=$webpageID] */ private static Element getItem(String webpageID) { - Element item = itemStore.get(webpageID); + // items are cached per document, so a reloaded navigation.xml never returns stale items + DocumentHolder navigation = NAV_DOCUMENT_CACHE.getUnchecked(NAV_RESOURCE); + Element item = webpageID == null ? null : navigation.items.get(webpageID); if (item == null) { XPathExpression xpath = XPATH_FACTORY.compile("//.[@href='" + webpageID + "']", Filters.element()); - item = xpath.evaluateFirst(getNavi()); - itemStore.put(webpageID, item); + item = xpath.evaluateFirst(navigation.parsedDocument); + if (item != null && webpageID != null) { + navigation.items.put(webpageID, item); + } } return item; } @@ -475,6 +472,9 @@ private static class DocumentHolder { long lastModified; + /** items of {@link #parsedDocument}, key is the webpage ID (item/@href) */ + final Map items = new ConcurrentHashMap<>(); + DocumentHolder(URL url) throws JDOMException, IOException { docURL = url; parseDocument(); diff --git a/mycore-base/src/test/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImplTest.java b/mycore-base/src/test/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImplTest.java index eef0e1bf45..749c81c8b0 100644 --- a/mycore-base/src/test/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImplTest.java +++ b/mycore-base/src/test/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImplTest.java @@ -600,6 +600,23 @@ public void setLabel() { assertEquals(description, rootNode.getLabel(lang).get().getDescription(), "Label does not match."); } + @Test + public void lastModifiedIncreasesOnEveryChange() { + addWorldClassification(); + String rootID = category.getId().getRootID(); + long lastModified = DAO.getLastModified(); + long rootLastModified = DAO.getLastModified(rootID); + // changes within the same millisecond must be detectable, too + for (int i = 0; i < 10; i++) { + DAO.setLabel(category.getId(), new MCRLabel("ju", "JUnit-Test " + i, null)); + assertTrue(DAO.getLastModified() > lastModified, "Last modified timestamp did not increase."); + assertTrue(DAO.getLastModified(rootID) > rootLastModified, + "Last modified timestamp of " + rootID + " did not increase."); + lastModified = DAO.getLastModified(); + rootLastModified = DAO.getLastModified(rootID); + } + } + @Test public void setLabels() { addWorldClassification(); diff --git a/mycore-user2/src/main/java/org/mycore/user2/MCRRoleManager.java b/mycore-user2/src/main/java/org/mycore/user2/MCRRoleManager.java index 2950af399d..5df15b15bc 100644 --- a/mycore-user2/src/main/java/org/mycore/user2/MCRRoleManager.java +++ b/mycore-user2/src/main/java/org/mycore/user2/MCRRoleManager.java @@ -21,7 +21,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.SortedSet; @@ -49,15 +49,10 @@ public class MCRRoleManager { private static final Logger LOGGER = LogManager.getLogger(); - /** Map of defined roles, key is the unique role name */ - private static Map rolesByName = new HashMap<>(); - - /** List of all defined roles */ - private static List rolesList = new ArrayList<>(); - private static final MCRCategoryDAO DAO = MCRCategoryDAOFactory.obtainInstance(); - private static long lastLoaded; + /** defined system roles, replaced as a whole if the classification changes */ + private static volatile SystemRoles systemRoles = new SystemRoles(-1, Map.of(), List.of()); private static final MCRCategLinkService CATEG_LINK_SERVICE = MCRCategLinkServiceFactory.obtainInstance(); @@ -65,23 +60,26 @@ public class MCRRoleManager { loadSystemRoles(); } - private static void loadSystemRoles() { - if (lastLoaded == DAO.getLastModified() || !DAO.exist(MCRUser2Constants.ROLE_CLASSID)) { - return; + private static SystemRoles loadSystemRoles() { + SystemRoles current = systemRoles; + long lastModified = DAO.getLastModified(); + if (current.lastLoaded() == lastModified || !DAO.exist(MCRUser2Constants.ROLE_CLASSID)) { + return current; } - lastLoaded = DAO.getLastModified(); int onlyNonHierarchicalRoles = 1; MCRCategory roleCategory = DAO.getCategory(MCRUser2Constants.ROLE_CLASSID, onlyNonHierarchicalRoles); - rolesByName.clear(); - rolesList.clear(); + Map rolesByName = new LinkedHashMap<>(); if (roleCategory != null) { for (MCRCategory child : roleCategory.getChildren()) { String name = child.getId().getId(); - MCRRole role = new MCRRole(name, child.getLabels()); - rolesByName.put(name, role); - rolesList.add(role); + rolesByName.put(name, new MCRRole(name, child.getLabels())); } } + // never modify the current instance, as other threads may read it concurrently + SystemRoles loaded = new SystemRoles(lastModified, Map.copyOf(rolesByName), + List.copyOf(rolesByName.values())); + systemRoles = loaded; + return loaded; } /** @@ -91,8 +89,7 @@ private static void loadSystemRoles() { * @return the role with the given role name, or null. */ public static MCRRole getRole(String name) { - loadSystemRoles(); - MCRRole mcrRole = rolesByName.get(name); + MCRRole mcrRole = loadSystemRoles().rolesByName().get(name); if (mcrRole == null) { return getExternalRole(name); } @@ -124,7 +121,7 @@ public static MCRRole getExternalRole(String name) { * @return array each element either MCRRole instance, or null */ public static MCRRole[] getRoles(String... names) { - loadSystemRoles(); + Map rolesByName = loadSystemRoles().rolesByName(); MCRRole[] roles = new MCRRole[names.length]; for (int i = 0; i < names.length; i++) { roles[i] = rolesByName.get(names[i]); @@ -142,7 +139,6 @@ public static MCRRole[] getRoles(String... names) { * @return collection each element is MCRRole instance. */ public static Collection getRoles(Collection names) { - loadSystemRoles(); String[] namesArr = names.toArray(String[]::new); MCRRole[] roles = getRoles(namesArr); return Arrays.asList(roles); @@ -151,10 +147,10 @@ public static Collection getRoles(Collection names) { /** * Returns a list of all defined roles * - * @return a list of all defined roles + * @return an unmodifiable list of all defined roles */ public static List listSystemRoles() { - return rolesList; + return loadSystemRoles().rolesList(); } /** @@ -261,4 +257,15 @@ private static MCRCategoryID getCategoryID(MCRRole role) { } return MCRCategoryID.ofString(role.getName()); } + + /** + * Immutable snapshot of the system roles. + * + * @param lastLoaded last modified timestamp of the classifications when the roles were loaded + * @param rolesByName defined roles, key is the unique role name + * @param rolesList all defined roles + */ + private record SystemRoles(long lastLoaded, Map rolesByName, List rolesList) { + } + } diff --git a/mycore-user2/src/test/java/org/mycore/user2/MCRRoleManagerTest.java b/mycore-user2/src/test/java/org/mycore/user2/MCRRoleManagerTest.java new file mode 100644 index 0000000000..b2689e68f0 --- /dev/null +++ b/mycore-user2/src/test/java/org/mycore/user2/MCRRoleManagerTest.java @@ -0,0 +1,64 @@ +/* + * This file is part of *** M y C o R e *** + * See https://www.mycore.de/ for details. + * + * MyCoRe is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * MyCoRe 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with MyCoRe. If not, see . + */ + +package org.mycore.user2; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.List; +import java.util.Set; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mycore.datamodel.classifications2.MCRLabel; +import org.mycore.test.MCRJPAExtension; +import org.mycore.test.MyCoReTest; + +@MyCoReTest +@ExtendWith({ MCRJPAExtension.class, MCRUserExtension.class }) +public class MCRRoleManagerTest { + + @Test + public void listSystemRoles() { + List roles = MCRRoleManager.listSystemRoles(); + assertEquals(List.of("admin", "editor"), roles.stream().map(MCRRole::getName).toList()); + assertThrows(UnsupportedOperationException.class, roles::clear, + "List of system roles should not be modifiable."); + } + + @Test + public void addAndDeleteRole() { + List roles = MCRRoleManager.listSystemRoles(); + assertNull(MCRRoleManager.getRole("submitter")); + + MCRRoleManager.addRole(new MCRRole("submitter", Set.of(new MCRLabel("en", "Submitter", null)))); + assertNotNull(MCRRoleManager.getRole("submitter"), "Added role should be available immediately."); + assertEquals(List.of("admin", "editor", "submitter"), + MCRRoleManager.listSystemRoles().stream().map(MCRRole::getName).toList()); + assertEquals(2, roles.size(), "Previously returned list should not change."); + + MCRRoleManager.deleteRole("submitter"); + assertNull(MCRRoleManager.getRole("submitter"), "Deleted role should not be available anymore."); + assertEquals(List.of("admin", "editor"), + MCRRoleManager.listSystemRoles().stream().map(MCRRole::getName).toList()); + } + +}