From 03059dc929844a05726da94c9d831be263133b09 Mon Sep 17 00:00:00 2001 From: Giles Westwood Date: Tue, 12 May 2026 14:35:52 +0100 Subject: [PATCH 01/15] client-details-bulk-prefetch --- .../core/adapter/v3/JpaJaxbWorkAdapter.java | 4 + .../v3/impl/JpaJaxbWorkAdapterImpl.java | 14 +++ .../adapter/v3/impl/MapperFacadeFactory.java | 9 +- .../ClientDetailsEntityCacheManager.java | 5 ++ .../ClientDetailsEntityCacheManagerImpl.java | 43 ++++++++++ .../ClientDetailsManagerReadOnly.java | 5 ++ .../ClientDetailsManagerReadOnlyImpl.java | 11 +++ .../ClientDetailsManagerReadOnly.java | 5 ++ .../ClientDetailsManagerReadOnlyImpl.java | 11 +++ .../impl/WorkManagerReadOnlyImpl.java | 36 +++----- .../orcid/core/utils/SourceEntityUtils.java | 24 +++++- ...ientDetailsEntityCacheManagerImplTest.java | 86 +++++++++++++++++++ .../core/utils/SourceEntityUtilsTest.java | 35 ++++++++ .../persistence/dao/ClientDetailsDao.java | 5 ++ .../dao/impl/ClientDetailsDaoImpl.java | 26 ++++++ 15 files changed, 291 insertions(+), 28 deletions(-) create mode 100644 orcid-core/src/test/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImplTest.java create mode 100644 orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java index cc949717eac..9a9af9e3719 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java @@ -2,11 +2,13 @@ import java.util.Collection; import java.util.List; +import java.util.Map; import org.orcid.jaxb.model.v3.release.record.Work; import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; import org.orcid.persistence.jpa.entities.MinimizedExtendedWorkEntity; import org.orcid.persistence.jpa.entities.MinimizedWorkEntity; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.persistence.jpa.entities.WorkEntity; import org.orcid.pojo.WorkExtended; import org.orcid.pojo.WorkSummaryExtended; @@ -34,6 +36,8 @@ public interface JpaJaxbWorkAdapter { List toWorkSummaryFromMinimized(Collection workEntities); + List toWorkSummaryFromMinimized(Collection workEntities, Map clientDetailsById); + List toWorkSummaryExtendedFromMinimized(Collection workEntities); WorkEntity toWorkEntity(Work work, WorkEntity existing); diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java index 5bdc7b62412..dd8e152dfe1 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java @@ -2,12 +2,16 @@ import java.util.Collection; import java.util.List; +import java.util.Map; +import ma.glasnost.orika.MappingContext; import ma.glasnost.orika.MapperFacade; +import org.orcid.core.utils.SourceEntityUtils; import org.orcid.core.adapter.v3.JpaJaxbWorkAdapter; import org.orcid.jaxb.model.v3.release.record.Work; import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.persistence.jpa.entities.MinimizedExtendedWorkEntity; import org.orcid.persistence.jpa.entities.MinimizedWorkEntity; import org.orcid.persistence.jpa.entities.WorkEntity; @@ -100,6 +104,16 @@ public List toWorkSummaryFromMinimized(Collection toWorkSummaryFromMinimized(Collection workEntities, Map clientDetailsById) { + if(workEntities == null) { + return null; + } + MappingContext context = new MappingContext.Factory().getContext(); + context.setProperty(SourceEntityUtils.CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY, clientDetailsById); + return mapperFacade.mapAsList(workEntities, WorkSummary.class, context); + } + @Override public List toWorkSummaryExtendedFromMinimized(Collection workEntities) { if(workEntities == null) { diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java index cc4b1a9daf3..22b2729ab51 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java @@ -387,9 +387,16 @@ public void registerSourceConverters(MapperFactory mapperFactory, ClassMapBuilde } private class SourceMapper extends CustomMapper> { + @SuppressWarnings("unchecked") @Override public void mapBtoA(SourceAwareEntity b, SourceAware a, MappingContext context) { - Source source = SourceEntityUtils.extractSourceFromEntityComplete(b, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager); + Map clientDetailsById = null; + if (context != null) { + // Orika context stores values as Object; this key is only set with this map type. + clientDetailsById = (Map) context.getProperty(SourceEntityUtils.CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY); + } + Source source = SourceEntityUtils.extractSourceFromEntityComplete(b, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, + clientDetailsById); a.setSource(source); } } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/ClientDetailsEntityCacheManager.java b/orcid-core/src/main/java/org/orcid/core/manager/ClientDetailsEntityCacheManager.java index c8c7cd078bf..8ba3d563483 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/ClientDetailsEntityCacheManager.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/ClientDetailsEntityCacheManager.java @@ -1,10 +1,15 @@ package org.orcid.core.manager; +import java.util.Collection; +import java.util.Map; + import org.orcid.persistence.jpa.entities.ClientDetailsEntity; public interface ClientDetailsEntityCacheManager { public ClientDetailsEntity retrieve(String clientId) throws IllegalArgumentException; + + public Map retrieveAll(Collection clientIds); public ClientDetailsEntity retrieveByIdP(String clientId) throws IllegalArgumentException; diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImpl.java index b1e331c8235..1b48cff5f60 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImpl.java @@ -1,6 +1,11 @@ package org.orcid.core.manager.impl; +import java.util.ArrayList; +import java.util.Collection; import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import javax.annotation.Resource; @@ -46,6 +51,44 @@ public ClientDetailsEntity retrieve(String clientId) throws IllegalArgumentExcep return clientDetails; } + @Override + public Map retrieveAll(Collection clientIds) { + Map clientDetailsById = new HashMap<>(); + if (clientIds == null || clientIds.isEmpty()) { + return clientDetailsById; + } + + List clientIdList = new ArrayList<>(clientIds); + Map lastModifiedByClientId = clientDetailsManager.getLastModifiedByClientIds(clientIdList); + if (lastModifiedByClientId == null) { + return clientDetailsById; + } + List staleOrMissingClientIds = new ArrayList<>(); + + for (String clientId : clientIdList) { + Date dbDate = lastModifiedByClientId.get(clientId); + if (dbDate == null) { + continue; + } + Object key = new ClientIdCacheKey(clientId, releaseName); + ClientDetailsEntity clientDetails = clientDetailsCache.get(key); + if (needsFresh(dbDate, clientDetails)) { + staleOrMissingClientIds.add(clientId); + } else { + clientDetailsById.put(clientId, clientDetails); + } + } + + if (!staleOrMissingClientIds.isEmpty()) { + for (ClientDetailsEntity clientDetails : clientDetailsManager.findByClientIds(staleOrMissingClientIds)) { + clientDetailsCache.put(new ClientIdCacheKey(clientDetails.getId(), releaseName), clientDetails); + clientDetailsById.put(clientDetails.getId(), clientDetails); + } + } + + return clientDetailsById; + } + @Override public ClientDetailsEntity retrieveByIdP(String idp) throws IllegalArgumentException { Object key = new ClientIdCacheKey("IdP+" + idp, releaseName); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/read_only/ClientDetailsManagerReadOnly.java b/orcid-core/src/main/java/org/orcid/core/manager/read_only/ClientDetailsManagerReadOnly.java index 8cbdf99badd..efae4e3e17b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/read_only/ClientDetailsManagerReadOnly.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/read_only/ClientDetailsManagerReadOnly.java @@ -2,6 +2,7 @@ import java.util.Date; import java.util.List; +import java.util.Map; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.springframework.security.oauth2.provider.ClientDetailsService; @@ -13,6 +14,10 @@ public interface ClientDetailsManagerReadOnly extends ClientDetailsService { Date getLastModified(String clientId); + Map getLastModifiedByClientIds(List clientIds); + + List findByClientIds(List clientIds); + Date getLastModifiedByIdp(String idp); boolean exists(String cliendId); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java index fed04a51b54..ccfa5a69902 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java @@ -3,6 +3,7 @@ import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import javax.annotation.Resource; @@ -156,6 +157,16 @@ public Date getLastModified(String clientId) { return clientDetailsDao.getLastModified(clientId); } + @Override + public Map getLastModifiedByClientIds(List clientIds) { + return clientDetailsDao.getLastModifiedByClientIds(clientIds); + } + + @Override + public List findByClientIds(List clientIds) { + return clientDetailsDao.findByClientIds(clientIds); + } + @Override public Date getLastModifiedByIdp(String idp) { try { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/ClientDetailsManagerReadOnly.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/ClientDetailsManagerReadOnly.java index 74c2b38e6b5..2df876cbc7b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/ClientDetailsManagerReadOnly.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/ClientDetailsManagerReadOnly.java @@ -2,6 +2,7 @@ import java.util.Date; import java.util.List; +import java.util.Map; import org.orcid.jaxb.model.v3.release.client.ClientSummary; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; @@ -14,6 +15,10 @@ public interface ClientDetailsManagerReadOnly extends ClientDetailsService { Date getLastModified(String clientId); + Map getLastModifiedByClientIds(List clientIds); + + List findByClientIds(List clientIds); + Date getLastModifiedByIdp(String idp); boolean exists(String cliendId); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/ClientDetailsManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/ClientDetailsManagerReadOnlyImpl.java index 50819febf2b..e5c93abf860 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/ClientDetailsManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/ClientDetailsManagerReadOnlyImpl.java @@ -3,6 +3,7 @@ import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import javax.annotation.Resource; @@ -157,6 +158,16 @@ public Date getLastModified(String clientId) { return clientDetailsDao.getLastModified(clientId); } + @Override + public Map getLastModifiedByClientIds(List clientIds) { + return clientDetailsDao.getLastModifiedByClientIds(clientIds); + } + + @Override + public List findByClientIds(List clientIds) { + return clientDetailsDao.findByClientIds(clientIds); + } + @Override public Date getLastModifiedByIdp(String idp) { try { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 1e8e27389b3..fcb437edcda 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -3,13 +3,11 @@ import java.math.BigInteger; import java.sql.Timestamp; import java.util.ArrayList; -import java.util.Arrays; -import java.util.concurrent.ForkJoinPool; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; -import javax.annotation.PostConstruct; import javax.annotation.Resource; import org.orcid.core.adapter.jsonidentifier.converter.JSONWorkExternalIdentifiersConverterV3; @@ -59,6 +57,7 @@ import org.orcid.pojo.ajaxForm.PojoUtil; import org.orcid.pojo.ajaxForm.WorkForm; import org.orcid.pojo.grouping.WorkGroupingSuggestion; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; @@ -116,14 +115,6 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements @Value("${org.orcid.core.work.contributors.ui.max:50}") private int maxContributorsForUI; - @Value("${org.orcid.core.manager.v3.read_only.impl.WorkManagerReadOnlyImpl.normalize.min.works.to.parallelize:20}") - private int minWorksToParallelize; - - @PostConstruct - public void init() { - LOGGER.info("Initializing WorkManagerReadOnlyImpl: minWorksToParallelize = " + minWorksToParallelize); - } - public WorkManagerReadOnlyImpl(@Value("${org.orcid.core.works.bulk.read.max:100}") Integer bulkReadSize) { this.maxWorksToRead = (bulkReadSize == null) ? 100 : bulkReadSize; } @@ -206,22 +197,15 @@ public WorkSummary getWorkSummary(String orcid, Long workId) { @Override public List getWorksSummaryList(String orcid) { List works = workEntityCacheManager.retrieveMinimizedWorks(orcid, getLastModified(orcid)); - List workSummaryResult = Arrays.asList(); - // Lets implement a parallel stream for when there are more than 10 works to be processed - if(works.size() < minWorksToParallelize) { - workSummaryResult = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works); - } else { - try { - // Execute the parallel stream within the custom thread pool - workSummaryResult = ForkJoinPool.commonPool().submit(() -> - works.parallelStream().map(minimizedWorkEntity -> jpaJaxbWorkAdapter.toWorkSummary(minimizedWorkEntity)).collect(Collectors.toList()) - ).get(); // use .get() to wait for completion and propagate exceptions - - } catch (Exception e) { - LOGGER.error("Error while generating the list of work summaries in parallel", e); - } + Set clientIds = works.stream() + .map(MinimizedWorkEntity::getClientSourceId) + .filter(clientId -> !PojoUtil.isEmpty(clientId)) + .collect(Collectors.toSet()); + if (clientIds.isEmpty()) { + return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works); } - return workSummaryResult; + Map clientDetailsById = clientDetailsEntityCacheManager.retrieveAll(clientIds); + return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, clientDetailsById); } /** diff --git a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java index 747c178af4f..67e5e08ad13 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java @@ -1,5 +1,7 @@ package org.orcid.core.utils; +import java.util.Map; + import javax.annotation.Resource; import org.apache.commons.lang3.StringUtils; @@ -19,6 +21,8 @@ public class SourceEntityUtils { + public static final String CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY = "clientDetailsById"; + @Resource(name = "recordNameManagerReadOnlyV3") private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; @@ -97,6 +101,11 @@ public static void populateSourceAwareEntityFromSource(Source from, SourceAwareE * */ public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager) { + return extractSourceFromEntity(e, clientDetailsEntityCacheManager, null); + } + + public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, + Map clientDetailsById) { Source source = new Source(); // orcid if (!StringUtils.isEmpty(e.getSourceId())) { @@ -107,7 +116,7 @@ public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetai if (!StringUtils.isEmpty(e.getClientSourceId())) { source.setSourceClientId(new SourceClientId(e.getClientSourceId())); if(e instanceof OrcidAware) { - ClientDetailsEntity clientSource = clientDetailsEntityCacheManager.retrieve(e.getClientSourceId()); + ClientDetailsEntity clientSource = getClientSource(e.getClientSourceId(), clientDetailsEntityCacheManager, clientDetailsById); if (clientSource.isUserOBOEnabled()) { String orcidId = null; if (e instanceof OrcidAware) { @@ -126,6 +135,12 @@ public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetai return source; } + private static ClientDetailsEntity getClientSource(String clientSourceId, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, + Map clientDetailsById) { + ClientDetailsEntity clientSource = clientDetailsById == null ? null : clientDetailsById.get(clientSourceId); + return clientSource == null ? clientDetailsEntityCacheManager.retrieve(clientSourceId) : clientSource; + } + public static Source extractSourceFromProfileComplete(ProfileEntity profile, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager) { Source source = new Source(); SourceEntity entity = profile.getSource(); @@ -146,6 +161,13 @@ public static Source extractSourceFromEntityComplete(SourceAwareEntity b, Sou return s; } + public static Source extractSourceFromEntityComplete(SourceAwareEntity b, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager, + ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, Map clientDetailsById) { + Source s = extractSourceFromEntity(b, clientDetailsEntityCacheManager, clientDetailsById); + populateSource(s, sourceNameCacheManager, orcidUrlManager); + return s; + } + public static void populateSource(Source s, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager) { // Set the source if (s.getSourceOrcid() != null && s.getSourceOrcid().getPath() != null) { diff --git a/orcid-core/src/test/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImplTest.java b/orcid-core/src/test/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImplTest.java new file mode 100644 index 00000000000..610b4a4b324 --- /dev/null +++ b/orcid-core/src/test/java/org/orcid/core/manager/impl/ClientDetailsEntityCacheManagerImplTest.java @@ -0,0 +1,86 @@ +package org.orcid.core.manager.impl; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Arrays; +import java.util.Date; +import java.util.List; +import java.util.Map; + +import org.ehcache.Cache; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.orcid.core.manager.ClientDetailsManager; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; +import org.springframework.test.util.ReflectionTestUtils; + +public class ClientDetailsEntityCacheManagerImplTest { + + private ClientDetailsEntityCacheManagerImpl cacheManager; + + @Mock + private ClientDetailsManager clientDetailsManager; + + @Mock + private Cache clientDetailsCache; + + @Mock + private Cache clientDetailsIdPCache; + + @Before + public void before() { + MockitoAnnotations.initMocks(this); + cacheManager = new ClientDetailsEntityCacheManagerImpl(); + ReflectionTestUtils.setField(cacheManager, "clientDetailsManager", clientDetailsManager); + ReflectionTestUtils.setField(cacheManager, "clientDetailsCache", clientDetailsCache); + ReflectionTestUtils.setField(cacheManager, "clientDetailsIdPCache", clientDetailsIdPCache); + } + + @Test + public void retrieveAllReturnsFreshCachedClientsWithoutSingleClientValidation() { + Date lastModified = new Date(); + ClientDetailsEntity cached = client("A", lastModified); + when(clientDetailsManager.getLastModifiedByClientIds(Arrays.asList("A"))).thenReturn(java.util.Collections.singletonMap("A", lastModified)); + when(clientDetailsCache.get(any())).thenReturn(cached); + + Map result = cacheManager.retrieveAll(Arrays.asList("A")); + + assertEquals(1, result.size()); + assertSame(cached, result.get("A")); + verify(clientDetailsManager).getLastModifiedByClientIds(Arrays.asList("A")); + verify(clientDetailsManager, never()).getLastModified("A"); + verify(clientDetailsManager, never()).findByClientIds(any(List.class)); + } + + @Test + public void retrieveAllReloadsStaleClientsInBulk() { + Date oldLastModified = new Date(1L); + Date newLastModified = new Date(2L); + ClientDetailsEntity cached = client("A", oldLastModified); + ClientDetailsEntity fresh = client("A", newLastModified); + when(clientDetailsManager.getLastModifiedByClientIds(Arrays.asList("A"))).thenReturn(java.util.Collections.singletonMap("A", newLastModified)); + when(clientDetailsCache.get(any())).thenReturn(cached); + when(clientDetailsManager.findByClientIds(Arrays.asList("A"))).thenReturn(Arrays.asList(fresh)); + + Map result = cacheManager.retrieveAll(Arrays.asList("A")); + + assertEquals(1, result.size()); + assertSame(fresh, result.get("A")); + verify(clientDetailsManager).findByClientIds(Arrays.asList("A")); + verify(clientDetailsCache).put(any(), any(ClientDetailsEntity.class)); + } + + private ClientDetailsEntity client(String clientId, Date lastModified) { + ClientDetailsEntity entity = new ClientDetailsEntity(); + entity.setId(clientId); + ReflectionTestUtils.setField(entity, "lastModified", lastModified); + return entity; + } +} diff --git a/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java b/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java new file mode 100644 index 00000000000..22cb6b826c8 --- /dev/null +++ b/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java @@ -0,0 +1,35 @@ +package org.orcid.core.utils; + +import static org.junit.Assert.assertNotNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import java.util.Collections; +import java.util.Date; +import java.util.Map; + +import org.junit.Test; +import org.orcid.core.manager.ClientDetailsEntityCacheManager; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; +import org.orcid.persistence.jpa.entities.MinimizedWorkEntity; +import org.springframework.test.util.ReflectionTestUtils; + +public class SourceEntityUtilsTest { + + @Test + public void extractSourceFromEntityUsesPreloadedClientDetailsWhenAvailable() { + ClientDetailsEntityCacheManager cacheManager = mock(ClientDetailsEntityCacheManager.class); + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); + clientDetails.setId("CLIENT"); + ReflectionTestUtils.setField(clientDetails, "lastModified", new Date()); + Map clientDetailsById = Collections.singletonMap("CLIENT", clientDetails); + MinimizedWorkEntity work = new MinimizedWorkEntity(); + work.setClientSourceId("CLIENT"); + work.setOrcid("0000-0000-0000-0001"); + + assertNotNull(SourceEntityUtils.extractSourceFromEntity(work, cacheManager, clientDetailsById)); + + verify(cacheManager, never()).retrieve("CLIENT"); + } +} diff --git a/orcid-persistence/src/main/java/org/orcid/persistence/dao/ClientDetailsDao.java b/orcid-persistence/src/main/java/org/orcid/persistence/dao/ClientDetailsDao.java index 60c155b5805..a03f4b5f849 100644 --- a/orcid-persistence/src/main/java/org/orcid/persistence/dao/ClientDetailsDao.java +++ b/orcid-persistence/src/main/java/org/orcid/persistence/dao/ClientDetailsDao.java @@ -2,6 +2,7 @@ import java.util.Date; import java.util.List; +import java.util.Map; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.persistence.jpa.entities.ClientSecretEntity; @@ -16,6 +17,10 @@ public interface ClientDetailsDao extends GenericDao getLastModifiedByClientIds(List clientIds); + + List findByClientIds(List clientIds); Date getLastModifiedByIdP(String idp); diff --git a/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ClientDetailsDaoImpl.java b/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ClientDetailsDaoImpl.java index a1db541f4d8..35d625cab90 100644 --- a/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ClientDetailsDaoImpl.java +++ b/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ClientDetailsDaoImpl.java @@ -1,7 +1,9 @@ package org.orcid.persistence.dao.impl; import java.util.Date; +import java.util.HashMap; import java.util.List; +import java.util.Map; import javax.persistence.NoResultException; import javax.persistence.PersistenceContext; @@ -51,6 +53,30 @@ public Date getLastModified(String clientId) { return query.getSingleResult(); } + @Override + public Map getLastModifiedByClientIds(List clientIds) { + if (clientIds == null || clientIds.isEmpty()) { + return new HashMap<>(); + } + TypedQuery query = entityManager.createQuery("select id, lastModified from ClientDetailsEntity where id in :clientIds", Object[].class); + query.setParameter("clientIds", clientIds); + Map lastModifiedByClientId = new HashMap<>(); + for (Object[] result : query.getResultList()) { + lastModifiedByClientId.put((String) result[0], (Date) result[1]); + } + return lastModifiedByClientId; + } + + @Override + public List findByClientIds(List clientIds) { + if (clientIds == null || clientIds.isEmpty()) { + return java.util.Collections.emptyList(); + } + TypedQuery query = entityManager.createQuery("from ClientDetailsEntity where id in :clientIds", ClientDetailsEntity.class); + query.setParameter("clientIds", clientIds); + return query.getResultList(); + } + @Override public Date getLastModifiedByIdP(String idp) { TypedQuery query = entityManager.createQuery("select lastModified from ClientDetailsEntity where authenticationProviderId = :idp", Date.class); From 4d92de004cd326bef847867f4a34f8cc5820edec Mon Sep 17 00:00:00 2001 From: Giles Westwood Date: Tue, 12 May 2026 14:49:47 +0100 Subject: [PATCH 02/15] Arrays removed in error --- .../core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index fcb437edcda..06aa1abe29d 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -3,6 +3,7 @@ import java.math.BigInteger; import java.sql.Timestamp; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Set; From 1701da3ca7de9efa0588373e07d5c9d9e6248269 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Thu, 14 May 2026 11:12:34 -0600 Subject: [PATCH 03/15] More improvements, lets build the source from the manager so it build it only once outside the facade mapper --- .../core/adapter/v3/JpaJaxbWorkAdapter.java | 3 +- .../v3/impl/JpaJaxbWorkAdapterImpl.java | 5 +- .../adapter/v3/impl/MapperFacadeFactory.java | 16 +++--- .../impl/WorkManagerReadOnlyImpl.java | 57 ++++++++++++++++--- .../orcid/core/utils/SourceEntityUtils.java | 37 +++--------- 5 files changed, 73 insertions(+), 45 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java index 9a9af9e3719..921a7329662 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/JpaJaxbWorkAdapter.java @@ -4,6 +4,7 @@ import java.util.List; import java.util.Map; +import org.orcid.jaxb.model.v3.release.common.Source; import org.orcid.jaxb.model.v3.release.record.Work; import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; import org.orcid.persistence.jpa.entities.MinimizedExtendedWorkEntity; @@ -36,7 +37,7 @@ public interface JpaJaxbWorkAdapter { List toWorkSummaryFromMinimized(Collection workEntities); - List toWorkSummaryFromMinimized(Collection workEntities, Map clientDetailsById); + List toWorkSummaryFromMinimized(Collection workEntities, Map sources); List toWorkSummaryExtendedFromMinimized(Collection workEntities); diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java index dd8e152dfe1..40f7bf35c53 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/JpaJaxbWorkAdapterImpl.java @@ -9,6 +9,7 @@ import org.orcid.core.utils.SourceEntityUtils; import org.orcid.core.adapter.v3.JpaJaxbWorkAdapter; +import org.orcid.jaxb.model.v3.release.common.Source; import org.orcid.jaxb.model.v3.release.record.Work; import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; @@ -105,12 +106,12 @@ public List toWorkSummaryFromMinimized(Collection toWorkSummaryFromMinimized(Collection workEntities, Map clientDetailsById) { + public List toWorkSummaryFromMinimized(Collection workEntities, Map sourceMap) { if(workEntities == null) { return null; } MappingContext context = new MappingContext.Factory().getContext(); - context.setProperty(SourceEntityUtils.CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY, clientDetailsById); + context.setProperty(SourceEntityUtils.SOURCE_MAP, sourceMap); return mapperFacade.mapAsList(workEntities, WorkSummary.class, context); } diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java index 22b2729ab51..cb55efef605 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java @@ -390,14 +390,16 @@ private class SourceMapper extends CustomMapper b, SourceAware a, MappingContext context) { - Map clientDetailsById = null; - if (context != null) { - // Orika context stores values as Object; this key is only set with this map type. - clientDetailsById = (Map) context.getProperty(SourceEntityUtils.CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY); + if (context != null && context.getProperty(SourceEntityUtils.SOURCE_MAP) != null) { + // The source map is set in the context, so we can use it to set the source. + Map sourceMap = (Map) context.getProperty(SourceEntityUtils.SOURCE_MAP); + Source source = sourceMap.get(SourceEntityUtils.getSourceKey(b)); + a.setSource(source); + } else { + // We have to manually build the source elements + Source source = SourceEntityUtils.extractSourceFromEntityComplete(b, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, null); + a.setSource(source); } - Source source = SourceEntityUtils.extractSourceFromEntityComplete(b, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, - clientDetailsById); - a.setSource(source); } } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 06aa1abe29d..b2c58f5b128 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -2,11 +2,8 @@ import java.math.BigInteger; import java.sql.Timestamp; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; +import java.util.concurrent.ForkJoinPool; import java.util.stream.Collectors; import javax.annotation.Resource; @@ -22,11 +19,13 @@ import org.orcid.core.manager.ClientDetailsEntityCacheManager; import org.orcid.core.manager.SourceNameCacheManager; import org.orcid.core.manager.WorkEntityCacheManager; +import org.orcid.core.manager.impl.OrcidUrlManager; import org.orcid.core.manager.v3.GroupingSuggestionManager; import org.orcid.core.manager.v3.WorksExtendedCacheManager; import org.orcid.core.manager.v3.read_only.ClientDetailsManagerReadOnly; import org.orcid.core.manager.v3.read_only.WorkManagerReadOnly; import org.orcid.core.togglz.Features; +import org.orcid.core.utils.SourceEntityUtils; import org.orcid.core.utils.v3.ContributorUtils; import org.orcid.core.utils.v3.activities.ActivitiesGroup; import org.orcid.core.utils.v3.activities.ActivitiesGroupGenerator; @@ -34,6 +33,7 @@ import org.orcid.core.utils.v3.activities.WorkGroupAndGroupingSuggestionGenerator; import org.orcid.jaxb.model.record.bulk.BulkElement; import org.orcid.jaxb.model.v3.release.common.PublicationDate; +import org.orcid.jaxb.model.v3.release.common.Source; import org.orcid.jaxb.model.v3.release.record.ExternalID; import org.orcid.jaxb.model.v3.release.record.ExternalIDs; import org.orcid.jaxb.model.v3.release.record.GroupAble; @@ -71,6 +71,9 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements public static final String BULK_PUT_CODES_DELIMITER = ","; + @Value("${single.thread:true}") + private boolean singleThread; + @Resource(name = "jpaJaxbWorkAdapterV3") protected JpaJaxbWorkAdapter jpaJaxbWorkAdapter; @@ -113,6 +116,9 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements @Resource private ContributorsRolesAndSequencesConverter contributorsRolesAndSequencesConverter; + @Resource + private OrcidUrlManager orcidUrlManager; + @Value("${org.orcid.core.work.contributors.ui.max:50}") private int maxContributorsForUI; @@ -202,11 +208,48 @@ public List getWorksSummaryList(String orcid) { .map(MinimizedWorkEntity::getClientSourceId) .filter(clientId -> !PojoUtil.isEmpty(clientId)) .collect(Collectors.toSet()); + + // Get the client details from the database + Map clientDetailsById = clientDetailsEntityCacheManager.retrieveAll(clientIds); + Map sources = new HashMap<>(); + works.stream().forEach(workEntity -> { + String sourceKey = SourceEntityUtils.getSourceKey(workEntity); + if(!sources.containsKey(sourceKey)) { + Source source = SourceEntityUtils.extractSourceFromEntityComplete(workEntity, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, + clientDetailsById); + sources.put(sourceKey, source); + } + }); + if (clientIds.isEmpty()) { return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works); } - Map clientDetailsById = clientDetailsEntityCacheManager.retrieveAll(clientIds); - return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, clientDetailsById); + + // This map should be read-only + Map readOnlySources = Collections.unmodifiableMap(sources); + // Single thread implementation + if(singleThread) { + long start = System.currentTimeMillis(); + List result = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); + long end = System.currentTimeMillis(); + System.out.println("Single thread - Time to convert to JAXB in millisecs: " + (end - start)); + return result; + } else { + List result = null; + long start = System.currentTimeMillis(); + try { + // Execute the parallel stream within the custom thread pool + result = ForkJoinPool.commonPool().submit(() -> + works.parallelStream().map(minimizedWorkEntity -> jpaJaxbWorkAdapter.toWorkSummary(minimizedWorkEntity)).collect(Collectors.toList()) + ).get(); // use .get() to wait for completion and propagate exceptions + + } catch (Exception e) { + LOGGER.error("Error while generating the list of work summaries in parallel", e); + } + long end = System.currentTimeMillis(); + System.out.println("Multi thread - Time to convert to JAXB in millisecs: " + (end - start)); + return result; + } } /** diff --git a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java index 67e5e08ad13..3af15a714f5 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java @@ -1,6 +1,7 @@ package org.orcid.core.utils; import java.util.Map; +import java.util.Optional; import javax.annotation.Resource; @@ -21,14 +22,11 @@ public class SourceEntityUtils { - public static final String CLIENT_DETAILS_BY_ID_MAPPING_CONTEXT_KEY = "clientDetailsById"; + public static final String SOURCE_MAP = "sourceMap"; @Resource(name = "recordNameManagerReadOnlyV3") private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; - @Resource - private OrcidUrlManager orcidUrlManager; - public String getSourceName(SourceEntity sourceEntity) { if (sourceEntity.getCachedSourceName() != null) { return sourceEntity.getCachedSourceName(); @@ -44,23 +42,6 @@ public String getSourceName(SourceEntity sourceEntity) { return null; } - /** - * Call this method before storing in cache to prevent a whole profile or - * client being serialized. - * - * WARNING: The entity must be detached (using DAO) so that the source is - * not made null in DB. - */ - public void prepareForCache(SourceEntity sourceEntity) { - if (!sourceEntity.isDetached()) { - throw new IllegalStateException("Must not prepare source entity for cache, unless it is detached"); - } - sourceEntity.setCachedSourceId(getSourceId(sourceEntity)); - sourceEntity.setCachedSourceName(getSourceName(sourceEntity)); - sourceEntity.setSourceClient(null); - sourceEntity.setSourceProfile(null); - } - public static String getSourceId(SourceEntity sourceEntity) { if (sourceEntity.getCachedSourceId() != null) { return sourceEntity.getCachedSourceId(); @@ -154,13 +135,6 @@ public static Source extractSourceFromProfileComplete(ProfileEntity profile, Sou return source; } - public static Source extractSourceFromEntityComplete(SourceAwareEntity b, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager, - ClientDetailsEntityCacheManager clientDetailsEntityCacheManager) { - Source s = extractSourceFromEntity(b, clientDetailsEntityCacheManager); - populateSource(s, sourceNameCacheManager, orcidUrlManager); - return s; - } - public static Source extractSourceFromEntityComplete(SourceAwareEntity b, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, Map clientDetailsById) { Source s = extractSourceFromEntity(b, clientDetailsEntityCacheManager, clientDetailsById); @@ -251,4 +225,11 @@ public static boolean isTheSameForPermissionChecking(Source activeSource, Source Source existing = extractSourceFromEntity(existingEntity, clientDetailsEntityCacheManager); return existing.equals(activeSource); } + + public static String getSourceKey(SourceAwareEntity e) { + String sourceOrcid = Optional.ofNullable(e.getSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + String clientSourceId = Optional.ofNullable(e.getClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + String assertionOriginClientSourceId = Optional.ofNullable(e.getAssertionOriginClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + return String.join("|", sourceOrcid, clientSourceId, assertionOriginClientSourceId); + } } From 5e02cf0b4d7babb2e4f2daad83092851904f5b85 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 25 May 2026 09:13:42 -0600 Subject: [PATCH 04/15] Some basic stats collected to compare with current code --- .../impl/WorkManagerReadOnlyImpl.java | 36 +++++-------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index b2c58f5b128..b46b00c0ee7 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -71,9 +71,6 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements public static final String BULK_PUT_CODES_DELIMITER = ","; - @Value("${single.thread:true}") - private boolean singleThread; - @Resource(name = "jpaJaxbWorkAdapterV3") protected JpaJaxbWorkAdapter jpaJaxbWorkAdapter; @@ -209,6 +206,9 @@ public List getWorksSummaryList(String orcid) { .filter(clientId -> !PojoUtil.isEmpty(clientId)) .collect(Collectors.toSet()); + System.out.println("Generating source list"); + long start = System.currentTimeMillis(); + long s0 = System.currentTimeMillis(); // Get the client details from the database Map clientDetailsById = clientDetailsEntityCacheManager.retrieveAll(clientIds); Map sources = new HashMap<>(); @@ -220,36 +220,18 @@ public List getWorksSummaryList(String orcid) { sources.put(sourceKey, source); } }); - + long end = System.currentTimeMillis(); + System.out.println("Time take generating source list: " + (end - start)); if (clientIds.isEmpty()) { return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works); } // This map should be read-only Map readOnlySources = Collections.unmodifiableMap(sources); - // Single thread implementation - if(singleThread) { - long start = System.currentTimeMillis(); - List result = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); - long end = System.currentTimeMillis(); - System.out.println("Single thread - Time to convert to JAXB in millisecs: " + (end - start)); - return result; - } else { - List result = null; - long start = System.currentTimeMillis(); - try { - // Execute the parallel stream within the custom thread pool - result = ForkJoinPool.commonPool().submit(() -> - works.parallelStream().map(minimizedWorkEntity -> jpaJaxbWorkAdapter.toWorkSummary(minimizedWorkEntity)).collect(Collectors.toList()) - ).get(); // use .get() to wait for completion and propagate exceptions - - } catch (Exception e) { - LOGGER.error("Error while generating the list of work summaries in parallel", e); - } - long end = System.currentTimeMillis(); - System.out.println("Multi thread - Time to convert to JAXB in millisecs: " + (end - start)); - return result; - } + List summaryList = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); + long s1 = System.currentTimeMillis(); + System.out.println("Total time: " + (s1 - s0)); + return summaryList; } /** From 08f831848345dbb2a6cadcacbb9af65c2bdf0e63 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 25 May 2026 10:35:30 -0600 Subject: [PATCH 05/15] Refactoring the SourceEntityUtil so it is less complex, not all functions are static and it removes duplicated functions --- .../adapter/v3/impl/MapperFacadeFactory.java | 9 +- .../exception/InvalidPutCodeException.java | 7 +- .../exception/OrcidCoreExceptionMapper.java | 5 +- .../core/manager/impl/AddressManagerImpl.java | 5 +- .../impl/ExternalIdentifierManagerImpl.java | 6 +- .../manager/impl/PeerReviewManagerImpl.java | 5 +- .../core/manager/impl/WorkManagerImpl.java | 9 +- .../manager/v3/impl/AddressManagerImpl.java | 13 +- .../v3/impl/AffiliationsManagerImpl.java | 9 +- .../impl/ExternalIdentifierManagerImpl.java | 13 +- .../v3/impl/OrcidSecurityManagerImpl.java | 5 +- .../manager/v3/impl/OtherNameManagerImpl.java | 13 +- .../v3/impl/PeerReviewManagerImpl.java | 9 +- .../v3/impl/ProfileFundingManagerImpl.java | 9 +- .../v3/impl/ProfileKeywordManagerImpl.java | 13 +- .../v3/impl/ResearchResourceManagerImpl.java | 9 +- .../v3/impl/ResearcherUrlManagerImpl.java | 13 +- .../core/manager/v3/impl/WorkManagerImpl.java | 19 ++- .../impl/RecordManagerReadOnlyImpl.java | 7 +- .../impl/WorkManagerReadOnlyImpl.java | 24 +--- .../v3/validator/ActivityValidator.java | 14 +- .../manager/v3/validator/PersonValidator.java | 15 +- .../manager/validator/ActivityValidator.java | 2 +- .../orcid/core/utils/SourceEntityUtils.java | 133 +++++++----------- 24 files changed, 180 insertions(+), 186 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java index cb55efef605..8bf575cbf87 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java @@ -176,8 +176,6 @@ public class MapperFacadeFactory implements FactoryBean { @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; - @Resource(name = "clientDetailsManagerReadOnlyV3") - private ClientDetailsManagerReadOnly clientDetailsManagerReadOnly; @Resource private IdentityProviderManager identityProviderManager; @@ -200,6 +198,9 @@ public class MapperFacadeFactory implements FactoryBean { @Resource private FundingContributorRoleConverter fundingContributorsRoleConverter; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public MapperFacade getObject() throws Exception { MapperFactory mapperFactory = new DefaultMapperFactory.Builder().build(); @@ -393,11 +394,11 @@ public void mapBtoA(SourceAwareEntity b, SourceAware a, MappingContext contex if (context != null && context.getProperty(SourceEntityUtils.SOURCE_MAP) != null) { // The source map is set in the context, so we can use it to set the source. Map sourceMap = (Map) context.getProperty(SourceEntityUtils.SOURCE_MAP); - Source source = sourceMap.get(SourceEntityUtils.getSourceKey(b)); + Source source = sourceMap.get(sourceEntityUtils.getSourceKey(b)); a.setSource(source); } else { // We have to manually build the source elements - Source source = SourceEntityUtils.extractSourceFromEntityComplete(b, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, null); + Source source = sourceEntityUtils.extractSourceFromEntityComplete(b); a.setSource(source); } } diff --git a/orcid-core/src/main/java/org/orcid/core/exception/InvalidPutCodeException.java b/orcid-core/src/main/java/org/orcid/core/exception/InvalidPutCodeException.java index 88805d9f44a..8e419539274 100644 --- a/orcid-core/src/main/java/org/orcid/core/exception/InvalidPutCodeException.java +++ b/orcid-core/src/main/java/org/orcid/core/exception/InvalidPutCodeException.java @@ -3,6 +3,7 @@ import java.util.HashMap; import java.util.Map; +import org.apache.commons.lang3.StringUtils; import org.orcid.core.utils.SourceEntityUtils; import org.orcid.jaxb.model.v3.release.common.Source; @@ -19,10 +20,10 @@ public InvalidPutCodeException(Map params) { super(params); } - public static InvalidPutCodeException forSource(Source activeSource) { + public static InvalidPutCodeException forSource(String sourceName) { Map params = new HashMap(); - if (activeSource != null) { - params.put("clientName", SourceEntityUtils.getSourceName(activeSource)); + if (StringUtils.isNoneBlank(sourceName)) { + params.put("clientName", sourceName); } return new InvalidPutCodeException(params); } diff --git a/orcid-core/src/main/java/org/orcid/core/exception/OrcidCoreExceptionMapper.java b/orcid-core/src/main/java/org/orcid/core/exception/OrcidCoreExceptionMapper.java index d09e1f386e6..8409f0657a4 100644 --- a/orcid-core/src/main/java/org/orcid/core/exception/OrcidCoreExceptionMapper.java +++ b/orcid-core/src/main/java/org/orcid/core/exception/OrcidCoreExceptionMapper.java @@ -47,6 +47,9 @@ public class OrcidCoreExceptionMapper { @Resource(name = "sourceManagerV3") private SourceManager sourceManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + private static Map, Pair> HTTP_STATUS_AND_ERROR_CODE_BY_THROWABLE_TYPE = new HashMap<>(); { // 301 @@ -226,7 +229,7 @@ public org.orcid.jaxb.model.v3.release.error.OrcidError getOrcidErrorV3(int erro Map params = null; if (t instanceof PutCodeFormatException) { params = new HashMap(); - params.put("clientName", SourceEntityUtils.getSourceName(sourceManager.retrieveActiveSource())); + params.put("clientName", sourceEntityUtils.getSourceName(sourceManager.retrieveActiveSource())); } else if (t instanceof ApplicationException) { params = ((ApplicationException) t).getParams(); } else if (t instanceof IllegalEnumValueException) { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/AddressManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/AddressManagerImpl.java index b2df894a4c2..b5d82f4d6a6 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/AddressManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/AddressManagerImpl.java @@ -33,6 +33,9 @@ public class AddressManagerImpl extends AddressManagerReadOnlyImpl implements Ad @Resource protected SourceManager sourceManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Resource private ProfileEntityCacheManager profileEntityCacheManager; @@ -130,7 +133,7 @@ private boolean isDuplicated(AddressEntity existing, Address address, SourceEnti if (!existing.getId().equals(address.getPutCode())) { // If they have the same source String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && existingSourceId.equals(SourceEntityUtils.getSourceId(source))) { + if (!PojoUtil.isEmpty(existingSourceId) && existingSourceId.equals(sourceEntityUtils.getSourceId(source))) { if (existing.getIso2Country().equals(address.getCountry().getValue())) { return true; } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/ExternalIdentifierManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/ExternalIdentifierManagerImpl.java index 52c41dacf33..edc8e3a8e30 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/ExternalIdentifierManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/ExternalIdentifierManagerImpl.java @@ -34,10 +34,10 @@ public class ExternalIdentifierManagerImpl extends ExternalIdentifierManagerRead private OrcidSecurityManager orcidSecurityManager; @Resource - private ProfileEntityManager profileEntityManager; + private ProfileEntityCacheManager profileEntityCacheManager; @Resource - private ProfileEntityCacheManager profileEntityCacheManager; + private SourceEntityUtils sourceEntityUtils; @Override public PersonExternalIdentifier createExternalIdentifier(String orcid, PersonExternalIdentifier externalIdentifier, boolean isApiRequest) { @@ -109,7 +109,7 @@ private boolean isDuplicated(ExternalIdentifierEntity existing, PersonExternalId if (!existing.getId().equals(newExternalIdentifier.getPutCode())) { // If they have the same source String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && existingSourceId.equals(SourceEntityUtils.getSourceId(source))) { + if (!PojoUtil.isEmpty(existingSourceId) && existingSourceId.equals(sourceEntityUtils.getSourceId(source))) { // And they have the same reference if ((PojoUtil.isEmpty(existing.getExternalIdReference()) && PojoUtil.isEmpty(newExternalIdentifier.getValue())) || (!PojoUtil.isEmpty(existing.getExternalIdReference()) && existing.getExternalIdReference().equals(newExternalIdentifier.getValue()))) { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/PeerReviewManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/PeerReviewManagerImpl.java index 58d161de081..f4114c01653 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/PeerReviewManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/PeerReviewManagerImpl.java @@ -78,6 +78,9 @@ public class PeerReviewManagerImpl extends PeerReviewManagerReadOnlyImpl impleme @Resource private ProfileEntityCacheManager profileEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public PeerReview createPeerReview(String orcid, PeerReview peerReview, boolean isApiRequest) { SourceEntity sourceEntity = sourceManager.retrieveSourceEntity(); @@ -90,7 +93,7 @@ public PeerReview createPeerReview(String orcid, PeerReview peerReview, boolean List peerReviews = peerReviewDao.getByUser(orcid, getLastModified(orcid)); // If it is the user adding the peer review, allow him to add // duplicates - if (!SourceEntityUtils.getSourceId(sourceEntity).equals(orcid)) { + if (!sourceEntityUtils.getSourceId(sourceEntity).equals(orcid)) { if (peerReviews != null) { for (PeerReviewEntity entity : peerReviews) { PeerReview existing = jpaJaxbPeerReviewAdapter.toPeerReview(entity); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/WorkManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/WorkManagerImpl.java index f56fb8dc841..29c8d73a551 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/WorkManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/WorkManagerImpl.java @@ -95,6 +95,9 @@ public class WorkManagerImpl extends WorkManagerReadOnlyImpl implements WorkMana @Resource private ContributorsRolesAndSequencesConverterV2 contributorsRolesAndSequencesConverterV2; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Value("${org.orcid.core.work.contributors.ui.max:50}") private int maxContributorsForUI; @@ -160,7 +163,7 @@ public Work createWork(String orcid, Work work, boolean isApiRequest) { activityValidator.validateWork(work, sourceEntity, true, isApiRequest, null); // If it is the user adding the peer review, allow him to add // duplicates - if (!SourceEntityUtils.getSourceId(sourceEntity).equals(orcid)) { + if (!sourceEntityUtils.getSourceId(sourceEntity).equals(orcid)) { List existingWorks = this.findWorks(orcid); if (existingWorks != null) { if ((existingWorks.size() + 1) > this.maxNumOfActivities) { @@ -222,7 +225,7 @@ public WorkBulk createWorks(String orcid, WorkBulk workBulk) { if (workBulk.getBulk() != null && !workBulk.getBulk().isEmpty()) { List bulk = workBulk.getBulk(); Map extIDPutCodeMap = new HashMap(); - Set existingExternalIdentifiers = buildExistingExternalIdsSet(existingWorks, SourceEntityUtils.getSourceId(sourceEntity), extIDPutCodeMap); + Set existingExternalIdentifiers = buildExistingExternalIdsSet(existingWorks, sourceEntityUtils.getSourceId(sourceEntity), extIDPutCodeMap); if ((existingWorks.size() + bulk.size()) > this.maxNumOfActivities) { throw new ExceedMaxNumberOfElementsException(); } @@ -248,7 +251,7 @@ public WorkBulk createWorks(String orcid, WorkBulk workBulk) { // identifier, then mark it as duplicated if (existingExternalIdentifiers.contains(extId) && Relationship.SELF.equals(extId.getRelationship())) { Map params = new HashMap(); - params.put("clientName", sourceNameCacheManager.retrieve(SourceEntityUtils.getSourceId(sourceEntity))); + params.put("clientName", sourceNameCacheManager.retrieve(sourceEntityUtils.getSourceId(sourceEntity))); if (extIDPutCodeMap.containsKey(extId)) { params.put("putCode", String.valueOf(extIDPutCodeMap.get(extId))); } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AddressManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AddressManagerImpl.java index 996eac8440e..e217ba7b13b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AddressManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AddressManagerImpl.java @@ -40,6 +40,9 @@ public class AddressManagerImpl extends AddressManagerReadOnlyImpl implements Ad @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override @Transactional public Address updateAddress(String orcid, Long putCode, Address address, boolean isApiRequest) { @@ -48,7 +51,7 @@ public Address updateAddress(String orcid, Long putCode, Address address, boolea Visibility originalVisibility = Visibility.fromValue(updatedEntity.getVisibility()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(updatedEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(updatedEntity); // If it is an update from the API, check the source and preserve the // original visibility @@ -75,7 +78,7 @@ public Address updateAddress(String orcid, Long putCode, Address address, boolea adapter.toAddressEntity(address, updatedEntity); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedEntity); addressDao.merge(updatedEntity); return adapter.toAddress(updatedEntity); @@ -101,7 +104,7 @@ public Address createAddress(String orcid, Address address, boolean isApiRequest ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); newEntity.setOrcid(orcid); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(newEntity, isApiRequest); setIncomingPrivacy(newEntity, profile); @@ -127,7 +130,7 @@ private boolean isDuplicated(AddressEntity existing, Address address, Source act if (!existing.getId().equals(address.getPutCode())) { // If they have the same source String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existing, clientDetailsEntityCacheManager)) { + if (!PojoUtil.isEmpty(existingSourceId) && sourceEntityUtils.isTheSameSource(activeSource, existing)) { // TODO: Not sure this works! String vs Iso3166Country enum if (existing.getIso2Country().equals(address.getCountry().getValue())) { return true; @@ -188,7 +191,7 @@ public Addresses updateAddresses(String orcid, Addresses addresses) { AddressEntity newAddress = adapter.toAddressEntity(updatedOrNew); Source activeSource = sourceManager.retrieveActiveSource(); newAddress.setOrcid(orcid); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newAddress); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newAddress); newAddress.setVisibility(updatedOrNew.getVisibility().name()); newAddress.setDisplayIndex(updatedOrNew.getDisplayIndex()); addressDao.persist(newAddress); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AffiliationsManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AffiliationsManagerImpl.java index d8cb5665d6e..b01b07c024b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AffiliationsManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/AffiliationsManagerImpl.java @@ -55,6 +55,9 @@ public class AffiliationsManagerImpl extends AffiliationsManagerReadOnlyImpl imp @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + /** * Add a new distinction to the given user * @@ -292,7 +295,7 @@ private Affiliation createAffiliation(String orcid, Affiliation affiliation, boo OrgEntity updatedOrganization = orgManager.getOrgEntity(affiliation); entity.setOrg(updatedOrganization); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, entity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, entity); ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); setIncomingWorkPrivacy(entity, profile, isApiRequest); @@ -342,7 +345,7 @@ public Affiliation updateAffiliation(String orcid, Affiliation affiliation, bool Source activeSource = sourceManager.retrieveActiveSource(); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(entity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(entity); String originalVisibility = entity.getVisibility(); orcidSecurityManager.checkSourceAndThrow(entity); @@ -385,7 +388,7 @@ public Affiliation updateAffiliation(String orcid, Affiliation affiliation, bool // Populate display index in case it is missing DisplayIndexCalculatorHelper.setDisplayIndexOnExistingEntity(entity, isApiRequest); - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, entity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, entity); // Updates the give organization with the latest organization from // database, or, create a new one diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ExternalIdentifierManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ExternalIdentifierManagerImpl.java index 4bff791ffcd..f15baa6245b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ExternalIdentifierManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ExternalIdentifierManagerImpl.java @@ -34,14 +34,11 @@ public class ExternalIdentifierManagerImpl extends ExternalIdentifierManagerRead @Resource(name = "orcidSecurityManagerV3") private OrcidSecurityManager orcidSecurityManager; - @Resource(name = "profileEntityManagerV3") - private ProfileEntityManager profileEntityManager; - @Resource private ProfileEntityCacheManager profileEntityCacheManager; @Resource - private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + private SourceEntityUtils sourceEntityUtils; @Override public PersonExternalIdentifier createExternalIdentifier(String orcid, PersonExternalIdentifier externalIdentifier, boolean isApiRequest) { @@ -63,7 +60,7 @@ public PersonExternalIdentifier createExternalIdentifier(String orcid, PersonExt ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); newEntity.setOrcid(orcid); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); setIncomingPrivacy(newEntity, profile); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(newEntity, isApiRequest); @@ -77,7 +74,7 @@ public PersonExternalIdentifier updateExternalIdentifier(String orcid, PersonExt ExternalIdentifierEntity updatedExternalIdentifierEntity = externalIdentifierDao.getExternalIdentifierEntity(orcid, externalIdentifier.getPutCode()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(updatedExternalIdentifierEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(updatedExternalIdentifierEntity); Visibility originalVisibility = Visibility.valueOf(updatedExternalIdentifierEntity.getVisibility()); // Validate external identifier @@ -96,7 +93,7 @@ public PersonExternalIdentifier updateExternalIdentifier(String orcid, PersonExt jpaJaxbExternalIdentifierAdapter.toExternalIdentifierEntity(externalIdentifier, updatedExternalIdentifierEntity); // Set source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedExternalIdentifierEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedExternalIdentifierEntity); externalIdentifierDao.merge(updatedExternalIdentifierEntity); return jpaJaxbExternalIdentifierAdapter.toExternalIdentifier(updatedExternalIdentifierEntity); @@ -106,7 +103,7 @@ private boolean isDuplicated(ExternalIdentifierEntity existing, PersonExternalId if (!existing.getId().equals(newExternalIdentifier.getPutCode())) { // If they have the same source String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existing, clientDetailsEntityCacheManager)) { + if (!PojoUtil.isEmpty(existingSourceId) && sourceEntityUtils.isTheSameSource(activeSource, existing)) { // And they have the same reference if ((PojoUtil.isEmpty(existing.getExternalIdReference()) && PojoUtil.isEmpty(newExternalIdentifier.getValue())) || (!PojoUtil.isEmpty(existing.getExternalIdReference()) && existing.getExternalIdReference().equals(newExternalIdentifier.getValue()))) { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OrcidSecurityManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OrcidSecurityManagerImpl.java index 894e448237f..0966ce45b34 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OrcidSecurityManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OrcidSecurityManagerImpl.java @@ -114,6 +114,9 @@ public class OrcidSecurityManagerImpl implements OrcidSecurityManager { @Resource private OrcidUserDetailsService orcidUserDetailsService; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public boolean isAdmin() { return orcidUserDetailsService.isAdmin(); @@ -217,7 +220,7 @@ private boolean isOldEnough(ProfileEntity profile) { @Override public void checkSourceAndThrow(SourceAwareEntity existingEntity) { Source activeSource = sourceManager.retrieveActiveSource(); - if (activeSource != null && !SourceEntityUtils.isTheSameForPermissionChecking(activeSource, existingEntity, clientDetailsEntityCacheManager)) { + if (activeSource != null && !sourceEntityUtils.isTheSameSource(activeSource, existingEntity)) { Map params = new HashMap(); params.put("activity", "work"); throw new WrongSourceException(params); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OtherNameManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OtherNameManagerImpl.java index e12b77b40a9..fd9113af3d5 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OtherNameManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/OtherNameManagerImpl.java @@ -40,6 +40,9 @@ public class OtherNameManagerImpl extends OtherNameManagerReadOnlyImpl implement @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override @Transactional public boolean deleteOtherName(String orcid, Long putCode, boolean checkSource) { @@ -78,7 +81,7 @@ public OtherName createOtherName(String orcid, OtherName otherName, boolean isAp ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); newEntity.setOrcid(orcid); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); setIncomingPrivacy(newEntity, profile); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(newEntity, isApiRequest); @@ -93,7 +96,7 @@ public OtherName updateOtherName(String orcid, Long putCode, OtherName otherName OtherNameEntity updatedOtherNameEntity = otherNameDao.getOtherName(orcid, putCode); Visibility originalVisibility = Visibility.fromValue(updatedOtherNameEntity.getVisibility()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(updatedOtherNameEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(updatedOtherNameEntity); // Validate the other name PersonValidator.validateOtherName(otherName, activeSource, false, isApiRequest, originalVisibility); @@ -111,7 +114,7 @@ public OtherName updateOtherName(String orcid, Long putCode, OtherName otherName orcidSecurityManager.checkSourceAndThrow(updatedOtherNameEntity); jpaJaxbOtherNameAdapter.toOtherNameEntity(otherName, updatedOtherNameEntity); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedOtherNameEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedOtherNameEntity); otherNameDao.merge(updatedOtherNameEntity); return jpaJaxbOtherNameAdapter.toOtherName(updatedOtherNameEntity); @@ -160,7 +163,7 @@ public OtherNames updateOtherNames(String orcid, OtherNames otherNames) { Source activeSource = sourceManager.retrieveActiveSource(); newOtherName.setOrcid(orcid); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newOtherName); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newOtherName); newOtherName.setVisibility(updatedOrNew.getVisibility().name()); newOtherName.setDisplayIndex(updatedOrNew.getDisplayIndex()); otherNameDao.persist(newOtherName); @@ -173,7 +176,7 @@ public OtherNames updateOtherNames(String orcid, OtherNames otherNames) { private boolean isDuplicated(OtherNameEntity existing, OtherName otherName, Source activeSource) { if (!existing.getId().equals(otherName.getPutCode())) { String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existing, clientDetailsEntityCacheManager)) { + if (!PojoUtil.isEmpty(existingSourceId) && sourceEntityUtils.isTheSameSource(activeSource, existing)) { if (existing.getDisplayName() != null && existing.getDisplayName().equals(otherName.getContent())) { return true; } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/PeerReviewManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/PeerReviewManagerImpl.java index 2d392b760a3..625ed5ce552 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/PeerReviewManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/PeerReviewManagerImpl.java @@ -82,6 +82,9 @@ public class PeerReviewManagerImpl extends PeerReviewManagerReadOnlyImpl impleme @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public PeerReview createPeerReview(String orcid, PeerReview peerReview, boolean isApiRequest) { Source activeSource = sourceManager.retrieveActiveSource(); @@ -117,7 +120,7 @@ public PeerReview createPeerReview(String orcid, PeerReview peerReview, boolean entity.setOrg(updatedOrganization); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, entity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, entity); ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); setIncomingPrivacy(entity, profile); @@ -137,7 +140,7 @@ public PeerReview updatePeerReview(String orcid, PeerReview peerReview, boolean Source activeSource = sourceManager.retrieveActiveSource(); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(existingEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(existingEntity); // If request comes from the API perform validations if (isApiRequest) { @@ -162,7 +165,7 @@ public PeerReview updatePeerReview(String orcid, PeerReview peerReview, boolean existingEntity.setVisibility(originalVisibility.name()); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, existingEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, existingEntity); createIssnGroupIdIfNecessary(peerReview); if (peerReview.getOrganization() != null) { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileFundingManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileFundingManagerImpl.java index 62eb974beb4..9e35872679c 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileFundingManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileFundingManagerImpl.java @@ -55,6 +55,9 @@ public class ProfileFundingManagerImpl extends ProfileFundingManagerReadOnlyImpl @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + /** * Removes the relationship that exists between a funding and a profile. * @@ -143,7 +146,7 @@ public Funding createFunding(String orcid, Funding funding, boolean isApiRequest profileFundingEntity.setOrg(updatedOrganization); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, profileFundingEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, profileFundingEntity); ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); setIncomingPrivacy(profileFundingEntity, profile, isApiRequest); @@ -183,7 +186,7 @@ public Funding updateFunding(String orcid, Funding funding, boolean isApiRequest Visibility originalVisibility = Visibility.valueOf(pfe.getVisibility()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(pfe, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(pfe); activityValidator.validateFunding(funding, activeSOurce, false, isApiRequest, originalVisibility); if (isApiRequest) { @@ -203,7 +206,7 @@ public Funding updateFunding(String orcid, Funding funding, boolean isApiRequest } // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, pfe); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, pfe); // Updates the give organization with the latest organization from // database, or, create a new one diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileKeywordManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileKeywordManagerImpl.java index 9eea8781519..e75a0f3db82 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileKeywordManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ProfileKeywordManagerImpl.java @@ -40,6 +40,9 @@ public class ProfileKeywordManagerImpl extends ProfileKeywordManagerReadOnlyImpl @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public boolean deleteKeyword(String orcid, Long putCode, boolean checkSource) { ProfileKeywordEntity entity = profileKeywordDao.getProfileKeyword(orcid, putCode); @@ -77,7 +80,7 @@ public Keyword createKeyword(String orcid, Keyword keyword, boolean isApiRequest newEntity.setOrcid(orcid); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); setIncomingPrivacy(newEntity, profile); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(newEntity, isApiRequest); @@ -93,7 +96,7 @@ public Keyword updateKeyword(String orcid, Long putCode, Keyword keyword, boolea Visibility originalVisibility = Visibility.valueOf(updatedEntity.getVisibility()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(updatedEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(updatedEntity); // Validate the keyword PersonValidator.validateKeyword(keyword, activeSource, false, isApiRequest, originalVisibility); @@ -113,7 +116,7 @@ public Keyword updateKeyword(String orcid, Long putCode, Keyword keyword, boolea adapter.toProfileKeywordEntity(keyword, updatedEntity); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedEntity); profileKeywordDao.merge(updatedEntity); return adapter.toKeyword(updatedEntity); @@ -162,7 +165,7 @@ public Keywords updateKeywords(String orcid, Keywords keywords) { Source activeSource = sourceManager.retrieveActiveSource(); newKeyword.setOrcid(orcid); // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newKeyword); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newKeyword); newKeyword.setVisibility(updatedOrNew.getVisibility().name()); newKeyword.setDisplayIndex(updatedOrNew.getDisplayIndex()); profileKeywordDao.persist(newKeyword); @@ -177,7 +180,7 @@ public Keywords updateKeywords(String orcid, Keywords keywords) { private boolean isDuplicated(ProfileKeywordEntity existing, org.orcid.jaxb.model.v3.release.record.Keyword keyword, Source activeSource) { if (!existing.getId().equals(keyword.getPutCode())) { String existingSourceId = existing.getElementSourceId(); - if (!PojoUtil.isEmpty(existingSourceId) && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existing, clientDetailsEntityCacheManager)) { + if (!PojoUtil.isEmpty(existingSourceId) && sourceEntityUtils.isTheSameSource(activeSource, existing)) { if (existing.getKeywordName() != null && existing.getKeywordName().equals(keyword.getContent())) { return true; } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearchResourceManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearchResourceManagerImpl.java index 457cb357d48..133091b8592 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearchResourceManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearchResourceManagerImpl.java @@ -59,6 +59,9 @@ public class ResearchResourceManagerImpl extends ResearchResourceManagerReadOnly @Resource(name = "notificationManagerV3") private NotificationManager notificationManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override @Transactional public ResearchResource createResearchResource(String orcid, ResearchResource rr, boolean isApiRequest) { @@ -88,7 +91,7 @@ public ResearchResource createResearchResource(String orcid, ResearchResource rr } // Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, researchResourceEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, researchResourceEntity); ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); setIncomingPrivacy(researchResourceEntity, profile); @@ -108,7 +111,7 @@ public ResearchResource updateResearchResource(String orcid, ResearchResource rr Visibility originalVisibility = Visibility.valueOf(rre.getVisibility()); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(rre, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(rre); activityValidator.validateResearchResource(rr, activeSource, false, isApiRequest, originalVisibility); if (!isApiRequest) { @@ -127,7 +130,7 @@ public ResearchResource updateResearchResource(String orcid, ResearchResource rr rre.setVisibility(originalVisibility.name()); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, rre); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, rre); // update orgs (ordering managed by @OrderColumn on lists) List updatedOrganizations = orgManager.getOrgEntities(rr.getProposal().getHosts()); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearcherUrlManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearcherUrlManagerImpl.java index f99d412b013..fbe42ccbb0a 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearcherUrlManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/ResearcherUrlManagerImpl.java @@ -44,6 +44,9 @@ public class ResearcherUrlManagerImpl extends ResearcherUrlManagerReadOnlyImpl i @Resource private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Override public boolean deleteResearcherUrl(String orcid, Long id, boolean checkSource) { boolean result = true; @@ -113,7 +116,7 @@ public ResearcherUrls updateResearcherUrls(String orcid, ResearcherUrls research ProfileEntity profile = new ProfileEntity(orcid); newResearcherUrl.setOrcid(orcid); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newResearcherUrl); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newResearcherUrl); newResearcherUrl.setVisibility(updatedOrNew.getVisibility().name()); newResearcherUrl.setDisplayIndex(updatedOrNew.getDisplayIndex()); @@ -133,7 +136,7 @@ public ResearcherUrl updateResearcherUrl(String orcid, ResearcherUrl researcherU Source activeSource = sourceManager.retrieveActiveSource(); // Save the original source - Source originalSource = SourceEntityUtils.extractSourceFromEntity(updatedResearcherUrlEntity, clientDetailsEntityCacheManager); + Source originalSource = sourceEntityUtils.extractSourceFromEntity(updatedResearcherUrlEntity); // Validate the researcher url PersonValidator.validateResearcherUrl(researcherUrl, activeSource, false, isApiRequest, originalVisibility); @@ -153,7 +156,7 @@ public ResearcherUrl updateResearcherUrl(String orcid, ResearcherUrl researcherU jpaJaxbResearcherUrlAdapter.toResearcherUrlEntity(researcherUrl, updatedResearcherUrlEntity); // Be sure it doesn't overwrite the source - SourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedResearcherUrlEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(originalSource, updatedResearcherUrlEntity); researcherUrlDao.merge(updatedResearcherUrlEntity); return jpaJaxbResearcherUrlAdapter.toResearcherUrl(updatedResearcherUrlEntity); @@ -179,7 +182,7 @@ public ResearcherUrl createResearcherUrl(String orcid, ResearcherUrl researcherU ProfileEntity profile = profileEntityCacheManager.retrieve(orcid); newEntity.setOrcid(orcid); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, newEntity); setIncomingPrivacy(newEntity, profile); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(newEntity, isApiRequest); @@ -192,7 +195,7 @@ private boolean isDuplicated(ResearcherUrlEntity existing, ResearcherUrl newRese // If they have the same source String existingSourceId = existing.getElementSourceId(); // If they have the same source - if (!PojoUtil.isEmpty(existingSourceId) && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existing, clientDetailsEntityCacheManager)) { + if (!PojoUtil.isEmpty(existingSourceId) && sourceEntityUtils.isTheSameSource(activeSource, existing)) { // If the url is the same if (existing.getUrl() != null && existing.getUrl().equals(newResearcherUrl.getUrl().getValue())) { return true; diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/WorkManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/WorkManagerImpl.java index d48199e9feb..097b5294114 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/WorkManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/impl/WorkManagerImpl.java @@ -88,12 +88,6 @@ public class WorkManagerImpl extends WorkManagerReadOnlyImpl implements WorkMana @Resource(name = "activityValidatorV3") private ActivityValidator activityValidator; - @Resource(name = "groupingSuggestionManagerV3") - private GroupingSuggestionManager groupingSuggestionManager; - - @Resource(name = "groupingSuggestionManagerReadOnlyV3") - private GroupingSuggestionManagerReadOnly groupingSuggestionManagerReadOnly; - @Resource private MessageSource messageSource; @@ -115,6 +109,9 @@ public class WorkManagerImpl extends WorkManagerReadOnlyImpl implements WorkMana @Resource private WorkContributorRoleConverter workContributorsRoleConverter; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Value("${org.orcid.core.work.contributors.ui.max:50}") private int maxContributorsForUI; @@ -213,7 +210,7 @@ public Work createWork(String orcid, Work work, boolean isApiRequest) { workEntity.setFeaturedDisplayIndex(0); //Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); setIncomingWorkPrivacy(workEntity, profile, isApiRequest); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(workEntity, isApiRequest); @@ -273,7 +270,7 @@ public WorkBulk createWorks(String orcid, WorkBulk workBulk) { // If the external id exists and is a SELF identifier, then mark it as duplicated if(existingExternalIdentifiers.contains(extId) && Relationship.SELF.equals(extId.getRelationship())) { Map params = new HashMap(); - params.put("clientName", SourceEntityUtils.getSourceName(activeSource)); + params.put("clientName", sourceEntityUtils.getSourceName(activeSource)); if(extIDPutCodeMap.containsKey(extId)) { params.put("putCode", String.valueOf(extIDPutCodeMap.get(extId))); } @@ -287,7 +284,7 @@ public WorkBulk createWorks(String orcid, WorkBulk workBulk) { workEntity.setAddedToProfileDate(new Date()); workEntity.setFeaturedDisplayIndex(0); - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); setIncomingWorkPrivacy(workEntity, profile); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(workEntity, true); @@ -332,7 +329,7 @@ private Set buildExistingExternalIdsSet(List existingWorks, So Set existingExternalIds = new HashSet(); for(Work work : existingWorks) { //If it is the same source - if(SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, work.getSource())) { + if(sourceEntityUtils.isTheSameSource(activeSource, work.getSource())) { if(work.getExternalIdentifiers() != null && work.getExternalIdentifiers().getExternalIdentifier() != null) { for(ExternalID extId : work.getExternalIdentifiers().getExternalIdentifier()) { if(extIdsPutCodeMap != null) { @@ -547,7 +544,7 @@ public Work createWork(String orcid, WorkForm workForm) { workEntity.setFeaturedDisplayIndex(0); //Set the source - SourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); + sourceEntityUtils.populateSourceAwareEntityFromSource(activeSource, workEntity); setIncomingWorkPrivacy(workEntity, profile, false); DisplayIndexCalculatorHelper.setDisplayIndexOnNewEntity(workEntity, false); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/RecordManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/RecordManagerReadOnlyImpl.java index efb9a5bcba7..768d210270b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/RecordManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/RecordManagerReadOnlyImpl.java @@ -43,7 +43,10 @@ public class RecordManagerReadOnlyImpl implements RecordManagerReadOnly { @Resource protected SourceNameCacheManager sourceNameCacheManager; - + + @Resource + private SourceEntityUtils sourceEntityUtils; + protected ProfileEntityCacheManager profileEntityCacheManager; protected EmailManagerReadOnly emailManager; @@ -146,7 +149,7 @@ private History getHistory(String orcid) { } if(profile.getSource() != null) { - Source source = SourceEntityUtils.extractSourceFromProfileComplete(profile, sourceNameCacheManager, orcidUrlManager); + Source source = sourceEntityUtils.extractSourceFromProfileComplete(profile); history.setSource(source); } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index b46b00c0ee7..c903a01db6b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -92,18 +92,12 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements @Resource(name = "contributorUtilsV3") private ContributorUtils contributorUtils; - @Resource - private WorkContributorRoleConverter workContributorsRoleConverter; - @Resource(name = "workContributorsConverter") private WorkContributorsConverter workContributorsConverter; @Resource private JSONWorkExternalIdentifiersConverterV3 jsonWorkExternalIdentifiersConverterV3; - @Resource(name = "clientDetailsManagerReadOnlyV3") - private ClientDetailsManagerReadOnly clientDetailsManagerReadOnly; - @Resource protected ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; @@ -116,6 +110,9 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements @Resource private OrcidUrlManager orcidUrlManager; + @Resource + private SourceEntityUtils sourceEntityUtils; + @Value("${org.orcid.core.work.contributors.ui.max:50}") private int maxContributorsForUI; @@ -206,32 +203,23 @@ public List getWorksSummaryList(String orcid) { .filter(clientId -> !PojoUtil.isEmpty(clientId)) .collect(Collectors.toSet()); - System.out.println("Generating source list"); - long start = System.currentTimeMillis(); - long s0 = System.currentTimeMillis(); // Get the client details from the database Map clientDetailsById = clientDetailsEntityCacheManager.retrieveAll(clientIds); Map sources = new HashMap<>(); works.stream().forEach(workEntity -> { - String sourceKey = SourceEntityUtils.getSourceKey(workEntity); + String sourceKey = sourceEntityUtils.getSourceKey(workEntity); if(!sources.containsKey(sourceKey)) { - Source source = SourceEntityUtils.extractSourceFromEntityComplete(workEntity, sourceNameCacheManager, orcidUrlManager, clientDetailsEntityCacheManager, - clientDetailsById); + Source source = sourceEntityUtils.extractSourceFromEntityComplete(workEntity); sources.put(sourceKey, source); } }); - long end = System.currentTimeMillis(); - System.out.println("Time take generating source list: " + (end - start)); if (clientIds.isEmpty()) { return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works); } // This map should be read-only Map readOnlySources = Collections.unmodifiableMap(sources); - List summaryList = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); - long s1 = System.currentTimeMillis(); - System.out.println("Total time: " + (s1 - s0)); - return summaryList; + return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); } /** diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java index 8f310d1a961..ed25da432e3 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java @@ -286,7 +286,7 @@ public void validateWork(Work work, Source activeSource, boolean createFlag, boo } if (work.getPutCode() != null && createFlag) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(sourceEntityUtils.getSourceName(activeSource)); } // Check that we are not changing the visibility @@ -340,7 +340,7 @@ public void validateFunding(Funding funding, Source activeSource, boolean create } if (funding.getPutCode() != null && createFlag) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(sourceEntityUtils.getSourceName(activeSource)); } if (isApiRequest) { @@ -447,7 +447,7 @@ private void validateDisambiguatedOrg(MultipleOrganizationHolder organizationHol public void validateAffiliation(Affiliation affiliation, Source activeSource, boolean createFlag, boolean isApiRequest, Visibility originalVisibility) { if (affiliation.getPutCode() != null && createFlag) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(sourceEntityUtils.getSourceName(activeSource)); } // Check that we are not changing the visibility @@ -486,7 +486,7 @@ public void validatePeerReview(PeerReview peerReview, Source activeSource, boole } if (peerReview.getPutCode() != null && createFlag) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(sourceEntityUtils.getSourceName(activeSource)); } if (peerReview.getType() == null) { @@ -555,9 +555,9 @@ public void checkExternalIdentifiersForDuplicates(ExternalIdentifiersAwareActivi existingId.setNormalized(new TransientNonEmptyString(norm.normalise(existingId.getType(), existingId.getValue()))); } if (areRelationshipsSameAndSelf(existingId.getRelationship(), newId.getRelationship()) && newId.equals(existingId) - && SourceEntityUtils.isTheSameForDuplicateChecking(activeSource, existingSource)) { + && sourceEntityUtils.isTheSameSource(activeSource, existingSource)) { Map params = new HashMap(); - params.put("clientName", SourceEntityUtils.getSourceName(activeSource)); + params.put("clientName", sourceEntityUtils.getSourceName(activeSource)); params.put("putCode", String.valueOf(theExisting.getPutCode())); throw new OrcidDuplicatedActivityException(params); } @@ -607,7 +607,7 @@ public void validateResearchResource(ResearchResource rr, Source activeSource, b } if (rr.getPutCode() != null && createFlag) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(sourceEntityUtils.getSourceName(activeSource)); } // Check that we are not changing the visibility diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/PersonValidator.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/PersonValidator.java index fd30b6de1e1..a19335dfc79 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/PersonValidator.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/PersonValidator.java @@ -7,6 +7,7 @@ import org.orcid.core.exception.OrcidValidationException; import org.orcid.core.exception.PutCodeRequiredException; import org.orcid.core.exception.VisibilityMismatchException; +import org.orcid.core.utils.SourceEntityUtils; import org.orcid.jaxb.model.common.Relationship; import org.orcid.jaxb.model.v3.release.common.Source; import org.orcid.jaxb.model.v3.release.common.Visibility; @@ -21,10 +22,12 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Resource; + public class PersonValidator { private static final Logger LOGGER = LoggerFactory.getLogger(PersonValidator.class); - + public static void validateResearcherUrl(ResearcherUrl researcherUrl, Source activeSource, boolean createFlag, boolean isApiRequest, Visibility originalVisibility) { if(researcherUrl == null) return; @@ -50,7 +53,7 @@ public static void validateResearcherUrl(ResearcherUrl researcherUrl, Source act } if(createFlag) { if(researcherUrl.getPutCode() != null) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(SourceEntityUtils.getSourceName(activeSource)); } } else { if(researcherUrl.getPutCode() == null) { @@ -66,7 +69,7 @@ public static void validateResearcherUrl(ResearcherUrl researcherUrl, Source act public static void validateOtherName(OtherName otherName, Source activeSource, boolean createFlag, boolean isApiRequest, Visibility originalVisibility) { if(createFlag) { if(otherName.getPutCode() != null) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(SourceEntityUtils.getSourceName(activeSource)); } } else { if(otherName.getPutCode() == null) { @@ -128,7 +131,7 @@ public static void validateExternalIdentifier(PersonExternalIdentifier externalI if(createFlag) { if(externalIdentifier.getPutCode() != null) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(SourceEntityUtils.getSourceName(activeSource)); } } else { if(externalIdentifier.getPutCode() == null) { @@ -144,7 +147,7 @@ public static void validateExternalIdentifier(PersonExternalIdentifier externalI public static void validateKeyword(Keyword keyword, Source activeSource, boolean createFlag, boolean isApiRequest, Visibility originalVisibility) { if(createFlag) { if(keyword.getPutCode() != null) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(SourceEntityUtils.getSourceName(activeSource)); } } else { if(keyword.getPutCode() == null) { @@ -166,7 +169,7 @@ public static void validateKeyword(Keyword keyword, Source activeSource, boolean public static void validateAddress(Address address, Source activeSource, boolean createFlag, boolean isApiRequest, Visibility originalVisibility) { if(createFlag) { if(address.getPutCode() != null) { - throw InvalidPutCodeException.forSource(activeSource); + throw InvalidPutCodeException.forSource(SourceEntityUtils.getSourceName(activeSource)); } } else { if(address.getPutCode() == null) { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/validator/ActivityValidator.java b/orcid-core/src/main/java/org/orcid/core/manager/validator/ActivityValidator.java index f4e86b931f0..c56c74b7919 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/validator/ActivityValidator.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/validator/ActivityValidator.java @@ -470,7 +470,7 @@ public void checkExternalIdentifiersForDuplicates(ExternalIdentifiersAwareActivi for (ExternalID existingId : existingExtIds.getExternalIdentifier()) { for (ExternalID newId : newExtIds.getExternalIdentifier()) { if (areRelationshipsSameButNotBothPartOf(existingId.getRelationship(), newId.getRelationship()) && newId.equals(existingId) - && SourceEntityUtils.getSourceId(sourceEntity).equals(getExistingSource(existingSource))) { + && sourceEntityUtils.getSourceId(sourceEntity).equals(getExistingSource(existingSource))) { Map params = new HashMap(); params.put("clientName", sourceEntityUtils.getSourceName(sourceEntity)); params.put("putCode", String.valueOf(theExisting.getPutCode())); diff --git a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java index 3af15a714f5..94e61c8fa6a 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java @@ -27,30 +27,53 @@ public class SourceEntityUtils { @Resource(name = "recordNameManagerReadOnlyV3") private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; - public String getSourceName(SourceEntity sourceEntity) { - if (sourceEntity.getCachedSourceName() != null) { - return sourceEntity.getCachedSourceName(); + @Resource(name = "sourceNameCacheManager") + private SourceNameCacheManager sourceNameCacheManager; + + @Resource + private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + + @Resource + private OrcidUrlManager orcidUrlManager; + + public static String getSourceId(SourceEntity sourceEntity) { + if (sourceEntity.getCachedSourceId() != null) { + return sourceEntity.getCachedSourceId(); } if (sourceEntity.getSourceClient() != null) { - return sourceEntity.getSourceClient().getClientName(); + return sourceEntity.getSourceClient().getClientId(); } if (sourceEntity.getSourceProfile() != null) { - String orcid = sourceEntity.getSourceProfile().getId(); - // Set the source name - return recordNameManagerReadOnlyV3.fetchDisplayablePublicName(orcid); + return sourceEntity.getSourceProfile().getId(); } return null; } - public static String getSourceId(SourceEntity sourceEntity) { - if (sourceEntity.getCachedSourceId() != null) { - return sourceEntity.getCachedSourceId(); + public static String getSourceName(Source activeSource) { + if (activeSource.getSourceName() != null && !StringUtils.isEmpty(activeSource.getSourceName().getContent())) + return activeSource.getSourceName().getContent(); + else + return null; + } + + public static String getSourceKey(SourceAwareEntity e) { + String sourceOrcid = Optional.ofNullable(e.getSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + String clientSourceId = Optional.ofNullable(e.getClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + String assertionOriginClientSourceId = Optional.ofNullable(e.getAssertionOriginClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); + return String.join("|", sourceOrcid, clientSourceId, assertionOriginClientSourceId); + } + + public String getSourceName(SourceEntity sourceEntity) { + if (sourceEntity.getCachedSourceName() != null) { + return sourceEntity.getCachedSourceName(); } if (sourceEntity.getSourceClient() != null) { - return sourceEntity.getSourceClient().getClientId(); + return sourceEntity.getSourceClient().getClientName(); } if (sourceEntity.getSourceProfile() != null) { - return sourceEntity.getSourceProfile().getId(); + String orcid = sourceEntity.getSourceProfile().getId(); + // Set the source name + return recordNameManagerReadOnlyV3.fetchDisplayablePublicName(orcid); } return null; } @@ -63,7 +86,7 @@ public static String getSourceId(SourceEntity sourceEntity) { * @param to */ @SuppressWarnings("deprecation") - public static void populateSourceAwareEntityFromSource(Source from, SourceAwareEntity to) { + public void populateSourceAwareEntityFromSource(Source from, SourceAwareEntity to) { // Set the source if (from.getSourceOrcid() != null && from.getSourceOrcid().getPath() != null) { to.setSourceId(from.getSourceOrcid().getPath()); @@ -79,14 +102,8 @@ public static void populateSourceAwareEntityFromSource(Source from, SourceAwareE /** * Utility that copies source ids from entity into new Source model. - * */ - public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager) { - return extractSourceFromEntity(e, clientDetailsEntityCacheManager, null); - } - - public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, - Map clientDetailsById) { + public Source extractSourceFromEntity(SourceAwareEntity e) { Source source = new Source(); // orcid if (!StringUtils.isEmpty(e.getSourceId())) { @@ -97,12 +114,9 @@ public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetai if (!StringUtils.isEmpty(e.getClientSourceId())) { source.setSourceClientId(new SourceClientId(e.getClientSourceId())); if(e instanceof OrcidAware) { - ClientDetailsEntity clientSource = getClientSource(e.getClientSourceId(), clientDetailsEntityCacheManager, clientDetailsById); + ClientDetailsEntity clientSource = clientDetailsEntityCacheManager.retrieve(e.getClientSourceId()); if (clientSource.isUserOBOEnabled()) { - String orcidId = null; - if (e instanceof OrcidAware) { - orcidId = ((OrcidAware) e).getOrcid(); - } + String orcidId = ((OrcidAware) e).getOrcid(); source.setAssertionOriginOrcid(new SourceOrcid(orcidId)); } } @@ -116,13 +130,7 @@ public static Source extractSourceFromEntity(SourceAwareEntity e, ClientDetai return source; } - private static ClientDetailsEntity getClientSource(String clientSourceId, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, - Map clientDetailsById) { - ClientDetailsEntity clientSource = clientDetailsById == null ? null : clientDetailsById.get(clientSourceId); - return clientSource == null ? clientDetailsEntityCacheManager.retrieve(clientSourceId) : clientSource; - } - - public static Source extractSourceFromProfileComplete(ProfileEntity profile, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager) { + public Source extractSourceFromProfileComplete(ProfileEntity profile) { Source source = new Source(); SourceEntity entity = profile.getSource(); if (entity.getSourceProfile() != null) { @@ -131,18 +139,17 @@ public static Source extractSourceFromProfileComplete(ProfileEntity profile, Sou if (entity.getSourceClient() != null) { source.setSourceClientId(new SourceClientId(entity.getSourceClient().getId())); } - populateSource(source, sourceNameCacheManager, orcidUrlManager); + populateSource(source); return source; } - public static Source extractSourceFromEntityComplete(SourceAwareEntity b, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager, - ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, Map clientDetailsById) { - Source s = extractSourceFromEntity(b, clientDetailsEntityCacheManager, clientDetailsById); - populateSource(s, sourceNameCacheManager, orcidUrlManager); + public Source extractSourceFromEntityComplete(SourceAwareEntity b) { + Source s = extractSourceFromEntity(b); + populateSource(s); return s; } - public static void populateSource(Source s, SourceNameCacheManager sourceNameCacheManager, OrcidUrlManager orcidUrlManager) { + private void populateSource(Source s) { // Set the source if (s.getSourceOrcid() != null && s.getSourceOrcid().getPath() != null) { s.getSourceOrcid().setHost(orcidUrlManager.getBaseHost()); @@ -182,54 +189,12 @@ public static void populateSource(Source s, SourceNameCacheManager sourceNameCac // ================================= // utils to help refactoring for OBO // ================================= - - /** - * Used to check for duplicates adding via API. - * - * @param active - * @param existing - * @return - */ - public static boolean isTheSameForDuplicateChecking(Source activeSource, SourceAwareEntity existingEntity, - ClientDetailsEntityCacheManager clientDetailsEntityCacheManager) { - Source existing = extractSourceFromEntity(existingEntity, clientDetailsEntityCacheManager); - return existing.equals(activeSource); - } - - public static boolean isTheSameForDuplicateChecking(Source active, Source existing) { + public boolean isTheSameSource(Source active, Source existing) { return existing.equals(active); } - /** - * Used only for errors when validating I think... - * - * @param activeSource - * @return - */ - public static String getSourceName(Source activeSource) { - if (activeSource.getSourceName() != null && !StringUtils.isEmpty(activeSource.getSourceName().getContent())) - return activeSource.getSourceName().getContent(); - else - return null; - } - - /** - * Used to check if activeSource can update/delete an item. - * - * @param activeSource - * @param existingEntity - * @return - */ - public static boolean isTheSameForPermissionChecking(Source activeSource, SourceAwareEntity existingEntity, - ClientDetailsEntityCacheManager clientDetailsEntityCacheManager) { - Source existing = extractSourceFromEntity(existingEntity, clientDetailsEntityCacheManager); - return existing.equals(activeSource); - } - - public static String getSourceKey(SourceAwareEntity e) { - String sourceOrcid = Optional.ofNullable(e.getSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); - String clientSourceId = Optional.ofNullable(e.getClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); - String assertionOriginClientSourceId = Optional.ofNullable(e.getAssertionOriginClientSourceId()).filter(StringUtils::isNotEmpty).orElse("-"); - return String.join("|", sourceOrcid, clientSourceId, assertionOriginClientSourceId); + public boolean isTheSameSource(Source active, SourceAwareEntity existingEntity) { + Source existing = extractSourceFromEntity(existingEntity); + return existing.equals(active); } } From 0cae180bc9a82356c8609bf1f55cd3d2d952245d Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 25 May 2026 16:29:51 -0600 Subject: [PATCH 06/15] Improvements in the way we load the source names on retrieveWorkSummaryExtended --- ...ontributorsRolesAndSequencesConverter.java | 13 +- .../adapter/v3/impl/MapperFacadeFactory.java | 12 +- .../orcid/core/cli/FilterTopContributors.java | 2 +- .../impl/WorkManagerReadOnlyImpl.java | 49 +++- .../orcid/core/utils/SourceEntityUtils.java | 2 +- .../orcid/core/utils/v3/ContributorUtils.java | 68 ++--- .../pojo/ContributorsRolesAndSequences.java | 3 +- .../src/main/resources/orcid-core-context.xml | 18 +- .../core/utils/SourceEntityUtilsTest.java | 266 ++++++++++++++++-- .../core/utils/v3/ContributorUtilsTest.java | 3 - .../resources/orcid-core-context-spam.xml | 10 +- 11 files changed, 317 insertions(+), 129 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/converter/ContributorsRolesAndSequencesConverter.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/converter/ContributorsRolesAndSequencesConverter.java index be6df9376f1..9f79a278390 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/converter/ContributorsRolesAndSequencesConverter.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/converter/ContributorsRolesAndSequencesConverter.java @@ -6,6 +6,7 @@ import ma.glasnost.orika.converter.BidirectionalConverter; import ma.glasnost.orika.metadata.Type; import org.orcid.core.contributors.roles.ContributorRoleConverter; +import org.orcid.core.contributors.roles.ContributorRoleConverterImpl; import org.orcid.core.contributors.roles.credit.CreditRole; import org.orcid.core.utils.JsonUtils; import org.orcid.core.utils.v3.ContributorUtils; @@ -15,6 +16,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Resource; import java.util.ArrayList; import java.util.List; @@ -22,11 +24,11 @@ public class ContributorsRolesAndSequencesConverter extends BidirectionalConvert private static final Logger LOGGER = LoggerFactory.getLogger(ContributorsRolesAndSequencesConverter.class); - private ContributorRoleConverter roleConverter; + @Resource(name = "workContributorRoleConverter") + private ContributorRoleConverter workContributorRoleConverter; - public ContributorsRolesAndSequencesConverter(ContributorRoleConverter roleConverter) { - this.roleConverter = roleConverter; - } + @Resource(name = "contributorUtilsV3") + private ContributorUtils contributorUtils; @Override public String convertTo(List source, Type destinationType) { @@ -39,7 +41,6 @@ public List convertFrom(String source, Type getContributorsRolesAndSequencesList(String source) { - ContributorUtils contributorUtils = new ContributorUtils(null); final ObjectMapper objectMapper = new ObjectMapper(); List contributorsRolesAndSequencesResult = new ArrayList<>(); try { @@ -54,7 +55,7 @@ public List getContributorsRolesAndSequencesList( if (cr != null) { providedRoleValue = cr.name(); } - crs.setContributorRole(contributorUtils.getCreditRole(roleConverter.toRoleValue(providedRoleValue))); + crs.setContributorRole(contributorUtils.getCreditRole(workContributorRoleConverter.toRoleValue(providedRoleValue))); } } } diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java index 8bf575cbf87..033d39781d7 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java @@ -170,13 +170,6 @@ public class MapperFacadeFactory implements FactoryBean { @Resource private WorkDao workDao; - @Resource - private SourceNameCacheManager sourceNameCacheManager; - - @Resource - private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; - - @Resource private IdentityProviderManager identityProviderManager; @@ -201,6 +194,9 @@ public class MapperFacadeFactory implements FactoryBean { @Resource private SourceEntityUtils sourceEntityUtils; + @Resource + private ContributorsRolesAndSequencesConverter contributorsRolesAndSequencesConverter; + @Override public MapperFacade getObject() throws Exception { MapperFactory mapperFactory = new DefaultMapperFactory.Builder().build(); @@ -546,8 +542,6 @@ public MapperFacade getWorkMapperFacade() { MapperFactory mapperFactory = new DefaultMapperFactory.Builder().build(); WorkContributorsConverter wcc = new WorkContributorsConverter(workContributorsRoleConverter); - ContributorsRolesAndSequencesConverter contributorsRolesAndSequencesConverter = new ContributorsRolesAndSequencesConverter(workContributorsRoleConverter); - ConverterFactory converterFactory = mapperFactory.getConverterFactory(); converterFactory.registerConverter("workExternalIdentifiersConverterId", new JSONWorkExternalIdentifiersConverterV3(norm, resolverService, localeManager)); converterFactory.registerConverter("workContributorsConverterId", wcc); diff --git a/orcid-core/src/main/java/org/orcid/core/cli/FilterTopContributors.java b/orcid-core/src/main/java/org/orcid/core/cli/FilterTopContributors.java index e8614a69c1b..b26492c6f45 100644 --- a/orcid-core/src/main/java/org/orcid/core/cli/FilterTopContributors.java +++ b/orcid-core/src/main/java/org/orcid/core/cli/FilterTopContributors.java @@ -65,7 +65,7 @@ private void filter() { private void filterTopContributors(Object[] workObject) { WorkEntity workEntity = workDao.find(((BigInteger) workObject[0]).longValue()); - ContributorUtils contributorUtils = new ContributorUtils(0); + ContributorUtils contributorUtils = new ContributorUtils(); WorkSummaryExtended wse = new WorkSummaryExtended.WorkSummaryExtendedBuilder(((BigInteger) workObject[0])) .contributors(workContributorsConverter.getContributorsList(isEmpty(workObject[1]))) .build(); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index c903a01db6b..42c3341db0a 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -8,6 +8,7 @@ import javax.annotation.Resource; +import org.apache.commons.lang3.StringUtils; import org.orcid.core.adapter.jsonidentifier.converter.JSONWorkExternalIdentifiersConverterV3; import org.orcid.core.adapter.v3.JpaJaxbWorkAdapter; import org.orcid.core.adapter.v3.converter.ContributorsRolesAndSequencesConverter; @@ -107,9 +108,6 @@ public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements @Resource private ContributorsRolesAndSequencesConverter contributorsRolesAndSequencesConverter; - @Resource - private OrcidUrlManager orcidUrlManager; - @Resource private SourceEntityUtils sourceEntityUtils; @@ -198,6 +196,7 @@ public WorkSummary getWorkSummary(String orcid, Long workId) { @Override public List getWorksSummaryList(String orcid) { List works = workEntityCacheManager.retrieveMinimizedWorks(orcid, getLastModified(orcid)); + long t0 = System.currentTimeMillis(); Set clientIds = works.stream() .map(MinimizedWorkEntity::getClientSourceId) .filter(clientId -> !PojoUtil.isEmpty(clientId)) @@ -219,7 +218,10 @@ public List getWorksSummaryList(String orcid) { // This map should be read-only Map readOnlySources = Collections.unmodifiableMap(sources); - return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); + List list = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); + long t1 = System.currentTimeMillis(); + System.out.println("Time to convert to JAXB WorkSummary: " + (t1 - t0)); + return list; } /** @@ -248,6 +250,8 @@ public List getWorksSummaryExtendedList(String orcid, boole private List retrieveWorkSummaryExtended(String orcid, boolean featuredOnly) { List workSummaryExtendedList = new ArrayList<>(); + Map isUserOBOEnabled = new HashMap(); + long l0 = System.currentTimeMillis(); List list = workDao.getWorksByOrcid(orcid, featuredOnly); for (Object[] q1 : list) { BigInteger putCode = (BigInteger) q1[0]; @@ -274,21 +278,34 @@ private List retrieveWorkSummaryExtended(String orcid, bool } String sourceName = null; String assertionOriginName = null; - if (clientSourceId != null) { - assertionOriginSourceId = contributorUtils.getAssertionOriginOrcid(clientSourceId, orcid, putCode.longValue(), clientDetailsEntityCacheManager, workDao); + if (StringUtils.isNotBlank(clientSourceId)) { + //Set the source name + sourceName = sourceNameCacheManager.retrieve(clientSourceId); + // Check if user OBO is enabled + if (!PojoUtil.isEmpty(assertionOriginSourceId)) { + if(!isUserOBOEnabled.containsKey(clientSourceId)) { + ClientDetailsEntity clientEntity = clientDetailsEntityCacheManager.retrieve(clientSourceId); + if(clientEntity != null && clientEntity.isUserOBOEnabled()) { + isUserOBOEnabled.put(clientSourceId, true); + } else { + isUserOBOEnabled.put(clientSourceId, false); + } + } + if(isUserOBOEnabled.get(clientSourceId)) { + assertionOriginName = sourceNameCacheManager.retrieve(assertionOriginSourceId); + } + } } - if (!PojoUtil.isEmpty(assertionOriginSourceId)) { - assertionOriginName = contributorUtils.getSourceName(assertionOriginSourceId, sourceNameCacheManager); + + // Check the sourceId name only if there is no clientSourceId + if (PojoUtil.isEmpty(sourceName) && !PojoUtil.isEmpty(sourceId)) { + sourceName = sourceNameCacheManager.retrieve(sourceId); } + if (!PojoUtil.isEmpty(assertionOriginClientSourceId)) { - assertionOriginName = contributorUtils.getSourceName(assertionOriginClientSourceId, sourceNameCacheManager); - } - if (!PojoUtil.isEmpty(sourceId)) { - sourceName = contributorUtils.getSourceName(sourceId, sourceNameCacheManager); - } - if (!PojoUtil.isEmpty(clientSourceId)) { - sourceName = contributorUtils.getSourceName(clientSourceId, sourceNameCacheManager); + assertionOriginName = sourceNameCacheManager.retrieve(assertionOriginClientSourceId); } + List contributorList = new ArrayList<>(); List contributorsRolesAndSequencesList = new ArrayList<>(); @@ -306,6 +323,8 @@ private List retrieveWorkSummaryExtended(String orcid, bool .build(); workSummaryExtendedList.add(wse); } + long l1 = System.currentTimeMillis(); + System.out.println("Time to retrieve WorkSummaryExtended: " + (l1 - l0)); return workSummaryExtendedList; } diff --git a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java index 94e61c8fa6a..fbd339afa55 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java @@ -115,7 +115,7 @@ public Source extractSourceFromEntity(SourceAwareEntity e) { source.setSourceClientId(new SourceClientId(e.getClientSourceId())); if(e instanceof OrcidAware) { ClientDetailsEntity clientSource = clientDetailsEntityCacheManager.retrieve(e.getClientSourceId()); - if (clientSource.isUserOBOEnabled()) { + if (clientSource != null && clientSource.isUserOBOEnabled()) { String orcidId = ((OrcidAware) e).getOrcid(); source.setAssertionOriginOrcid(new SourceOrcid(orcidId)); } diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java index cbabb769235..28326df093a 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java @@ -11,6 +11,8 @@ import org.orcid.core.manager.SourceNameCacheManager; import org.orcid.core.manager.v3.ActivityManager; import org.orcid.core.manager.v3.ProfileEntityManager; +import org.orcid.core.manager.v3.read_only.ProfileEntityManagerReadOnly; +import org.orcid.core.manager.v3.read_only.RecordNameManagerReadOnly; import org.orcid.jaxb.model.v3.release.common.Contributor; import org.orcid.jaxb.model.v3.release.common.ContributorAttributes; import org.orcid.jaxb.model.v3.release.common.CreditName; @@ -24,23 +26,21 @@ import org.orcid.pojo.ajaxForm.PojoUtil; import org.springframework.beans.factory.annotation.Value; +import javax.annotation.Resource; + public class ContributorUtils { - private final Integer BATCH_SIZE; - - private ActivityManager cacheManager; + @Resource(name = "recordNameManagerReadOnlyV3") + private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; - private ProfileEntityManager profileEntityManager; + @Resource(name = "profileEntityManagerV3") + private ProfileEntityManager profileEntityManagerV3; - protected ProfileLastModifiedAspect profileLastModifiedAspect; - - public ContributorUtils(@Value("${org.orcid.contributor.names.batch_size:2500}") Integer batchSize) { - if(batchSize == null) { - BATCH_SIZE = 2500; - } else { - BATCH_SIZE = batchSize; - } - } + @Resource + private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + + @Resource(name = "workDaoReadOnly") + private WorkDao workDaoReadOnly; public void filterContributorPrivateData(Funding funding) { if (funding.getContributors() != null && funding.getContributors().getContributor() != null) { @@ -48,10 +48,10 @@ public void filterContributorPrivateData(Funding funding) { contributor.setContributorEmail(null); if (!PojoUtil.isEmpty(contributor.getContributorOrcid())) { String contributorOrcid = contributor.getContributorOrcid().getPath(); - if (profileEntityManager.orcidExists(contributorOrcid)) { + if (profileEntityManagerV3.orcidExists(contributorOrcid)) { // contributor is an ORCID user - visibility of user's // name in record must be taken into account - String publicContributorCreditName = cacheManager.getPublicCreditName(contributorOrcid); + String publicContributorCreditName = recordNameManagerReadOnlyV3.fetchDisplayablePublicName(contributorOrcid); CreditName creditName = new CreditName(publicContributorCreditName != null ? publicContributorCreditName : ""); contributor.setCreditName(creditName); } @@ -60,18 +60,6 @@ public void filterContributorPrivateData(Funding funding) { } } - public void setCacheManager(ActivityManager cacheManager) { - this.cacheManager = cacheManager; - } - - public void setProfileEntityManager(ProfileEntityManager profileEntityManager) { - this.profileEntityManager = profileEntityManager; - } - - public void setProfileLastModifiedAspect(ProfileLastModifiedAspect profileLastModifiedAspect) { - this.profileLastModifiedAspect = profileLastModifiedAspect; - } - public List getContributorsGroupedByOrcid(List contributors, Integer maxContributorsForUI) { List contributorsRolesAndSequencesList = new ArrayList<>(); for(Contributor contributor : contributors) { @@ -156,32 +144,12 @@ private ContributorsRolesAndSequences addContributor(Contributor contributor) { return crs; } - public String getCreditRole(String contributorRole) { + public static String getCreditRole(String contributorRole) { try { - CreditRole cr = CreditRole.fromValue(contributorRole); - return cr.getUiValue(); - } catch(IllegalArgumentException e) { + return CreditRole.fromValue(contributorRole).getUiValue(); + } catch(Exception e) { return contributorRole; } } - public String getAssertionOriginOrcid(String clientSourceId, String orcid, Long putCode, ClientDetailsEntityCacheManager clientDetailsEntityCacheManager, WorkDao workDao) { - String assertionOriginOrcid = null; - ClientDetailsEntity clientSource = clientDetailsEntityCacheManager.retrieve(clientSourceId); - if (clientSource.isUserOBOEnabled()) { - WorkEntity e = workDao.getWork(orcid, putCode); - - String orcidId = null; - if (e instanceof OrcidAware) { - orcidId = ((OrcidAware) e).getOrcid(); - } - assertionOriginOrcid = orcidId; - } - - return assertionOriginOrcid; - } - - public String getSourceName(String sourceId, SourceNameCacheManager sourceNameCacheManager) { - return sourceNameCacheManager.retrieve(sourceId); - } } diff --git a/orcid-core/src/main/java/org/orcid/pojo/ContributorsRolesAndSequences.java b/orcid-core/src/main/java/org/orcid/pojo/ContributorsRolesAndSequences.java index ba9c9a59ade..b60fb8bdfee 100644 --- a/orcid-core/src/main/java/org/orcid/pojo/ContributorsRolesAndSequences.java +++ b/orcid-core/src/main/java/org/orcid/pojo/ContributorsRolesAndSequences.java @@ -59,10 +59,9 @@ public boolean compare(Object obj) { } AtomicBoolean isDifferent = new AtomicBoolean(false); WorkContributorRoleConverter roleConverter = new WorkContributorRoleConverter(); - ContributorUtils contributorUtils = new ContributorUtils(null); for (int i = 0; i < rolesAndSequences.size() ; i++) { if (rolesAndSequences.get(i).getContributorRole() != null && other.rolesAndSequences.get(i).getContributorRole() != null) { - if (!WorkForm.compareStrings(rolesAndSequences.get(i).getContributorRole(), contributorUtils.getCreditRole(roleConverter.toRoleValue(other.rolesAndSequences.get(i).getContributorRole())))) { + if (!WorkForm.compareStrings(rolesAndSequences.get(i).getContributorRole(), ContributorUtils.getCreditRole(roleConverter.toRoleValue(other.rolesAndSequences.get(i).getContributorRole())))) { isDifferent.set(true); break; } diff --git a/orcid-core/src/main/resources/orcid-core-context.xml b/orcid-core/src/main/resources/orcid-core-context.xml index 87edc0962fd..c4bac0e2ff0 100644 --- a/orcid-core/src/main/resources/orcid-core-context.xml +++ b/orcid-core/src/main/resources/orcid-core-context.xml @@ -413,19 +413,9 @@ - - - - - - - - - - - + - + @@ -1232,9 +1222,7 @@ - - - + diff --git a/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java b/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java index 22cb6b826c8..886dcb74b71 100644 --- a/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java +++ b/orcid-core/src/test/java/org/orcid/core/utils/SourceEntityUtilsTest.java @@ -1,35 +1,265 @@ package org.orcid.core.utils; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; - -import java.util.Collections; -import java.util.Date; -import java.util.Map; +import static org.mockito.Mockito.when; +import org.junit.Before; import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import org.orcid.core.manager.ClientDetailsEntityCacheManager; +import org.orcid.core.manager.SourceNameCacheManager; +import org.orcid.core.manager.impl.OrcidUrlManager; +import org.orcid.core.manager.v3.read_only.RecordNameManagerReadOnly; +import org.orcid.jaxb.model.v3.release.common.Source; +import org.orcid.jaxb.model.v3.release.common.SourceClientId; +import org.orcid.jaxb.model.v3.release.common.SourceName; +import org.orcid.jaxb.model.v3.release.common.SourceOrcid; +import org.orcid.persistence.jpa.entities.AddressEntity; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; -import org.orcid.persistence.jpa.entities.MinimizedWorkEntity; -import org.springframework.test.util.ReflectionTestUtils; +import org.orcid.persistence.jpa.entities.ProfileEntity; +import org.orcid.persistence.jpa.entities.SourceAwareEntity; +import org.orcid.persistence.jpa.entities.SourceEntity; public class SourceEntityUtilsTest { + @Mock + private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; + + @Mock + private SourceNameCacheManager sourceNameCacheManager; + + @Mock + private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; + + @Mock + private OrcidUrlManager orcidUrlManager; + + @InjectMocks + private SourceEntityUtils sourceEntityUtils; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + when(orcidUrlManager.getBaseHost()).thenReturn("orcid.org"); + when(orcidUrlManager.getBaseUrl()).thenReturn("https://orcid.org"); + } + + @Test + public void testGetSourceId() { + SourceEntity sourceEntity = new SourceEntity(); + + // Test cached + sourceEntity.setCachedSourceId("cached-id"); + assertEquals("cached-id", SourceEntityUtils.getSourceId(sourceEntity)); + + // Test client + sourceEntity.setCachedSourceId(null); + ClientDetailsEntity client = new ClientDetailsEntity("client-id"); + sourceEntity.setSourceClient(client); + assertEquals("client-id", SourceEntityUtils.getSourceId(sourceEntity)); + + // Test profile + sourceEntity.setSourceClient(null); + ProfileEntity profile = new ProfileEntity("orcid-id"); + sourceEntity.setSourceProfile(profile); + assertEquals("orcid-id", SourceEntityUtils.getSourceId(sourceEntity)); + + // Test null + sourceEntity.setSourceProfile(null); + assertNull(SourceEntityUtils.getSourceId(sourceEntity)); + } + + @Test + public void testGetSourceNameFromSource() { + Source source = new Source(); + assertNull(SourceEntityUtils.getSourceName(source)); + + source.setSourceName(new SourceName("Source Name")); + assertEquals("Source Name", SourceEntityUtils.getSourceName(source)); + + source.setSourceName(new SourceName("")); + assertNull(SourceEntityUtils.getSourceName(source)); + } + + @Test + public void testGetSourceKey() { + // Mocking SourceAwareEntity since it's abstract + SourceAwareEntity entity = mock(SourceAwareEntity.class); + when(entity.getSourceId()).thenReturn("orcid"); + when(entity.getClientSourceId()).thenReturn("client"); + when(entity.getAssertionOriginClientSourceId()).thenReturn("assertion"); + + assertEquals("orcid|client|assertion", SourceEntityUtils.getSourceKey(entity)); + + when(entity.getSourceId()).thenReturn(null); + when(entity.getClientSourceId()).thenReturn(""); + when(entity.getAssertionOriginClientSourceId()).thenReturn(null); + assertEquals("-|-|-", SourceEntityUtils.getSourceKey(entity)); + } + + @Test + public void testGetSourceNameFromEntity() { + SourceEntity sourceEntity = new SourceEntity(); + + // Test cached + sourceEntity.setCachedSourceName("cached-name"); + assertEquals("cached-name", sourceEntityUtils.getSourceName(sourceEntity)); + + // Test client + sourceEntity.setCachedSourceName(null); + ClientDetailsEntity client = new ClientDetailsEntity("client-id", "Client Name"); + sourceEntity.setSourceClient(client); + assertEquals("Client Name", sourceEntityUtils.getSourceName(sourceEntity)); + + // Test profile + sourceEntity.setSourceClient(null); + ProfileEntity profile = new ProfileEntity("orcid-id"); + sourceEntity.setSourceProfile(profile); + when(recordNameManagerReadOnlyV3.fetchDisplayablePublicName("orcid-id")).thenReturn("Profile Name"); + assertEquals("Profile Name", sourceEntityUtils.getSourceName(sourceEntity)); + + // Test null + sourceEntity.setSourceProfile(null); + assertNull(sourceEntityUtils.getSourceName(sourceEntity)); + } + @Test - public void extractSourceFromEntityUsesPreloadedClientDetailsWhenAvailable() { - ClientDetailsEntityCacheManager cacheManager = mock(ClientDetailsEntityCacheManager.class); + public void testPopulateSourceAwareEntityFromSource() { + Source source = new Source(); + source.setSourceOrcid(new SourceOrcid("orcid-id")); + source.setSourceClientId(new SourceClientId("client-id")); + source.setAssertionOriginClientId(new SourceClientId("assertion-id")); + + SourceAwareEntity entity = new AddressEntity(); // Using a concrete subclass + sourceEntityUtils.populateSourceAwareEntityFromSource(source, entity); + + assertEquals("orcid-id", entity.getSourceId()); + assertEquals("client-id", entity.getClientSourceId()); + assertEquals("assertion-id", entity.getAssertionOriginClientSourceId()); + } + + @Test + public void testExtractSourceFromEntity() { + AddressEntity entity = new AddressEntity(); + entity.setSourceId("orcid-id"); + entity.setClientSourceId("client-id"); + entity.setAssertionOriginClientSourceId("assertion-id"); + entity.setOrcid("user-orcid"); + + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); + clientDetails.setUserOBOEnabled(true); + when(clientDetailsEntityCacheManager.retrieve("client-id")).thenReturn(clientDetails); + + Source source = sourceEntityUtils.extractSourceFromEntity(entity); + + assertEquals("orcid-id", source.getSourceOrcid().getPath()); + assertEquals("client-id", source.getSourceClientId().getPath()); + assertEquals("assertion-id", source.getAssertionOriginClientId().getPath()); + assertEquals("user-orcid", source.getAssertionOriginOrcid().getPath()); + + // Test without OBO + clientDetails.setUserOBOEnabled(false); + source = sourceEntityUtils.extractSourceFromEntity(entity); + assertNull(source.getAssertionOriginOrcid()); + } + + @Test + public void testExtractSourceFromProfileComplete() { + ProfileEntity profile = new ProfileEntity("user-orcid"); + SourceEntity sourceEntity = new SourceEntity(); + sourceEntity.setSourceProfile(new ProfileEntity("source-orcid")); + sourceEntity.setSourceClient(new ClientDetailsEntity("source-client")); + profile.setSource(sourceEntity); + + when(sourceNameCacheManager.retrieve("source-orcid")).thenReturn("Source Orcid Name"); + when(sourceNameCacheManager.retrieve("source-client")).thenReturn("Source Client Name"); + + Source source = sourceEntityUtils.extractSourceFromProfileComplete(profile); + + assertEquals("source-orcid", source.getSourceOrcid().getPath()); + assertEquals("orcid.org", source.getSourceOrcid().getHost()); + assertEquals("https://orcid.org/source-orcid", source.getSourceOrcid().getUri()); + + assertEquals("source-client", source.getSourceClientId().getPath()); + assertEquals("orcid.org", source.getSourceClientId().getHost()); + assertEquals("https://orcid.org/client/source-client", source.getSourceClientId().getUri()); + + assertEquals("Source Client Name", source.getSourceName().getContent()); + } + + @Test + public void testExtractSourceFromEntityComplete() { + AddressEntity entity = new AddressEntity(); + entity.setSourceId("orcid-id"); + entity.setClientSourceId("client-id"); + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); - clientDetails.setId("CLIENT"); - ReflectionTestUtils.setField(clientDetails, "lastModified", new Date()); - Map clientDetailsById = Collections.singletonMap("CLIENT", clientDetails); - MinimizedWorkEntity work = new MinimizedWorkEntity(); - work.setClientSourceId("CLIENT"); - work.setOrcid("0000-0000-0000-0001"); + clientDetails.setUserOBOEnabled(false); + when(clientDetailsEntityCacheManager.retrieve("client-id")).thenReturn(clientDetails); + + when(sourceNameCacheManager.retrieve("orcid-id")).thenReturn("Orcid Name"); + when(sourceNameCacheManager.retrieve("client-id")).thenReturn("Client Name"); + + Source source = sourceEntityUtils.extractSourceFromEntityComplete(entity); + + assertEquals("orcid-id", source.getSourceOrcid().getPath()); + assertEquals("client-id", source.getSourceClientId().getPath()); + assertEquals("Client Name", source.getSourceName().getContent()); + } + + @Test + public void testIsTheSameSource_SourceOrcid() { + Source s1 = new Source(); + s1.setSourceOrcid(new SourceOrcid("orcid-1")); + + Source s2 = new Source(); + s2.setSourceOrcid(new SourceOrcid("orcid-1")); + + assertTrue(sourceEntityUtils.isTheSameSource(s1, s2)); + + s2.setSourceOrcid(new SourceOrcid("orcid-2")); + assertFalse(sourceEntityUtils.isTheSameSource(s1, s2)); + + AddressEntity entity = new AddressEntity(); + entity.setSourceId("orcid-1"); + assertTrue(sourceEntityUtils.isTheSameSource(s1, entity)); + + entity.setSourceId("orcid-2"); + assertFalse(sourceEntityUtils.isTheSameSource(s1, entity)); + } + + @Test + public void testIsTheSameSource_SourceClientId() { + ClientDetailsEntity clientDetails1 = new ClientDetailsEntity(); + clientDetails1.setId("client-1"); + clientDetails1.setUserOBOEnabled(true); + when(clientDetailsEntityCacheManager.retrieve("client-1")).thenReturn(clientDetails1); + + Source s1 = new Source(); + s1.setSourceClientId(new SourceClientId("client-1")); + + Source s2 = new Source(); + s2.setSourceClientId(new SourceClientId("client-1")); + + assertTrue(sourceEntityUtils.isTheSameSource(s1, s2)); + + Source s3 = new Source(); + s3.setSourceClientId(new SourceClientId("client-1")); + s3.setAssertionOriginOrcid(new SourceOrcid("orcid-1")); - assertNotNull(SourceEntityUtils.extractSourceFromEntity(work, cacheManager, clientDetailsById)); + AddressEntity entity = new AddressEntity(); + entity.setOrcid("orcid-1"); + entity.setClientSourceId("client-1"); + assertTrue(sourceEntityUtils.isTheSameSource(s3, entity)); - verify(cacheManager, never()).retrieve("CLIENT"); + entity.setClientSourceId("client-2"); + assertFalse(sourceEntityUtils.isTheSameSource(s3, entity)); } } diff --git a/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java b/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java index ed25b0ee1a5..3484e866279 100644 --- a/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java +++ b/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java @@ -67,9 +67,6 @@ public class ContributorUtilsTest { @Before public void setUp() { MockitoAnnotations.initMocks(this); - contributorUtils.setCacheManager(cacheManager); - contributorUtils.setProfileEntityManager(profileEntityManager); - contributorUtils.setProfileLastModifiedAspect(profileLastModifiedAspect); when(profileLastModifiedAspect.retrieveLastModifiedDate(anyString())).thenReturn(new Date()); when(contributorsNameCache.containsKey(anyString())).thenReturn(false); } diff --git a/orcid-web/src/main/resources/orcid-core-context-spam.xml b/orcid-web/src/main/resources/orcid-core-context-spam.xml index 124a0da9ad7..2b7a83464cb 100644 --- a/orcid-web/src/main/resources/orcid-core-context-spam.xml +++ b/orcid-web/src/main/resources/orcid-core-context-spam.xml @@ -412,15 +412,7 @@ - - - - - - - - - + From f8a83252c8a9d9b67e12b69c6926fd4a9939ba1d Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 25 May 2026 16:49:17 -0600 Subject: [PATCH 07/15] Fix unit tests --- .../impl/WorkManagerReadOnlyImpl.java | 35 +++++-------------- .../v3/validator/ActivityValidator.java | 1 - .../orcid/core/utils/SourceEntityUtils.java | 3 +- .../orcid/core/utils/v3/ContributorUtils.java | 23 ++---------- .../core/utils/v3/ContributorUtilsTest.java | 12 ++++--- 5 files changed, 20 insertions(+), 54 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 42c3341db0a..55dbfcfde4f 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -1,29 +1,18 @@ package org.orcid.core.manager.v3.read_only.impl; -import java.math.BigInteger; -import java.sql.Timestamp; -import java.util.*; -import java.util.concurrent.ForkJoinPool; -import java.util.stream.Collectors; - -import javax.annotation.Resource; - import org.apache.commons.lang3.StringUtils; import org.orcid.core.adapter.jsonidentifier.converter.JSONWorkExternalIdentifiersConverterV3; import org.orcid.core.adapter.v3.JpaJaxbWorkAdapter; import org.orcid.core.adapter.v3.converter.ContributorsRolesAndSequencesConverter; import org.orcid.core.adapter.v3.converter.WorkContributorsConverter; -import org.orcid.core.contributors.roles.works.WorkContributorRoleConverter; import org.orcid.core.exception.ExceedMaxNumberOfPutCodesException; import org.orcid.core.exception.OrcidCoreExceptionMapper; import org.orcid.core.exception.PutCodeFormatException; import org.orcid.core.manager.ClientDetailsEntityCacheManager; import org.orcid.core.manager.SourceNameCacheManager; import org.orcid.core.manager.WorkEntityCacheManager; -import org.orcid.core.manager.impl.OrcidUrlManager; import org.orcid.core.manager.v3.GroupingSuggestionManager; import org.orcid.core.manager.v3.WorksExtendedCacheManager; -import org.orcid.core.manager.v3.read_only.ClientDetailsManagerReadOnly; import org.orcid.core.manager.v3.read_only.WorkManagerReadOnly; import org.orcid.core.togglz.Features; import org.orcid.core.utils.SourceEntityUtils; @@ -35,35 +24,29 @@ import org.orcid.jaxb.model.record.bulk.BulkElement; import org.orcid.jaxb.model.v3.release.common.PublicationDate; import org.orcid.jaxb.model.v3.release.common.Source; -import org.orcid.jaxb.model.v3.release.record.ExternalID; -import org.orcid.jaxb.model.v3.release.record.ExternalIDs; -import org.orcid.jaxb.model.v3.release.record.GroupAble; -import org.orcid.jaxb.model.v3.release.record.GroupableActivity; -import org.orcid.jaxb.model.v3.release.record.Work; -import org.orcid.jaxb.model.v3.release.record.WorkBulk; +import org.orcid.jaxb.model.v3.release.record.*; import org.orcid.jaxb.model.v3.release.record.summary.WorkGroup; import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; import org.orcid.jaxb.model.v3.release.record.summary.Works; import org.orcid.persistence.dao.WorkDao; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.persistence.jpa.entities.MinimizedWorkEntity; import org.orcid.persistence.jpa.entities.WorkEntity; import org.orcid.persistence.jpa.entities.WorkLastModifiedEntity; -import org.orcid.pojo.ActivityTitle; -import org.orcid.pojo.ActivityTitleSearchResult; -import org.orcid.pojo.ContributorsRolesAndSequences; -import org.orcid.pojo.WorkContributorsList; -import org.orcid.pojo.WorkExtended; -import org.orcid.pojo.WorkGroupExtended; -import org.orcid.pojo.WorkSummaryExtended; -import org.orcid.pojo.WorksExtended; +import org.orcid.pojo.*; import org.orcid.pojo.ajaxForm.PojoUtil; import org.orcid.pojo.ajaxForm.WorkForm; import org.orcid.pojo.grouping.WorkGroupingSuggestion; -import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; +import javax.annotation.Resource; +import java.math.BigInteger; +import java.sql.Timestamp; +import java.util.*; +import java.util.stream.Collectors; + import static org.orcid.pojo.ajaxForm.PojoUtil.getWorkForm; public class WorkManagerReadOnlyImpl extends ManagerReadOnlyBaseImpl implements WorkManagerReadOnly { diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java index ed25da432e3..ba33bfab4df 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/validator/ActivityValidator.java @@ -58,7 +58,6 @@ import org.orcid.jaxb.model.v3.release.common.Year; import org.orcid.jaxb.model.v3.release.groupid.GroupIdRecord; import org.orcid.jaxb.model.v3.release.record.Affiliation; -import org.orcid.jaxb.model.v3.release.record.AffiliationType; import org.orcid.jaxb.model.v3.release.record.Education; import org.orcid.jaxb.model.v3.release.record.Employment; import org.orcid.jaxb.model.v3.release.record.ExternalID; diff --git a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java index fbd339afa55..fcabb8accba 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/SourceEntityUtils.java @@ -1,6 +1,5 @@ package org.orcid.core.utils; -import java.util.Map; import java.util.Optional; import javax.annotation.Resource; @@ -50,7 +49,7 @@ public static String getSourceId(SourceEntity sourceEntity) { } public static String getSourceName(Source activeSource) { - if (activeSource.getSourceName() != null && !StringUtils.isEmpty(activeSource.getSourceName().getContent())) + if (activeSource != null && activeSource.getSourceName() != null && !StringUtils.isEmpty(activeSource.getSourceName().getContent())) return activeSource.getSourceName().getContent(); else return null; diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java index 28326df093a..8c910bc2e85 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/v3/ContributorUtils.java @@ -1,32 +1,21 @@ package org.orcid.core.utils.v3; -import java.util.ArrayList; -import java.util.List; -import java.util.stream.Collectors; - import org.apache.commons.lang3.StringUtils; -import org.orcid.core.aop.ProfileLastModifiedAspect; import org.orcid.core.contributors.roles.credit.CreditRole; -import org.orcid.core.manager.ClientDetailsEntityCacheManager; -import org.orcid.core.manager.SourceNameCacheManager; -import org.orcid.core.manager.v3.ActivityManager; import org.orcid.core.manager.v3.ProfileEntityManager; -import org.orcid.core.manager.v3.read_only.ProfileEntityManagerReadOnly; import org.orcid.core.manager.v3.read_only.RecordNameManagerReadOnly; import org.orcid.jaxb.model.v3.release.common.Contributor; import org.orcid.jaxb.model.v3.release.common.ContributorAttributes; import org.orcid.jaxb.model.v3.release.common.CreditName; import org.orcid.jaxb.model.v3.release.record.Funding; import org.orcid.jaxb.model.v3.release.record.FundingContributor; -import org.orcid.persistence.dao.WorkDao; -import org.orcid.persistence.jpa.entities.ClientDetailsEntity; -import org.orcid.persistence.jpa.entities.OrcidAware; -import org.orcid.persistence.jpa.entities.WorkEntity; import org.orcid.pojo.ContributorsRolesAndSequences; import org.orcid.pojo.ajaxForm.PojoUtil; -import org.springframework.beans.factory.annotation.Value; import javax.annotation.Resource; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; public class ContributorUtils { @@ -36,12 +25,6 @@ public class ContributorUtils { @Resource(name = "profileEntityManagerV3") private ProfileEntityManager profileEntityManagerV3; - @Resource - private ClientDetailsEntityCacheManager clientDetailsEntityCacheManager; - - @Resource(name = "workDaoReadOnly") - private WorkDao workDaoReadOnly; - public void filterContributorPrivateData(Funding funding) { if (funding.getContributors() != null && funding.getContributors().getContributor() != null) { for (FundingContributor contributor : funding.getContributors().getContributor()) { diff --git a/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java b/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java index 3484e866279..00dcaeb5445 100644 --- a/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java +++ b/orcid-core/src/test/java/org/orcid/core/utils/v3/ContributorUtilsTest.java @@ -24,6 +24,7 @@ import org.orcid.core.manager.ProfileEntityCacheManager; import org.orcid.core.manager.v3.ActivityManager; import org.orcid.core.manager.v3.ProfileEntityManager; +import org.orcid.core.manager.v3.read_only.RecordNameManagerReadOnly; import org.orcid.jaxb.model.v3.release.common.Contributor; import org.orcid.jaxb.model.v3.release.common.ContributorEmail; import org.orcid.jaxb.model.v3.release.common.ContributorOrcid; @@ -51,13 +52,13 @@ public class ContributorUtilsTest { @Mock private ProfileEntityManager profileEntityManager; - - @Mock - private RecordNameDao recordNameDao; - + @Mock private ProfileLastModifiedAspect profileLastModifiedAspect; - + + @Mock + private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; + @Mock private Cache contributorsNameCache; @@ -90,6 +91,7 @@ public void testFilterContributorPrivateDataForFundingWithPublicName() { when(profileEntityManager.orcidExists(anyString())).thenReturn(true); when(profileEntityCacheManager.retrieve(anyString())).thenReturn(new ProfileEntity()); when(cacheManager.getPublicCreditName(any(String.class))).thenReturn("a public name"); + when(recordNameManagerReadOnlyV3.fetchDisplayablePublicName(anyString())).thenReturn("a public name"); Funding funding = getFundingWithOrcidContributor(); contributorUtils.filterContributorPrivateData(funding); From 63114e5399bfbf0ac5d80299d94b6ca888c9d533 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Tue, 26 May 2026 13:50:06 -0600 Subject: [PATCH 08/15] More improvements in the UI endpoint --- .../impl/SourceNameCacheManagerImpl.java | 8 +----- .../org/orcid/pojo/grouping/WorkGroup.java | 6 +++-- .../src/main/resources/orcid-core-context.xml | 17 ++++-------- .../controllers/PublicProfileController.java | 6 ++++- .../resources/orcid-core-context-spam.xml | 27 +++++-------------- 5 files changed, 22 insertions(+), 42 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/SourceNameCacheManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/SourceNameCacheManagerImpl.java index c567e21a9d7..96fc8bd1f6b 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/SourceNameCacheManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/SourceNameCacheManagerImpl.java @@ -4,7 +4,6 @@ import org.apache.commons.lang.StringUtils; import org.ehcache.Cache; -import org.orcid.core.manager.ProfileEntityManager; import org.orcid.core.manager.SourceNameCacheManager; import org.orcid.core.manager.v3.read_only.RecordNameManagerReadOnly; import org.orcid.core.utils.ReleaseNameUtils; @@ -35,8 +34,6 @@ public class SourceNameCacheManagerImpl implements SourceNameCacheManager { private ClientDetailsDao clientDetailsDao; - private ProfileEntityManager profileEntityManager; - @Resource(name = "recordNameManagerReadOnlyV3") private RecordNameManagerReadOnly recordNameManagerReadOnlyV3; @@ -47,10 +44,7 @@ public void setRecordNameDao(RecordNameDao recordNameDao) { public void setClientDetailsDao(ClientDetailsDao clientDetailsDao) { this.clientDetailsDao = clientDetailsDao; } - - public void setProfileEntityManager(ProfileEntityManager profileEntityManager) { - this.profileEntityManager = profileEntityManager; - } + @Override public String retrieve(String sourceId) throws IllegalArgumentException { diff --git a/orcid-core/src/main/java/org/orcid/pojo/grouping/WorkGroup.java b/orcid-core/src/main/java/org/orcid/pojo/grouping/WorkGroup.java index 2dbf34923d8..5ca5b5d2daf 100644 --- a/orcid-core/src/main/java/org/orcid/pojo/grouping/WorkGroup.java +++ b/orcid-core/src/main/java/org/orcid/pojo/grouping/WorkGroup.java @@ -110,14 +110,14 @@ public static WorkGroup valueOf(org.orcid.jaxb.model.v3.release.record.summary.W public static WorkGroup valueOf(WorkGroupExtended workGroup, int id, String orcid) { WorkGroup group = new WorkGroup(); group.setGroupId(id); - group.setWorks(new ArrayList<>()); + List works = new ArrayList<>(); WorkType workType = null; Long maxDisplayIndex = null; for (WorkSummaryExtended workSummary : workGroup.getWorkSummary()) { WorkForm workForm = getWorkForm(workSummary); - group.getWorks().add(workForm); + works.add(workForm); Long displayIndex = Long.parseLong(workSummary.getDisplayIndex()); if (maxDisplayIndex == null || displayIndex > maxDisplayIndex) { @@ -134,6 +134,8 @@ public static WorkGroup valueOf(WorkGroupExtended workGroup, int id, String orci workType = workSummary.getType(); } + group.setWorks(works); + if (workGroup.getIdentifiers() != null) { List workExternalIdentifiersList = new ArrayList(); for (ExternalID extId : workGroup.getIdentifiers().getExternalIdentifier()) { diff --git a/orcid-core/src/main/resources/orcid-core-context.xml b/orcid-core/src/main/resources/orcid-core-context.xml index c4bac0e2ff0..a57873e45df 100644 --- a/orcid-core/src/main/resources/orcid-core-context.xml +++ b/orcid-core/src/main/resources/orcid-core-context.xml @@ -298,7 +298,7 @@ - + @@ -379,17 +379,10 @@ - - - - - - - - + @@ -410,7 +403,7 @@ - + @@ -420,7 +413,7 @@ - + @@ -470,7 +463,7 @@ - + diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java index b4a797485ca..554f22db1a8 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java @@ -473,7 +473,11 @@ private boolean isRecordReadyForIndexing(ProfileEntity profile) { } catch (Exception e) { return new Page(); } - return worksPaginator.getWorksExtendedPage(orcid, offset, pageSize, true, sort, sortAsc); + long l0 = System.currentTimeMillis(); + Page r = worksPaginator.getWorksExtendedPage(orcid, offset, pageSize, true, sort, sortAsc); + long l1 = System.currentTimeMillis(); + System.out.println("Total time taken: " + (l1 - l0)); + return r; } @RequestMapping(value = "/{orcid:(?:\\d{4}-){3,}\\d{3}[\\dX]}/featuredWorks.json", method = RequestMethod.GET) diff --git a/orcid-web/src/main/resources/orcid-core-context-spam.xml b/orcid-web/src/main/resources/orcid-core-context-spam.xml index 2b7a83464cb..1312dd78f0d 100644 --- a/orcid-web/src/main/resources/orcid-core-context-spam.xml +++ b/orcid-web/src/main/resources/orcid-core-context-spam.xml @@ -366,15 +366,8 @@ - - - - - - - - - + + @@ -401,23 +394,17 @@ - + - - - - - - - - + + - + @@ -467,7 +454,7 @@ - + From 1d8e6f5673a20212b5653a5c924dd7844de3d68c Mon Sep 17 00:00:00 2001 From: amontenegro Date: Tue, 26 May 2026 17:06:50 -0600 Subject: [PATCH 09/15] Some logs and improvements --- .../impl/WorkManagerReadOnlyImpl.java | 22 +- .../activities/ActivitiesGroupGenerator.java | 2 +- ...rkGroupAndGroupingSuggestionGenerator.java | 1 - ...upAndGroupingSuggestionGeneratorJunie.java | 131 ++++++++ ...dGroupingSuggestionGeneratorJunieTest.java | 301 ++++++++++++++++++ .../web/pagination/WorksPaginator.java | 3 + 6 files changed, 453 insertions(+), 7 deletions(-) create mode 100644 orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java create mode 100644 orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 55dbfcfde4f..0b89d8e4d75 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -17,10 +17,7 @@ import org.orcid.core.togglz.Features; import org.orcid.core.utils.SourceEntityUtils; import org.orcid.core.utils.v3.ContributorUtils; -import org.orcid.core.utils.v3.activities.ActivitiesGroup; -import org.orcid.core.utils.v3.activities.ActivitiesGroupGenerator; -import org.orcid.core.utils.v3.activities.WorkComparators; -import org.orcid.core.utils.v3.activities.WorkGroupAndGroupingSuggestionGenerator; +import org.orcid.core.utils.v3.activities.*; import org.orcid.jaxb.model.record.bulk.BulkElement; import org.orcid.jaxb.model.v3.release.common.PublicationDate; import org.orcid.jaxb.model.v3.release.common.Source; @@ -218,6 +215,7 @@ public List getWorksSummaryList(String orcid) { public List getWorksSummaryExtendedList(String orcid, boolean featuredOnly) { List wseList = retrieveWorkSummaryExtended(orcid, featuredOnly); // Filter the contributors list + long t0 = System.currentTimeMillis(); for (WorkSummaryExtended wse : wseList) { if (wse.getContributorsGroupedByOrcid() != null && wse.getContributorsGroupedByOrcid().size() > 0) { wse.setNumberOfContributors(wse.getContributorsGroupedByOrcid().size()); @@ -228,6 +226,8 @@ public List getWorksSummaryExtendedList(String orcid, boole wse.setNumberOfContributors(contributorsGroupedByOrcid.size()); } } + long t1 = System.currentTimeMillis(); + System.out.println("Time on getWorksSummaryExtendedList: " + (t1 - t0)); return wseList; } @@ -347,13 +347,25 @@ public Works groupWorksAndGenerateGroupingSuggestions(List summarie @Override public WorksExtended groupWorksExtendedAndGenerateGroupingSuggestions(List summaries, String orcid) { - WorkGroupAndGroupingSuggestionGenerator groupGenerator = new WorkGroupAndGroupingSuggestionGenerator(); + long t0 = System.currentTimeMillis(); + long t02 = System.currentTimeMillis(); + WorkGroupAndGroupingSuggestionGeneratorJunie groupGenerator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); for (WorkSummaryExtended work : summaries) { groupGenerator.group(work); } + long t03 = System.currentTimeMillis(); + System.out.println("GROUPING WITH JUNIE: " + (t03 - t02)); + long t1 = System.currentTimeMillis(); WorksExtended works = processGroupedWorksExtended(groupGenerator.getGroups()); + long t2 = System.currentTimeMillis(); List suggestions = groupGenerator.getGroupingSuggestions(orcid); + long t3 = System.currentTimeMillis(); groupingSuggestionsManager.cacheGroupingSuggestions(orcid, suggestions); + long t4 = System.currentTimeMillis(); + System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 1: " + (t1 - t0)); + System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 2: " + (t2 - t1)); + System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 3: " + (t3 - t2)); + System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 4: " + (t4 - t3)); return works; } diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/ActivitiesGroupGenerator.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/ActivitiesGroupGenerator.java index ef4d4f7dcd0..a01a17f5939 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/ActivitiesGroupGenerator.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/ActivitiesGroupGenerator.java @@ -55,7 +55,7 @@ protected ActivitiesGroup createNewGroup(GroupableActivity activity) { protected List generateBelongsToList(GroupableActivity activity) { List belongsTo = new ArrayList(); ActivitiesGroup thisGroup = new ActivitiesGroup(activity); - for (GroupAble g :thisGroup.getGroupKeys()){ + for (GroupAble g : thisGroup.getGroupKeys()){ if (lookup.containsKey(g)) { belongsTo.add(lookup.get(g)); } diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGenerator.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGenerator.java index 227b52fd1f3..60e07df241f 100644 --- a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGenerator.java +++ b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGenerator.java @@ -33,7 +33,6 @@ public void group(GroupableActivity activity) { workSummary = (WorkSummary) activity; } - if (groups.isEmpty()) { // If it is the first activity, create a new group for it ActivitiesGroup newGroup = createNewGroup(activity); diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java new file mode 100644 index 00000000000..dcbfac06120 --- /dev/null +++ b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java @@ -0,0 +1,131 @@ +package org.orcid.core.utils.v3.activities; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.orcid.jaxb.model.v3.release.record.ExternalID; +import org.orcid.jaxb.model.v3.release.record.GroupableActivity; +import org.orcid.jaxb.model.v3.release.record.WorkTitle; +import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; +import org.orcid.pojo.WorkSummaryExtended; +import org.orcid.pojo.grouping.WorkGroupingSuggestion; + +public class WorkGroupAndGroupingSuggestionGeneratorJunie extends ActivitiesGroupGenerator { + + // Optimized mapping: group -> set of titles it is associated with + private Map> groupToTitles = new HashMap<>(); + // Optimized mapping: title -> set of groups associated with it + private Map> titleToGroups = new HashMap<>(); + + @Override + public void group(GroupableActivity activity) { + if (!(activity instanceof WorkSummary || activity instanceof WorkSummaryExtended)) { + throw new IllegalArgumentException("Argument must be of type WorkSummary"); + } + + WorkSummary workSummary = null; + if (activity instanceof WorkSummaryExtended) { + workSummary = (WorkSummaryExtended) activity; + } else if (activity instanceof WorkSummary) { + workSummary = (WorkSummary) activity; + } + + List belongsTo = generateBelongsToList(activity); + + ActivitiesGroup targetGroup; + if (belongsTo.isEmpty()) { + targetGroup = createNewGroup(activity); + } else { + targetGroup = belongsTo.get(0); + targetGroup.add(activity); + + if (belongsTo.size() > 1) { + for (int i = 1; i < belongsTo.size(); i++) { + ActivitiesGroup groupToMerge = belongsTo.get(i); + mergeAndRemoveGroup(targetGroup, groupToMerge); + fastSwitchGroup(groupToMerge, targetGroup); + } + } + updateLookupKeys(targetGroup); + } + mapGroupToTitle(targetGroup, workSummary); + } + + public List getGroupingSuggestions(String orcid) { + List suggestions = new ArrayList<>(); + for (Map.Entry> entry : titleToGroups.entrySet()) { + Set groupsForTitle = entry.getValue(); + if (groupsForTitle.size() > 1) { + WorkGroupingSuggestion suggestion = new WorkGroupingSuggestion(); + suggestion.setOrcid(orcid); + List putCodes = new ArrayList<>(); + + boolean groupableExternalIdFound = false; + for (ActivitiesGroup group : groupsForTitle) { + for (GroupableActivity activity : group.getActivities()) { + WorkSummary workSummary = (WorkSummary) activity; + putCodes.add(workSummary.getPutCode()); + if (!groupableExternalIdFound && workSummary.getExternalIdentifiers() != null) { + for (ExternalID externalId : workSummary.getExternalIdentifiers().getExternalIdentifier()) { + if (externalId.isGroupAble()) { + groupableExternalIdFound = true; + break; + } + } + } + } + } + + if (groupableExternalIdFound) { + suggestion.setPutCodes(putCodes); + suggestions.add(suggestion); + } + } + } + return suggestions; + } + + private void fastSwitchGroup(ActivitiesGroup oldGroup, ActivitiesGroup newGroup) { + Set titles = groupToTitles.remove(oldGroup); + if (titles != null) { + for (String title : titles) { + Set groupsForTitle = titleToGroups.get(title); + if (groupsForTitle != null) { + groupsForTitle.remove(oldGroup); + groupsForTitle.add(newGroup); + } + + Set newGroupTitles = groupToTitles.computeIfAbsent(newGroup, k -> new HashSet<>()); + newGroupTitles.add(title); + } + } + } + + private void mapGroupToTitle(ActivitiesGroup group, WorkSummary workSummary) { + if (!workTitleEmpty(workSummary.getTitle())) { + String title = transformForTitleComparison(workSummary.getTitle().getTitle().getContent()); + + titleToGroups.computeIfAbsent(title, k -> new HashSet<>()).add(group); + groupToTitles.computeIfAbsent(group, k -> new HashSet<>()).add(title); + } + } + + private String transformForTitleComparison(String titleContent) { + return titleContent.toLowerCase().replaceAll("\\s", ""); + } + + private boolean workTitleEmpty(WorkTitle workTitle) { + return workTitle == null || workTitle.getTitle() == null || workTitle.getTitle().getContent() == null || workTitle.getTitle().getContent().isEmpty(); + } + + @Override + protected void mergeAndRemoveGroup(ActivitiesGroup keep, ActivitiesGroup discard) { + super.mergeAndRemoveGroup(keep, discard); + // We don't remove from titleToGroups/groupToTitles here because fastSwitchGroup handles it + // and it's called immediately after in group() + } +} diff --git a/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java b/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java new file mode 100644 index 00000000000..8ca74e95238 --- /dev/null +++ b/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java @@ -0,0 +1,301 @@ +package org.orcid.core.utils.v3.activities; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.Test; +import org.orcid.jaxb.model.common.Relationship; +import org.orcid.jaxb.model.v3.release.common.Title; +import org.orcid.jaxb.model.v3.release.record.ExternalID; +import org.orcid.jaxb.model.v3.release.record.ExternalIDs; +import org.orcid.jaxb.model.v3.release.record.WorkTitle; +import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; +import org.orcid.pojo.grouping.WorkGroupingSuggestion; + +public class WorkGroupAndGroupingSuggestionGeneratorJunieTest { + + @Test + public void testFourGroupsOneSuggestion() { + WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + for (WorkSummary summary : getWorkSummariesFourGroupsOneSuggestion()) { + generator.group(summary); + } + assertEquals(4, generator.getGroups().size()); + + List suggestions = generator.getGroupingSuggestions("orcid"); + assertEquals(1, suggestions.size()); + assertPutCodesEqual("4,7", suggestions.get(0).getPutCodesAsString()); + assertEquals("orcid", suggestions.get(0).getOrcid()); + } + + @Test + public void testFiveGroupsTwoSuggestions() { + WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + for (WorkSummary summary : getWorkSummariesFiveGroupsTwoSuggestions()) { + generator.group(summary); + } + assertEquals(5, generator.getGroups().size()); + + List suggestions = generator.getGroupingSuggestions("orcid"); + assertEquals(2, suggestions.size()); + + List suggestionPutCodes = suggestions.stream() + .map(WorkGroupingSuggestion::getPutCodesAsString) + .collect(Collectors.toList()); + + assertTrue(containsPutCodes(suggestionPutCodes, "4,7")); + assertTrue(containsPutCodes(suggestionPutCodes, "1,2,3,8")); + + for (WorkGroupingSuggestion suggestion : suggestions) { + assertEquals("orcid", suggestion.getOrcid()); + } + } + + @Test + public void testNoGroupableIDs() { + WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + for (WorkSummary summary : getWorkSummariesNoGroupableIds()) { + generator.group(summary); + } + List suggestions = generator.getGroupingSuggestions("orcid"); + assertEquals(0, suggestions.size()); + } + + @Test + public void testLargeNumberOfWorks() { + int numWorks = 2000; + List works = new ArrayList<>(); + for (int i = 0; i < numWorks; i++) { + WorkSummary summary = new WorkSummary(); + summary.setPutCode((long) i); + summary.setTitle(getWorkTitle("Title " + (i % 200))); // 200 unique titles + + ExternalIDs ids = new ExternalIDs(); + ids.getExternalIdentifier().add(getExternalID("doi", "doi-" + i)); // unique DOI + summary.setExternalIdentifiers(ids); + works.add(summary); + } + + // Warmup + for (int i=0; i<10; i++) { + WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + for (WorkSummary summary : works) generator.group(summary); + generator.getGroupingSuggestions("orcid"); + + WorkGroupAndGroupingSuggestionGenerator original = new WorkGroupAndGroupingSuggestionGenerator(); + for (WorkSummary summary : works) original.group(summary); + original.getGroupingSuggestions("orcid"); + } + + long start = System.nanoTime(); + WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + for (WorkSummary summary : works) { + generator.group(summary); + } + generator.getGroupingSuggestions("orcid"); + long end = System.nanoTime(); + System.out.println("[DEBUG_LOG] Junie generator took " + (end - start) / 1000000.0 + "ms for " + numWorks + " works"); + + start = System.nanoTime(); + WorkGroupAndGroupingSuggestionGenerator original = new WorkGroupAndGroupingSuggestionGenerator(); + for (WorkSummary summary : works) { + original.group(summary); + } + original.getGroupingSuggestions("orcid"); + end = System.nanoTime(); + System.out.println("[DEBUG_LOG] Original generator took " + (end - start) / 1000000.0 + "ms for " + numWorks + " works"); + } + + private void assertPutCodesEqual(String expected, String actual) { + List expectedList = Arrays.asList(expected.split(",")); + List actualList = Arrays.asList(actual.split(",")); + Collections.sort(expectedList); + Collections.sort(actualList); + assertEquals(expectedList, actualList); + } + + private boolean containsPutCodes(List suggestionPutCodes, String target) { + List targetList = Arrays.asList(target.split(",")); + Collections.sort(targetList); + for (String s : suggestionPutCodes) { + List sList = Arrays.asList(s.split(",")); + Collections.sort(sList); + if (sList.equals(targetList)) { + return true; + } + } + return false; + } + + private List getWorkSummariesFourGroupsOneSuggestion() { + WorkSummary first = new WorkSummary(); + first.setPutCode(1L); + first.setTitle(getWorkTitle("a title")); + + ExternalIDs firstIds = new ExternalIDs(); + firstIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + first.setExternalIdentifiers(firstIds); + + WorkSummary second = new WorkSummary(); + second.setPutCode(2L); + second.setTitle(getWorkTitle("second title")); + + ExternalIDs secondIds = new ExternalIDs(); + secondIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + second.setExternalIdentifiers(secondIds); + + WorkSummary third = new WorkSummary(); + third.setPutCode(3L); + third.setTitle(getWorkTitle("SECOND title")); + + ExternalIDs thirdIds = new ExternalIDs(); + thirdIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + third.setExternalIdentifiers(thirdIds); + + WorkSummary fourth = new WorkSummary(); + fourth.setPutCode(4L); + fourth.setTitle(getWorkTitle("something totally different")); + + ExternalIDs fourthIds = new ExternalIDs(); + fourthIds.getExternalIdentifier().add(getExternalID("doi", "group2")); + fourth.setExternalIdentifiers(fourthIds); + + WorkSummary fifth = new WorkSummary(); + fifth.setPutCode(5L); + fifth.setTitle(getWorkTitle("third title")); + + ExternalIDs fifthIds = new ExternalIDs(); + fifthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); + fifth.setExternalIdentifiers(fifthIds); + + WorkSummary sixth = new WorkSummary(); + sixth.setPutCode(6L); + sixth.setTitle(getWorkTitle("3rd title")); + + ExternalIDs sixthIds = new ExternalIDs(); + sixthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); + sixth.setExternalIdentifiers(sixthIds); + + WorkSummary seventh = new WorkSummary(); + seventh.setPutCode(7L); + seventh.setTitle(getWorkTitle("SOMETHINGTOTALLYDIFFERENT")); + + ExternalIDs seventhIds = new ExternalIDs(); + seventhIds.getExternalIdentifier().add(getExternalID("doi", "group4")); + seventh.setExternalIdentifiers(seventhIds); + + return Arrays.asList(first, second, third, fourth, fifth, sixth, seventh); + } + + private List getWorkSummariesFiveGroupsTwoSuggestions() { + WorkSummary first = new WorkSummary(); + first.setPutCode(1L); + first.setTitle(getWorkTitle("a title")); + + ExternalIDs firstIds = new ExternalIDs(); + firstIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + first.setExternalIdentifiers(firstIds); + + WorkSummary second = new WorkSummary(); + second.setPutCode(2L); + second.setTitle(getWorkTitle("second title")); + + ExternalIDs secondIds = new ExternalIDs(); + secondIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + second.setExternalIdentifiers(secondIds); + + WorkSummary third = new WorkSummary(); + third.setPutCode(3L); + third.setTitle(getWorkTitle("SECOND title")); + + ExternalIDs thirdIds = new ExternalIDs(); + thirdIds.getExternalIdentifier().add(getExternalID("doi", "group1")); + third.setExternalIdentifiers(thirdIds); + + WorkSummary fourth = new WorkSummary(); + fourth.setPutCode(4L); + fourth.setTitle(getWorkTitle("something totally different")); + + ExternalIDs fourthIds = new ExternalIDs(); + fourthIds.getExternalIdentifier().add(getExternalID("doi", "group2")); + fourth.setExternalIdentifiers(fourthIds); + + WorkSummary fifth = new WorkSummary(); + fifth.setPutCode(5L); + fifth.setTitle(getWorkTitle("third title")); + + ExternalIDs fifthIds = new ExternalIDs(); + fifthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); + fifth.setExternalIdentifiers(fifthIds); + + WorkSummary sixth = new WorkSummary(); + sixth.setPutCode(6L); + sixth.setTitle(getWorkTitle("3rd title")); + + ExternalIDs sixthIds = new ExternalIDs(); + sixthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); + sixth.setExternalIdentifiers(sixthIds); + + WorkSummary seventh = new WorkSummary(); + seventh.setPutCode(7L); + seventh.setTitle(getWorkTitle("SOMETHINGTOTALLYDIFFERENT")); + + ExternalIDs seventhIds = new ExternalIDs(); + seventhIds.getExternalIdentifier().add(getExternalID("doi", "group4")); + seventh.setExternalIdentifiers(seventhIds); + + WorkSummary eighth = new WorkSummary(); + eighth.setPutCode(8L); + eighth.setTitle(getWorkTitle("ATITLE")); + + ExternalIDs eighthIds = new ExternalIDs(); + eighthIds.getExternalIdentifier().add(getExternalID("doi", "group5")); + eighth.setExternalIdentifiers(eighthIds); + + return Arrays.asList(first, second, third, fourth, fifth, sixth, seventh, eighth); + } + + private List getWorkSummariesNoGroupableIds() { + WorkSummary first = new WorkSummary(); + first.setPutCode(1L); + first.setTitle(getWorkTitle("a title")); + + ExternalIDs firstIds = new ExternalIDs(); + ExternalID externalID = getExternalID("issn", "1234-5678"); + externalID.setRelationship(Relationship.PART_OF); + firstIds.getExternalIdentifier().add(externalID); + first.setExternalIdentifiers(firstIds); + + WorkSummary second = new WorkSummary(); + second.setPutCode(2L); + second.setTitle(getWorkTitle("second title")); + + ExternalIDs secondIds = new ExternalIDs(); + ExternalID secondExternalID = getExternalID("issn", "1234-5678"); + secondExternalID.setRelationship(Relationship.PART_OF); + secondIds.getExternalIdentifier().add(secondExternalID); + second.setExternalIdentifiers(secondIds); + + return Arrays.asList(first, second); + } + + private WorkTitle getWorkTitle(String title) { + WorkTitle workTitle = new WorkTitle(); + workTitle.setTitle(new Title(title)); + return workTitle; + } + + private ExternalID getExternalID(String type, String value) { + ExternalID id = new ExternalID(); + id.setType(type); + id.setValue(value); + return id; + } + +} diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java b/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java index a0e2892bb94..ba2e61aab51 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java @@ -70,6 +70,7 @@ public Page getWorksPage(String orcid, int offset, int pageSize, bool public Page getWorksExtendedPage(String orcid, int offset, int pageSize, boolean justPublic, String sort, boolean sortAsc) { WorksExtended works = worksExtendedCacheManager.getGroupedWorksExtended(orcid); Page worksPage = new Page(); + long t0 = System.currentTimeMillis(); if (works != null) { List filteredGroups = filterWorksExtended(works, justPublic); if ("source".equals(sort)) { @@ -88,6 +89,8 @@ public Page getWorksExtendedPage(String orcid, int offset, int pageSi worksPage.setGroups(workGroups); worksPage.setNextOffset(offset + pageSize); } + long t1 = System.currentTimeMillis(); + System.out.println("Time on getWorksExtendedPage: " + (t1 - t0)); return worksPage; } From 93d19ede6014a3ed2737188c7653eb9f9f07be4c Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 08:53:40 -0600 Subject: [PATCH 10/15] More improvements --- .../impl/WorkManagerReadOnlyImpl.java | 3 ++ ...upAndGroupingSuggestionGeneratorJunie.java | 31 ++++++++++--------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 0b89d8e4d75..6b151213ef0 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -351,6 +351,9 @@ public WorksExtended groupWorksExtendedAndGenerateGroupingSuggestions(List belongsTo = generateBelongsToList(activity); - ActivitiesGroup targetGroup; - if (belongsTo.isEmpty()) { + if (groups.isEmpty()) { targetGroup = createNewGroup(activity); } else { - targetGroup = belongsTo.get(0); - targetGroup.add(activity); - - if (belongsTo.size() > 1) { - for (int i = 1; i < belongsTo.size(); i++) { - ActivitiesGroup groupToMerge = belongsTo.get(i); - mergeAndRemoveGroup(targetGroup, groupToMerge); - fastSwitchGroup(groupToMerge, targetGroup); + List belongsTo = generateBelongsToList(activity); + + if (belongsTo.isEmpty()) { + targetGroup = createNewGroup(activity); + } else { + targetGroup = belongsTo.get(0); + targetGroup.add(activity); + + if (belongsTo.size() > 1) { + for (int i = 1; i < belongsTo.size(); i++) { + ActivitiesGroup groupToMerge = belongsTo.get(i); + mergeAndRemoveGroup(targetGroup, groupToMerge); + fastSwitchGroup(groupToMerge, targetGroup); + } } + updateLookupKeys(targetGroup); } - updateLookupKeys(targetGroup); } mapGroupToTitle(targetGroup, workSummary); } @@ -92,14 +96,13 @@ public List getGroupingSuggestions(String orcid) { private void fastSwitchGroup(ActivitiesGroup oldGroup, ActivitiesGroup newGroup) { Set titles = groupToTitles.remove(oldGroup); if (titles != null) { + Set newGroupTitles = groupToTitles.computeIfAbsent(newGroup, k -> new HashSet<>()); for (String title : titles) { Set groupsForTitle = titleToGroups.get(title); if (groupsForTitle != null) { groupsForTitle.remove(oldGroup); groupsForTitle.add(newGroup); } - - Set newGroupTitles = groupToTitles.computeIfAbsent(newGroup, k -> new HashSet<>()); newGroupTitles.add(title); } } From 3dd3c522142329aedf0ac7140198df47afb92e6c Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 10:05:10 -0600 Subject: [PATCH 11/15] Remove system outs --- .../impl/WorkManagerReadOnlyImpl.java | 30 +--- ...upAndGroupingSuggestionGeneratorJunie.java | 134 ------------------ .../controllers/PublicProfileController.java | 6 +- 3 files changed, 3 insertions(+), 167 deletions(-) delete mode 100644 orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java diff --git a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java index 6b151213ef0..e37d95a2830 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/v3/read_only/impl/WorkManagerReadOnlyImpl.java @@ -176,7 +176,6 @@ public WorkSummary getWorkSummary(String orcid, Long workId) { @Override public List getWorksSummaryList(String orcid) { List works = workEntityCacheManager.retrieveMinimizedWorks(orcid, getLastModified(orcid)); - long t0 = System.currentTimeMillis(); Set clientIds = works.stream() .map(MinimizedWorkEntity::getClientSourceId) .filter(clientId -> !PojoUtil.isEmpty(clientId)) @@ -197,11 +196,7 @@ public List getWorksSummaryList(String orcid) { } // This map should be read-only - Map readOnlySources = Collections.unmodifiableMap(sources); - List list = jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, readOnlySources); - long t1 = System.currentTimeMillis(); - System.out.println("Time to convert to JAXB WorkSummary: " + (t1 - t0)); - return list; + return jpaJaxbWorkAdapter.toWorkSummaryFromMinimized(works, Collections.unmodifiableMap(sources)); } /** @@ -215,7 +210,6 @@ public List getWorksSummaryList(String orcid) { public List getWorksSummaryExtendedList(String orcid, boolean featuredOnly) { List wseList = retrieveWorkSummaryExtended(orcid, featuredOnly); // Filter the contributors list - long t0 = System.currentTimeMillis(); for (WorkSummaryExtended wse : wseList) { if (wse.getContributorsGroupedByOrcid() != null && wse.getContributorsGroupedByOrcid().size() > 0) { wse.setNumberOfContributors(wse.getContributorsGroupedByOrcid().size()); @@ -226,15 +220,12 @@ public List getWorksSummaryExtendedList(String orcid, boole wse.setNumberOfContributors(contributorsGroupedByOrcid.size()); } } - long t1 = System.currentTimeMillis(); - System.out.println("Time on getWorksSummaryExtendedList: " + (t1 - t0)); return wseList; } private List retrieveWorkSummaryExtended(String orcid, boolean featuredOnly) { List workSummaryExtendedList = new ArrayList<>(); Map isUserOBOEnabled = new HashMap(); - long l0 = System.currentTimeMillis(); List list = workDao.getWorksByOrcid(orcid, featuredOnly); for (Object[] q1 : list) { BigInteger putCode = (BigInteger) q1[0]; @@ -306,8 +297,6 @@ private List retrieveWorkSummaryExtended(String orcid, bool .build(); workSummaryExtendedList.add(wse); } - long l1 = System.currentTimeMillis(); - System.out.println("Time to retrieve WorkSummaryExtended: " + (l1 - l0)); return workSummaryExtendedList; } @@ -347,28 +336,13 @@ public Works groupWorksAndGenerateGroupingSuggestions(List summarie @Override public WorksExtended groupWorksExtendedAndGenerateGroupingSuggestions(List summaries, String orcid) { - long t0 = System.currentTimeMillis(); - long t02 = System.currentTimeMillis(); - WorkGroupAndGroupingSuggestionGeneratorJunie groupGenerator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); + WorkGroupAndGroupingSuggestionGenerator groupGenerator = new WorkGroupAndGroupingSuggestionGenerator(); for (WorkSummaryExtended work : summaries) { - if(work.getTitle().getTitle().getContent().equals("Title 1000000011") || work.getTitle().getTitle().getContent().equals("Title 1000000015") || work.getTitle().getTitle().getContent().equals("Title 1000000045") || work.getTitle().getTitle().getContent().equals("Title 1000000053")) { - System.out.println("PROCESSING: " + work.getTitle().getTitle().getContent()); - } groupGenerator.group(work); } - long t03 = System.currentTimeMillis(); - System.out.println("GROUPING WITH JUNIE: " + (t03 - t02)); - long t1 = System.currentTimeMillis(); WorksExtended works = processGroupedWorksExtended(groupGenerator.getGroups()); - long t2 = System.currentTimeMillis(); List suggestions = groupGenerator.getGroupingSuggestions(orcid); - long t3 = System.currentTimeMillis(); groupingSuggestionsManager.cacheGroupingSuggestions(orcid, suggestions); - long t4 = System.currentTimeMillis(); - System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 1: " + (t1 - t0)); - System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 2: " + (t2 - t1)); - System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 3: " + (t3 - t2)); - System.out.println("Time on groupWorksExtendedAndGenerateGroupingSuggestions 4: " + (t4 - t3)); return works; } diff --git a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java b/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java deleted file mode 100644 index edad105e0a3..00000000000 --- a/orcid-core/src/main/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunie.java +++ /dev/null @@ -1,134 +0,0 @@ -package org.orcid.core.utils.v3.activities; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; - -import org.orcid.jaxb.model.v3.release.record.ExternalID; -import org.orcid.jaxb.model.v3.release.record.GroupableActivity; -import org.orcid.jaxb.model.v3.release.record.WorkTitle; -import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; -import org.orcid.pojo.WorkSummaryExtended; -import org.orcid.pojo.grouping.WorkGroupingSuggestion; - -public class WorkGroupAndGroupingSuggestionGeneratorJunie extends ActivitiesGroupGenerator { - - // Optimized mapping: group -> set of titles it is associated with - private Map> groupToTitles = new HashMap<>(); - // Optimized mapping: title -> set of groups associated with it - private Map> titleToGroups = new HashMap<>(); - - @Override - public void group(GroupableActivity activity) { - if (!(activity instanceof WorkSummary || activity instanceof WorkSummaryExtended)) { - throw new IllegalArgumentException("Argument must be of type WorkSummary"); - } - - WorkSummary workSummary = null; - if (activity instanceof WorkSummaryExtended) { - workSummary = (WorkSummaryExtended) activity; - } else if (activity instanceof WorkSummary) { - workSummary = (WorkSummary) activity; - } - - ActivitiesGroup targetGroup; - if (groups.isEmpty()) { - targetGroup = createNewGroup(activity); - } else { - List belongsTo = generateBelongsToList(activity); - - if (belongsTo.isEmpty()) { - targetGroup = createNewGroup(activity); - } else { - targetGroup = belongsTo.get(0); - targetGroup.add(activity); - - if (belongsTo.size() > 1) { - for (int i = 1; i < belongsTo.size(); i++) { - ActivitiesGroup groupToMerge = belongsTo.get(i); - mergeAndRemoveGroup(targetGroup, groupToMerge); - fastSwitchGroup(groupToMerge, targetGroup); - } - } - updateLookupKeys(targetGroup); - } - } - mapGroupToTitle(targetGroup, workSummary); - } - - public List getGroupingSuggestions(String orcid) { - List suggestions = new ArrayList<>(); - for (Map.Entry> entry : titleToGroups.entrySet()) { - Set groupsForTitle = entry.getValue(); - if (groupsForTitle.size() > 1) { - WorkGroupingSuggestion suggestion = new WorkGroupingSuggestion(); - suggestion.setOrcid(orcid); - List putCodes = new ArrayList<>(); - - boolean groupableExternalIdFound = false; - for (ActivitiesGroup group : groupsForTitle) { - for (GroupableActivity activity : group.getActivities()) { - WorkSummary workSummary = (WorkSummary) activity; - putCodes.add(workSummary.getPutCode()); - if (!groupableExternalIdFound && workSummary.getExternalIdentifiers() != null) { - for (ExternalID externalId : workSummary.getExternalIdentifiers().getExternalIdentifier()) { - if (externalId.isGroupAble()) { - groupableExternalIdFound = true; - break; - } - } - } - } - } - - if (groupableExternalIdFound) { - suggestion.setPutCodes(putCodes); - suggestions.add(suggestion); - } - } - } - return suggestions; - } - - private void fastSwitchGroup(ActivitiesGroup oldGroup, ActivitiesGroup newGroup) { - Set titles = groupToTitles.remove(oldGroup); - if (titles != null) { - Set newGroupTitles = groupToTitles.computeIfAbsent(newGroup, k -> new HashSet<>()); - for (String title : titles) { - Set groupsForTitle = titleToGroups.get(title); - if (groupsForTitle != null) { - groupsForTitle.remove(oldGroup); - groupsForTitle.add(newGroup); - } - newGroupTitles.add(title); - } - } - } - - private void mapGroupToTitle(ActivitiesGroup group, WorkSummary workSummary) { - if (!workTitleEmpty(workSummary.getTitle())) { - String title = transformForTitleComparison(workSummary.getTitle().getTitle().getContent()); - - titleToGroups.computeIfAbsent(title, k -> new HashSet<>()).add(group); - groupToTitles.computeIfAbsent(group, k -> new HashSet<>()).add(title); - } - } - - private String transformForTitleComparison(String titleContent) { - return titleContent.toLowerCase().replaceAll("\\s", ""); - } - - private boolean workTitleEmpty(WorkTitle workTitle) { - return workTitle == null || workTitle.getTitle() == null || workTitle.getTitle().getContent() == null || workTitle.getTitle().getContent().isEmpty(); - } - - @Override - protected void mergeAndRemoveGroup(ActivitiesGroup keep, ActivitiesGroup discard) { - super.mergeAndRemoveGroup(keep, discard); - // We don't remove from titleToGroups/groupToTitles here because fastSwitchGroup handles it - // and it's called immediately after in group() - } -} diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java index 554f22db1a8..b4a797485ca 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/PublicProfileController.java @@ -473,11 +473,7 @@ private boolean isRecordReadyForIndexing(ProfileEntity profile) { } catch (Exception e) { return new Page(); } - long l0 = System.currentTimeMillis(); - Page r = worksPaginator.getWorksExtendedPage(orcid, offset, pageSize, true, sort, sortAsc); - long l1 = System.currentTimeMillis(); - System.out.println("Total time taken: " + (l1 - l0)); - return r; + return worksPaginator.getWorksExtendedPage(orcid, offset, pageSize, true, sort, sortAsc); } @RequestMapping(value = "/{orcid:(?:\\d{4}-){3,}\\d{3}[\\dX]}/featuredWorks.json", method = RequestMethod.GET) From e3a6e4b6bc7cb21cf18c0e0d4a938d868f68922e Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 10:07:34 -0600 Subject: [PATCH 12/15] Remove system outs --- ...dGroupingSuggestionGeneratorJunieTest.java | 301 ------------------ .../web/pagination/WorksPaginator.java | 3 - 2 files changed, 304 deletions(-) delete mode 100644 orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java diff --git a/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java b/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java deleted file mode 100644 index 8ca74e95238..00000000000 --- a/orcid-core/src/test/java/org/orcid/core/utils/v3/activities/WorkGroupAndGroupingSuggestionGeneratorJunieTest.java +++ /dev/null @@ -1,301 +0,0 @@ -package org.orcid.core.utils.v3.activities; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.stream.Collectors; - -import org.junit.Test; -import org.orcid.jaxb.model.common.Relationship; -import org.orcid.jaxb.model.v3.release.common.Title; -import org.orcid.jaxb.model.v3.release.record.ExternalID; -import org.orcid.jaxb.model.v3.release.record.ExternalIDs; -import org.orcid.jaxb.model.v3.release.record.WorkTitle; -import org.orcid.jaxb.model.v3.release.record.summary.WorkSummary; -import org.orcid.pojo.grouping.WorkGroupingSuggestion; - -public class WorkGroupAndGroupingSuggestionGeneratorJunieTest { - - @Test - public void testFourGroupsOneSuggestion() { - WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); - for (WorkSummary summary : getWorkSummariesFourGroupsOneSuggestion()) { - generator.group(summary); - } - assertEquals(4, generator.getGroups().size()); - - List suggestions = generator.getGroupingSuggestions("orcid"); - assertEquals(1, suggestions.size()); - assertPutCodesEqual("4,7", suggestions.get(0).getPutCodesAsString()); - assertEquals("orcid", suggestions.get(0).getOrcid()); - } - - @Test - public void testFiveGroupsTwoSuggestions() { - WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); - for (WorkSummary summary : getWorkSummariesFiveGroupsTwoSuggestions()) { - generator.group(summary); - } - assertEquals(5, generator.getGroups().size()); - - List suggestions = generator.getGroupingSuggestions("orcid"); - assertEquals(2, suggestions.size()); - - List suggestionPutCodes = suggestions.stream() - .map(WorkGroupingSuggestion::getPutCodesAsString) - .collect(Collectors.toList()); - - assertTrue(containsPutCodes(suggestionPutCodes, "4,7")); - assertTrue(containsPutCodes(suggestionPutCodes, "1,2,3,8")); - - for (WorkGroupingSuggestion suggestion : suggestions) { - assertEquals("orcid", suggestion.getOrcid()); - } - } - - @Test - public void testNoGroupableIDs() { - WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); - for (WorkSummary summary : getWorkSummariesNoGroupableIds()) { - generator.group(summary); - } - List suggestions = generator.getGroupingSuggestions("orcid"); - assertEquals(0, suggestions.size()); - } - - @Test - public void testLargeNumberOfWorks() { - int numWorks = 2000; - List works = new ArrayList<>(); - for (int i = 0; i < numWorks; i++) { - WorkSummary summary = new WorkSummary(); - summary.setPutCode((long) i); - summary.setTitle(getWorkTitle("Title " + (i % 200))); // 200 unique titles - - ExternalIDs ids = new ExternalIDs(); - ids.getExternalIdentifier().add(getExternalID("doi", "doi-" + i)); // unique DOI - summary.setExternalIdentifiers(ids); - works.add(summary); - } - - // Warmup - for (int i=0; i<10; i++) { - WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); - for (WorkSummary summary : works) generator.group(summary); - generator.getGroupingSuggestions("orcid"); - - WorkGroupAndGroupingSuggestionGenerator original = new WorkGroupAndGroupingSuggestionGenerator(); - for (WorkSummary summary : works) original.group(summary); - original.getGroupingSuggestions("orcid"); - } - - long start = System.nanoTime(); - WorkGroupAndGroupingSuggestionGeneratorJunie generator = new WorkGroupAndGroupingSuggestionGeneratorJunie(); - for (WorkSummary summary : works) { - generator.group(summary); - } - generator.getGroupingSuggestions("orcid"); - long end = System.nanoTime(); - System.out.println("[DEBUG_LOG] Junie generator took " + (end - start) / 1000000.0 + "ms for " + numWorks + " works"); - - start = System.nanoTime(); - WorkGroupAndGroupingSuggestionGenerator original = new WorkGroupAndGroupingSuggestionGenerator(); - for (WorkSummary summary : works) { - original.group(summary); - } - original.getGroupingSuggestions("orcid"); - end = System.nanoTime(); - System.out.println("[DEBUG_LOG] Original generator took " + (end - start) / 1000000.0 + "ms for " + numWorks + " works"); - } - - private void assertPutCodesEqual(String expected, String actual) { - List expectedList = Arrays.asList(expected.split(",")); - List actualList = Arrays.asList(actual.split(",")); - Collections.sort(expectedList); - Collections.sort(actualList); - assertEquals(expectedList, actualList); - } - - private boolean containsPutCodes(List suggestionPutCodes, String target) { - List targetList = Arrays.asList(target.split(",")); - Collections.sort(targetList); - for (String s : suggestionPutCodes) { - List sList = Arrays.asList(s.split(",")); - Collections.sort(sList); - if (sList.equals(targetList)) { - return true; - } - } - return false; - } - - private List getWorkSummariesFourGroupsOneSuggestion() { - WorkSummary first = new WorkSummary(); - first.setPutCode(1L); - first.setTitle(getWorkTitle("a title")); - - ExternalIDs firstIds = new ExternalIDs(); - firstIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - first.setExternalIdentifiers(firstIds); - - WorkSummary second = new WorkSummary(); - second.setPutCode(2L); - second.setTitle(getWorkTitle("second title")); - - ExternalIDs secondIds = new ExternalIDs(); - secondIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - second.setExternalIdentifiers(secondIds); - - WorkSummary third = new WorkSummary(); - third.setPutCode(3L); - third.setTitle(getWorkTitle("SECOND title")); - - ExternalIDs thirdIds = new ExternalIDs(); - thirdIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - third.setExternalIdentifiers(thirdIds); - - WorkSummary fourth = new WorkSummary(); - fourth.setPutCode(4L); - fourth.setTitle(getWorkTitle("something totally different")); - - ExternalIDs fourthIds = new ExternalIDs(); - fourthIds.getExternalIdentifier().add(getExternalID("doi", "group2")); - fourth.setExternalIdentifiers(fourthIds); - - WorkSummary fifth = new WorkSummary(); - fifth.setPutCode(5L); - fifth.setTitle(getWorkTitle("third title")); - - ExternalIDs fifthIds = new ExternalIDs(); - fifthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); - fifth.setExternalIdentifiers(fifthIds); - - WorkSummary sixth = new WorkSummary(); - sixth.setPutCode(6L); - sixth.setTitle(getWorkTitle("3rd title")); - - ExternalIDs sixthIds = new ExternalIDs(); - sixthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); - sixth.setExternalIdentifiers(sixthIds); - - WorkSummary seventh = new WorkSummary(); - seventh.setPutCode(7L); - seventh.setTitle(getWorkTitle("SOMETHINGTOTALLYDIFFERENT")); - - ExternalIDs seventhIds = new ExternalIDs(); - seventhIds.getExternalIdentifier().add(getExternalID("doi", "group4")); - seventh.setExternalIdentifiers(seventhIds); - - return Arrays.asList(first, second, third, fourth, fifth, sixth, seventh); - } - - private List getWorkSummariesFiveGroupsTwoSuggestions() { - WorkSummary first = new WorkSummary(); - first.setPutCode(1L); - first.setTitle(getWorkTitle("a title")); - - ExternalIDs firstIds = new ExternalIDs(); - firstIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - first.setExternalIdentifiers(firstIds); - - WorkSummary second = new WorkSummary(); - second.setPutCode(2L); - second.setTitle(getWorkTitle("second title")); - - ExternalIDs secondIds = new ExternalIDs(); - secondIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - second.setExternalIdentifiers(secondIds); - - WorkSummary third = new WorkSummary(); - third.setPutCode(3L); - third.setTitle(getWorkTitle("SECOND title")); - - ExternalIDs thirdIds = new ExternalIDs(); - thirdIds.getExternalIdentifier().add(getExternalID("doi", "group1")); - third.setExternalIdentifiers(thirdIds); - - WorkSummary fourth = new WorkSummary(); - fourth.setPutCode(4L); - fourth.setTitle(getWorkTitle("something totally different")); - - ExternalIDs fourthIds = new ExternalIDs(); - fourthIds.getExternalIdentifier().add(getExternalID("doi", "group2")); - fourth.setExternalIdentifiers(fourthIds); - - WorkSummary fifth = new WorkSummary(); - fifth.setPutCode(5L); - fifth.setTitle(getWorkTitle("third title")); - - ExternalIDs fifthIds = new ExternalIDs(); - fifthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); - fifth.setExternalIdentifiers(fifthIds); - - WorkSummary sixth = new WorkSummary(); - sixth.setPutCode(6L); - sixth.setTitle(getWorkTitle("3rd title")); - - ExternalIDs sixthIds = new ExternalIDs(); - sixthIds.getExternalIdentifier().add(getExternalID("doi", "group3")); - sixth.setExternalIdentifiers(sixthIds); - - WorkSummary seventh = new WorkSummary(); - seventh.setPutCode(7L); - seventh.setTitle(getWorkTitle("SOMETHINGTOTALLYDIFFERENT")); - - ExternalIDs seventhIds = new ExternalIDs(); - seventhIds.getExternalIdentifier().add(getExternalID("doi", "group4")); - seventh.setExternalIdentifiers(seventhIds); - - WorkSummary eighth = new WorkSummary(); - eighth.setPutCode(8L); - eighth.setTitle(getWorkTitle("ATITLE")); - - ExternalIDs eighthIds = new ExternalIDs(); - eighthIds.getExternalIdentifier().add(getExternalID("doi", "group5")); - eighth.setExternalIdentifiers(eighthIds); - - return Arrays.asList(first, second, third, fourth, fifth, sixth, seventh, eighth); - } - - private List getWorkSummariesNoGroupableIds() { - WorkSummary first = new WorkSummary(); - first.setPutCode(1L); - first.setTitle(getWorkTitle("a title")); - - ExternalIDs firstIds = new ExternalIDs(); - ExternalID externalID = getExternalID("issn", "1234-5678"); - externalID.setRelationship(Relationship.PART_OF); - firstIds.getExternalIdentifier().add(externalID); - first.setExternalIdentifiers(firstIds); - - WorkSummary second = new WorkSummary(); - second.setPutCode(2L); - second.setTitle(getWorkTitle("second title")); - - ExternalIDs secondIds = new ExternalIDs(); - ExternalID secondExternalID = getExternalID("issn", "1234-5678"); - secondExternalID.setRelationship(Relationship.PART_OF); - secondIds.getExternalIdentifier().add(secondExternalID); - second.setExternalIdentifiers(secondIds); - - return Arrays.asList(first, second); - } - - private WorkTitle getWorkTitle(String title) { - WorkTitle workTitle = new WorkTitle(); - workTitle.setTitle(new Title(title)); - return workTitle; - } - - private ExternalID getExternalID(String type, String value) { - ExternalID id = new ExternalID(); - id.setType(type); - id.setValue(value); - return id; - } - -} diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java b/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java index ba2e61aab51..a0e2892bb94 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/pagination/WorksPaginator.java @@ -70,7 +70,6 @@ public Page getWorksPage(String orcid, int offset, int pageSize, bool public Page getWorksExtendedPage(String orcid, int offset, int pageSize, boolean justPublic, String sort, boolean sortAsc) { WorksExtended works = worksExtendedCacheManager.getGroupedWorksExtended(orcid); Page worksPage = new Page(); - long t0 = System.currentTimeMillis(); if (works != null) { List filteredGroups = filterWorksExtended(works, justPublic); if ("source".equals(sort)) { @@ -89,8 +88,6 @@ public Page getWorksExtendedPage(String orcid, int offset, int pageSi worksPage.setGroups(workGroups); worksPage.setNextOffset(offset + pageSize); } - long t1 = System.currentTimeMillis(); - System.out.println("Time on getWorksExtendedPage: " + (t1 - t0)); return worksPage; } From 2606f11ba0350b0299d5ab20e2f307e8822a1c92 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 11:02:53 -0600 Subject: [PATCH 13/15] Clean imports --- .../org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java index 033d39781d7..0f4885a8072 100644 --- a/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java +++ b/orcid-core/src/main/java/org/orcid/core/adapter/v3/impl/MapperFacadeFactory.java @@ -31,12 +31,9 @@ import org.orcid.core.contributors.roles.works.WorkContributorRoleConverter; import org.orcid.core.exception.OrcidValidationException; import org.orcid.core.locale.LocaleManager; -import org.orcid.core.manager.ClientDetailsEntityCacheManager; import org.orcid.core.manager.EncryptionManager; import org.orcid.core.manager.IdentityProviderManager; -import org.orcid.core.manager.SourceNameCacheManager; import org.orcid.core.manager.impl.OrcidUrlManager; -import org.orcid.core.manager.v3.read_only.ClientDetailsManagerReadOnly; import org.orcid.core.utils.JsonUtils; import org.orcid.core.utils.SourceEntityUtils; import org.orcid.core.utils.v3.identifiers.PIDNormalizationService; @@ -140,7 +137,6 @@ import org.orcid.persistence.jpa.entities.SpamEntity; import org.orcid.persistence.jpa.entities.StartDateEntity; import org.orcid.persistence.jpa.entities.WorkEntity; -import org.orcid.persistence.jpa.entities.keys.ClientRedirectUriPk; import org.orcid.pojo.WorkExtended; import org.orcid.pojo.WorkSummaryExtended; import org.orcid.pojo.ajaxForm.PojoUtil; From eb40cc31a895c06ef2ad4e95ff66f860ba716026 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 20:44:51 -0600 Subject: [PATCH 14/15] Fix test by refactoring it to use mocks, so, we dont need transactional tests --- .../ManageMembersControllerTest.java | 571 ++++++++---------- 1 file changed, 252 insertions(+), 319 deletions(-) diff --git a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java index 4b2275aa7ca..bf62fd745ae 100644 --- a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java +++ b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java @@ -1,147 +1,93 @@ package org.orcid.frontend.web.controllers; -/** - * @author Angel Montenegro (amontenegro) Date: 29/08/2013 - */ -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import static org.mockito.Mockito.when; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; - -import javax.annotation.Resource; -import javax.transaction.Transactional; - -import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.MockitoAnnotations; -import org.orcid.core.common.manager.EmailFrequencyManager; -import org.orcid.core.exception.ClientAlreadyActiveException; -import org.orcid.core.exception.ClientAlreadyDeactivatedException; +import org.mockito.junit.MockitoJUnitRunner; +import org.orcid.core.locale.LocaleManager; import org.orcid.core.manager.v3.ClientDetailsManager; +import org.orcid.core.manager.v3.ClientManager; +import org.orcid.core.manager.v3.EmailManager; import org.orcid.core.manager.v3.MembersManager; -import org.orcid.core.manager.v3.ProfileHistoryEventManager; -import org.orcid.core.manager.v3.SourceManager; +import org.orcid.core.manager.v3.read_only.ClientDetailsManagerReadOnly; +import org.orcid.core.manager.v3.read_only.ClientManagerReadOnly; import org.orcid.core.security.OrcidRoles; import org.orcid.jaxb.model.clientgroup.MemberType; -import org.orcid.persistence.dao.ClientDetailsDao; -import org.orcid.persistence.dao.ProfileDao; -import org.orcid.persistence.jpa.entities.ProfileEntity; -import org.orcid.persistence.jpa.entities.SourceEntity; +import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.pojo.ClientActivationRequest; import org.orcid.pojo.ajaxForm.Client; import org.orcid.pojo.ajaxForm.Member; -import org.orcid.pojo.ajaxForm.PojoUtil; import org.orcid.pojo.ajaxForm.RedirectUri; import org.orcid.pojo.ajaxForm.Text; -import org.orcid.test.DBUnitTest; -import org.orcid.test.OrcidJUnit4ClassRunner; -import org.orcid.test.TargetProxyHelper; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.User; import org.springframework.security.core.userdetails.UserDetails; -import org.springframework.test.context.ContextConfiguration; -import org.springframework.test.context.web.WebAppConfiguration; -import org.springframework.test.util.ReflectionTestUtils; -@RunWith(OrcidJUnit4ClassRunner.class) -@WebAppConfiguration -@ContextConfiguration(locations = { "classpath:test-frontend-web-servlet.xml" }) -public class ManageMembersControllerTest extends DBUnitTest { +import java.util.ArrayList; +import java.util.Date; +import java.util.List; - @Resource(name = "membersManagerV3") +import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class ManageMembersControllerTest { + + @Mock(name = "membersManagerV3") MembersManager membersManager; - - @Resource - ManageMembersController manageMembers; - @Resource - private ProfileDao profileDao; + @Mock(name = "emailManagerV3") + EmailManager emailManager; - @Resource - ClientsController groupAdministratorController; + @Mock(name = "clientManagerV3") + private ClientManager clientManager; + + @Mock(name = "clientManagerReadOnlyV3") + private ClientManagerReadOnly clientManagerReadOnly; + + @Mock(name = "clientDetailsManagerReadOnlyV3") + private ClientDetailsManagerReadOnly clientDetailsManagerReadOnly; + + @Mock(name = "clientDetailsManagerV3") + private ClientDetailsManager clientDetailsManager; - @Resource - ClientDetailsDao clientDetailsDao; - - @Resource(name = "sourceManagerV3") - SourceManager sourceManager; - @Mock - SourceManager mockSourceManager; - - @Mock - ProfileHistoryEventManager mockProfileHistoryEventManager; - - @Resource - ProfileHistoryEventManager profileHistoryEventManager; - - @Resource - EmailFrequencyManager emailFrequencyManager; - + ClientsController groupAdministratorController; + @Mock - private ClientDetailsManager mockClientDetailsManager; - - @Resource(name = "clientDetailsManagerV3") - private ClientDetailsManager clientDetailsManager; - + protected LocaleManager localeManager; + + @InjectMocks + ManageMembersController manageMembers; + @Before public void beforeInstance() { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - MockitoAnnotations.initMocks(this); - TargetProxyHelper.injectIntoProxy(membersManager, "sourceManager", mockSourceManager); - TargetProxyHelper.injectIntoProxy(emailFrequencyManager, "profileHistoryEventManager", mockProfileHistoryEventManager); - - SourceEntity sourceEntity = new SourceEntity(); - sourceEntity.setSourceProfile(new ProfileEntity("5555-5555-5555-0000")); - when(mockSourceManager.retrieveActiveSourceEntity()).thenReturn(sourceEntity); + when(localeManager.resolveMessage(anyString(), any())).thenAnswer(invocation -> { + return invocation.getArgument(0); + }); } - - @BeforeClass - public static void beforeClass() throws Exception { - initDBUnitData(Arrays.asList("/data/EmptyEntityData.xml", "/data/PremiumInstitutionMemberData.xml")); - } - - @After - public void after() { - TargetProxyHelper.injectIntoProxy(membersManager, "sourceManager", sourceManager); - TargetProxyHelper.injectIntoProxy(emailFrequencyManager, "profileHistoryEventManager", profileHistoryEventManager); - } - - @AfterClass - public static void afterClass() throws Exception { - removeDBUnitData(Arrays.asList("/data/PremiumInstitutionMemberData.xml")); - } - - protected Authentication getAuthentication() { + + protected Authentication getAuthentication() { String orcid = "4444-4444-4444-4440"; - UserDetails details = new User(orcid, - "password", Arrays.asList(new SimpleGrantedAuthority(OrcidRoles.ROLE_ADMIN.name()))); + UserDetails details = new User(orcid, "password", List.of(new SimpleGrantedAuthority(OrcidRoles.ROLE_ADMIN.name()))); UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken(orcid, "password", details.getAuthorities()); auth.setDetails(details); return auth; } - - @Test - @Transactional + + @Test public void createMemberProfileWithInvalidEmailsTest() throws Exception { String existingEmail = "premium_institution@group.com"; - + Member group = new Member(); group.setGroupName(Text.valueOf("Group Name")); group.setType(Text.valueOf("basic")); @@ -149,24 +95,25 @@ public void createMemberProfileWithInvalidEmailsTest() throws Exception { // Validate already existing email address group.setEmail(Text.valueOf(existingEmail)); + when(emailManager.emailExists(eq(existingEmail))).thenReturn(true); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.email.already_used", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.email.already_used", group.getErrors().get(0)); // Validate empty email address group.setEmail(Text.valueOf("")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("NotBlank.group.email", new ArrayList()), group.getErrors().get(0)); + assertEquals("NotBlank.group.email", group.getErrors().get(0)); // Validate invalid email address group.setEmail(Text.valueOf("invalidemail")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.email.invalid_email", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.email.invalid_email", group.getErrors().get(0)); } - @Test + @Test public void createMemberProfileWithInvalidGroupNameTest() throws Exception { Member group = new Member(); group.setEmail(Text.valueOf("group@email.com")); @@ -177,17 +124,17 @@ public void createMemberProfileWithInvalidGroupNameTest() throws Exception { group.setGroupName(Text.valueOf("")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("NotBlank.group.name", new ArrayList()), group.getErrors().get(0)); + assertEquals("NotBlank.group.name", group.getErrors().get(0)); // validate too long group name - 151 chars - group.setGroupName(Text - .valueOf("1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901")); + group.setGroupName(Text.valueOf( + "1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.name.too_long", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.name.too_long", group.getErrors().get(0)); } - @Test + @Test public void createMemberProfileWithInvalidTypeTest() throws Exception { Member group = new Member(); group.setEmail(Text.valueOf("group@email.com")); @@ -198,70 +145,70 @@ public void createMemberProfileWithInvalidTypeTest() throws Exception { group.setType(Text.valueOf("")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("NotBlank.group.type", new ArrayList()), group.getErrors().get(0)); + assertEquals("NotBlank.group.type", group.getErrors().get(0)); // Validate invalid type group.setType(Text.valueOf("invalid")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.type.invalid", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.type.invalid", group.getErrors().get(0)); } - - @Test + + @Test public void createMemberProfileWithInvalidSalesforceIdTest() throws Exception { Member group = new Member(); group.setEmail(Text.valueOf("group@email.com")); group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("basic")); + group.setType(Text.valueOf("basic")); - // Validate empty type + // Validate empty type group.setSalesforceId(Text.valueOf("1")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.salesforce_id.invalid_length", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.salesforce_id.invalid_length", group.getErrors().get(0)); - // Validate invalid type + // Validate invalid type group.setSalesforceId(Text.valueOf("1234567890abcd!")); group = manageMembers.createMember(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.salesforce_id.invalid", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.salesforce_id.invalid", group.getErrors().get(0)); } - @Test + @Test public void createMemberProfileTest() throws Exception { Member group = new Member(); - String email = "group" + System.currentTimeMillis() + "@email.com"; + String email = "group@email.com"; group.setEmail(Text.valueOf(email)); group.setGroupName(Text.valueOf("Group Name")); group.setType(Text.valueOf("premium-institution")); group.setSalesforceId(Text.valueOf("")); + + Member createdMember = new Member(); + createdMember.setGroupOrcid(Text.valueOf("5555-5555-5555-5555")); + + when(membersManager.createMember(any(Member.class))).thenReturn(createdMember); + group = manageMembers.createMember(group); assertEquals(0, group.getErrors().size()); - assertFalse(PojoUtil.isEmpty(group.getGroupOrcid())); + assertEquals("5555-5555-5555-5555", group.getGroupOrcid().getValue()); } - @Test + @Test public void findMemberByOrcidTest() throws Exception { - Member group = new Member(); - String email = "group" + System.currentTimeMillis() + "@email.com"; - group.setEmail(Text.valueOf(email)); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); - group = manageMembers.createMember(group); - assertEquals(0, group.getErrors().size()); - assertFalse(PojoUtil.isEmpty(group.getGroupOrcid())); + String email = "group@email.com"; + String orcid = "5555-5555-5555-5555"; + Member member = new Member(); + member.setEmail(Text.valueOf(email)); + member.setGroupName(Text.valueOf("Group Name")); + member.setSalesforceId(Text.valueOf("1234567890abcde")); + member.setGroupOrcid(Text.valueOf(orcid)); + + when(membersManager.getMember(orcid)).thenReturn(member); + when(membersManager.getMember(email)).thenReturn(member); // Test find by orcid - String orcid = group.getGroupOrcid().getValue(); Member newGroup = manageMembers.findMember(orcid); assertNotNull(newGroup); - - assertFalse(PojoUtil.isEmpty(newGroup.getGroupOrcid())); - assertFalse(PojoUtil.isEmpty(newGroup.getEmail())); - assertFalse(PojoUtil.isEmpty(newGroup.getSalesforceId())); - assertFalse(PojoUtil.isEmpty(newGroup.getGroupName())); - assertEquals(email, newGroup.getEmail().getValue()); assertEquals("Group Name", newGroup.getGroupName().getValue()); assertEquals("1234567890abcde", newGroup.getSalesforceId().getValue()); @@ -270,275 +217,261 @@ public void findMemberByOrcidTest() throws Exception { // Test find by email Member newGroup2 = manageMembers.findMember(email); assertNotNull(newGroup2); - - assertFalse(PojoUtil.isEmpty(newGroup2.getGroupOrcid())); - assertFalse(PojoUtil.isEmpty(newGroup2.getEmail())); - assertFalse(PojoUtil.isEmpty(newGroup2.getSalesforceId())); - assertFalse(PojoUtil.isEmpty(newGroup2.getGroupName())); - assertEquals(email, newGroup2.getEmail().getValue()); - assertEquals("Group Name", newGroup2.getGroupName().getValue()); - assertEquals("1234567890abcde", newGroup2.getSalesforceId().getValue()); assertEquals(orcid, newGroup2.getGroupOrcid().getValue()); // Test: Find member by ORCID with clients and check deactivated status - Member newGroup3 = manageMembers.findMember("5555-5555-5555-0000"); + Member memberWithClients = new Member(); + List clients = new ArrayList<>(); + clients.add(createClient("APP-0000000000000001", false)); + clients.add(createClient("APP-0000000000000002", false)); + clients.add(createClient("APP-0000000000000003", true)); + memberWithClients.setClients(clients); - List clients = newGroup3.getClients(); - - Client activeClient1 = findClientById(clients, "APP-0000000000000001"); - Client activeClient2 = findClientById(clients, "APP-0000000000000002"); - Client deactivatedClient = findClientById(clients, "APP-0000000000000003"); + when(membersManager.getMember("5555-5555-5555-0000")).thenReturn(memberWithClients); + Member newGroup3 = manageMembers.findMember("5555-5555-5555-0000"); assertNotNull(newGroup3); + List clientsResult = newGroup3.getClients(); + assertEquals(3, clientsResult.size()); + assertFalse(findClientById(clientsResult, "APP-0000000000000001").isDeactivated()); + assertFalse(findClientById(clientsResult, "APP-0000000000000002").isDeactivated()); + assertTrue(findClientById(clientsResult, "APP-0000000000000003").isDeactivated()); + } - assertEquals(3, clients.size()); - assertEquals(false, activeClient1.isDeactivated()); - assertEquals(false, activeClient2.isDeactivated()); - assertEquals(true, deactivatedClient.isDeactivated()); + private Client createClient(String id, boolean deactivated) { + Client c = new Client(); + c.setClientId(Text.valueOf(id)); + c.setDeactivated(deactivated); + return c; } - - + @Test public void editMemberTest() throws Exception { + String orcid = "5555-5555-5555-5555"; Member group = new Member(); - group.setEmail(Text.valueOf("group@email.com")); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); - group = manageMembers.createMember(group); - assertEquals(0, group.getErrors().size()); - assertFalse(PojoUtil.isEmpty(group.getGroupOrcid())); - + group.setGroupOrcid(Text.valueOf(orcid)); group.setEmail(Text.valueOf("new_email@user.com")); - group.setSalesforceId(Text.valueOf("")); group.setGroupName(Text.valueOf("Updated Group Name")); - + group.setType(Text.valueOf("premium-institution")); + group.setSalesforceId(Text.valueOf("1234567890abcde")); + + when(membersManager.getMember(orcid)).thenReturn(group); + manageMembers.updateMember(group); - Member updatedGroup = manageMembers.findMember(group.getGroupOrcid().getValue()); + Member updatedGroup = manageMembers.findMember(orcid); assertNotNull(updatedGroup); - assertEquals(group.getGroupOrcid().getValue(), updatedGroup.getGroupOrcid().getValue()); + assertEquals(orcid, updatedGroup.getGroupOrcid().getValue()); assertEquals("Updated Group Name", updatedGroup.getGroupName().getValue()); } - + @Test public void editMemberWithInvalidEmailTest() throws Exception { - //Create one member + String email = "group1@email.com"; + when(emailManager.emailExists(email)).thenReturn(true); + Member group = new Member(); - String email = "group1" + System.currentTimeMillis() + "@email.com"; - group.setEmail(Text.valueOf(email)); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); - group = manageMembers.createMember(group); - assertNotNull(group); - assertEquals(0, group.getErrors().size()); - //Try to create another member with the same email - group = new Member(); group.setEmail(Text.valueOf(email)); group.setGroupName(Text.valueOf("Group Name")); group.setType(Text.valueOf("premium-institution")); group.setSalesforceId(Text.valueOf("1234567890abcde")); + group = manageMembers.createMember(group); assertNotNull(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.email.already_used", new ArrayList()), group.getErrors().get(0)); + assertEquals("group.email.already_used", group.getErrors().get(0)); } - + @Test public void editMemberWithInvalidSalesforceIdTest() throws Exception { - //Create one member Member group = new Member(); - String email = "group" + System.currentTimeMillis() + "@email.com"; - group.setEmail(Text.valueOf(email)); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); - group = manageMembers.createMember(group); - assertNotNull(group); - assertEquals(0, group.getErrors().size()); - //Try to create another member with the same email - group = new Member(); group.setEmail(Text.valueOf("group2@email.com")); group.setGroupName(Text.valueOf("Group Name")); group.setType(Text.valueOf("premium-institution")); group.setSalesforceId(Text.valueOf("1234567890abcd!")); + group = manageMembers.createMember(group); assertNotNull(group); assertEquals(1, group.getErrors().size()); - assertEquals(manageMembers.getMessage("group.salesforce_id.invalid", new ArrayList()), group.getErrors().get(0)); - } - - @Test + assertEquals("group.salesforce_id.invalid", group.getErrors().get(0)); + } + + @Test public void findClientTest() throws Exception { - //Client with all redirect uris default - Client client_0002 = manageMembers.findClient("APP-0000000000000002"); - assertNotNull(client_0002); - assertNotNull(client_0002.getDisplayName()); - assertEquals("Client # 2", client_0002.getDisplayName().getValue()); - assertNotNull(client_0002.getRedirectUris()); - assertEquals(1, client_0002.getRedirectUris().size()); - assertEquals("http://www.google.com/APP-0000000000000002/redirect/oauth", client_0002.getRedirectUris().get(0).getValue().getValue()); - assertEquals(false, client_0002.isDeactivated()); - - //Client with redirect uri not default - Client client_0003 = manageMembers.findClient("APP-0000000000000003"); - assertNotNull(client_0003); - assertNotNull(client_0003.getDisplayName()); - assertEquals("Client # 3", client_0003.getDisplayName().getValue()); - assertNotNull(client_0003.getRedirectUris()); - assertEquals(2, client_0003.getRedirectUris().size()); - assertEquals(true, client_0003.isDeactivated()); + String clientId2 = "APP-0000000000000002"; + + ClientDetailsEntity clientDetails2 = new ClientDetailsEntity(); + clientDetails2.setDeactivatedDate(null); + + org.orcid.jaxb.model.v3.release.client.Client modelClient2 = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(modelClient2.getId()).thenReturn(clientId2); + when(modelClient2.getName()).thenReturn("Client # 2"); + when(modelClient2.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + + when(clientManagerReadOnly.get(clientId2)).thenReturn(modelClient2); + when(clientDetailsManagerReadOnly.findByClientId(clientId2)).thenReturn(clientDetails2); + + Client result2 = manageMembers.findClient(clientId2); + assertNotNull(result2); + assertEquals("Client # 2", result2.getDisplayName().getValue()); + assertFalse(result2.isDeactivated()); + + String clientId3 = "APP-0000000000000003"; + org.orcid.jaxb.model.v3.release.client.Client modelClient3 = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(modelClient3.getId()).thenReturn(clientId3); + when(modelClient3.getName()).thenReturn("Client # 3"); + when(modelClient3.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); - RedirectUri rUri1 = client_0003.getRedirectUris().get(0); - if("http://www.google.com/APP-0000000000000003/redirect/oauth".equals(rUri1.getValue().getValue())) { - assertNotNull(rUri1.getType()); - assertEquals("default", rUri1.getType().getValue()); - assertNotNull(rUri1.getScopes()); - assertEquals(0, rUri1.getScopes().size()); - } else if ("http://www.google.com/APP-0000000000000003/redirect/oauth/grant_read_wizard".equals(rUri1.getValue().getValue())) { - assertNotNull(rUri1.getType()); - assertEquals("grant-read-wizard", rUri1.getType().getValue()); - assertNotNull(rUri1.getScopes()); - assertEquals(1, rUri1.getScopes().size()); - assertEquals("/funding/read-limited", rUri1.getScopes().get(0)); - } else { - fail("Invalid redirect uri: " + rUri1.getValue().getValue()); - } - - RedirectUri rUri2 = client_0003.getRedirectUris().get(1); - if("http://www.google.com/APP-0000000000000003/redirect/oauth".equals(rUri2.getValue().getValue())) { - assertNotNull(rUri2.getType()); - assertEquals("default", rUri2.getType().getValue()); - assertNotNull(rUri2.getScopes()); - assertEquals(0, rUri2.getScopes().size()); - } else if ("http://www.google.com/APP-0000000000000003/redirect/oauth/grant_read_wizard".equals(rUri2.getValue().getValue())) { - assertNotNull(rUri2.getType()); - assertEquals("grant-read-wizard", rUri2.getType().getValue()); - assertNotNull(rUri2.getScopes()); - assertEquals(1, rUri2.getScopes().size()); - assertEquals("/funding/read-limited", rUri2.getScopes().get(0)); - } else { - fail("Invalid redirect uri: " + rUri2.getValue().getValue()); - } + ClientDetailsEntity clientDetails3 = new ClientDetailsEntity(); + clientDetails3.setDeactivatedDate(new Date()); + + when(clientManagerReadOnly.get(clientId3)).thenReturn(modelClient3); + when(clientDetailsManagerReadOnly.findByClientId(clientId3)).thenReturn(clientDetails3); + + Client result3 = manageMembers.findClient(clientId3); + assertNotNull(result3); + assertTrue(result3.isDeactivated()); } - - @Test + + @Test public void editClientWithInvalidRedirectUriTest() throws Exception { - //Client with all redirect uris default - Client client_0002 = manageMembers.findClient("APP-0000000000000002"); - assertNotNull(client_0002); + String clientId = "APP-0000000000000002"; + org.orcid.jaxb.model.v3.release.client.Client modelClient = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(modelClient.getId()).thenReturn(clientId); + when(modelClient.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); + + when(clientManagerReadOnly.get(clientId)).thenReturn(modelClient); + when(clientDetailsManagerReadOnly.findByClientId(clientId)).thenReturn(clientDetails); + + Client clientToUpdate = manageMembers.findClient(clientId); RedirectUri rUri = new RedirectUri(); rUri.setType(Text.valueOf("default")); rUri.setValue(Text.valueOf("http://érm.com")); + clientToUpdate.getRedirectUris().add(rUri); - client_0002.getRedirectUris().add(rUri); - - client_0002 = manageMembers.updateClient(client_0002); + when(groupAdministratorController.validateRedirectUris(any(Client.class), eq(true))).thenAnswer(invocation -> { + Client c = invocation.getArgument(0); + c.getErrors().add("manage.developer_tools.invalid_redirect_uri"); + return c; + }); + + Client result = manageMembers.updateClient(clientToUpdate); - assertNotNull(client_0002); - assertEquals(1, client_0002.getErrors().size()); - assertEquals(manageMembers.getMessage("manage.developer_tools.invalid_redirect_uri"), client_0002.getErrors().get(0)); + assertNotNull(result); + assertEquals(1, result.getErrors().size()); + assertEquals("manage.developer_tools.invalid_redirect_uri", result.getErrors().get(0)); } - + @Test public void editMemberDoesntChangePersistentTokenEnabledValueTest() throws Exception { - Client clientWithPersistentTokensEnabled = manageMembers.findClient("APP-0000000000000001"); - assertNotNull(clientWithPersistentTokensEnabled); - assertNotNull(clientWithPersistentTokensEnabled.getDisplayName()); - assertEquals("Client # 1", clientWithPersistentTokensEnabled.getDisplayName().getValue()); - assertNotNull(clientWithPersistentTokensEnabled.getPersistentTokenEnabled()); - assertTrue(clientWithPersistentTokensEnabled.getPersistentTokenEnabled().getValue()); + String clientId = "APP-0000000000000001"; + org.orcid.jaxb.model.v3.release.client.Client modelClient = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(modelClient.getId()).thenReturn(clientId); + when(modelClient.getName()).thenReturn("Client # 1"); + when(modelClient.isPersistentTokensEnabled()).thenReturn(true); + when(modelClient.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); + + when(clientManagerReadOnly.get(clientId)).thenReturn(modelClient); + when(clientDetailsManagerReadOnly.findByClientId(clientId)).thenReturn(clientDetails); + + Client clientFromFind = manageMembers.findClient(clientId); + clientFromFind.getDisplayName().setValue("Updated Name"); + + org.orcid.jaxb.model.v3.release.client.Client updatedModel = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(updatedModel.getId()).thenReturn(clientId); + when(updatedModel.getName()).thenReturn("Updated Name"); + when(updatedModel.isPersistentTokensEnabled()).thenReturn(true); + when(updatedModel.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); - clientWithPersistentTokensEnabled.getDisplayName().setValue("Updated Name"); - manageMembers.updateClient(clientWithPersistentTokensEnabled); + when(clientManager.edit(any(org.orcid.jaxb.model.v3.release.client.Client.class), eq(true))).thenReturn(updatedModel); + + Client result = manageMembers.updateClient(clientFromFind); - Client updatedClient = manageMembers.findClient("APP-0000000000000001"); - assertNotNull(updatedClient); - assertNotNull(updatedClient.getDisplayName()); - assertEquals("Updated Name", updatedClient.getDisplayName().getValue()); - assertNotNull(updatedClient.getPersistentTokenEnabled()); - assertTrue(updatedClient.getPersistentTokenEnabled().getValue()); + assertEquals("Updated Name", result.getDisplayName().getValue()); + assertTrue(result.getPersistentTokenEnabled().getValue()); } - + @Test public void editGroupTypeTest() throws Exception { - Member group_0000 = manageMembers.findMember("5555-5555-5555-0000"); - assertNotNull(group_0000); - assertNotNull(group_0000.getType()); - assertEquals(MemberType.PREMIUM_INSTITUTION.value(), group_0000.getType().getValue()); + String orcid = "5555-5555-5555-0000"; + Member member = new Member(); + member.setGroupOrcid(Text.valueOf(orcid)); + member.setType(Text.valueOf(MemberType.PREMIUM_INSTITUTION.value())); + member.setEmail(Text.valueOf("group@email.com")); + member.setGroupName(Text.valueOf("Group Name")); + member.setSalesforceId(Text.valueOf("1234567890abcde")); - // Update group type to basic - group_0000.setType(Text.valueOf(MemberType.BASIC.value())); - manageMembers.updateMember(group_0000); + when(membersManager.getMember(orcid)).thenReturn(member); - group_0000 = manageMembers.findMember("5555-5555-5555-0000"); - assertNotNull(group_0000); - assertNotNull(group_0000.getType()); - assertEquals(MemberType.BASIC.value(), group_0000.getType().getValue()); + Member group = manageMembers.findMember(orcid); + assertEquals(MemberType.PREMIUM_INSTITUTION.value(), group.getType().getValue()); + + group.setType(Text.valueOf(MemberType.BASIC.value())); + manageMembers.updateMember(group); + Member updatedGroup = manageMembers.findMember(orcid); + assertEquals(MemberType.BASIC.value(), updatedGroup.getType().getValue()); } - + @Test - public void testDeactivateClient() throws ClientAlreadyDeactivatedException { + public void testDeactivateClient() throws Exception { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", mockClientDetailsManager); - Mockito.when(mockClientDetailsManager.exists(Mockito.eq("test"))).thenReturn(true); - Mockito.doNothing().when(mockClientDetailsManager).deactivateClientDetails(Mockito.eq("test"), Mockito.eq("4444-4444-4444-4440")); + when(clientDetailsManager.exists("test")).thenReturn(true); + ClientActivationRequest clientDeactivation = new ClientActivationRequest(); clientDeactivation.setClientId("test"); clientDeactivation = manageMembers.deactivateClient(clientDeactivation); + assertNull(clientDeactivation.getError()); - Mockito.verify(mockClientDetailsManager, Mockito.times(1)).deactivateClientDetails(Mockito.eq("test"), Mockito.eq("4444-4444-4444-4440")); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", clientDetailsManager); + Mockito.verify(clientDetailsManager, Mockito.times(1)).deactivateClientDetails(eq("test"), eq("4444-4444-4444-4440")); } - + @Test - public void testActivateClient() throws ClientAlreadyActiveException { + public void testActivateClient() throws Exception { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", mockClientDetailsManager); - Mockito.when(mockClientDetailsManager.exists(Mockito.eq("test"))).thenReturn(true); - Mockito.doNothing().when(mockClientDetailsManager).activateClientDetails(Mockito.eq("test")); + when(clientDetailsManager.exists("test")).thenReturn(true); + ClientActivationRequest clientActivation = new ClientActivationRequest(); clientActivation.setClientId("test"); clientActivation = manageMembers.activateClient(clientActivation); + assertNull(clientActivation.getError()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", clientDetailsManager); + Mockito.verify(clientDetailsManager, Mockito.times(1)).activateClientDetails(eq("test")); } - + @Test - public void testDeactivateClientAlreadyDeactivated() throws ClientAlreadyDeactivatedException { + public void testDeactivateClientAlreadyDeactivated() throws Exception { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", mockClientDetailsManager); - Mockito.when(mockClientDetailsManager.exists(Mockito.eq("test"))).thenReturn(true); - Mockito.doThrow(new ClientAlreadyDeactivatedException("already-deactivated")).when(mockClientDetailsManager).deactivateClientDetails(Mockito.eq("test"), Mockito.eq("4444-4444-4444-4440")); + when(clientDetailsManager.exists("test")).thenReturn(true); + Mockito.doThrow(new org.orcid.core.exception.ClientAlreadyDeactivatedException("already-deactivated")).when(clientDetailsManager).deactivateClientDetails(eq("test"), eq("4444-4444-4444-4440")); + ClientActivationRequest clientDeactivation = new ClientActivationRequest(); clientDeactivation.setClientId("test"); clientDeactivation = manageMembers.deactivateClient(clientDeactivation); + assertNotNull(clientDeactivation.getError()); assertEquals("already-deactivated", clientDeactivation.getError()); - Mockito.verify(mockClientDetailsManager, Mockito.times(1)).deactivateClientDetails(Mockito.eq("test"), Mockito.eq("4444-4444-4444-4440")); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", clientDetailsManager); } - + @Test - public void testActivateClientAlreadyActive() throws ClientAlreadyActiveException { + public void testActivateClientAlreadyActive() throws Exception { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", mockClientDetailsManager); - Mockito.when(mockClientDetailsManager.exists(Mockito.eq("test"))).thenReturn(true); - Mockito.doThrow(new ClientAlreadyActiveException("already-active")).when(mockClientDetailsManager).activateClientDetails(Mockito.eq("test")); + when(clientDetailsManager.exists("test")).thenReturn(true); + Mockito.doThrow(new org.orcid.core.exception.ClientAlreadyActiveException("already-active")).when(clientDetailsManager).activateClientDetails(eq("test")); + ClientActivationRequest clientActivation = new ClientActivationRequest(); clientActivation.setClientId("test"); clientActivation = manageMembers.activateClient(clientActivation); + assertNotNull(clientActivation.getError()); assertEquals("already-active", clientActivation.getError()); - ReflectionTestUtils.setField(manageMembers, "clientDetailsManager", clientDetailsManager); } private Client findClientById(List clients, String id) { - return clients.stream() - .filter(c -> id.equals(c.getClientId().getValue())) - .findFirst() - .orElse(null); + return clients.stream().filter(c -> id.equals(c.getClientId().getValue())).findFirst().orElse(null); } } From 34528c2a3ada983dc2217592fa55108161ee4567 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 27 May 2026 20:45:09 -0600 Subject: [PATCH 15/15] Fix test by refactoring it to use mocks, so, we dont need transactional tests --- .../ManageMembersControllerTest.java | 643 +++++++++--------- 1 file changed, 320 insertions(+), 323 deletions(-) diff --git a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java index bf62fd745ae..4b8a127ecde 100644 --- a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java +++ b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/ManageMembersControllerTest.java @@ -5,8 +5,9 @@ import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.orcid.core.exception.ClientAlreadyActiveException; +import org.orcid.core.exception.ClientAlreadyDeactivatedException; import org.orcid.core.locale.LocaleManager; import org.orcid.core.manager.v3.ClientDetailsManager; import org.orcid.core.manager.v3.ClientManager; @@ -16,6 +17,7 @@ import org.orcid.core.manager.v3.read_only.ClientManagerReadOnly; import org.orcid.core.security.OrcidRoles; import org.orcid.jaxb.model.clientgroup.MemberType; +import org.orcid.jaxb.model.clientgroup.RedirectUriType; import org.orcid.persistence.jpa.entities.ClientDetailsEntity; import org.orcid.pojo.ClientActivationRequest; import org.orcid.pojo.ajaxForm.Client; @@ -28,24 +30,19 @@ import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.User; import org.springframework.security.core.userdetails.UserDetails; +import org.springframework.web.servlet.ModelAndView; -import java.util.ArrayList; -import java.util.Date; -import java.util.List; +import java.util.*; import static org.junit.Assert.*; import static org.mockito.ArgumentMatchers.*; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @RunWith(MockitoJUnitRunner.class) public class ManageMembersControllerTest { @Mock(name = "membersManagerV3") - MembersManager membersManager; - - @Mock(name = "emailManagerV3") - EmailManager emailManager; + private MembersManager membersManager; @Mock(name = "clientManagerV3") private ClientManager clientManager; @@ -59,24 +56,26 @@ public class ManageMembersControllerTest { @Mock(name = "clientDetailsManagerV3") private ClientDetailsManager clientDetailsManager; + @Mock(name = "emailManagerV3") + private EmailManager emailManager; + @Mock - ClientsController groupAdministratorController; + private ClientsController groupAdministratorController; @Mock - protected LocaleManager localeManager; + private LocaleManager localeManager; @InjectMocks - ManageMembersController manageMembers; + private ManageMembersController manageMembers; @Before - public void beforeInstance() { + public void setUp() { SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - when(localeManager.resolveMessage(anyString(), any())).thenAnswer(invocation -> { - return invocation.getArgument(0); - }); + // Mock localeManager to return the message code as the translated message + when(localeManager.resolveMessage(anyString(), any())).thenAnswer(invocation -> invocation.getArgument(0)); } - protected Authentication getAuthentication() { + private Authentication getAuthentication() { String orcid = "4444-4444-4444-4440"; UserDetails details = new User(orcid, "password", List.of(new SimpleGrantedAuthority(OrcidRoles.ROLE_ADMIN.name()))); UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken(orcid, "password", details.getAuthorities()); @@ -85,393 +84,391 @@ protected Authentication getAuthentication() { } @Test - public void createMemberProfileWithInvalidEmailsTest() throws Exception { - String existingEmail = "premium_institution@group.com"; - - Member group = new Member(); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("basic")); - group.setSalesforceId(Text.valueOf("")); - - // Validate already existing email address - group.setEmail(Text.valueOf(existingEmail)); - when(emailManager.emailExists(eq(existingEmail))).thenReturn(true); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.email.already_used", group.getErrors().get(0)); - - // Validate empty email address - group.setEmail(Text.valueOf("")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("NotBlank.group.email", group.getErrors().get(0)); - - // Validate invalid email address - group.setEmail(Text.valueOf("invalidemail")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.email.invalid_email", group.getErrors().get(0)); + public void testGetManageMembersPage() { + ModelAndView mav = manageMembers.getManageMembersPage(); + assertEquals("/admin/manage_members", mav.getViewName()); } @Test - public void createMemberProfileWithInvalidGroupNameTest() throws Exception { - Member group = new Member(); - group.setEmail(Text.valueOf("group@email.com")); - group.setType(Text.valueOf("basic")); - group.setSalesforceId(Text.valueOf("")); - - // Validate empty group name - group.setGroupName(Text.valueOf("")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("NotBlank.group.name", group.getErrors().get(0)); - - // validate too long group name - 151 chars - group.setGroupName(Text.valueOf( - "1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.name.too_long", group.getErrors().get(0)); + public void testGetEmptyGroup() { + Member member = manageMembers.getEmptyGroup(); + assertNotNull(member); + assertEquals("", member.getEmail().getValue()); + assertEquals("", member.getGroupName().getValue()); + assertEquals("", member.getGroupOrcid().getValue()); + assertEquals("", member.getSalesforceId().getValue()); + assertEquals(MemberType.BASIC.value(), member.getType().getValue()); } @Test - public void createMemberProfileWithInvalidTypeTest() throws Exception { - Member group = new Member(); - group.setEmail(Text.valueOf("group@email.com")); - group.setGroupName(Text.valueOf("Group Name")); - group.setSalesforceId(Text.valueOf("")); + public void testFind_ClientBranch() { + String id = "APP-123"; + when(clientDetailsManagerReadOnly.exists(id)).thenReturn(true); + + // Mock findClient behavior + org.orcid.jaxb.model.v3.release.client.Client modelClient = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(modelClient.getId()).thenReturn(id); + when(modelClient.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + when(clientManagerReadOnly.get(id)).thenReturn(modelClient); + + ClientDetailsEntity clientDetails = new ClientDetailsEntity(); + when(clientDetailsManagerReadOnly.findByClientId(id)).thenReturn(clientDetails); - // Validate empty type - group.setType(Text.valueOf("")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("NotBlank.group.type", group.getErrors().get(0)); + Object resultObj = manageMembers.find(id); + assertTrue((Boolean) reflectGet(resultObj, "isClient")); + Object clientObj = reflectGet(resultObj, "clientObject"); + assertNotNull(clientObj); + assertEquals(id, ((Client) clientObj).getClientId().getValue()); + } - // Validate invalid type - group.setType(Text.valueOf("invalid")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.type.invalid", group.getErrors().get(0)); + private Object reflectGet(Object obj, String fieldName) { + try { + java.lang.reflect.Field field = obj.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + return field.get(obj); + } catch (Exception e) { + throw new RuntimeException(e); + } } @Test - public void createMemberProfileWithInvalidSalesforceIdTest() throws Exception { - Member group = new Member(); - group.setEmail(Text.valueOf("group@email.com")); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("basic")); - - // Validate empty type - group.setSalesforceId(Text.valueOf("1")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.salesforce_id.invalid_length", group.getErrors().get(0)); - - // Validate invalid type - group.setSalesforceId(Text.valueOf("1234567890abcd!")); - group = manageMembers.createMember(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.salesforce_id.invalid", group.getErrors().get(0)); + public void testFind_MemberBranch() { + String id = "0000-0000-0000-0001"; + when(clientDetailsManagerReadOnly.exists(id)).thenReturn(false); + + Member member = new Member(); + member.setGroupOrcid(Text.valueOf(id)); + when(membersManager.getMember(id)).thenReturn(member); + + Object resultObj = manageMembers.find(id); + assertFalse((Boolean) reflectGet(resultObj, "isClient")); + Object memberObj = reflectGet(resultObj, "memberObject"); + assertNotNull(memberObj); + assertEquals(id, ((Member) memberObj).getGroupOrcid().getValue()); } @Test - public void createMemberProfileTest() throws Exception { - Member group = new Member(); - String email = "group@email.com"; - group.setEmail(Text.valueOf(email)); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("")); + public void testFindMember_Empty() { + Member result = manageMembers.findMember(""); + assertEquals(1, result.getErrors().size()); + assertEquals("manage_member.not_blank", result.getErrors().get(0)); + } - Member createdMember = new Member(); - createdMember.setGroupOrcid(Text.valueOf("5555-5555-5555-5555")); + @Test + public void testFindMember_Success() { + String id = "some-id"; + Member member = new Member(); + when(membersManager.getMember(id)).thenReturn(member); + Member result = manageMembers.findMember(id); + assertSame(member, result); + } - when(membersManager.createMember(any(Member.class))).thenReturn(createdMember); + @Test + public void testCreateMember_Validation_EmailEmpty() { + Member member = createValidMember(); + member.setEmail(Text.valueOf("")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("NotBlank.group.email")); + } - group = manageMembers.createMember(group); - assertEquals(0, group.getErrors().size()); - assertEquals("5555-5555-5555-5555", group.getGroupOrcid().getValue()); + @Test + public void testCreateMember_Validation_EmailInvalid() { + Member member = createValidMember(); + member.setEmail(Text.valueOf("invalid-email")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.email.invalid_email")); } @Test - public void findMemberByOrcidTest() throws Exception { - String email = "group@email.com"; - String orcid = "5555-5555-5555-5555"; - Member member = new Member(); - member.setEmail(Text.valueOf(email)); - member.setGroupName(Text.valueOf("Group Name")); - member.setSalesforceId(Text.valueOf("1234567890abcde")); - member.setGroupOrcid(Text.valueOf(orcid)); + public void testCreateMember_Validation_EmailExists() { + Member member = createValidMember(); + when(emailManager.emailExists(member.getEmail().getValue())).thenReturn(true); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.email.already_used")); + } - when(membersManager.getMember(orcid)).thenReturn(member); - when(membersManager.getMember(email)).thenReturn(member); + @Test + public void testCreateMember_Validation_GroupNameEmpty() { + Member member = createValidMember(); + member.setGroupName(Text.valueOf("")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("NotBlank.group.name")); + } - // Test find by orcid - Member newGroup = manageMembers.findMember(orcid); - assertNotNull(newGroup); - assertEquals(email, newGroup.getEmail().getValue()); - assertEquals("Group Name", newGroup.getGroupName().getValue()); - assertEquals("1234567890abcde", newGroup.getSalesforceId().getValue()); - assertEquals(orcid, newGroup.getGroupOrcid().getValue()); + @Test + public void testCreateMember_Validation_GroupNameTooLong() { + Member member = createValidMember(); + member.setGroupName(Text.valueOf("a".repeat(151))); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.name.too_long")); + } - // Test find by email - Member newGroup2 = manageMembers.findMember(email); - assertNotNull(newGroup2); - assertEquals(email, newGroup2.getEmail().getValue()); - assertEquals(orcid, newGroup2.getGroupOrcid().getValue()); + @Test + public void testCreateMember_Validation_TypeEmpty() { + Member member = createValidMember(); + member.setType(Text.valueOf("")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("NotBlank.group.type")); + } - // Test: Find member by ORCID with clients and check deactivated status - Member memberWithClients = new Member(); - List clients = new ArrayList<>(); - clients.add(createClient("APP-0000000000000001", false)); - clients.add(createClient("APP-0000000000000002", false)); - clients.add(createClient("APP-0000000000000003", true)); - memberWithClients.setClients(clients); + @Test + public void testCreateMember_Validation_TypeInvalid() { + Member member = createValidMember(); + member.setType(Text.valueOf("INVALID_TYPE")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.type.invalid")); + } - when(membersManager.getMember("5555-5555-5555-0000")).thenReturn(memberWithClients); + @Test + public void testCreateMember_Validation_SalesforceIdInvalidLength() { + Member member = createValidMember(); + member.setSalesforceId(Text.valueOf("123")); + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.salesforce_id.invalid_length")); + } - Member newGroup3 = manageMembers.findMember("5555-5555-5555-0000"); - assertNotNull(newGroup3); - List clientsResult = newGroup3.getClients(); - assertEquals(3, clientsResult.size()); - assertFalse(findClientById(clientsResult, "APP-0000000000000001").isDeactivated()); - assertFalse(findClientById(clientsResult, "APP-0000000000000002").isDeactivated()); - assertTrue(findClientById(clientsResult, "APP-0000000000000003").isDeactivated()); + @Test + public void testCreateMember_Validation_SalesforceIdInvalidPattern() { + Member member = createValidMember(); + member.setSalesforceId(Text.valueOf("12345678901234!")); // 15 chars but with '!' + + Member result = manageMembers.createMember(member); + assertTrue(result.getErrors().contains("group.salesforce_id.invalid")); } - private Client createClient(String id, boolean deactivated) { - Client c = new Client(); - c.setClientId(Text.valueOf(id)); - c.setDeactivated(deactivated); - return c; + @Test + public void testCreateMember_Success() { + Member member = createValidMember(); + when(membersManager.createMember(any(Member.class))).thenReturn(member); + + Member result = manageMembers.createMember(member); + assertEquals(0, result.getErrors().size()); + verify(membersManager).createMember(member); } @Test - public void editMemberTest() throws Exception { - String orcid = "5555-5555-5555-5555"; - Member group = new Member(); - group.setGroupOrcid(Text.valueOf(orcid)); - group.setEmail(Text.valueOf("new_email@user.com")); - group.setGroupName(Text.valueOf("Updated Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); + public void testUpdateMember_Success() { + Member member = createValidMember(); + member.setGroupOrcid(Text.valueOf("0000-0000-0000-0001")); + when(membersManager.updateMemeber(any(Member.class))).thenReturn(member); + + Member result = manageMembers.updateMember(member); + assertEquals(0, result.getErrors().size()); + verify(membersManager).updateMemeber(member); + } - when(membersManager.getMember(orcid)).thenReturn(group); + @Test + public void testUpdateMember_EmailOwnership_SameOwner() { + Member member = createValidMember(); + String orcid = "0000-0000-0000-0001"; + member.setGroupOrcid(Text.valueOf(orcid)); + String email = "test@orcid.org"; + member.setEmail(Text.valueOf(email)); + + when(emailManager.emailExists(email)).thenReturn(true); + Map owners = new HashMap<>(); + owners.put(email, orcid); + when(emailManager.findOricdIdsByCommaSeparatedEmails(email)).thenReturn(owners); + when(membersManager.updateMemeber(any(Member.class))).thenReturn(member); - manageMembers.updateMember(group); - Member updatedGroup = manageMembers.findMember(orcid); - assertNotNull(updatedGroup); - assertEquals(orcid, updatedGroup.getGroupOrcid().getValue()); - assertEquals("Updated Group Name", updatedGroup.getGroupName().getValue()); + Member result = manageMembers.updateMember(member); + assertEquals(0, result.getErrors().size()); } @Test - public void editMemberWithInvalidEmailTest() throws Exception { - String email = "group1@email.com"; + public void testUpdateMember_EmailOwnership_DifferentOwner() { + Member member = createValidMember(); + String orcid = "0000-0000-0000-0001"; + member.setGroupOrcid(Text.valueOf(orcid)); + String email = "test@orcid.org"; + member.setEmail(Text.valueOf(email)); + when(emailManager.emailExists(email)).thenReturn(true); + Map owners = new HashMap<>(); + owners.put(email, "0000-0000-0000-0002"); // different owner + when(emailManager.findOricdIdsByCommaSeparatedEmails(email)).thenReturn(owners); - Member group = new Member(); - group.setEmail(Text.valueOf(email)); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcde")); - - group = manageMembers.createMember(group); - assertNotNull(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.email.already_used", group.getErrors().get(0)); + Member result = manageMembers.updateMember(member); + assertTrue(result.getErrors().contains("group.email.already_used")); } @Test - public void editMemberWithInvalidSalesforceIdTest() throws Exception { - Member group = new Member(); - group.setEmail(Text.valueOf("group2@email.com")); - group.setGroupName(Text.valueOf("Group Name")); - group.setType(Text.valueOf("premium-institution")); - group.setSalesforceId(Text.valueOf("1234567890abcd!")); - - group = manageMembers.createMember(group); - assertNotNull(group); - assertEquals(1, group.getErrors().size()); - assertEquals("group.salesforce_id.invalid", group.getErrors().get(0)); + public void testFindClient_Empty() { + Client result = manageMembers.findClient(""); + assertEquals(1, result.getErrors().size()); + assertEquals("manage_member.not_blank", result.getErrors().get(0)); } @Test - public void findClientTest() throws Exception { - String clientId2 = "APP-0000000000000002"; + public void testFindClient_Success() { + String clientId = "APP-123"; + org.orcid.jaxb.model.v3.release.client.Client model = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(model.getId()).thenReturn(clientId); + when(model.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); - ClientDetailsEntity clientDetails2 = new ClientDetailsEntity(); - clientDetails2.setDeactivatedDate(null); - - org.orcid.jaxb.model.v3.release.client.Client modelClient2 = mock(org.orcid.jaxb.model.v3.release.client.Client.class); - when(modelClient2.getId()).thenReturn(clientId2); - when(modelClient2.getName()).thenReturn("Client # 2"); - when(modelClient2.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + when(clientManagerReadOnly.get(clientId)).thenReturn(model); - when(clientManagerReadOnly.get(clientId2)).thenReturn(modelClient2); - when(clientDetailsManagerReadOnly.findByClientId(clientId2)).thenReturn(clientDetails2); - - Client result2 = manageMembers.findClient(clientId2); - assertNotNull(result2); - assertEquals("Client # 2", result2.getDisplayName().getValue()); - assertFalse(result2.isDeactivated()); - - String clientId3 = "APP-0000000000000003"; - org.orcid.jaxb.model.v3.release.client.Client modelClient3 = mock(org.orcid.jaxb.model.v3.release.client.Client.class); - when(modelClient3.getId()).thenReturn(clientId3); - when(modelClient3.getName()).thenReturn("Client # 3"); - when(modelClient3.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + ClientDetailsEntity details = new ClientDetailsEntity(); + details.setDeactivatedDate(new Date()); + when(clientDetailsManagerReadOnly.findByClientId(clientId)).thenReturn(details); - ClientDetailsEntity clientDetails3 = new ClientDetailsEntity(); - clientDetails3.setDeactivatedDate(new Date()); - - when(clientManagerReadOnly.get(clientId3)).thenReturn(modelClient3); - when(clientDetailsManagerReadOnly.findByClientId(clientId3)).thenReturn(clientDetails3); - - Client result3 = manageMembers.findClient(clientId3); - assertNotNull(result3); - assertTrue(result3.isDeactivated()); + Client result = manageMembers.findClient(clientId); + assertEquals(clientId, result.getClientId().getValue()); + assertTrue(result.isDeactivated()); } @Test - public void editClientWithInvalidRedirectUriTest() throws Exception { - String clientId = "APP-0000000000000002"; - org.orcid.jaxb.model.v3.release.client.Client modelClient = mock(org.orcid.jaxb.model.v3.release.client.Client.class); - when(modelClient.getId()).thenReturn(clientId); - when(modelClient.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + public void testUpdateClient_Success() { + Client client = new Client(); + client.setDisplayName(Text.valueOf("Name")); + client.setWebsite(Text.valueOf("http://website.com")); + client.setShortDescription(Text.valueOf("Desc")); + client.setRedirectUris(new ArrayList<>()); - ClientDetailsEntity clientDetails = new ClientDetailsEntity(); - - when(clientManagerReadOnly.get(clientId)).thenReturn(modelClient); - when(clientDetailsManagerReadOnly.findByClientId(clientId)).thenReturn(clientDetails); + org.orcid.jaxb.model.v3.release.client.Client model = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(model.getId()).thenReturn("APP-123"); + when(model.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); - Client clientToUpdate = manageMembers.findClient(clientId); - RedirectUri rUri = new RedirectUri(); - rUri.setType(Text.valueOf("default")); - rUri.setValue(Text.valueOf("http://érm.com")); - clientToUpdate.getRedirectUris().add(rUri); + when(clientManager.edit(any(), eq(true))).thenReturn(model); - when(groupAdministratorController.validateRedirectUris(any(Client.class), eq(true))).thenAnswer(invocation -> { - Client c = invocation.getArgument(0); - c.getErrors().add("manage.developer_tools.invalid_redirect_uri"); - return c; - }); + Client result = manageMembers.updateClient(client); + assertEquals(0, result.getErrors().size()); + verify(clientManager).edit(any(), eq(true)); + } - Client result = manageMembers.updateClient(clientToUpdate); + @Test + public void testUpdateClient_WithIdP_NoInstitutionalRedirectUri() { + Client client = new Client(); + client.setDisplayName(Text.valueOf("Name")); + client.setAuthenticationProviderId(Text.valueOf("IDP-123")); + client.setRedirectUris(new ArrayList<>()); - assertNotNull(result); - assertEquals(1, result.getErrors().size()); - assertEquals("manage.developer_tools.invalid_redirect_uri", result.getErrors().get(0)); + Client result = manageMembers.updateClient(client); + assertTrue(result.getErrors().contains("manage.developer_tools.client.idp.error.no_redirect_uri_found")); } @Test - public void editMemberDoesntChangePersistentTokenEnabledValueTest() throws Exception { - String clientId = "APP-0000000000000001"; - org.orcid.jaxb.model.v3.release.client.Client modelClient = mock(org.orcid.jaxb.model.v3.release.client.Client.class); - when(modelClient.getId()).thenReturn(clientId); - when(modelClient.getName()).thenReturn("Client # 1"); - when(modelClient.isPersistentTokensEnabled()).thenReturn(true); - when(modelClient.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); - - ClientDetailsEntity clientDetails = new ClientDetailsEntity(); - - when(clientManagerReadOnly.get(clientId)).thenReturn(modelClient); - when(clientDetailsManagerReadOnly.findByClientId(clientId)).thenReturn(clientDetails); - - Client clientFromFind = manageMembers.findClient(clientId); - clientFromFind.getDisplayName().setValue("Updated Name"); - - org.orcid.jaxb.model.v3.release.client.Client updatedModel = mock(org.orcid.jaxb.model.v3.release.client.Client.class); - when(updatedModel.getId()).thenReturn(clientId); - when(updatedModel.getName()).thenReturn("Updated Name"); - when(updatedModel.isPersistentTokensEnabled()).thenReturn(true); - when(updatedModel.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + public void testUpdateClient_WithIdP_WithInstitutionalRedirectUri() { + Client client = new Client(); + client.setDisplayName(Text.valueOf("Name")); + client.setAuthenticationProviderId(Text.valueOf("IDP-123")); + RedirectUri rUri = new RedirectUri(); + rUri.setType(Text.valueOf(RedirectUriType.INSTITUTIONAL_SIGN_IN.value())); + rUri.setValue(Text.valueOf("http://redirect.uri")); + client.setRedirectUris(List.of(rUri)); - when(clientManager.edit(any(org.orcid.jaxb.model.v3.release.client.Client.class), eq(true))).thenReturn(updatedModel); + org.orcid.jaxb.model.v3.release.client.Client model = mock(org.orcid.jaxb.model.v3.release.client.Client.class); + when(model.getId()).thenReturn("APP-123"); + when(model.getClientType()).thenReturn(org.orcid.jaxb.model.clientgroup.ClientType.CREATOR); + when(clientManager.edit(any(), eq(true))).thenReturn(model); - Client result = manageMembers.updateClient(clientFromFind); - - assertEquals("Updated Name", result.getDisplayName().getValue()); - assertTrue(result.getPersistentTokenEnabled().getValue()); + Client result = manageMembers.updateClient(client); + assertFalse(result.getErrors().contains("manage.developer_tools.client.idp.error.no_redirect_uri_found")); } @Test - public void editGroupTypeTest() throws Exception { - String orcid = "5555-5555-5555-0000"; - Member member = new Member(); - member.setGroupOrcid(Text.valueOf(orcid)); - member.setType(Text.valueOf(MemberType.PREMIUM_INSTITUTION.value())); - member.setEmail(Text.valueOf("group@email.com")); - member.setGroupName(Text.valueOf("Group Name")); - member.setSalesforceId(Text.valueOf("1234567890abcde")); - - when(membersManager.getMember(orcid)).thenReturn(member); - - Member group = manageMembers.findMember(orcid); - assertEquals(MemberType.PREMIUM_INSTITUTION.value(), group.getType().getValue()); - - group.setType(Text.valueOf(MemberType.BASIC.value())); - manageMembers.updateMember(group); - - Member updatedGroup = manageMembers.findMember(orcid); - assertEquals(MemberType.BASIC.value(), updatedGroup.getType().getValue()); + public void testGetEmptyRedirectUri() { + RedirectUri result = manageMembers.getEmptyRedirectUri(); + assertEquals(RedirectUriType.DEFAULT.value(), result.getType().getValue()); + assertEquals("", result.getValue().getValue()); } @Test - public void testDeactivateClient() throws Exception { - SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - when(clientDetailsManager.exists("test")).thenReturn(true); + public void testDeactivateClient_NotFound() { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-MISSING"); + when(clientDetailsManager.exists("APP-MISSING")).thenReturn(false); - ClientActivationRequest clientDeactivation = new ClientActivationRequest(); - clientDeactivation.setClientId("test"); - clientDeactivation = manageMembers.deactivateClient(clientDeactivation); - - assertNull(clientDeactivation.getError()); - Mockito.verify(clientDetailsManager, Mockito.times(1)).deactivateClientDetails(eq("test"), eq("4444-4444-4444-4440")); + ClientActivationRequest result = manageMembers.deactivateClient(request); + assertEquals("Client not found", result.getError()); } @Test - public void testActivateClient() throws Exception { - SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - when(clientDetailsManager.exists("test")).thenReturn(true); + public void testDeactivateClient_AlreadyDeactivated() throws ClientAlreadyDeactivatedException { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-123"); + when(clientDetailsManager.exists("APP-123")).thenReturn(true); + doThrow(new ClientAlreadyDeactivatedException("already deactivated")) + .when(clientDetailsManager).deactivateClientDetails(eq("APP-123"), anyString()); - ClientActivationRequest clientActivation = new ClientActivationRequest(); - clientActivation.setClientId("test"); - clientActivation = manageMembers.activateClient(clientActivation); - - assertNull(clientActivation.getError()); - Mockito.verify(clientDetailsManager, Mockito.times(1)).activateClientDetails(eq("test")); + ClientActivationRequest result = manageMembers.deactivateClient(request); + assertEquals("already deactivated", result.getError()); } @Test - public void testDeactivateClientAlreadyDeactivated() throws Exception { - SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - when(clientDetailsManager.exists("test")).thenReturn(true); - Mockito.doThrow(new org.orcid.core.exception.ClientAlreadyDeactivatedException("already-deactivated")).when(clientDetailsManager).deactivateClientDetails(eq("test"), eq("4444-4444-4444-4440")); + public void testDeactivateClient_Success() throws ClientAlreadyDeactivatedException { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-123"); + when(clientDetailsManager.exists("APP-123")).thenReturn(true); - ClientActivationRequest clientDeactivation = new ClientActivationRequest(); - clientDeactivation.setClientId("test"); - clientDeactivation = manageMembers.deactivateClient(clientDeactivation); + ClientActivationRequest result = manageMembers.deactivateClient(request); + assertNull(result.getError()); + verify(clientDetailsManager).deactivateClientDetails(eq("APP-123"), eq("4444-4444-4444-4440")); + } + + @Test + public void testActivateClient_NotFound() { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-MISSING"); + when(clientDetailsManager.exists("APP-MISSING")).thenReturn(false); - assertNotNull(clientDeactivation.getError()); - assertEquals("already-deactivated", clientDeactivation.getError()); + ClientActivationRequest result = manageMembers.activateClient(request); + assertEquals("Client not found", result.getError()); } @Test - public void testActivateClientAlreadyActive() throws Exception { - SecurityContextHolder.getContext().setAuthentication(getAuthentication()); - when(clientDetailsManager.exists("test")).thenReturn(true); - Mockito.doThrow(new org.orcid.core.exception.ClientAlreadyActiveException("already-active")).when(clientDetailsManager).activateClientDetails(eq("test")); + public void testActivateClient_AlreadyActive() throws ClientAlreadyActiveException { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-123"); + when(clientDetailsManager.exists("APP-123")).thenReturn(true); + doThrow(new ClientAlreadyActiveException("already active")) + .when(clientDetailsManager).activateClientDetails("APP-123"); - ClientActivationRequest clientActivation = new ClientActivationRequest(); - clientActivation.setClientId("test"); - clientActivation = manageMembers.activateClient(clientActivation); + ClientActivationRequest result = manageMembers.activateClient(request); + assertEquals("already active", result.getError()); + } + + @Test + public void testActivateClient_Success() throws ClientAlreadyActiveException { + ClientActivationRequest request = new ClientActivationRequest(); + request.setClientId("APP-123"); + when(clientDetailsManager.exists("APP-123")).thenReturn(true); - assertNotNull(clientActivation.getError()); - assertEquals("already-active", clientActivation.getError()); + ClientActivationRequest result = manageMembers.activateClient(request); + assertNull(result.getError()); + verify(clientDetailsManager).activateClientDetails("APP-123"); + } + + @Test + public void testGetRedirectUriTypes() { + Map types = manageMembers.getRedirectUriTypes(); + assertFalse(types.containsKey(RedirectUriType.SSO_AUTHENTICATION.value())); + assertTrue(types.containsKey(RedirectUriType.DEFAULT.value())); } - private Client findClientById(List clients, String id) { - return clients.stream().filter(c -> id.equals(c.getClientId().getValue())).findFirst().orElse(null); + @Test + public void testRetrieveGroupTypes() { + Map types = manageMembers.retrieveGroupTypes(); + assertFalse(types.containsKey(MemberType.BASIC_INSTITUTION.value())); + assertFalse(types.containsKey(MemberType.PREMIUM_INSTITUTION.value())); + assertTrue(types.containsKey(MemberType.BASIC.value())); + assertEquals("basic", types.get(MemberType.BASIC.value())); + } + + private Member createValidMember() { + Member member = new Member(); + member.setEmail(Text.valueOf("test@orcid.org")); + member.setGroupName(Text.valueOf("Test Group")); + member.setType(Text.valueOf(MemberType.BASIC.value())); + member.setSalesforceId(Text.valueOf("1234567890abcde")); + return member; } }