Skip to content

MCR-3723 Modernize and harmonize PI generator configuration, add unit tests#2983

Open
toKrause wants to merge 2 commits into
mainfrom
issues/MCR-3723_Modernize-PI-generator-configuration
Open

MCR-3723 Modernize and harmonize PI generator configuration, add unit tests#2983
toKrause wants to merge 2 commits into
mainfrom
issues/MCR-3723_Modernize-PI-generator-configuration

Conversation

@toKrause

@toKrause toKrause commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Link to jira.

Depends on #2982.

Pull Request Checklist (Author)

Please go through the following checklist before assigning the PR for review:

Ticket & Documentation

  • The issue in the ticket is clearly described and the solution is documented.
  • Design decisions (if any) are explained.
  • The ticket references the correct source and target branches.
  • The fixed-version is correctly set in the ticket and matches the PR's target branch (main).

Feature & Improvement Specific Checks

  • Instructions on how to test or use the feature are included or linked (e.g. to documentation).
  • For UI changes: before & after screenshots are attached.
  • New features or migrations are documented.
  • Does this change affect existing applications, data, or configurations?
    • Yes: Is a migration required? If yes, describe it.
    • Breaking change is marked in the commit message.

Testing

  • I have tested the changes locally.
  • The feature behaves as described in the ticket.
  • Were existing tests modified?
    • Yes: explain the changes for reviewers.

MCR Conventions & Metadata

  • MCR naming conventions are followed
  • If the public API has changed:
    • Old API is deprecated or a migration is documented.
    • If not, no action needed.
  • Java license headers are added where necessary.
  • Javadoc is written for non-self-explanatory classes/methods (Clean Code).
  • All configuration options are documented in Javadoc and mycore.properties.
  • No default properties are hardcoded — all set via mycore.properties.

Multi-Repo Considerations

  • Is an equivalent PR in MIR required?
    • If yes, is it already created?

To be used in conjunction with MyCoRe-Org/mir#1390.

@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 7 times, most recently from 4cc6f8c to 4bd4efb Compare June 14, 2026 20:54
@toKrause toKrause changed the title Issues/mcr 3723 modernize pi generator configuration MCR-3723 Modernize pi generator configuration Jun 14, 2026
@toKrause toKrause changed the title MCR-3723 Modernize pi generator configuration MCR-3723 Modernize PI generator configuration Jun 14, 2026
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 4 times, most recently from 24b239e to c4102c8 Compare June 15, 2026 12:19
@toKrause toKrause changed the title MCR-3723 Modernize PI generator configuration MCR-3723 Modernize and harmonize PI generator configuration Jun 15, 2026
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 3 times, most recently from d10df41 to 3a483ac Compare June 15, 2026 16:31
@toKrause
toKrause marked this pull request as ready for review June 15, 2026 17:23
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 6 times, most recently from 95b6a2b to f0dbf41 Compare June 18, 2026 09:31
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch from f0dbf41 to 137843d Compare June 19, 2026 11:01
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch from 137843d to 9737839 Compare July 6, 2026 12:52
@toKrause toKrause removed the postponed label Jul 6, 2026
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 2 times, most recently from 53dcf09 to 06e4ed2 Compare July 7, 2026 19:00
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 3 times, most recently from 72cd1ae to aa743cf Compare July 7, 2026 19:22
@toKrause toKrause changed the title MCR-3723 Modernize and harmonize PI generator configuration Modernize and harmonize PI generator configuration, add unit tests Jul 8, 2026
@toKrause toKrause changed the title Modernize and harmonize PI generator configuration, add unit tests MCR-3723 Modernize and harmonize PI generator configuration, add unit tests Jul 8, 2026
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch from aa743cf to 62ba61f Compare July 13, 2026 14:04
@toKrause
toKrause requested a review from sebhofmann July 13, 2026 14:05
@toKrause

Copy link
Copy Markdown
Contributor Author

@sebhofmann As requested, I moved the data-formatting-stuff into a separate PR: #3040

@@ -300,7 +323,8 @@ private String applyCount(String resultingPI) {
LOGGER.info("Count is {}", count);
final String pattern = IntStream.range(0, Math.abs(countPrecision)).mapToObj((i) -> "0")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should use formatCount.

}

