Crossref as a DOI provider (experimental) #10806

stevenwinship · 2024-08-27T19:46:41Z

What this PR does / why we need it:
This pr adds new doi provider - CrossRef. Implemented logic was mostly inspired by other providers such as DataCite and PermaLink.

Which issue(s) this PR closes: 8581

Closes Adding Crossref as a DOI provider #8581

Special notes for your reviewer:

Suggestions on how to test this:
In order to test this you need to have test crossref account and set all required properties, described in doc/sphinx-guides/source/installation/config.rst file
Update: CrossRef does not create test accounts. This may need to be tested by @JacekChudzik

Does this PR introduce a user interface change? If mockups are available, please link/include them here: None

Is there a release notes update needed for this change?: included

Additional documentation: Installation configuration

https://dataverse-guide--10806.org.readthedocs.build/en/10806/installation/config.html#crossref-specific-settings

This reverts commit 6df7513.

This reverts commit e1bf3c3.

coveralls · 2024-08-27T19:58:26Z

coverage: 20.714% (-0.05%) from 20.764%
when pulling 8dfd5f3 on 8581-crossref-provider2
into 34bfb1b on develop.

qqmyers

Looks like this is close to working but I think right now it mixes the way legacy providers and new multiple pid providers work w.r.t. settings. I've tried to leave comments about how to disentangle that, but am happy to talk through it as well. I also commented on where I think some test changes/additions are needed.

I also noted that we'll need to decide if the updates to DOI metadata coming in other PRs should also apply to CrossRef - if so we probably need a new issue once everything is merged.

qqmyers · 2024-08-27T20:53:48Z

src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java

-
+
+    // PROVIDER CROSSREF
+    CROSSREF_URL(SCOPE_PID_PROVIDER, "url"),


There should be a SCOPE_PID_CROSSREF(SCOPE_PID_PROVIDER, "crossref"),
entry and then all of the rest should use that scope rather than SCOPE_PID_PROVIDER. The effect of that is to let you define jvm options of the form dataverse.pid.<id>.crossref.url for each of the potentially many crossref providers (rather than having only one global setting for all crossref providers).

ok. I had that but removed it to have 'dataverse.pid.< id >.url' without the .crossref.
I'll make that change

qqmyers · 2024-08-27T21:01:34Z

doc/sphinx-guides/source/installation/config.rst

+
+.. _dataverse.pid.crossref.password:
+
+dataverse.pid.crossref.password


When you edit this to move it to the multiple provide section, please add a note like the one on DataCite suggesting that more secure options should be used (versus having the password as a plain text jvm-option)

The PR was made before multiPid was supported/before docs were improved with that, etc.

Will modify the doc

qqmyers · 2024-08-27T21:03:29Z

doc/sphinx-guides/source/installation/config.rst

@@ -617,6 +617,15 @@ this provider.
 - :ref:`dataverse.pid.ezid.username`
 - :ref:`dataverse.pid.ezid.password`

+**JVM Options for CrossRef:**


