Merge pull request #9876 from kshepherd/orcid_access_token_init_fix

Fix DSpace startup failures when ORCID http connections fail

dspace-api/src/main/java/org/dspace/authority/orcid/Orcidv3SolrAuthorityImpl.java

@@ -7,27 +7,22 @@
  */
 package org.dspace.authority.orcid;
 
-import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InputStreamReader;
 import java.net.URLEncoder;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 
 import org.apache.commons.lang.StringUtils;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.methods.HttpPost;
-import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.dspace.authority.AuthorityValue;
 import org.dspace.authority.SolrAuthorityInterface;
 import org.dspace.external.OrcidRestConnector;
 import org.dspace.external.provider.orcid.xml.XMLtoBio;
-import org.json.JSONObject;
+import org.dspace.orcid.model.factory.OrcidFactoryUtils;
 import org.orcid.jaxb.model.v3.release.common.OrcidIdentifier;
 import org.orcid.jaxb.model.v3.release.record.Person;
 import org.orcid.jaxb.model.v3.release.search.Result;
@@ -50,6 +45,11 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
 
     private String accessToken;
 
+    /**
+     * Maximum retries to allow for the access token retrieval
+     */
+    private int maxClientRetries = 3;
+
     public void setOAUTHUrl(String oAUTHUrl) {
         OAUTHUrl = oAUTHUrl;
     }
@@ -62,46 +62,32 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
         this.clientSecret = clientSecret;
     }
 
+    public String getAccessToken() {
+        return accessToken;
+    }
+
+    public void setAccessToken(String accessToken) {
+        this.accessToken = accessToken;
+    }
+
     /**
      *  Initialize the accessToken that is required for all subsequent calls to ORCID
      */
     public void init() {
-        if (StringUtils.isBlank(accessToken)
+        // Initialize access token at spring instantiation. If it fails, the access token will be null rather
-                && StringUtils.isNotBlank(clientSecret)
+        // than causing a fatal Spring startup error
-                && StringUtils.isNotBlank(clientId)
+        initializeAccessToken();
-                && StringUtils.isNotBlank(OAUTHUrl)) {
+    }
-            String authenticationParameters = "?client_id=" + clientId +
-                    "&client_secret=" + clientSecret +
-                    "&scope=/read-public&grant_type=client_credentials";
-            try {
-                HttpPost httpPost = new HttpPost(OAUTHUrl + authenticationParameters);
-                httpPost.addHeader("Accept", "application/json");
-                httpPost.addHeader("Content-Type", "application/x-www-form-urlencoded");
 
-                HttpClient httpClient = HttpClientBuilder.create().build();
+    public void initializeAccessToken() {
-                HttpResponse getResponse = httpClient.execute(httpPost);
+        // If we have reaches max retries or the access token is already set, return immediately
-
+        if (maxClientRetries <= 0 || org.apache.commons.lang3.StringUtils.isNotBlank(accessToken)) {
-                JSONObject responseObject = null;
+            return;
-                try (InputStream is = getResponse.getEntity().getContent();
+        }
-                     BufferedReader streamReader = new BufferedReader(new InputStreamReader(is, "UTF-8"))) {
-                    String inputStr;
-                    while ((inputStr = streamReader.readLine()) != null && responseObject == null) {
-                        if (inputStr.startsWith("{") && inputStr.endsWith("}") && inputStr.contains("access_token")) {
         try {
-                                responseObject = new JSONObject(inputStr);
+            accessToken = OrcidFactoryUtils.retrieveAccessToken(clientId, clientSecret, OAUTHUrl).orElse(null);
-                            } catch (Exception e) {
+        } catch (IOException e) {
-                                //Not as valid as I'd hoped, move along
+            log.error("Error retrieving ORCID access token, {} retries left", --maxClientRetries);
-                                responseObject = null;
-                            }
-                        }
-                    }
-                }
-                if (responseObject != null && responseObject.has("access_token")) {
-                    accessToken = (String) responseObject.get("access_token");
-                }
-            } catch (Exception e) {
-                throw new RuntimeException("Error during initialization of the Orcid connector", e);
-            }
         }
     }
 