@Override
protected String buildDOI(MCRBase base, String additional) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be synchronized

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

* </code></pre>
*/
@MCRConfigurationProxy(proxyClass = MCRCreateDateDNBURNGenerator.Factory.class)
public class MCRCreateDateDNBURNGenerator extends MCRDNBURNGeneratorBase {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need that. The MCRGenericPIGenerator can do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, simply added for symmetry, because MCRCreateDateDOIGenerator exists.

@MCRConfigurationProxy(proxyClass = MCRCreateDateDOIGenerator.Factory.class)
public class MCRCreateDateDOIGenerator extends MCRDOIGeneratorBase {

public static final String DEFAULT_DATE_FORMAT = "yyyyMMdd-HHmmss";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just deprecate that thing, because MCRGenericPIGenerator can do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I will make this a small agenda item in today's web-call. Either

  • keep MCRCreateDateDOIGenerator and add MCRCreateDateDNBURNGenerator or
  • deprecate MCRCreateDateDOIGenerator and drop MCRCreateDateDNBURNGenerator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true actually, MCRGenericPIGenerator only supports the current date, not the object creation date.

* </code></pre>
*/
@MCRConfigurationProxy(proxyClass = MCRCurrentDateDOIGenerator.Factory.class)
public class MCRCurrentDateDOIGenerator extends MCRDOIGeneratorBase {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really anyone using it?

@toKrause toKrause Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, simply added for symmetry, because MCRCurrentDateDNBURNGenerator (formally kown as MCRFLURNGenerator) exists.

import org.mycore.pi.util.MCRFLDateScrambler;

/**
* {@link MCRCurrentDateDOIGenerator} is a {@link MCRPIGenerator} for {@link MCRDNBURN} identifiers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its for DOI not for URN

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

public static final String DEFAULT_DATE_FORMAT = "ddMMyyyy";

static final String PLACE_HOLDER_OBJECT_TYPE = "$ObjectType";
public static final Locale DFAULT_DATE_LOCALE = Locale.ROOT;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYPO

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 2 times, most recently from d69d40e to 425a2e2 Compare July 14, 2026 10:17
@toKrause

toKrause commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

With regards to removing or deprecating specialized generators:

  • MCRCreateDateDOIGenerator / MCRCreateDateDNBURNGenerator can not be replaced with MCRGenericPIGenerator because the generic generator only has a placeholder for the current date, not the creation date.
  • MCRCurrentDateDOIGenerator / MCRCurrentDateDNBURNGenerator can not be replaced with MCRGenericPIGenerator due to the vastly differing date formatting strategies; this would require MCR-3670, which is a follow-up to this PR.

Therefore, I suggest the following course of action:

  • merge this PR as is (keeps the code base backwards compatible and consistent)
  • merge MCR-3760 (keeps the code base backwards compatible and makes it more flexible)
  • create PR for MCR-3763 that enhances the generic generator and deprecates the specialized generators

@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch 2 times, most recently from 562da4f to e0a5b53 Compare July 15, 2026 09:07
@yagee-de
yagee-de requested a review from Copilot July 21, 2026 11:41
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch from e0a5b53 to bbc567b Compare July 21, 2026 11:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes and harmonizes the Persistent Identifier (PI) generator implementation and configuration style in mycore-pi, introducing new generator base classes and configuration-proxy factories, while expanding unit test coverage across URN/DOI/PURL generators.

Changes:

  • Refactors PI generators to use constructor-based configuration via @MCRConfigurationProxy factories (and deprecates older generator types).
  • Adds new URN and DOI generators (UUID/current-date/create-date/“other PI” based variants) plus shared utility helpers.
  • Reworks/extends unit tests to cover the new generator variants and updated configuration approach.

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
mycore-pi/src/test/java/org/mycore/pi/urn/rest/MCRURNGranularRESTServiceTest.java Updates URN generator used in REST service test configuration.
mycore-pi/src/test/java/org/mycore/pi/urn/rest/MCRURNGranularRESTRegistrationTaskTest.java Updates test utility usage and exception signature to align with new generator behavior.
mycore-pi/src/test/java/org/mycore/pi/urn/MCRUUIDDNBURNGeneratorTest.java Adds unit tests for UUID-based DNB URN generation.
mycore-pi/src/test/java/org/mycore/pi/urn/MCROtherPIDNBURNGeneratorTest.java Adds unit tests for URN generation based on extracting values from another PI.
mycore-pi/src/test/java/org/mycore/pi/urn/MCRDNBURNGeneratorTest.java Removes legacy URN generator test.
mycore-pi/src/test/java/org/mycore/pi/urn/MCRCurrentDateDNBURNGeneratorTest.java Adds unit tests for current-date-based DNB URN generation.
mycore-pi/src/test/java/org/mycore/pi/urn/MCRCreateDateDNBURNGeneratorTest.java Adds unit tests for create-date + counter DNB URN generation.
mycore-pi/src/test/java/org/mycore/pi/purl/MCRIDPURLGeneratorTest.java Updates PURL generator tests to use direct construction instead of configuration lookup.
mycore-pi/src/test/java/org/mycore/pi/MCRPIUtils.java Updates test helpers to use the new UUID DNB URN generator and pass namespace explicitly.
mycore-pi/src/test/java/org/mycore/pi/MCRPIManagerTest.java Adjusts test configuration (removes generator namespace/test property usage).
mycore-pi/src/test/java/org/mycore/pi/MCRMockIdentifierParser.java Tightens parser generics to return the mock identifier type.
mycore-pi/src/test/java/org/mycore/pi/MCRMockIdentifierGenerator.java Refactors mock generator to the new MCRPIGenerator interface style.
mycore-pi/src/test/java/org/mycore/pi/MCRGenericPIGeneratorTest.java Reworks tests to match the new MCRGenericPIGenerator constructor/config inputs and adds multi-generate assertions.
mycore-pi/src/test/java/org/mycore/pi/doi/MCRUUIDDOIGeneratorTest.java Adds unit tests for UUID-based DOI generation.
mycore-pi/src/test/java/org/mycore/pi/doi/MCROtherPIDOIGeneratorTest.java Adds unit tests for DOI generation based on extracting values from another PI.
mycore-pi/src/test/java/org/mycore/pi/doi/MCRMapObjectIDDOIGeneratorTest.java Refactors DOI mapping generator tests to constructor-based configuration and expands cases.
mycore-pi/src/test/java/org/mycore/pi/doi/MCRCurrentDateDOIGeneratorTest.java Adds unit tests for current-date-based DOI generation.
mycore-pi/src/test/java/org/mycore/pi/doi/MCRCreateDateDOIGeneratorTest.java Adds unit tests for create-date + counter DOI generation.
mycore-pi/src/main/resources/components/pi/config/mycore.properties Adds a new configured generator entry (DateDOI).
mycore-pi/src/main/java/org/mycore/pi/util/MCRPIGeneratorUtils.java Introduces shared utility functions for count handling and create-date retrieval.
mycore-pi/src/main/java/org/mycore/pi/util/MCROtherPIValueExtractor.java Adds reusable value extraction logic for “other PI” generators.
mycore-pi/src/main/java/org/mycore/pi/util/MCRFLDateScrambler.java Extracts FL date scrambling into a reusable utility.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRUUIDURNGenerator.java Deprecates legacy UUID URN generator in favor of the new UUID DNB URN generator (proxy-based).
mycore-pi/src/main/java/org/mycore/pi/urn/MCRUUIDDNBURNGenerator.java Adds UUID-based DNB URN generator with configuration proxy.
mycore-pi/src/main/java/org/mycore/pi/urn/MCROtherPIDNBURNGenerator.java Adds “other PI”-based DNB URN generator with configuration proxy.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRFLURNGenerator.java Deprecates legacy FL URN generator in favor of current-date DNB URN generator (proxy-based).
mycore-pi/src/main/java/org/mycore/pi/urn/MCRDNBURNGeneratorBase.java Introduces a new base class for DNB URN generators with explicit namespace/delimiter handling.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRDNBURNGenerator.java Deprecates legacy base generator in favor of MCRDNBURNGeneratorBase.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRCurrentDateDNBURNGenerator.java Adds current-date DNB URN generator with proxy-based configuration.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRCreateDateDNBURNGenerator.java Adds create-date + counter DNB URN generator with proxy-based configuration.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRCountingDNBURNGeneratorBase.java Introduces new counting base class for DNB URN generators.
mycore-pi/src/main/java/org/mycore/pi/urn/MCRCountingDNBURNGenerator.java Deprecates legacy counting base generator in favor of MCRCountingDNBURNGeneratorBase.
mycore-pi/src/main/java/org/mycore/pi/purl/MCRIDPURLGenerator.java Refactors PURL generator to constructor-based config + proxy factory and adds validation.
mycore-pi/src/main/java/org/mycore/pi/MCRPIGenerator.java Converts MCRPIGenerator from an abstract class to an interface.
mycore-pi/src/main/java/org/mycore/pi/MCRPICreationEventHandler.java Changes PI auto-creation flow to iteratively register newly-creatable services.
mycore-pi/src/main/java/org/mycore/pi/MCRGenericPIGenerator.java Refactors generic generator to constructor/proxy-based configuration, new mapping types, and shared count utilities.
mycore-pi/src/main/java/org/mycore/pi/doi/MCRUUIDDOIGenerator.java Refactors UUID DOI generator to proxy-based configuration and a shared DOI base class.
mycore-pi/src/main/java/org/mycore/pi/doi/MCROtherPIDOIGenerator.java Adds “other PI”-based DOI generator with proxy factory.
mycore-pi/src/main/java/org/mycore/pi/doi/MCRMapObjectIDDOIGenerator.java Refactors map-based DOI generator to proxy-based configuration and shared base class.
mycore-pi/src/main/java/org/mycore/pi/doi/MCRDOIGeneratorBase.java Introduces shared DOI generator base class.
mycore-pi/src/main/java/org/mycore/pi/doi/MCRCurrentDateDOIGenerator.java Adds current-date DOI generator with proxy factory.
mycore-pi/src/main/java/org/mycore/pi/doi/MCRCreateDateDOIGenerator.java Refactors create-date DOI generator to shared utilities and proxy-based configuration.
mycore-mods/src/main/java/org/mycore/mods/classification/mapping/MCRMODSXMappingClassificationGenerator.java Minor documentation wording adjustments (“behavior”, formatting).
mycore-base/src/main/java/org/mycore/datamodel/classifications2/mapping/MCRXPathClassificationMappingCondition.java Documentation wording adjustments (“XPath”).
mycore-base/src/main/java/org/mycore/datamodel/classifications2/mapping/MCRDefaultXMappingClassificationGenerator.java Minor documentation wording adjustments (“behavior”, formatting).
Comments suppressed due to low confidence (1)

mycore-pi/src/main/java/org/mycore/pi/doi/MCRDOIGeneratorBase.java:39

  • parser.parse(...).get() will throw NoSuchElementException when a generated DOI does not pass MCRDOIParser validation (e.g. missing 10. prefix or /). Convert this into an MCRPersistentIdentifierException with a useful message so callers get the expected checked exception type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mycore-pi/src/main/java/org/mycore/pi/doi/MCRCreateDateDOIGenerator.java Outdated
Comment on lines +106 to +108
String prefixWithDate = formatDate(getCreateDate(base)) + "-";
int count = getCount(Pattern.quote(namespace + delimiter + prefixWithDate) + countPattern
+ Pattern.quote(delimiter) + "[0-9]");
Comment on lines +222 to +225
final Date objectCreateDate = base.getService().getDate(MCRObjectService.DATE_TYPE_CREATEDATE);
if (objectCreateDate == null) {
throw new MCRPersistenceException("Object " + base.getId() + " doesn't have a create date!");
}
Comment thread mycore-pi/src/test/java/org/mycore/pi/urn/MCRCreateDateDNBURNGeneratorTest.java Outdated
Comment thread mycore-pi/src/test/java/org/mycore/pi/urn/MCRUUIDDNBURNGeneratorTest.java Outdated
Comment thread mycore-pi/src/test/java/org/mycore/pi/doi/MCRUUIDDOIGeneratorTest.java Outdated
@toKrause
toKrause force-pushed the issues/MCR-3723_Modernize-PI-generator-configuration branch from bbc567b to 7c1ee25 Compare July 21, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants