[Port to dtq-dev] Issue ufal/clarin-dspace#1351 simple ror authority#1349
Open
kosarko wants to merge 1 commit into
Open
[Port to dtq-dev] Issue ufal/clarin-dspace#1351 simple ror authority#1349kosarko wants to merge 1 commit into
kosarko wants to merge 1 commit into
Conversation
* Issue 1351: SimpleRORAuthority * integration test * code cleanup * add debug messages to find test failure * Revert "add debug messages to find test failure" This reverts commit 286fde4. * test failures * Revert "test failures" This reverts commit 8808f5e. * resolve Copilot comments * resolve PR comments, add ROR lookup to Publisher field * rollback changes in VocabularyEntryLinkRepository * fixing failing tests * implement PR Comments * implementation improvement * resolve PR comments (O.Kosarko) * Address review nits: shared ObjectMapper, commons-lang3, IT cleanup - Reuse a single static ObjectMapper in SimpleRORAuthority instead of constructing one per call. - Switch to the non-deprecated org.apache.commons.lang3.LocaleUtils. - Reset the ChoiceAuthority plugin configuration in @afterclass so VocabularyEntryLinkRepositoryIT no longer leaks the SimpleRORAuthority registration into other integration tests. * implement cache for gertLabel() * resolve PR Comments --------- Co-authored-by: Ondrej Kosarko <kosarko@ufal.mff.cuni.cz> (cherry picked from commit 25a5a25)
📝 WalkthroughWalkthroughAdds a ChangesROR Authority Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
dspace-api/src/main/java/org/dspace/content/authority/SimpleRORAuthority.java (1)
68-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Javadoc for
getChoice().This new public override is the only undocumented public method in the class. As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/main/java/org/dspace/content/authority/SimpleRORAuthority.java` around lines 68 - 71, `SimpleRORAuthority.getChoice(String authKey, String locale)` is the only public override missing documentation. Add a Javadoc comment directly above `getChoice()` that describes its purpose and parameters, matching the style used for the other public methods in `SimpleRORAuthority`.Source: Coding guidelines
dspace-api/src/test/java/org/dspace/external/MockRorRestConnector.java (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
pagein the mock so pagination mapping is actually tested.Right now every query returns the same fixture regardless of the requested upstream page. The paging ITs therefore prove the downstream slicing logic, but not that
getMatches()sends the correct RORpagevalue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/test/java/org/dspace/external/MockRorRestConnector.java` around lines 28 - 33, The mock in MockRorRestConnector#getByQuery currently ignores the page argument, so it always returns the same fixture and never verifies upstream pagination. Update getByQuery(String query, int page) to branch on the requested page value and return different mock responses for different pages, while preserving the exact-query behavior based on query. This should let the paging ITs exercise getMatches() and confirm it sends the correct ROR page value.dspace-api/src/main/java/org/dspace/external/model/ror/Location.java (1)
23-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Javadocs to the public API in this model.
The class-level Javadoc is present, but the new public constructors/getters here are undocumented. As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
Also applies to: 46-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/main/java/org/dspace/external/model/ror/Location.java` around lines 23 - 35, Add Javadoc comments for the new public API in Location: document the public constructor and the getters getGeonamesId() and getGeonamesDetails(), following the existing class-level style. Make sure the documentation clearly describes the parameters and returned values, and apply the same treatment to the other public methods in this model mentioned by the review so all exposed API members are covered.Source: Coding guidelines
dspace-api/src/test/data/dspaceFolder/config/spring/api/ror-authority-services.xml (1)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the repository XML conventions.
This new XML file is missing the required license header and the body is indented with 4 spaces instead of 2. As per coding guidelines, "Ensure all source files include required license headers" and "Use 4-space indents for Java code, 2-space for XML, with NO TABS".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/test/data/dspaceFolder/config/spring/api/ror-authority-services.xml` around lines 1 - 15, The new Spring XML in ror-authority-services.xml needs to match repository conventions: add the required license header at the top and reformat the file body to use 2-space XML indentation with no tabs. Update the structure around the existing beans/context entries while keeping the same bean definitions, and ensure the file layout follows the same XML style used by other test resources.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dspace-api/src/main/java/org/dspace/external/model/ror/RorItem.java`:
- Around line 31-40: The RorItem constructor currently stores names, locations,
and types as-is, but RorRestConnector.toChoice() expects them to be non-null and
calls collection methods on them. Update RorItem to null-normalize these fields
in the constructor so partial ROR payloads become empty values instead of null,
and verify any related getters/usage in toChoice() and the RorItem constructor
path still work with empty collections/arrays.
In `@dspace-api/src/main/java/org/dspace/external/model/ror/RorItems.java`:
- Around line 29-35: `RorItems` currently stores a nullable `items` list, which
can cause `RorRestConnector.getMatches()` and `getBestMatch()` to fail when they
immediately call `getItems().isEmpty()`. Update the `RorItems` constructor to
normalize a missing or null `items` argument to an empty list, and ensure
`getItems()` always returns a non-null list so lookup code can safely check
emptiness.
In `@dspace-api/src/main/java/org/dspace/external/ror/CacheLogger.java`:
- Around line 22-23: Add Javadoc for the public override in
CacheLogger.onEvent(...). Document the method’s purpose and its CacheEvent
parameter so it complies with the project rule that all public Java methods must
have Javadoc, keeping the comment aligned with the existing behavior in this
override.
In `@dspace-api/src/main/java/org/dspace/external/RorRestConnector.java`:
- Around line 89-93: The current RorRestConnector.getLabel() cache policy is
storing the fallback raw rorID when getChoice() misses or errors, since the
method always returns a non-null value. Update the caching behavior so only real
labels from Choice are cached and the bare id is never persisted; if needed,
move the fallback handling out of the cached path or gate caching on the lookup
result before returning rorID.
- Line 50: The RorRestConnector client is created without explicit timeouts, so
update the static Client initialization to configure connection/read timeouts on
the ClientBuilder/newClient setup. Also fix getLabel() so it does not store the
raw rorID in rorLabels when the lookup result is null; instead only cache
successful labels and return the fallback rorID directly for that call.
In
`@dspace-server-webapp/src/test/java/org/dspace/app/rest/VocabularyEntryLinkRepositoryIT.java`:
- Around line 118-130: The locale-switching tests in
VocabularyEntryLinkRepositoryIT are leaking configuration state by restoring
ror.authority.stored-name-type to a hardcoded value instead of its original
value. In each affected test, capture the existing
ror.authority.stored-name-type before setting it, then restore that saved value
in the finally block alongside default.locale so the tests remain isolated and
order-independent.
In `@dspace/config/spring/api/ror-authority-services.xml`:
- Line 1: The new Spring XML config is missing the repository’s required
source-file license header. Add the standard license block at the top of this
file, before the existing XML declaration, following the same header format used
by other XML configs in the repo so it complies with the `**/*.{java,xml}`
guideline.
---
Nitpick comments:
In
`@dspace-api/src/main/java/org/dspace/content/authority/SimpleRORAuthority.java`:
- Around line 68-71: `SimpleRORAuthority.getChoice(String authKey, String
locale)` is the only public override missing documentation. Add a Javadoc
comment directly above `getChoice()` that describes its purpose and parameters,
matching the style used for the other public methods in `SimpleRORAuthority`.
In `@dspace-api/src/main/java/org/dspace/external/model/ror/Location.java`:
- Around line 23-35: Add Javadoc comments for the new public API in Location:
document the public constructor and the getters getGeonamesId() and
getGeonamesDetails(), following the existing class-level style. Make sure the
documentation clearly describes the parameters and returned values, and apply
the same treatment to the other public methods in this model mentioned by the
review so all exposed API members are covered.
In
`@dspace-api/src/test/data/dspaceFolder/config/spring/api/ror-authority-services.xml`:
- Around line 1-15: The new Spring XML in ror-authority-services.xml needs to
match repository conventions: add the required license header at the top and
reformat the file body to use 2-space XML indentation with no tabs. Update the
structure around the existing beans/context entries while keeping the same bean
definitions, and ensure the file layout follows the same XML style used by other
test resources.
In `@dspace-api/src/test/java/org/dspace/external/MockRorRestConnector.java`:
- Around line 28-33: The mock in MockRorRestConnector#getByQuery currently
ignores the page argument, so it always returns the same fixture and never
verifies upstream pagination. Update getByQuery(String query, int page) to
branch on the requested page value and return different mock responses for
different pages, while preserving the exact-query behavior based on query. This
should let the paging ITs exercise getMatches() and confirm it sends the correct
ROR page value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f731f9e8-4f42-46a6-999f-aeda5a2256cc
📒 Files selected for processing (15)
dspace-api/src/main/java/org/dspace/content/authority/SimpleRORAuthority.javadspace-api/src/main/java/org/dspace/external/RorRestConnector.javadspace-api/src/main/java/org/dspace/external/model/ror/Location.javadspace-api/src/main/java/org/dspace/external/model/ror/RorItem.javadspace-api/src/main/java/org/dspace/external/model/ror/RorItems.javadspace-api/src/main/java/org/dspace/external/ror/CacheLogger.javadspace-api/src/test/data/dspaceFolder/config/spring/api/ror-authority-services.xmldspace-api/src/test/java/org/dspace/external/MockRorRestConnector.javadspace-api/src/test/resources/org/dspace/external/ror/UniversityOfPisa.jsondspace-api/src/test/resources/org/dspace/external/ror/UniversityOfPisaByID.jsondspace-api/src/test/resources/org/dspace/external/ror/UniversityOfPisaByQueryExact.jsondspace-server-webapp/src/test/java/org/dspace/app/rest/VocabularyEntryLinkRepositoryIT.javadspace/config/ehcache.xmldspace/config/features/enable-ror.cfgdspace/config/spring/api/ror-authority-services.xml
Comment on lines
+118
to
+130
| ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService(); | ||
| String defaultLocale = configurationService.getProperty("default.locale"); | ||
| configurationService.setProperty("default.locale", "it"); | ||
| configurationService.setProperty("ror.authority.stored-name-type", "locale_label"); | ||
|
|
||
| try { | ||
| checkSingleItemResponse(getClient().perform(get(ROR_AUTHORITY_ENTRIES_URL) | ||
| .param("filter", "University of Pisa") | ||
| .param("exact", "true")), "Università di Pisa", "Università di Pisa"); | ||
| } finally { | ||
| configurationService.setProperty("default.locale", defaultLocale); | ||
| configurationService.setProperty("ror.authority.stored-name-type", "en_label"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore the original ror.authority.stored-name-type after each test.
Both locale-switching tests overwrite ror.authority.stored-name-type and then hardcode it back to "en_label" in finally. If the suite starts with a different value, this class leaks state into later tests and makes the outcome order-dependent.
Suggested change
ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
String defaultLocale = configurationService.getProperty("default.locale");
+ String originalStoredNameType =
+ configurationService.getProperty("ror.authority.stored-name-type", "en_label");
configurationService.setProperty("default.locale", "it");
configurationService.setProperty("ror.authority.stored-name-type", "locale_label");
@@
} finally {
configurationService.setProperty("default.locale", defaultLocale);
- configurationService.setProperty("ror.authority.stored-name-type", "en_label");
+ configurationService.setProperty("ror.authority.stored-name-type", originalStoredNameType);
}Also applies to: 135-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@dspace-server-webapp/src/test/java/org/dspace/app/rest/VocabularyEntryLinkRepositoryIT.java`
around lines 118 - 130, The locale-switching tests in
VocabularyEntryLinkRepositoryIT are leaking configuration state by restoring
ror.authority.stored-name-type to a hardcoded value instead of its original
value. In each affected test, capture the existing
ror.authority.stored-name-type before setting it, then restore that saved value
in the finally block alongside default.locale so the tests remain isolated and
order-independent.
There was a problem hiding this comment.
Pull request overview
Ports CLARIN-DSpace ROR authority support to dtq-dev, enabling DSpace ChoiceAuthority lookups against the ROR API (primarily for dc.publisher) with locale-aware labels and cached label resolution.
Changes:
- Added
SimpleRORAuthorityandRorRestConnectorto query ROR by search or ID and map results into DSpaceChoice/Choices. - Introduced Spring wiring + feature toggle config (
enable-ror.cfg) and Ehcache configuration forror-labels. - Added mock connector + JSON fixtures and a REST integration test covering search, exact match, pagination constraints, and locale behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| dspace/config/spring/api/ror-authority-services.xml | Registers RorRestConnector bean and binds ROR API URL/client-id config. |
| dspace/config/features/enable-ror.cfg | Adds feature config to register SimpleRORAuthority and configure publisher choices + ROR settings. |
| dspace/config/ehcache.xml | Adds ror-labels cache and template for caching ROR label lookups. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/VocabularyEntryLinkRepositoryIT.java | Integration tests for ROR vocabulary endpoint behavior (size/page/exact/locale/ID). |
| dspace-api/src/test/resources/org/dspace/external/ror/UniversityOfPisa*.json | Test fixtures for query/ID/exact query responses. |
| dspace-api/src/test/java/org/dspace/external/MockRorRestConnector.java | Test connector stub returning fixture data. |
| dspace-api/src/test/data/dspaceFolder/config/spring/api/ror-authority-services.xml | Test Spring override wiring RorRestConnector to the mock implementation. |
| dspace-api/src/main/java/org/dspace/external/RorRestConnector.java | Core ROR REST integration + mapping into DSpace authority Choices, plus label caching. |
| dspace-api/src/main/java/org/dspace/external/ror/CacheLogger.java | Ehcache event listener for ROR cache debugging. |
| dspace-api/src/main/java/org/dspace/external/model/ror/RorItems.java | Model for ROR search responses. |
| dspace-api/src/main/java/org/dspace/external/model/ror/RorItem.java | Model for a single ROR item. |
| dspace-api/src/main/java/org/dspace/external/model/ror/Location.java | Model for ROR location details. |
| dspace-api/src/main/java/org/dspace/content/authority/SimpleRORAuthority.java | ChoiceAuthority implementation delegating to RorRestConnector. |
Comment on lines
+161
to
+179
| public Choices getBestMatch(String text, String locale) { | ||
| try (Response response = getByQuery(sanitizeQuery(text))) { | ||
| if (response.getStatus() == Response.Status.OK.getStatusCode()) { | ||
| try (InputStream is = response.readEntity(InputStream.class)) { | ||
| RorItems rorItems = OBJECT_MAPPER.readValue(is, RorItems.class); | ||
| List<RorItem> items = rorItems.getItems(); | ||
| if (items.isEmpty()) { | ||
| return new Choices(false); | ||
| } | ||
| Choice[] choices = {toChoice(items.get(0), getLocaleLanguage(locale), resolveStoredNameType())}; | ||
| return new Choices(choices, 0, 1, Choices.CF_UNCERTAIN, false); | ||
| } catch (Exception e) { | ||
| log.error("Error during search", e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return new Choices(true); | ||
| } |
Comment on lines
+195
to
+202
| private static String getLocaleLanguage(String locale) { | ||
| try { | ||
| return Optional.ofNullable(LocaleUtils.toLocale(locale)).map(Locale::getLanguage).orElse("en"); | ||
| } catch (IllegalArgumentException e) { | ||
| log.warn("Invalid locale format: " + locale + ", using default 'en' locale."); | ||
| return "en"; | ||
| } | ||
| } |
Comment on lines
+299
to
+301
| if (!rorItem.getLocations().isEmpty()) { | ||
| Location location = rorItem.getLocations().get(0); | ||
| Location.GeonamesDetails geonamesDetails = location.getGeonamesDetails(); |
| } | ||
|
|
||
| public Response getByID(String rorID) { | ||
| if (rorID.matches(ROR_ID_PATTERN)) { |
Collaborator
There was a problem hiding this comment.
@kuchtiak-ufal This comment seem to be valid, please would you look at it?
| Choice c = new Choice(); | ||
| c.authority = authority; | ||
|
|
||
| List<RorItem.Name> names = rorItem.getNames(); |
milanmajchrak
requested changes
Jul 1, 2026
| } | ||
|
|
||
| public Response getByID(String rorID) { | ||
| if (rorID.matches(ROR_ID_PATTERN)) { |
Collaborator
There was a problem hiding this comment.
@kuchtiak-ufal This comment seem to be valid, please would you look at it?
Comment on lines
+161
to
+179
| public Choices getBestMatch(String text, String locale) { | ||
| try (Response response = getByQuery(sanitizeQuery(text))) { | ||
| if (response.getStatus() == Response.Status.OK.getStatusCode()) { | ||
| try (InputStream is = response.readEntity(InputStream.class)) { | ||
| RorItems rorItems = OBJECT_MAPPER.readValue(is, RorItems.class); | ||
| List<RorItem> items = rorItems.getItems(); | ||
| if (items.isEmpty()) { | ||
| return new Choices(false); | ||
| } | ||
| Choice[] choices = {toChoice(items.get(0), getLocaleLanguage(locale), resolveStoredNameType())}; | ||
| return new Choices(choices, 0, 1, Choices.CF_UNCERTAIN, false); | ||
| } catch (Exception e) { | ||
| log.error("Error during search", e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return new Choices(true); | ||
| } |
Comment on lines
+195
to
+202
| private static String getLocaleLanguage(String locale) { | ||
| try { | ||
| return Optional.ofNullable(LocaleUtils.toLocale(locale)).map(Locale::getLanguage).orElse("en"); | ||
| } catch (IllegalArgumentException e) { | ||
| log.warn("Invalid locale format: " + locale + ", using default 'en' locale."); | ||
| return "en"; | ||
| } | ||
| } |
| Choice c = new Choice(); | ||
| c.authority = authority; | ||
|
|
||
| List<RorItem.Name> names = rorItem.getNames(); |
Comment on lines
+299
to
+301
| if (!rorItem.getLocations().isEmpty()) { | ||
| Location location = rorItem.getLocations().get(0); | ||
| Location.GeonamesDetails geonamesDetails = location.getGeonamesDetails(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of ufal#1352 by @kuchtiak-ufal to
dtq-dev.Summary by CodeRabbit
New Features
Bug Fixes
Tests