The documentation, and some of the code, is for 'legacy' single pid configuration but I think we could/should only support handling the new multipid way of doing config. Although I think you've added it, I think it's reasonable to not support the legacy style at all for cross ref (since the idea of legacy was to support people who had already deployed with the old single provider jvm options and no one could have used CrossRef before now (except the developer's group).

To change the docs - see the DataCite section in the guide and add the info there (probably right after DataCite/before Handle.

will modify the doc

qqmyers · 2024-08-27T21:07:21Z

src/main/java/edu/harvard/iq/dataverse/pidproviders/PidProviderFactoryBean.java

@@ -188,6 +188,9 @@ private void loadProviders() {
                        legacy = new FakeDOIProvider("legacy", "legacy", authority, shoulder, identifierGenerationStyle,
                                dataFilePidFormat, "", "");
                        break;
+                    case "CrossRef":


I think you should drop this. If you want to support legacy still, this section should be doing like the DataCite legacy section above to read the legacy-style options and feed them into the provider directly (creating a new CrossRefDOIProvider rather than calling a factory method). Again - probably better to drop it.

will remove

qqmyers · 2024-08-27T21:09:01Z

src/main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIPidProvider.java

+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+public class CrossRefDOIPidProvider extends AbstractDOIProvider {


The other DOI providers drop the Pid part of the name (or use DOI instead of Pid since it is the type of Pid they support), so this would be more consistent as CrossRefDOIProvider.

qqmyers · 2024-08-27T21:19:33Z

...main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIRegisterService.java

+    }
+}
+
+class CrossRefMetadataTemplate {


Out of scope perhaps for now - we should coordinate and see if the enhancements in #10615/#10632 should be applied to CrossRef as well (as part of that PR or a new one once they're merged). It's possible that CrossRef doesn't accept all the new metadata that gets added in those, but if it does, I assume we'd want to send it to CrossRef as well. @konradperlowski - any thoughts?

qqmyers · 2024-08-27T21:27:07Z

src/test/java/edu/harvard/iq/dataverse/pidproviders/PidUtilTest.java

+@JvmSetting(key = JvmSettings.PID_PROVIDER_LABEL, value = "CrossRef 1", varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_TYPE, value = CrossRefDOIPidProvider.TYPE, varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_AUTHORITY, value = "10.5072", varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_SHOULDER, value = "FK2/", varArgs = "crossref1")


These are very close to the values for the provider ez1 above - perhaps changing to 10.5075 and/or to FK5/ to make a few more characters different would make it clearer.

will use sample CrossRef values

qqmyers · 2024-08-27T21:29:25Z

src/test/java/edu/harvard/iq/dataverse/pidproviders/PidUtilTest.java

+@JvmSetting(key = JvmSettings.PID_PROVIDER_TYPE, value = CrossRefDOIPidProvider.TYPE, varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_AUTHORITY, value = "10.5072", varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_SHOULDER, value = "FK2/", varArgs = "crossref1")
+@JvmSetting(key = JvmSettings.PID_PROVIDER_MANAGED_LIST, value = "cr:20.20.20/FK2ABCDEF", varArgs ="crossref1")


The values in the managed list for this provider need to be DOIs - of the form doi:10.5076/FK7ABCDEF, etc. (The hdl example above uses the standard Handle form.)

Making this an empty list

qqmyers · 2024-08-27T21:29:37Z

src/test/java/edu/harvard/iq/dataverse/pidproviders/PidUtilTest.java

@@ -133,6 +145,7 @@ public static void setUpClass() throws Exception {
        pidProviderFactoryMap.put(HandlePidProvider.TYPE, new HandleProviderFactory());
        pidProviderFactoryMap.put(FakeDOIProvider.TYPE, new FakeProviderFactory());
        pidProviderFactoryMap.put(EZIdDOIProvider.TYPE, new EZIdProviderFactory());
+        pidProviderFactoryMap.put(CrossRefDOIPidProvider.TYPE, new CrossRefDOIProviderFactory());


My guess is that the test changes don't yet do anything because the code above doesn't look for jvm options with crossref1 in them. Once those changes are made, I think adding to the tests here to assure that a crossref doi - one with authority 10.5072 and shoulder FK2/ as currently defined, is recognized by a provider with the id crossref1.

add that to JvmSettingsTest.java

qqmyers · 2024-09-04T15:35:50Z

doc/sphinx-guides/source/installation/config.rst

+CrossRef has two APIs that are used in Dataverse:
+
+The base URL of the `CrossRef <https://api.crossref.org>`_,
+used to mint and manage DOIs. Current valid values for ``dataverse.pid.*.datacite.mds-api-url`` are "https://mds.datacite.org" (production) and "https://mds.test.datacite.org" (testing, the default).


This is about DataCite values?

modified the doc

qqmyers · 2024-09-04T15:39:05Z

doc/sphinx-guides/source/installation/config.rst

@@ -2841,6 +2871,50 @@ should delete the old JVM option and the wrapped password alias, then recreate
 as shown for :ref:`dataverse.pid.datacite.password` but with the EZID alias
 name.

+.. _dataverse.pid.crossref.url:


These are about legacy support which is no longer implemented - they can all be removed

qqmyers · 2024-09-04T15:55:27Z

...main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIRegisterService.java

+            CrossRefRESTfullClient client = getClient();
+            doiExists = client.testDOIExists(identifier.substring(identifier.indexOf(":") + 1));
+        } catch (Exception e) {
+            logger.log(Level.INFO, identifier, e);


Should be a warning?

It's called a lot just to see if the DOI exists. It's just a check before it creates one.

qqmyers · 2024-09-04T15:55:49Z

...main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIRegisterService.java

+            logger.log(Level.FINE, jsonMetadata);
+            metadata.put("_status", mappedJson.get("status").toString());
+        } catch (RuntimeException e) {
+            logger.log(Level.INFO, identifier, e);


same - a warning

I copied the code from DOIDataCiteRegisterService which logs info level

qqmyers · 2024-09-04T15:56:13Z

...main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIRegisterService.java

+    }
+
+    public String reserveIdentifier(String identifier, DvObject dvObject) throws IOException {
+        logger.info("Crossref reserveIdentifier");


and probably fine for all of these that just tell you that you used the method

qqmyers · 2024-09-04T15:59:30Z

...main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefDOIRegisterService.java

+
+class CrossRefFileUtil {
+
+    public static void close(InputStream in) {


Looks like this is just used with readAndClose where the stream comin in is in a try-with-resources statement. Do we need any of this (the finally/close parts)?

removed try catch finally

So close() is no longer used anywhere?

qqmyers · 2024-09-04T17:47:39Z

src/main/java/edu/harvard/iq/dataverse/pidproviders/doi/crossref/CrossRefRESTfullClient.java

+            String data = EntityUtils.toString(response.getEntity(), encoding);
+            if (response.getStatusLine().getStatusCode() != 200) {
+                String errMsg = "Response from getMetadata: " + response.getStatusLine().getStatusCode() + ", " + data;
+                logger.info(errMsg);


more that should be .warning

qqmyers · 2024-09-04T17:55:44Z

src/test/java/edu/harvard/iq/dataverse/pidproviders/PidUtilTest.java

@@ -133,6 +145,7 @@ public static void setUpClass() throws Exception {
        pidProviderFactoryMap.put(HandlePidProvider.TYPE, new HandleProviderFactory());
        pidProviderFactoryMap.put(FakeDOIProvider.TYPE, new FakeProviderFactory());
        pidProviderFactoryMap.put(EZIdDOIProvider.TYPE, new EZIdProviderFactory());
+        pidProviderFactoryMap.put(CrossRefDOIProvider.TYPE, new CrossRefDOIProviderFactory());


Add to the end of the testDOIParsing() test:

String pid7String = "doi:10.11111/DVN/ABCDEF"; GlobalId pid7 = PidUtil.parseAsGlobalID(pid7String); assertEquals(pid7String, pid7.asString()); assertEquals("crossref1", pid7.getProviderId());

That at least tests that the a CrossRef provider gets created by the factory and the authority/shoulder params are being set correctly.

qqmyers · 2024-09-04T17:57:16Z

src/test/java/edu/harvard/iq/dataverse/settings/JvmSettingsTest.java

@@ -21,11 +21,17 @@ void lookupSetting() {
    void lookupPidProviderSetting() {
        assertEquals("test", JvmSettings.DATACITE_USERNAME.lookup("datacite"));
    }
-
+


I'm not sure this adds anything/suggest removing. The DataCite example is nominally just a test of the lookup(param) call and this just repeats that with another setting.

I'll remove it

qqmyers

Looks good. I suggested a little cleanup and a new test. I did look through the 'original' code that interacts with the CrossRef service and creates the required metadata, but did not dig into details of whether the API calls work/etc. (I assume it was working code when it came to us.)

Beyond that, I think it looks OK to move forward. It appears that we are not going to be able to acquire CrossRef credentials to test the code in the short term, which is basically like the EzID provider where we don't have creds to test (afaik), so we'll have to rely on users to report bugs.

github-actions · 2024-09-17T15:41:19Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:8581-crossref-provider2

ghcr.io/gdcc/configbaker:8581-crossref-provider2

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

pdurbin · 2024-09-17T17:54:45Z

Merged. As discussed at standup (and mentioned above) this support for Crossref as a DOI provider is experimental, especially because we were unable to obtain and credentials to test with. Community, please report bugs and create pull requests to fix them as our hands somewhat tied.

I did run the code locally and was able to publish a dataset using the FAKE provider. I also confirmed that API tests are passing.

Konrad Perłowski and others added 6 commits January 15, 2024 13:02

Add CrossRef provider logic

e1bf3c3

Add CrossRef config properties description

6df7513

Revert "Add CrossRef config properties description"

74bf796

This reverts commit 6df7513.

Revert "Add CrossRef provider logic"

eeb260e

This reverts commit e1bf3c3.

Merge branch 'develop' into 8581-crossref-provider2

5d4302c

add CrossRef Pid Provider

ac1b23c

stevenwinship self-assigned this Aug 27, 2024

stevenwinship added Type: Feature a feature request Size: 10 A percentage of a sprint. 7 hours. FY25 Sprint 4 FY25 Sprint 4 labels Aug 27, 2024

stevenwinship mentioned this pull request Aug 27, 2024

Adding Crossref as a DOI provider #8581

Closed

This comment has been minimized.

Sign in to view

qqmyers requested changes Aug 27, 2024

View reviewed changes

stevenwinship added 2 commits August 28, 2024 10:55

making changes per review

37c7411

fix doc error

7946c5a

cmbz added the FY25 Sprint 5 FY25 sprint 5 label Aug 28, 2024

pdurbin changed the title ~~8581 crossref provider2~~ Crossref as a DOI provider Aug 28, 2024

This comment has been minimized.

Sign in to view

stevenwinship removed their assignment Aug 29, 2024

qqmyers self-assigned this Sep 4, 2024

qqmyers reviewed Sep 4, 2024

View reviewed changes

qqmyers requested changes Sep 4, 2024

View reviewed changes

qqmyers assigned stevenwinship and unassigned qqmyers Sep 4, 2024

stevenwinship added 2 commits September 10, 2024 16:16

changes per review comments

405da59

changes per review comments

4939183

This comment has been minimized.

Sign in to view

changes per review comments

0fe339d

stevenwinship assigned qqmyers and unassigned stevenwinship Sep 11, 2024

This comment has been minimized.

Sign in to view

qqmyers approved these changes Sep 11, 2024

View reviewed changes

qqmyers removed their assignment Sep 11, 2024

cmbz added the FY25 Sprint 6 FY25 Sprint 6 label Sep 11, 2024

update doc

8dfd5f3

pdurbin changed the title ~~Crossref as a DOI provider~~ Crossref as a DOI provider (experimental) Sep 17, 2024

pdurbin self-assigned this Sep 17, 2024

pdurbin merged commit ec882e3 into develop Sep 17, 2024
12 checks passed

pdurbin deleted the 8581-crossref-provider2 branch September 17, 2024 17:51

pdurbin added this to the 6.4 milestone Sep 17, 2024

pdurbin removed their assignment Sep 17, 2024



		// PROVIDER CROSSREF
		CROSSREF_URL(SCOPE_PID_PROVIDER, "url"),


		.. _dataverse.pid.crossref.password:

		dataverse.pid.crossref.password


		class CrossRefFileUtil {

		public static void close(InputStream in) {

Crossref as a DOI provider (experimental) #10806

Crossref as a DOI provider (experimental) #10806

Conversation

stevenwinship commented Aug 27, 2024 • edited by pdurbin Loading

coveralls commented Aug 27, 2024 • edited Loading

This comment has been minimized.

qqmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenwinship Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqmyers left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Sep 17, 2024

pdurbin commented Sep 17, 2024

stevenwinship commented Aug 27, 2024 •

edited by pdurbin

Loading

coveralls commented Aug 27, 2024 •

edited

Loading

stevenwinship Aug 28, 2024 •

edited

Loading

qqmyers left a comment •

edited

Loading