@@ -116,7 +102,7 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
      */
     @Override
     public List<AuthorityValue> queryAuthorities(String text, int max) {
-        init();
+        initializeAccessToken();
         List<Person> bios = queryBio(text, max);
         List<AuthorityValue> result = new ArrayList<>();
         for (Person person : bios) {
@@ -135,7 +121,7 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
      */
     @Override
     public AuthorityValue queryAuthorityID(String id) {
-        init();
+        initializeAccessToken();
         Person person = getBio(id);
         AuthorityValue valueFromPerson = Orcidv3AuthorityValue.create(person);
         return valueFromPerson;
@@ -151,11 +137,14 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
         if (!isValid(id)) {
             return null;
         }
-        init();
+        if (orcidRestConnector == null) {
+            log.error("ORCID REST connector is null, returning null Person");
+            return null;
+        }
+        initializeAccessToken();
         InputStream bioDocument = orcidRestConnector.get(id + ((id.endsWith("/person")) ? "" : "/person"), accessToken);
         XMLtoBio converter = new XMLtoBio();
-        Person person = converter.convertSinglePerson(bioDocument);
+        return converter.convertSinglePerson(bioDocument);
-        return person;
     }
 
 
@@ -167,10 +156,16 @@ public class Orcidv3SolrAuthorityImpl implements SolrAuthorityInterface {
      * @return List<Person>
      */
     public List<Person> queryBio(String text, int start, int rows) {
-        init();
         if (rows > 100) {
             throw new IllegalArgumentException("The maximum number of results to retrieve cannot exceed 100.");
         }
+        // Check REST connector is initialized
+        if (orcidRestConnector == null) {
+            log.error("ORCID REST connector is not initialized, returning empty list");
+            return Collections.emptyList();
+        }
+        // Check / init access token
+        initializeAccessToken();
 
         String searchPath = "search?q=" + URLEncoder.encode(text) + "&start=" + start + "&rows=" + rows;
         log.debug("queryBio searchPath=" + searchPath + " accessToken=" + accessToken);

dspace-api/src/main/java/org/dspace/external/provider/impl/OrcidV3AuthorDataProvider.java

@@ -7,24 +7,17 @@
  */
 package org.dspace.external.provider.impl;
 
-import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InputStreamReader;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Collectors;
 
 import org.apache.commons.lang3.StringUtils;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.methods.HttpPost;
-import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.dspace.content.dto.MetadataValueDTO;
@@ -32,7 +25,7 @@ import org.dspace.external.OrcidRestConnector;
 import org.dspace.external.model.ExternalDataObject;
 import org.dspace.external.provider.AbstractExternalDataProvider;
 import org.dspace.external.provider.orcid.xml.XMLtoBio;
-import org.json.JSONObject;
+import org.dspace.orcid.model.factory.OrcidFactoryUtils;
 import org.orcid.jaxb.model.v3.release.common.OrcidIdentifier;
 import org.orcid.jaxb.model.v3.release.record.Person;
 import org.orcid.jaxb.model.v3.release.search.Result;
@@ -60,6 +53,11 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
 
     private XMLtoBio converter;
 
+    /**
+     * Maximum retries to allow for the access token retrieval
+     */
+    private int maxClientRetries = 3;
+
     public static final String ORCID_ID_SYNTAX = "\\d{4}-\\d{4}-\\d{4}-(\\d{3}X|\\d{4})";
     private static final int MAX_INDEX = 10000;
 
@@ -78,47 +76,37 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
      * @throws java.io.IOException passed through from HTTPclient.
      */
     public void init() throws IOException {
-        if (StringUtils.isNotBlank(clientSecret) && StringUtils.isNotBlank(clientId)
+        // Initialize access token at spring instantiation. If it fails, the access token will be null rather
-            && StringUtils.isNotBlank(OAUTHUrl)) {
+        // than causing a fatal Spring startup error
-            String authenticationParameters = "?client_id=" + clientId +
+        initializeAccessToken();
-                    "&client_secret=" + clientSecret +
+    }
-                    "&scope=/read-public&grant_type=client_credentials";
-            HttpPost httpPost = new HttpPost(OAUTHUrl + authenticationParameters);
-            httpPost.addHeader("Accept", "application/json");
-            httpPost.addHeader("Content-Type", "application/x-www-form-urlencoded");
 
-            HttpClient httpClient = HttpClientBuilder.create().build();
+    /**
-            HttpResponse getResponse = httpClient.execute(httpPost);
+     * Initialize access token, logging an error and decrementing remaining retries if an IOException is thrown.
-
+     * If the optional access token result is empty, set to null instead.
-            JSONObject responseObject = null;
+     */
-            try (InputStream is = getResponse.getEntity().getContent();
+    public void initializeAccessToken() {
-                 BufferedReader streamReader = new BufferedReader(new InputStreamReader(is, "UTF-8"))) {
+        // If we have reaches max retries or the access token is already set, return immediately
-                String inputStr;
+        if (maxClientRetries <= 0 || StringUtils.isNotBlank(accessToken)) {
-                while ((inputStr = streamReader.readLine()) != null && responseObject == null) {
+            return;
-                    if (inputStr.startsWith("{") && inputStr.endsWith("}") && inputStr.contains("access_token")) {
+        }
         try {
-                            responseObject = new JSONObject(inputStr);
+            accessToken = OrcidFactoryUtils.retrieveAccessToken(clientId, clientSecret, OAUTHUrl).orElse(null);
-                        } catch (Exception e) {
+        } catch (IOException e) {
-                            //Not as valid as I'd hoped, move along
+            log.error("Error retrieving ORCID access token, {} retries left", --maxClientRetries);
-                            responseObject = null;
-                        }
-                    }
-                }
-            }
-            if (responseObject != null && responseObject.has("access_token")) {
-                accessToken = (String) responseObject.get("access_token");
-            }
         }
     }
 
     @Override
     public Optional<ExternalDataObject> getExternalDataObject(String id) {
+        initializeAccessToken();
         Person person = getBio(id);
         ExternalDataObject externalDataObject = convertToExternalDataObject(person);
         return Optional.of(externalDataObject);
     }
 
     protected ExternalDataObject convertToExternalDataObject(Person person) {
+        initializeAccessToken();
         ExternalDataObject externalDataObject = new ExternalDataObject(sourceIdentifier);
         if (person.getName() != null) {
             String lastName = "";
@@ -167,6 +155,11 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
         if (!isValid(id)) {
             return null;
         }
+        if (orcidRestConnector == null) {
+            log.error("ORCID REST connector is null, returning null ORCID Person Bio");
+            return null;
+        }
+        initializeAccessToken();
         InputStream bioDocument = orcidRestConnector.get(id + ((id.endsWith("/person")) ? "" : "/person"), accessToken);
         Person person = converter.convertSinglePerson(bioDocument);
         try {
@@ -188,12 +181,18 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
 
     @Override
     public List<ExternalDataObject> searchExternalDataObjects(String query, int start, int limit) {
+        initializeAccessToken();
         if (limit > 100) {
             throw new IllegalArgumentException("The maximum number of results to retrieve cannot exceed 100.");
         }
         if (start > MAX_INDEX) {
             throw new IllegalArgumentException("The starting number of results to retrieve cannot exceed 10000.");
         }
+        // Check REST connector is initialized
+        if (orcidRestConnector == null) {
+            log.error("ORCID REST connector is not initialized, returning empty list");
+            return Collections.emptyList();
+        }
 
         String searchPath = "search?q=" + URLEncoder.encode(query, StandardCharsets.UTF_8)
                 + "&start=" + start
@@ -218,9 +217,6 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
         } catch (IOException e) {
             log.error(e.getMessage(), e);
         }
-        if (Objects.isNull(bios)) {
-            return Collections.emptyList();
-        }
         return bios.stream().map(bio -> convertToExternalDataObject(bio)).collect(Collectors.toList());
     }
 
@@ -231,6 +227,11 @@ public class OrcidV3AuthorDataProvider extends AbstractExternalDataProvider {
 
     @Override
     public int getNumberOfResults(String query) {
+        if (orcidRestConnector == null) {
+            log.error("ORCID REST connector is null, returning 0");
+            return 0;
+        }
+        initializeAccessToken();
         String searchPath = "search?q=" + URLEncoder.encode(query, StandardCharsets.UTF_8)
                 + "&start=" + 0
                 + "&rows=" + 0;

dspace-api/src/main/java/org/dspace/orcid/model/factory/OrcidFactoryUtils.java

@@ -7,10 +7,21 @@
  */
 package org.dspace.orcid.model.factory;
 
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.json.JSONObject;
 
 /**
  * Utility class for Orcid factory classes. This is used to parse the
@@ -65,4 +76,48 @@ public final class OrcidFactoryUtils {
         return configurations;
     }
 
+    /**
+     * Retrieve access token from ORCID, given a client ID, client secret and OAuth URL
+     *
+     * @param clientId ORCID client ID
+     * @param clientSecret ORCID client secret
+     * @param oauthUrl ORCID oauth redirect URL
+     * @return response object as Optional string
+     * @throws IOException if any errors are encountered making the connection or reading a response
+     */
+    public static Optional<String> retrieveAccessToken(String clientId, String clientSecret, String oauthUrl)
+            throws IOException {
+        if (StringUtils.isNotBlank(clientSecret) && StringUtils.isNotBlank(clientId)
+                && StringUtils.isNotBlank(oauthUrl)) {
+            String authenticationParameters = "?client_id=" + clientId +
+                    "&client_secret=" + clientSecret +
+                    "&scope=/read-public&grant_type=client_credentials";
+            HttpPost httpPost = new HttpPost(oauthUrl + authenticationParameters);
+            httpPost.addHeader("Accept", "application/json");
+            httpPost.addHeader("Content-Type", "application/x-www-form-urlencoded");
+
+            HttpResponse response;
+            try (CloseableHttpClient httpClient = HttpClientBuilder.create().build()) {
+                response = httpClient.execute(httpPost);
+            }
+            JSONObject responseObject = null;
+            if (response != null && response.getStatusLine().getStatusCode() == 200) {
+                try (InputStream is = response.getEntity().getContent();
+                     BufferedReader streamReader = new BufferedReader(new InputStreamReader(is,
+                             StandardCharsets.UTF_8))) {
+                    String inputStr;
+                    while ((inputStr = streamReader.readLine()) != null && responseObject == null) {
+                        if (inputStr.startsWith("{") && inputStr.endsWith("}") && inputStr.contains("access_token")) {
+                            responseObject = new JSONObject(inputStr);
+                        }
+                    }
+                }
+            }
+            if (responseObject != null && responseObject.has("access_token")) {
+                return Optional.of((String) responseObject.get("access_token"));
+            }
+        }
+        // Return empty by default
+        return Optional.empty();
+    }
 }

dspace-api/src/test/data/dspaceFolder/config/spring/api/orcid-authority-services.xml

@@ -16,7 +16,7 @@
     <bean id="dspace.DSpaceAuthorityIndexer" class="org.dspace.authority.indexer.DSpaceAuthorityIndexer"/>
 
     <alias name="OrcidSource" alias="AuthoritySource"/>
-    <bean name="OrcidSource" class="org.dspace.authority.orcid.MockOrcid" />
+    <bean name="OrcidSource" class="org.dspace.authority.orcid.MockOrcid" init-method="init" />
  
     <bean name="AuthorityTypes" class="org.dspace.authority.AuthorityTypes">
         <property name="types">

dspace-api/src/test/java/org/dspace/authority/orcid/MockOrcid.java

@@ -26,9 +26,19 @@ import org.mockito.stubbing.Answer;
  */
 public class MockOrcid extends Orcidv3SolrAuthorityImpl {
 
+    OrcidRestConnector orcidRestConnector;
+
     @Override
     public void init() {
-        OrcidRestConnector orcidRestConnector = Mockito.mock(OrcidRestConnector.class);
+        initializeAccessToken();
+        orcidRestConnector = Mockito.mock(OrcidRestConnector.class);
+    }
+
+    /**
+     * Call this to set up mocking for any test classes that need it. We don't set it in init()
+     * or other AbstractIntegrationTest implementations will complain of unnecessary Mockito stubbing
+     */
+    public void setupNoResultsSearch() {
         when(orcidRestConnector.get(ArgumentMatchers.startsWith("search?"), ArgumentMatchers.any()))
                 .thenAnswer(new Answer<InputStream>() {
                     @Override
@@ -36,6 +46,12 @@ public class MockOrcid extends Orcidv3SolrAuthorityImpl {
                         return this.getClass().getResourceAsStream("orcid-search-noresults.xml");
                     }
                 });
+    }
+    /**
+     * Call this to set up mocking for any test classes that need it. We don't set it in init()
+     * or other AbstractIntegrationTest implementations will complain of unnecessary Mockito stubbing
+     */
+    public void setupSingleSearch() {
         when(orcidRestConnector.get(ArgumentMatchers.startsWith("search?q=Bollini"), ArgumentMatchers.any()))
                 .thenAnswer(new Answer<InputStream>() {
                     @Override
@@ -43,6 +59,12 @@ public class MockOrcid extends Orcidv3SolrAuthorityImpl {
                         return this.getClass().getResourceAsStream("orcid-search.xml");
                     }
                 });
+    }
+    /**
+     * Call this to set up mocking for any test classes that need it. We don't set it in init()
+     * or other AbstractIntegrationTest implementations will complain of unnecessary Mockito stubbing
+     */
+    public void setupSearchWithResults() {
         when(orcidRestConnector.get(ArgumentMatchers.endsWith("/person"), ArgumentMatchers.any()))
                 .thenAnswer(new Answer<InputStream>() {
                     @Override
@@ -54,4 +76,10 @@ public class MockOrcid extends Orcidv3SolrAuthorityImpl {
         setOrcidRestConnector(orcidRestConnector);
     }
 
+    @Override
+    public void initializeAccessToken() {
+        if (getAccessToken() == null) {
+            setAccessToken("mock-access-token");
+        }
+    }
 }

dspace-server-webapp/src/test/java/org/dspace/app/rest/VocabularyRestRepositoryIT.java

@@ -22,6 +22,7 @@ import org.dspace.app.rest.test.AbstractControllerIntegrationTest;
 import org.dspace.authority.AuthorityValueServiceImpl;
 import org.dspace.authority.PersonAuthorityValue;
 import org.dspace.authority.factory.AuthorityServiceFactory;
+import org.dspace.authority.orcid.MockOrcid;
 import org.dspace.builder.CollectionBuilder;
 import org.dspace.builder.CommunityBuilder;
 import org.dspace.content.Collection;
@@ -29,11 +30,13 @@ import org.dspace.content.authority.DCInputAuthority;
 import org.dspace.content.authority.service.ChoiceAuthorityService;
 import org.dspace.core.service.PluginService;
 import org.dspace.services.ConfigurationService;
+import org.dspace.services.factory.DSpaceServicesFactory;
 import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.ApplicationContext;
 
 /**
  * This class handles all Authority related IT. It alters some config to run the tests, but it gets cleared again
@@ -56,6 +59,17 @@ public class VocabularyRestRepositoryIT extends AbstractControllerIntegrationTes
     @Before
     public void setup() throws Exception {
         super.setUp();
+
+        // Explicitly set stubbing for the MockOrcid class. We don't do it in the init() or constructor
+        // of the MockOrcid class itself or Mockito will complain of unnecessary stubbing in certain other
+        // AbstractIntegrationTest implementations (depending on how config is (re)loaded)
+        ApplicationContext applicationContext = DSpaceServicesFactory.getInstance()
+                .getServiceManager().getApplicationContext();
+        MockOrcid mockOrcid = applicationContext.getBean(MockOrcid.class);
+        mockOrcid.setupNoResultsSearch();
+        mockOrcid.setupSingleSearch();
+        mockOrcid.setupSearchWithResults();
+
         configurationService.setProperty("plugin.named.org.dspace.content.authority.ChoiceAuthority",
                 new String[] {
                         "org.dspace.content.authority.SolrAuthority = SolrAuthorAuthority",