Skip to content

Commit 3b0379b

Browse files
Micheleboychukvins01-4science
Micheleboychuk
authored and
vins01-4science
committed
Merged in task/dspace-cris-2023_02_x/DSC-2848 (pull request DSpace#5812)
DSC-2848: [2023] Improve Jisc OpenPolicyFinder (OPF) Approved-by: Vincenzo Mecca
2 parents ac57160 + cdfeb34 commit 3b0379b

7 files changed

Lines changed: 206 additions & 30 deletions

File tree

‎dspace-api/src/main/java/org/dspace/app/openpolicyfinder/OpenPolicyFinderService.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,9 @@ public OpenPolicyFinderPublisherResponse performPublisherRequest(String type, St
140140
sleepBetweenTimeouts));
141141

142142
try (CloseableHttpClient client = DSpaceHttpClientFactory.getInstance().buildWithoutAutomaticRetries(5)) {
143-
Thread.sleep(sleepBetweenTimeouts);
144-
143+
if (numberOfTries > 1) {
144+
Thread.sleep(sleepBetweenTimeouts);
145+
}
145146
// Construct a default HTTP method (first result)
146147
method = constructHttpGet(type, field, predicate, value, start, limit);
147148

@@ -155,6 +156,7 @@ public OpenPolicyFinderPublisherResponse performPublisherRequest(String type, St
155156
+ statusCode);
156157
String errorBody = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8);
157158
log.error("Error from Open Policy Finder HTTP request: " + errorBody);
159+
continue;
158160
}
159161

160162
HttpEntity responseBody = response.getEntity();
@@ -245,7 +247,9 @@ public OpenPolicyFinderResponse performRequest(String type, String field, String
245247
sleepBetweenTimeouts));
246248

