MCR-3578 Changes modsperson attribute handling by erodde · Pull Request #2773 · MyCoRe-Org/mycore

erodde · 2025-12-12T10:31:04Z

Link to jira.
This PR: #2733, but renamed branch and Jira Ticket

…-handling

mycore-base/src/main/java/org/mycore/datamodel/legalentity/MCRIdentifier.java

golsch · 2025-12-16T07:29:19Z

mycore-base/src/main/java/org/mycore/datamodel/legalentity/MCRLegalEntityService.java

+    Set<MCRIdentifier> getAllIdentifiers(MCRIdentifier identifier);
+
+    /**
+     * Gets a legal entity's identifiers of a given type. The legal entity is determined by a specific identifier.
+     * @param primaryIdentifier unique identifier of legal entity, not null
+     * @param identifierType the type of looked up identifiers as a string, not null
+     * @return a set of identifiers a legal entity owns
+     */
+    Set<MCRIdentifier> getTypedIdentifiers(MCRIdentifier primaryIdentifier, String identifierType);


Suggestion: I would not call the methods "get" but rather findAllIdentifiers / findAllIdentifiersByType or lookupAllIdentifiers / lookupAllIdentifiersByType...

golsch · 2025-12-16T08:12:59Z

mycore-mods/src/main/java/org/mycore/mods/MCRUser2MODSPersonIdentifierService.java

