fix: issues in exporters and citations for PermaLink/non-DOI PIDs #10790

vera · 2024-08-22T11:45:03Z

What this PR does / why we need it:

This PR fixes three issues with PermaLink PIDs in metadata exporters and citations.

DDI & DDI HTML exporters: Previously, the export included <IDNo agency="perma">perma:N4H-/M015D56B</IDNo> (DDI XML) and "Identification Number: perma:N4H-/M015D56B" (DDI HTML). This change removes the extra slash in the middle of the identifier.
EndNote XML citation: Previously, the citation included <electronic-resource-num>perma/N4H-/M015D56B</electronic-resource-num>. With this change, perma/ is replaced with perma: and the extra slash in the middle of the identifier is also removed. (Note, this is the only change that also affects DOIs: for DOIs, doi/ is replaced with doi:.)
BibTeX citation: Previously, doi = {...} was always included in the citation. With this change, it is only included when the PID is actually a DOI. A url = {...} is always included (this was already the case).

Which issue(s) this PR closes:

In combination with #10775 and #10759 (or #10615), this addresses #10769 and #10768

Special notes for your reviewer:

-

Suggestions on how to test this:

Create dataset with PermaLink PID and check the metadata exports and citations.

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

-

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

-

Additional documentation:

-

qqmyers · 2024-08-22T12:45:28Z

src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java

@@ -163,7 +163,7 @@ private static void createStdyDscr(XMLStreamWriter xmlw, DatasetDTO datasetDto)
        String persistentAuthority = datasetDto.getAuthority();
        String persistentId = datasetDto.getIdentifier();

-        String pid = persistentProtocol + ":" + persistentAuthority + "/" + persistentId;
+        String pid = persistentProtocol + ":" + persistentAuthority + ("perma".equals(persistentAgency) ? "" : "/") + persistentId;


permalinks can have a separator - seems like we need to add that to the DTO?

Yes! I just tested with a PermaLink PID that uses a separator and it is indeed missing in the DDI exports.

I thought maybe it would be merged into the authority, like I think a configured DOI authority and shoulder are merged in persistentAuthority.

shoulder just becomes part of the identifier

I've just pushed a commit adding the separator to the DTO. It's marked as WIP because for some reason it doesn't work yet (separator is always null). I will have time to troubleshoot tomorrow

If I am on the wrong track with that commit, please let me know!

Do we really need to change the domain object? Can't we delegate the PID string generation to GlobalId#toString()?

I've just pushed a commit adding the separator to the DTO. It's marked as WIP because for some reason it doesn't work yet (separator is always null). I will have time to troubleshoot tomorrow

This issue is now fixed. Tested with DOI, PermaLink without separator and PermaLink with separator

For PermaLinks with separators, the database needs to be manually updated (alter table dvobject set separator = 'whatever' where protocol = 'perma' and authority = 'myauthority';). This is a little bit cumbersome but I don't know of a better way to do it.

Can't we delegate the PID string generation to GlobalId#toString()?

@qqmyers would you prefer a solution using the GlobalId method instead of adding the separator to the database? I could take a look at implementing it that way instead (later today or on Monday)

At first I was thinking that you didn't need the new column on dvobject (you can use PidUtil.parseAsGlobalID(String protocol, String authority, String identifier) to get a GlobalId), but it actually solves a limitation of permalinks right now - you can't have two providers that differ only by the separator because we don't store the separator and have to infer it. With that in mind, I think the new column is a good way to go. (I'd still use the GlobalId convenience methods whenever they're available, to avoid replicating the formatting code - looked like you mostly do that now except in DDIExportUtil where there's weirdness from tests sending null identifiers and for the one format with { in it.)

qqmyers · 2024-08-22T13:39:16Z

src/main/java/edu/harvard/iq/dataverse/DataCitation.java