247249
try (CloseableHttpClient client = DSpaceHttpClientFactory.getInstance().buildWithoutAutomaticRetries(5)) {
248-
Thread.sleep(sleepBetweenTimeouts);
250+
if (numberOfTries > 1) {
251+
Thread.sleep(sleepBetweenTimeouts);
252+
}
249253

250254
// Construct a default HTTP method (first result)
251255
method = constructHttpGet(type, field, predicate, value, start, limit);
@@ -254,14 +258,15 @@ public OpenPolicyFinderResponse performRequest(String type, String field, String
254258
try (CloseableHttpResponse response = client.execute(method)) {
255259
int statusCode = response.getStatusLine().getStatusCode();
256260

257-
log.debug(response.getStatusLine().getStatusCode() + ": "
258-
+ response.getStatusLine().getReasonPhrase());
261+
log.debug(statusCode
262+
+ ": " + response.getStatusLine().getReasonPhrase());
259263

260264
if (statusCode != HttpStatus.SC_OK) {
261265
opfResponse = new OpenPolicyFinderResponse(
262266
"Open Policy Finder return not OK status: " + statusCode);
263267
String errorBody = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8);
264268
log.error("Error from Open Policy Finder HTTP request: " + errorBody);
269+
continue;
265270
}
266271

267272
HttpEntity responseBody = response.getEntity();

‎dspace-api/src/main/java/org/dspace/content/authority/ItemAuthority.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public class ItemAuthority implements ChoiceAuthority, LinkableEntityAuthority {
5757
final static String CHOICES_EXTERNALSOURCE_PREFIX = "choises.externalsource.";
5858

5959
/** the name assigned to the specific instance by the PluginService, @see {@link NameAwarePlugin} **/
60-
private String authorityName;
60+
protected String authorityName;
6161

6262
protected DSpace dspace = new DSpace();
6363

@@ -77,7 +77,7 @@ public class ItemAuthority implements ChoiceAuthority, LinkableEntityAuthority {
7777
private ExternalDataService externalDataService = ExternalServiceFactory.getInstance().getExternalDataService();
7878

7979
// map of field key to presentation type
80-
protected Map<String, String> externalSource = new HashMap<String, String>();
80+
protected Map<String, String> externalSource = new HashMap<>();
8181

8282
public static final String DEFAULT = "local";
8383

@@ -253,7 +253,6 @@ public Map<String, String> getExternalSource() {
253253
} else {
254254
log.warn("Skipping invalid external source authority configuration property: " + sourceIdentifier +
255255
" does not exist");
256-
continue;
257256
}
258257
}
259258
}
@@ -267,7 +266,7 @@ public Map<String, String> getExtra(String key, String locale) {
267266
SolrClient solr = searchService.getSolrSearchCore().getSolr();
268267
if (Objects.isNull(solr)) {
269268
log.error("unable to find solr instance");
270-
return new HashMap<String, String>();
269+
return new HashMap<>();
271270
}
272271

273272

@@ -284,7 +283,7 @@ public Map<String, String> getExtra(String key, String locale) {
284283
List<Choice> choiceList = getChoiceListFromQueryResults(queryResponse.getResults(), key, false);
285284
if (choiceList.isEmpty()) {
286285
log.warn("No documents found for key=" + key);
287-
return new HashMap<String, String>();
286+
return new HashMap<>();
288287
}
289288

290289
return choiceList.iterator().next().extras;
@@ -293,7 +292,7 @@ public Map<String, String> getExtra(String key, String locale) {
293292
log.error(e.getMessage(), e);
294293
}
295294

296-
return new HashMap<String, String>();
295+
return new HashMap<>();
297296
}
298297

299298
protected int calculateConfidence(Choice[] choices) {

‎dspace-api/src/main/java/org/dspace/content/authority/OpenPolicyFinderAuthority.java‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import org.dspace.app.openpolicyfinder.OpenPolicyFinderService;
2222
import org.dspace.app.openpolicyfinder.v2.OpenPolicyFinderJournal;
2323
import org.dspace.app.openpolicyfinder.v2.OpenPolicyFinderResponse;
24-
import org.dspace.core.NameAwarePlugin;
2524
import org.dspace.services.ConfigurationService;
2625
import org.dspace.services.factory.DSpaceServicesFactory;
2726
import org.dspace.utils.DSpace;
@@ -41,12 +40,6 @@ public class OpenPolicyFinderAuthority extends ItemAuthority {
4140
private static final String PREDICATE_EQUALS = "equals";
4241
private static final String PREDICATE_CONTAINS_WORD = "contains word";
4342

44-
/**
45-
* the name assigned to the specific instance by the PluginService, @see
46-
* {@link NameAwarePlugin}
47-
**/
48-
private String authorityName;
49-
5043
private ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
5144

5245
private DSpace dspace = new DSpace();

‎dspace-api/src/main/java/org/dspace/content/authority/RorOrgUnitAuthority.java‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@ public class RorOrgUnitAuthority extends ItemAuthority {
3737
DSpaceServicesFactory.getInstance().getConfigurationService();
3838
private final PluginService pluginService = CoreServiceFactory.getInstance().getPluginService();
3939

40-
private String authorityName;
41-
4240
@Override
4341
public Choices getMatches(String text, int start, int limit, String locale) {
4442

@@ -170,14 +168,4 @@ private String composeAuthorityValue(String rorId) {
170168
public String getLinkedEntityType() {
171169
return configurationService.getProperty("cris.ItemAuthority." + authorityName + ".entityType");
172170
}
173-
174-
@Override
175-
public void setPluginInstanceName(String name) {
176-
authorityName = name;
177-
}
178-
179-
@Override
180-
public String getPluginInstanceName() {
181-
return authorityName;
182-
}
183171
}

‎dspace-api/src/test/java/org/dspace/content/authority/ItemAuthorityTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* The contents of this file are subject to the license and copyright
3+
* detailed in the LICENSE and NOTICE files at the root of the source
4+
* tree and available online at
5+
*
6+
* http://www.dspace.org/license/
7+
*/
8+
package org.dspace.content.authority;
9+
10+
import static org.junit.Assert.assertEquals;
11+
12+
import org.dspace.AbstractUnitTest;
13+
import org.dspace.services.ConfigurationService;
14+
import org.dspace.services.factory.DSpaceServicesFactory;
15+
import org.junit.Before;
16+
import org.junit.Test;
17+
18+
/**
19+
* Unit tests for ItemAuthority refactored code (DSC-2848)
20+
* Tests getSource() method, protected fields, and inheritance.
21+
*/
22+
public class ItemAuthorityTest extends AbstractUnitTest {
23+
24+
private ConfigurationService configurationService;
25+
private ItemAuthority itemAuthority;
26+
27+
@Before
28+
public void setUp() {
29+
configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
30+
itemAuthority = new ItemAuthority();
31+
itemAuthority.setPluginInstanceName("testAuthority");
32+
}
33+
34+
/**
35+
* Test that getSource() returns configured value when set.
36+
*/
37+
@Test
38+
public void testGetSourceWithConfiguredValue() {
39+
configurationService.setProperty("cris.ItemAuthority.testAuthority.source", "openpolicyfinder");
40+
String source = itemAuthority.getSource();
41+
assertEquals("Source should match configured value", "openpolicyfinder", source);
42+
}
43+
44+
/**
45+
* Test that getSource() returns default "local" when not configured.
46+
*/
47+
@Test
48+
public void testGetSourceWithDefaultValue() {
49+
String source = itemAuthority.getSource();
50+
assertEquals("Source should default to 'local'", "local", source);
51+
}
52+
53+
/**
54+
* Test that authorityName is accessible to subclasses (protected field).
55+
*/
56+
@Test
57+
public void testAuthorityNameIsProtected() {
58+
itemAuthority.setPluginInstanceName("testProtectedAccess");
59+
assertEquals("Authority name should be accessible", "testProtectedAccess",
60+
itemAuthority.getPluginInstanceName());
61+
// Verify the field is protected by checking subclass access
62+
OpenPolicyFinderAuthority subAuthority = new OpenPolicyFinderAuthority();
63+
subAuthority.setPluginInstanceName("subAuthority");
64+
assertEquals("Subclass should access protected authorityName", "subAuthority",
65+
subAuthority.getPluginInstanceName());
66+
}
67+
}

‎dspace-api/src/test/java/org/dspace/content/authority/OpenPolicyFinderAuthorityTest.java‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* The contents of this file are subject to the license and copyright
3+
* detailed in the LICENSE and NOTICE files at the root of the source
4+
* tree and available online at
5+
*
6+
* http://www.dspace.org/license/
7+
*/
8+
package org.dspace.content.authority;
9+
10+
import static org.junit.Assert.assertEquals;
11+
import static org.junit.Assert.assertNotNull;
12+
13+
import org.dspace.AbstractUnitTest;
14+
import org.dspace.services.ConfigurationService;
15+
import org.dspace.services.factory.DSpaceServicesFactory;
16+
import org.junit.Before;
17+
import org.junit.Test;
18+
19+
/**
20+
* Unit tests for OpenPolicyFinderAuthority refactoring (DSC-2848)
21+
* Validates:
22+
* - No duplicate authorityName field (inherits from ItemAuthority)
23+
* - getSource() works via inheritance from ItemAuthority
24+
* - No unused imports (validated by compile)
25+
*/
26+
public class OpenPolicyFinderAuthorityTest extends AbstractUnitTest {
27+
28+
private ConfigurationService configurationService;
29+
private OpenPolicyFinderAuthority authority;
30+
31+
@Before
32+
public void setUp() {
33+
configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
34+
authority = new OpenPolicyFinderAuthority();
35+
authority.setPluginInstanceName("testOPFAuthority");
36+
}
37+
38+
/**
39+
* Test that getSource() returns configured value via inheritance from ItemAuthority.
40+
*/
41+
@Test
42+
public void testGetSourceInheritance() {
43+
// Test with configured value
44+
configurationService.setProperty("cris.ItemAuthority.testOPFAuthority.source", "openpolicyfinder");
45+
String source = authority.getSource();
46+
assertEquals("getSource() should return configured value via inheritance", "openpolicyfinder", source);
47+
48+
// Test default value
49+
configurationService.setProperty("cris.ItemAuthority.testOPFAuthority.source", null);
50+
source = authority.getSource();
51+
assertEquals("getSource() should return default 'local'", "local", source);
52+
}
53+
54+
/**
55+
* Test that authorityName is inherited from ItemAuthority (no duplicate field).
56+
*/
57+
@Test
58+
public void testAuthorityNameInheritance() {
59+
authority.setPluginInstanceName("testName");
60+
assertEquals("authorityName should be inherited from ItemAuthority",
61+
"testName", authority.getPluginInstanceName());
62+
}
63+
64+
/**
65+
* Test that convertToChoice uses getSource() correctly.
66+
* Note: This is an integration test that requires mock OpenPolicyFinderService.
67+
*/
68+
@Test
69+
public void testConvertToChoiceUsesGetSource() {
70+
// Verify that the method exists and returns non-null source
71+
String source = authority.getSource();
72+
assertNotNull("getSource() should not return null", source);
73+
}
74+
}

‎dspace-api/src/test/java/org/dspace/content/authority/RorOrgUnitAuthorityTest.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* The contents of this file are subject to the license and copyright
3+
* detailed in the LICENSE and NOTICE files at the root of the source
4+
* tree and available online at
5+
*
6+
* http://www.dspace.org/license/
7+
*/
8+
package org.dspace.content.authority;
9+
10+
import static org.junit.Assert.assertEquals;
11+
12+
import org.dspace.AbstractUnitTest;
13+
import org.dspace.services.ConfigurationService;
14+
import org.dspace.services.factory.DSpaceServicesFactory;
15+
import org.junit.Before;
16+
import org.junit.Test;
17+
18+
public class RorOrgUnitAuthorityTest extends AbstractUnitTest {
19+
20+
private RorOrgUnitAuthority authority;
21+
private ConfigurationService configurationService;
22+
23+
@Before
24+
public void setUp() throws Exception {
25+
authority = new RorOrgUnitAuthority();
26+
authority.setPluginInstanceName("RorOrgUnitAuthority");
27+
configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
28+
}
29+
30+
@Test
31+
public void testGetSourceDefault() {
32+
String source = authority.getSource();
33+
assertEquals("local", source);
34+
}
35+
36+
@Test
37+
public void testGetSourceConfigured() {
38+
configurationService.setProperty("cris.ItemAuthority.RorOrgUnitAuthority.source", "ror");
39+
String source = authority.getSource();
40+
assertEquals("ror", source);
41+
configurationService.setProperty("cris.ItemAuthority.RorOrgUnitAuthority.source", null);
42+
}
43+
44+
@Test
45+
public void testAuthorityNameInheritance() {
46+
authority.setPluginInstanceName("testRorAuthority");
47+
assertEquals("testRorAuthority", authority.getPluginInstanceName());
48+
}
49+
50+
}

0 commit comments

Comments
 (0)