Issue #1181 doi configuration per community#1370
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for configuring multiple DOI providers scoped to specific communities (Issue #1181), enabling different DOI prefixes/credentials per community while delegating DOI operations to the appropriate provider based on an Item’s community.
Changes:
- Introduces
ClarinDOIIdentifierProvider(dispatcher) andClarinCommunityDOIIdentifierProvider(community-scoped provider) for community-specific DOI minting/registration. - Adds
ClarinDataCiteConnectorand extendsDataCiteConnectorto support overriding DOI prefix via setters (in addition to config). - Adds an integration test covering mint/register/reserve/update and online operations for community-scoped DOI providers, plus example Spring configuration (commented) for enabling it.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| dspace/config/spring/api/identifier-service.xml | Adds commented example Spring wiring for community-specific DOI providers and a CLARIN-specific DataCite connector bean. |
| dspace-api/src/main/java/org/dspace/identifier/doi/DataCiteConnector.java | Adds cached DOI prefix getter and uses it when preparing DataCite crosswalk parameters. |
| dspace-api/src/main/java/org/dspace/identifier/doi/ClarinDataCiteConnector.java | New connector subclass exposing setters for username/password/prefix to support per-community configuration. |
| dspace-api/src/main/java/org/dspace/identifier/ClarinDOIIdentifierProvider.java | New dispatcher DOI provider selecting a community-specific provider per item (and per DOI prefix for deleteOnline). |
| dspace-api/src/main/java/org/dspace/identifier/ClarinCommunityDOIIdentifierProvider.java | New community-scoped provider that configures connector credentials/prefix based on the configured DOI prefix. |
| dspace-api/src/test/java/org/dspace/identifier/ClarinDOIIdentifierProviderIT.java | New integration test validating delegation and DOI lifecycle behavior across multiple community-specific providers. |
kosarko
left a comment
kosarko
left a comment
There was a problem hiding this comment.
@kuchtiak-ufal I have a fix prepared for most of the changes. I'll ping you on the new PR
|
|
||
| private boolean isCollectionInProvider(ClarinCommunityDOIIdentifierProvider provider, | ||
| Collection collection) throws SQLException { | ||
| List<Community> parentCommunities = collection.getCommunities(); |
There was a problem hiding this comment.
collection.getCommunities() returns only the direct parent communities, not ancestors. If a provider is configured with a top-level community UUID/handle but the collection lives under a sub-community, no provider matches and the item silently gets no DOI (mint returns null with just an info log).
Either walk up the tree (e.g. communityService.getAllParents(...)) or document that only direct-parent communities are supported. The IT only covers the flat case, so it wouldn't catch this.
There was a problem hiding this comment.
Only direct-parent is consistent with the behavior we have for handles. Keep it
| assertTrue(doi4.startsWith("doi:10.1/1-")); | ||
| // check if the DOI for itemV2 is different from the one for item1 | ||
| assertFalse(doi4.startsWith(doi1)); | ||
| checkDoi(doi2, DOIIdentifierProvider.MINTED); |
There was a problem hiding this comment.
This re-checks doi2 — should this be checkDoi(doi4, DOIIdentifierProvider.MINTED)? As written, the persisted status of the freshly minted version DOI is never verified (testRegister below gets it right at line 200).
|
|
||
| private Set<String> communities = new HashSet<>(); | ||
|
|
||
| public void init() { |
There was a problem hiding this comment.
Pushing username/password/prefix onto the connector at init() is safe with the example wiring only because each provider gets its own inner-bean connector. If someone later rewires the connector as a shared ref, all providers silently collapse onto the last-initialized credentials/prefix — DOIs minted against the wrong DataCite account, no error.
Since ClarinDataCiteConnector already has the setters, consider configuring username/password/prefix directly on each connector bean in Spring and dropping this push (and the identifier.doi.<prefix>.user/.password indirection) entirely. Smaller class, no hidden coupling.
|
|
||
| private List<ClarinCommunityDOIIdentifierProvider> providers; | ||
|
|
||
| protected final SimpleCache<UUID, ClarinCommunityDOIIdentifierProvider> simpleCache = new SimpleCache<>(5); |
There was a problem hiding this comment.
Suggest dropping SimpleCache altogether. The lookup it guards is a handful of in-memory set-contains checks, DOI operations aren't a hot path, and with capacity 5 it will barely ever hit during a DOIOrganiser batch over many items. It adds a hand-rolled LRU plus concurrency surface for negligible benefit.
(Also note getProviderForItem only caches the null result in the parent-collection branch, so unmatched items re-run the full lookup each call — moot if the cache goes away.)
| return providers.stream() | ||
| .filter(provider -> provider.getPrefix().equals(prefix)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new DOIIdentifierException("No provider found for DOI prefix: " + prefix)); |
There was a problem hiding this comment.
This throw makes deleteOnline behave differently from every other delegation method, which log-and-skip when no provider matches. In a DOIOrganiser batch, a single legacy/foreign-prefix DOI will abort the whole run. Failing loud is defensible, but pick one behaviour deliberately and make it consistent.
| } | ||
|
|
||
| protected String getDoiPrefix() { | ||
| if (this.doiPrefix == null) { |
There was a problem hiding this comment.
Minor: doiPrefix is non-volatile, so two threads can race on this lazy init. The race is benign (both write the same config value), and ClarinDataCiteConnector sets the field at init so it never hits this path — just noting it's intentional-benign rather than overlooked.
No description provided.