@@ -619,8 +621,7 @@ private void createEndNoteXML(XMLStreamWriter xmlw) throws XMLStreamException {
        }
        if (persistentId != null) {
            xmlw.writeStartElement("electronic-resource-num");
-            String electResourceNum = persistentId.getProtocol() + "/" + persistentId.getAuthority() + "/"
-                    + persistentId.getIdentifier();
+            String electResourceNum = persistentId.asString();


Has it been verified that an initial : after protocol is OK for EndNote import? It seems like the original code used / because ICPSR did. Can someone look at what EndNote exports and make sure we match it here? @adam3smith says he can check this next week if no one else has access.

I see, I wondered about that slash there. I tried to find some info on the EndNote XML format but found only this schema, which didn't specify anything about the format of electronic-resource-num, so I thought it might have been a mistake to use the slash. It would be great if someone could check.

pdurbin · 2024-08-22T14:33:46Z

src/test/java/edu/harvard/iq/dataverse/DataCitationTest.java

-            "<electronic-resource-num>doi/10.5072/FK2/LK0D1H</electronic-resource-num>" +
+            "<electronic-resource-num>doi:10.5072/FK2/LK0D1H</electronic-resource-num>" +


It's weird that this old test expected doi/10... instead of doi:10.... It feels like this PR is cleaning things up! ❤️

@qqmyers commented above that this slash might be required by EndNote, although it does look weird to me 🤔

qqmyers · 2024-08-23T12:34:56Z

src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java

        String pidUri = pid;
        //Some tests don't send real PIDs - don't try to get their URL form
-        if(!pidUri.equals("null:null/null")) {
+        if(!pidUri.equals("null:nullnullnull")) {
            pidUri= PidUtil.parseAsGlobalID(persistentProtocol, persistentAuthority, persistentId).asURL();


Can you avoid line 167 by checking for parseAsGlobalId returning null here and setting pid/pidUri using GlobalId if it exists and to the constant "null:nullnullnull" if it doesn't? (Mostly a minor style thing but it avoids writing out unexpected invalid identifier entries as well.)

qqmyers · 2024-08-23T12:35:47Z

src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java


        xmlw.writeStartElement("docDscr");
        xmlw.writeStartElement("citation");
        xmlw.writeStartElement("titlStmt");
        writeFullElement(xmlw, "titl", dto2Primitive(version, DatasetFieldConstant.title), datasetDto.getMetadataLanguage());
        xmlw.writeStartElement("IDNo");
        writeAttribute(xmlw, "agency", persistentAgency);
-        xmlw.writeCharacters(persistentProtocol + ":" + persistentAuthority + "/" + persistentId);
+        xmlw.writeCharacters(persistentProtocol + ":" + persistentAuthority + persistentSeparator + persistentId);


same - can you parse the authority/separator/identifier and use GlobalId.asString() ?

vera · 2024-08-26T15:22:29Z

Hm. I tested this with DOIs, PermaLinks without separators and PermaLinks with separators and the export looks fine. However, I just saw that when I create a new dataset with a PermaLink with a separator, it doesn't actually get stored in the db (separator is null).

I think a setSeparator call needs to be added here in AbstractPidProvider#generatePid?

https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/pidproviders/AbstractPidProvider.java#L183

I also briefly tried adding unit tests for the export of PermaLink datasets to DdiExportUtilTest, but I didn't find an easy way to configure a PermaLink provider for a unit test, is there one?

qqmyers · 2024-08-26T16:19:54Z

You probably need to set the separator to / in the Abstract class and to the real one in the Perma provider.

There are some unit tests for the providers - see the PidUtilTest class and I think you can set up whatever provider you want that way.

vera · 2024-08-27T11:11:58Z

Thanks, fixed the bug and added two tests. Now I think the only open issue is the question about the format of the EndNote citation

vera added 3 commits August 22, 2024 13:22

fix: remove unnecessary slash in perma PIDs in DDI exporters

2b2da99

fix: remove doi property for perma PIDs in BibTeX citation

c8cdb8b

fix: fix PID formatting in EndNote XML citation

ccf74f9

qqmyers reviewed Aug 22, 2024

View reviewed changes

pdurbin reviewed Aug 22, 2024

View reviewed changes

vera added 3 commits August 22, 2024 18:00

fix: use correct PID separator in DDI exporters (WIP)

34a795c

fix: use correct PID separator in DDI exporters (WIP)

825d503

fix: use correct PID separator in DDI exporters

b8a068e

qqmyers reviewed Aug 23, 2024

View reviewed changes

fix: use GlobalId methods to create PID URLs/strings in DdiExportUtil

70031f3

vera added 2 commits August 27, 2024 10:32

fix: set separator when creating new dataset

f6e1b06

test: add tests for DDI exports for datasets with PermaLinks

46de8fb

pdurbin added the Size: 10 A percentage of a sprint. 7 hours. label Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issues in exporters and citations for PermaLink/non-DOI PIDs #10790

fix: issues in exporters and citations for PermaLink/non-DOI PIDs #10790

vera commented Aug 22, 2024 •

edited

Loading

qqmyers Aug 22, 2024

vera Aug 22, 2024

qqmyers Aug 22, 2024

vera Aug 22, 2024

vera Aug 22, 2024

johannes-darms Aug 23, 2024

vera Aug 23, 2024 •

edited

Loading

vera Aug 23, 2024

qqmyers Aug 23, 2024

qqmyers Aug 22, 2024

vera Aug 22, 2024 •

edited

Loading

pdurbin Aug 22, 2024

vera Aug 22, 2024

qqmyers Aug 23, 2024

vera Aug 26, 2024

qqmyers Aug 23, 2024

vera Aug 26, 2024

vera commented Aug 26, 2024 •

edited

Loading

qqmyers commented Aug 26, 2024

vera commented Aug 27, 2024

		"<electronic-resource-num>doi/10.5072/FK2/LK0D1H</electronic-resource-num>" +
		"<electronic-resource-num>doi:10.5072/FK2/LK0D1H</electronic-resource-num>" +

fix: issues in exporters and citations for PermaLink/non-DOI PIDs #10790

Are you sure you want to change the base?

fix: issues in exporters and citations for PermaLink/non-DOI PIDs #10790

Conversation

vera commented Aug 22, 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

vera Aug 23, 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

vera Aug 22, 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

vera commented Aug 26, 2024 • edited Loading

qqmyers commented Aug 26, 2024

vera commented Aug 27, 2024

vera commented Aug 22, 2024 •

edited

Loading

vera Aug 23, 2024 •

edited

Loading

vera Aug 22, 2024 •

edited

Loading

vera commented Aug 26, 2024 •

edited

Loading