Pr1370 review#1390
Conversation
…ent communities only, like handle prefixes
…n Spring instead of pushing them in init()
There was a problem hiding this comment.
Pull request overview
This PR addresses follow-up review feedback from #1370 (DOI configuration per community) by clarifying the “community membership” semantics, moving per-prefix DataCite credentials into Spring wiring (via config placeholders), and improving provider dispatch behavior and related integration tests.
Changes:
- Document and enforce (via behavior + tests) that provider matching uses only the direct parent communities of the owning collection (top-level community does not cover sub-communities).
- Update the sample Spring configuration to wire per-prefix DataCite connector credentials/prefix via
${...}placeholders resolved from DSpace configuration. - Refactor provider lookup to cache per-collection provider matches and add a dedicated prefix-based provider lookup for
deleteOnline, plus extend/adjust integration tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| dspace/config/spring/api/identifier-service.xml | Expands documentation and demonstrates per-provider connector wiring with username/password/doiPrefix from config placeholders. |
| dspace-api/src/test/java/org/dspace/identifier/ClarinDOIIdentifierProviderIT.java | Fixes an assertion target and adds coverage for sub-community minting semantics. |
| dspace-api/src/main/java/org/dspace/identifier/doi/DataCiteConnector.java | Simplifies getDoiPrefix() to prefer the explicitly injected prefix (if set) without mutating cached state. |
| dspace-api/src/main/java/org/dspace/identifier/ClarinDOIIdentifierProvider.java | Reworks provider dispatch to cache matches per collection and ensures deleteOnline selects provider by DOI prefix. |
| dspace-api/src/main/java/org/dspace/identifier/ClarinCommunityDOIIdentifierProvider.java | Shifts connector credential/prefix configuration responsibility to Spring wiring and keeps provider prefix/namespace validation. |
| log.info("Item {} is not in a configured CLARIN community, skipping DOI {}", dso.getID(), actionName); | ||
| } | ||
|
|
||
| protected static class SimpleCache<K, V> { |
There was a problem hiding this comment.
The SimpleCache was limited to 5 items, and is replaced by unlimited ConcurrentHashMap.
My concern is that It may occupy lot of memory.
| <bean parent = "org.dspace.identifier.doi.DOIConnector"> | ||
| <property name="username" value="${identifier.doi.10.83111.user}" /> | ||
| <property name="password" value="${identifier.doi.10.83111.password}" /> | ||
| <property name="doiPrefix" value="10.83111" /> |
There was a problem hiding this comment.
Can we avoid the same doiPrefix to be declared twice ?
I'd leave the doiPrefix to be the property of Provider, and username, password properties of DataCiteConnector,
and use the init() method to pass the doiPrefix to connector.
BTW, after this change, it doesn't matter what format of username/password properties will be used for connector. It could be whatever format. What's mentioned here is just a convention.
Problem description
addressing the review comments of #1370
Copilot review