Normalize error handling for DOI service

pass-core-doi-service/pom.xml

@@ -49,6 +49,11 @@
             <artifactId>commons-io</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>com.squareup.okhttp3</groupId>
+            <artifactId>mockwebserver</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

pass-core-doi-service/src/main/java/org/eclipse/pass/doi/service/ElideConnector.java

@@ -37,13 +37,15 @@
 import org.eclipse.pass.object.model.Journal;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Component;
 
 /**
  * This class manages Journal objects related to a journal lookup on Crossref - creating or updating
  * a Journal through the Elide interface when necessary
  *
  * @author jrm
  */
+@Component
 public class ElideConnector {
     private static final Logger LOG = LoggerFactory.getLogger(ElideConnector.class);
 

pass-core-doi-service/src/main/java/org/eclipse/pass/doi/service/ExternalDoiService.java

@@ -16,12 +16,7 @@
  */
 package org.eclipse.pass.doi.service;
 
-import static java.lang.Thread.sleep;
-
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -35,9 +30,6 @@
  * @author jrm
  */
 public abstract class ExternalDoiService {
-    private final Set<String> activeJobSet = new HashSet<>();
-    private final Set<String> syncActiveJobSet = Collections.synchronizedSet(activeJobSet);
-
     final static String MAILTO = "pass@jhu.edu";
 
     /**
@@ -56,13 +48,13 @@ public abstract class ExternalDoiService {
      * A key, value map of query parameters used by the external service; null if there aren't any.
      * @return the map
      */
-    public  abstract HashMap<String, String> parameterMap();
+    public  abstract Map<String, String> parameterMap();
 
     /**
      * A key, value map of headers used by the external service; null if there aren't any.
      * @return the map
      */
-    public  abstract HashMap<String, String> headerMap();
+    public  abstract Map<String, String> headerMap();
 
     /**
      * A method to transform the raw external service's JSON response to suit the UI requirements
@@ -89,62 +81,4 @@ String verify(String doi) {
         Matcher matcher = pattern.matcher(suffix);
         return matcher.matches() ? suffix : null;
     }
-
-    /**
-     * this simply protects the external service from a person hammering on a request thinking
-     * it wasn't processed, when it really is just slow coming back
-     *
-     * @param doi the doi to check active
-     * @return whether the doi lookup is still active
-     */
-    boolean isAlreadyActive(String doi) {
-        //check cache map for existence of doi
-        //put doi on map if absent
-
-        if (syncActiveJobSet.contains(doi)) {
-            return true;
-        } else {
-            // this DOI is not actively being processed
-            // let's temporarily prohibit new requests for this DOI
-            syncActiveJobSet.add(doi);
-            //longest time we expect it should take to create a Journal object, in
-            //milliseconds
-            int cachePeriod = 30000;
-            Thread t = new Thread(new ExternalDoiService.ExpiringLock(doi, cachePeriod));
-            t.start();
-        }
-        return false;
-    }
-
-    /**
-     * Once a doi has been processed, this method removes it from the locked list.
-     * @param doi the doi
-     */
-    void unlockDoi(String doi) {
-        syncActiveJobSet.remove(doi);
-    }
-
-    /**
-     * A class to manage locking so that an active process for a DOI will finish executing before
-     * another one begins
-     */
-    public class ExpiringLock implements Runnable {
-        private final String key;
-        private final int duration;
-
-        ExpiringLock(String key, int duration) {
-            this.key = key;
-            this.duration = duration;
-        }
-
-        @Override
-        public void run() {
-            try {
-                sleep(duration);
-                syncActiveJobSet.remove(key);
-            } catch (InterruptedException e) {
-                syncActiveJobSet.remove(key);
-            }
-        }
-    }
 }

pass-core-doi-service/src/main/java/org/eclipse/pass/doi/service/ExternalDoiServiceConnector.java

@@ -19,7 +19,7 @@
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 import java.io.IOException;
-import java.io.StringReader;
+import java.io.Reader;
 import java.util.Objects;
 
 import jakarta.json.Json;
@@ -34,14 +34,17 @@
 import okhttp3.Response;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Component;
 
 /**
  * A class which manages the retrieval of JSON from external DOI services (Unpaywall, Crossref)
  *
  * @author jrm
  */
+@Component
 public class ExternalDoiServiceConnector {
     private static final Logger LOG = LoggerFactory.getLogger(ExternalDoiServiceConnector.class);
+    static final String HTTP_STATUS_CODE = "HTTP_STATUS_CODE";
 
     private final OkHttpClient client;
 
@@ -54,54 +57,50 @@ public class ExternalDoiServiceConnector {
     }
 
     /**
-     * consult external service to get a json object for a supplied doi
+     * Consult external service to get a json object for a supplied doi.
      *
      * @param doi - the supplied doi string, prefix trimmed if necessary
-     * @return a string representing the works object if successful; an empty string if not found; null if IO exception
+     * @return a JSON object if successful,
+     *      null if an error occurs interacting with the external service,
+     *      {error: "error message", HTTP_STATUS_CODE: http status code} if
+     *      the external service returns an error status code
      */
     JsonObject retrieveMetadata(String doi, ExternalDoiService service) {
         HttpUrl.Builder urlBuilder = Objects.requireNonNull(HttpUrl.parse(service.baseUrl() + doi)).newBuilder();
 
-        if ( service.parameterMap() != null ) {
-            for ( String key : service.parameterMap().keySet() ) {
+        if (service.parameterMap() != null) {
+            for (String key : service.parameterMap().keySet()) {
                 urlBuilder.addQueryParameter(key, service.parameterMap().get(key));
             }
         }
 
-        String url = urlBuilder.build().toString();
+        Request.Builder requestBuilder = new Request.Builder().url(urlBuilder.build());
 
-        Request.Builder requestBuilder =  new Request.Builder()
-            .url(url);
-        if ( service.headerMap() != null ) {
+        if (service.headerMap() != null) {
             requestBuilder.headers(Headers.of(service.headerMap()));
         }
-        Request okHttpRequest =  requestBuilder.build();
 
+        Request okHttpRequest =  requestBuilder.build();
         Call call = client.newCall(okHttpRequest);
-        JsonReader reader;
-        JsonObject metadataJsonObject;
-        String responseString = null;
 
         try (Response okHttpResponse = call.execute()) {
-            responseString = Objects.requireNonNull(okHttpResponse.body()).string();
-            reader = Json.createReader(new StringReader(responseString));
-            metadataJsonObject = reader.readObject();
-            reader.close();
-
-            service.unlockDoi(doi);
-
-            return metadataJsonObject;
-        } catch (JsonParsingException e) {
-            if (responseString != null) {
-                return Json.createObjectBuilder()
-                           .add("error", responseString)
-                           .build();
+            if (okHttpResponse.isSuccessful()) {
+                try (Reader reader = okHttpResponse.body().charStream();
+                        JsonReader jsonReader = Json.createReader(reader)) {
+                    return jsonReader.readObject();
+                } catch (JsonParsingException e) {
+                    LOG.error("Error parsing JSON of external service: " + okHttpRequest.url(), e);
+                    return null;
+                }
             }
+
+            // Set response as the error field and save the status code.
+            return Json.createObjectBuilder().add("error", okHttpResponse.body().string()).
+                    add(HTTP_STATUS_CODE, Json.createValue(okHttpResponse.code())).build();
         } catch (IOException e) {
-            LOG.error(e.getMessage(), e);
+            LOG.error("Error accessing external service: " + okHttpRequest.url(), e);
+            return null;
         }
-        return null;
     }
-
 }