+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class MCRUser2MODSPersonIdentifierService implements MCRLegalEntityService {


I find the name confusing. Maybe just MCRMODSPersonIdentifierService?

golsch · 2025-12-16T08:15:55Z

...ods/src/test/resources/MCRUser2MODSPersonIdentifierServiceTest/junit_modsperson_00000001.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<mycoreobject xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="datamodel-modsperson.xsd" ID="junit_modsperson_00000001">
+    <metadata>


Please use two spaces for indentation.

golsch · 2025-12-16T08:16:02Z

...ods/src/test/resources/MCRUser2MODSPersonIdentifierServiceTest/junit_modsperson_00000002.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<mycoreobject xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="datamodel-modsperson.xsd" ID="junit_modsperson_00000002">
+    <metadata>


Please use two spaces for indentation.

golsch · 2025-12-16T08:18:33Z

mycore-orcid2/src/main/java/org/mycore/orcid2/user/MCRORCIDUser.java

+        legalEntityService = MCRConfiguration2.getInstanceOfOrThrow(
+            MCRLegalEntityService.class, MCRORCIDConstants.CONFIG_PREFIX + "LegalEntityService.Class");


Would it perhaps make more sense to have a centralized service? Something like a DefaultLegalEntityService where you can configure the default service? There are certainly other places where this service is needed.

In the current implementation, each class makes its own decision which LegalEntityService to use through a configuration string. The MCROrcidUser uses the class configured in MCR.ORCID2.LegalEntityService.Class. Would you propose to have one centralized configuration for all classes that want to use a LegalEntityService? Or should all configurations be managed in that DefaultLegalEntityService? Or would that DefaultService be a fallback to a specific, configurable Service?

However, this would prevent the "wrong" service from being called. In addition, the service is independent of the MCRORCIDUser and does not need to be loaded in the constructor every time. I would expect something like a DefaultLegalEntityService that manages the corresponding implementation.

After discussing this with @golsch and @toKrause, the globally configured LegalEntityService implementation ist now instantiated via the LegalEntityService interface and can be accessed from there through a static method.

golsch · 2025-12-16T08:18:54Z

mycore-orcid2/src/main/java/org/mycore/orcid2/user/MCRORCIDUser.java

 import org.mycore.orcid2.client.MCRORCIDCredential;
 import org.mycore.orcid2.exception.MCRORCIDException;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


Fix package ordner.

golsch · 2025-12-16T08:20:54Z

mycore-orcid2/src/main/java/org/mycore/orcid2/user/MCRORCIDUserUtils.java

 import org.mycore.orcid2.exception.MCRORCIDException;
 import org.mycore.orcid2.oauth.MCRORCIDOAuthClient;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


Fix package order

golsch · 2025-12-16T08:23:15Z

mycore-orcid2/src/test/java/org/mycore/orcid2/user/MCRORCIDUserTest.java

+    @BeforeEach
+    public void prepare() throws NoSuchFieldException, IllegalAccessException {
+        userMock = new MCRUser("junit");
+        MCRLegalEntityServiceMock legalEntityServiceMock = new MCRLegalEntityServiceMock();


Why not use mockito?

I'm pretty sure I tried that and failed, but will look into it again..

Okay, I mocked the service using mockito.

golsch · 2025-12-16T08:23:55Z

mycore-orcid2/src/main/java/org/mycore/orcid2/v3/rest/resources/MCRORCIDObjectResource.java

 import org.mycore.orcid2.user.MCRORCIDUser;
 import org.mycore.orcid2.user.MCRORCIDUserUtils;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


Package order

…-handling

mycore-mods/src/main/java/org/mycore/mods/MCRMODSPersonIdentifierService.java

mycore-base/src/main/resources/config/mycore.properties

mycore-user2/src/main/java/org/mycore/user2/MCRUserIdentifierService.java

…-handling

erodde · 2026-02-18T13:44:54Z

@golsch should be ready for review again. Should I add the PR checklist to this PR?

golsch · 2026-03-24T07:32:16Z

mycore-mods/src/main/java/org/mycore/mods/MCRMODSPersonIdentifierService.java

+        }
+        MCRMODSWrapper wrapper = new MCRMODSWrapper(modspersonOptional.get());
+        Element modsName = wrapper.getMODS().getChild(MODS_NAME, MCRConstants.MODS_NAMESPACE);
+        if (modsName == null) {


How could that happen?

golsch · 2026-03-24T07:36:47Z

mycore-orcid2/src/main/java/org/mycore/orcid2/user/MCRORCIDUser.java

-        return user.getAttributes().stream()
-            .filter(a -> Objects.equals(a.getName(), ATTR_ORCID_ID))
-            .map(MCRUserAttribute::getValue).collect(Collectors.toSet());
+        final MCRIdentifier userid = new MCRIdentifier("userid", user.getUserID());


Could userid somehow be moved to a global constant? It seems like it is the magic reference, right?

golsch · 2026-03-24T07:37:59Z

mycore-orcid2/src/main/java/org/mycore/orcid2/v3/work/MCRORCIDWorkEventHandlerImpl.java

 import org.mycore.orcid2.client.MCRORCIDCredential;
 import org.mycore.orcid2.metadata.MCRORCIDPutCodeInfo;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


golsch · 2026-03-24T07:38:09Z

mycore-orcid2/src/main/java/org/mycore/orcid2/v3/work/MCRORCIDWorkService.java

 import org.mycore.orcid2.metadata.MCRORCIDPutCodeInfo;
 import org.mycore.orcid2.metadata.MCRORCIDUserInfo;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


golsch · 2026-03-24T07:38:21Z

mycore-orcid2/src/main/java/org/mycore/orcid2/v3/work/MCRORCIDWorkSummaryUtils.java

 import org.mycore.orcid2.exception.MCRORCIDTransformationException;
 import org.mycore.orcid2.metadata.MCRORCIDPutCodeInfo;
-import org.mycore.orcid2.util.MCRIdentifier;
+import org.mycore.datamodel.legalentity.MCRIdentifier;


golsch · 2026-03-24T07:43:57Z

mycore-mods/src/main/java/org/mycore/mods/MCRMODSPersonIdentifierService.java

+            if (LOGGER.isWarnEnabled()) {
+                LOGGER.warn("Could not update modsperson object for user id {}",
+                    userId.getValue(), e);
+            }


That would be an inconsistent state or a general error. In that case, I would rather expect an exception.

golsch · 2026-03-24T07:45:14Z

mycore-mods/src/main/java/org/mycore/mods/MCRMODSPersonIdentifierService.java

+     * @return all known identifiers or an empty set
+     */
+    @Override
+    public Set<MCRIdentifier> findAllIdentifiers(MCRIdentifier userId) {


see comment for MCRLegalEntityService#findAllIdentifiers

golsch · 2026-03-24T07:45:27Z

mycore-mods/src/main/java/org/mycore/mods/MCRMODSPersonIdentifierService.java

+     * @param attributeToAdd the nameIdentifier to add to the modsperson
+     */
+    @Override
+    public void addIdentifier(MCRIdentifier userId, MCRIdentifier attributeToAdd) {


see comment for MCRLegalEntityService#addIdentifier

golsch · 2026-03-24T07:46:20Z

mycore-user2/src/main/java/org/mycore/user2/MCRUserIdentifierService.java

+     * @param userId the user id
+     * @return a nullable Optional that might contain a user
+     */
+    private Optional<MCRUser> findUserByUserID(MCRIdentifier userId) {


see comments for MCRLegalEntityService.

golsch · 2026-03-24T07:48:58Z

mycore-base/src/main/resources/config/mycore.properties

+# LegalEntityService
+##############################################################################
+# No-op Implementation of MCRLegalEntityService for querying UserIdentifier
+MCR.LegalEntityService.Class=org.mycore.datamodel.legalentity.MCRNoOpLegalEntityService


Wouldn’t it be better to just leave it empty? It doesn’t seem to serve any purpose…

erodde and others added 11 commits November 12, 2025 08:05

MCR-3441 base for new generic interface

1bd71fc

MCR-3441 fix PMD

cbcfb05

MCR-3441 using LegalEntityService in ORCID2 + unit tests

fff7193

MCR-3441 fix pom.xml

2df6d0e

MCR-3441 checkstyle

b11cd3c

MCR-3441 pmd

3007f9c

MCR-3441 missing dependency

6bf8d8a

MCR-3441 Added cases to MCRORCIDUserTest

fb2ec01

Merge branch 'main' into issues/MCR-3441-changes-modsperson-attribute…

b73e199

…-handling

MCR-3441 small improvements

a2b3298

MCR-3441 codestyle

4e5f633

erodde requested review from fluetze, golsch and kkrebs December 12, 2025 10:31

erodde mentioned this pull request Dec 12, 2025

MCR-3578 Changes modsperson attribute handling #2733

Closed

golsch requested changes Dec 16, 2025

View reviewed changes

erodde and others added 5 commits January 7, 2026 11:12

MCR-3578 code review + use mockito for mocking

94f1811

Merge branch 'main' into issues/MCR-3578-changes-modsperson-attribute…

b8ba51c

…-handling

MCR-3578 access MCRLegalEntityService implementation through interface

12415b3

MCR-3578 remove method findTypedIdentifiers

e548f01

Merge branch 'main' into issues/MCR-3578-changes-modsperson-attribute…

b621947

…-handling

erodde requested a review from golsch January 19, 2026 12:19

toKrause requested changes Jan 19, 2026

View reviewed changes

MCR-3578 javadoc and fallback implementation MCRNoOpLegalEntityService

abf299f

toKrause requested review from toKrause and removed request for toKrause January 19, 2026 14:56

toKrause added the postponed label Feb 10, 2026

Merge branch 'main' into issues/MCR-3578-changes-modsperson-attribute…

7f8554e

…-handling

golsch requested changes Mar 24, 2026

View reviewed changes

		legalEntityService = MCRConfiguration2.getInstanceOfOrThrow(
		MCRLegalEntityService.class, MCRORCIDConstants.CONFIG_PREFIX + "LegalEntityService.Class");

Conversation

erodde commented Dec 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golsch Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erodde commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

golsch Dec 22, 2025 •

edited

Loading