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:
(from @qqmyers)

Configure permalink PID providers w/ "" and "/" separators, create datasets using them and verify that the DDI/DDI Html exports do not/do contain the / separator (respectively). Confirm that a Fake DOI provider (or test DataCite provider) still work as before w.r.t. how they appear in the DDI/DDI Html exports.
Verify that the EndNote citation for datasets using DOIs and PermaLinks include the PIDs as specified in the best option as discussed in this comment and below. Handles could also be tested but I'm not sure if there's a test setup for that.
Verify that the BibTeX citation format uses doi= for DOIs and only url= for PermaLinks/Handles (if we can test them). (pre-PR PermaLinks would have an extra doi= line.

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?:
Provided

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

qqmyers · 2024-10-22T16:55:12Z

@vera we're trying to move this forward through review/QA. Can you update from develop and fix the merge issues? I'm checking on the question about EndNote again.

# Conflicts: # src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java # src/main/resources/db/migration/V6.3.0.2.sql

vera · 2024-10-23T16:39:22Z

@qqmyers that's great, thank you. I've resolved the merge issues.

see IQSS#10775

qqmyers · 2024-10-23T16:54:46Z

Thanks @vera - now it looks like there are DdiExportUtilTest failures. The results are missing /citation?persistentId=perma:

OK - looks like you saw it already :-)

coveralls · 2024-10-23T17:04:17Z

coverage: 22.583% (+0.01%) from 22.573%
when pulling 0d75b88 on vera:fix/perma-exporters-and-citations
into ed391eb on IQSS:develop.

vera · 2024-10-23T17:05:26Z

Yep should be fixed :)

adam3smith · 2024-10-23T20:39:59Z

Jim asked me to look at the Endnote XML export change: this doesn't cause new issues, but doi: isn't ideal either.
Both Endnote and Zotero will import the string in <electronic-resource-num> as is to the DOI field. That means both doi:10.1234/5678 and doi/10.1234/5678 produce incorrect citations unless fixed.
Here are some suggestion for better handling this:

Best option: Completely separate the handling of DOI and handle on export:
DOI goes into <electronic-resource-num> without any prefix
handle goes into <urls><web-urls><url>

Second best option: Parse the perma prefix separately:
DOI goes into <electronic-resource-num> without any prefix
handle goes into <electronic-resource-num> with either hdl/ or the full handle URL prefix

Third best option: Put all identifiers into <electronic-resource-num> without prefix (will get handles wrong, but they can be detected otherwise.

I think the behavior in this PR is fourth best if the goal of metadata export is to get metadata into reference managers and citations so they produce correct&accurate references

vera · 2024-10-24T11:14:35Z

Thanks for checking!

Should I go ahead and implement one of those options? (the best option?)

qqmyers · 2024-10-24T11:18:46Z

Given that the EndNote format now changes, I'd suggest adding a short release note pointing out this improvement/backward incompatibility.

vera · 2024-10-24T12:27:50Z

@adam3smith Currently, all PID URLs (https://doi.org/..., https://hdl.handle.net/... or the permalink) are already inserted under <urls><related-urls><url>:

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

Lines 599 to 605 in 07e327c

    
           xmlw.writeStartElement("urls"); 
        
           xmlw.writeStartElement("related-urls"); 
        
           xmlw.writeStartElement("url"); 
        
           xmlw.writeCharacters(getPersistentId().asURL()); 
        
           xmlw.writeEndElement(); // url 
        
           xmlw.writeEndElement(); // related-urls 
        
           xmlw.writeEndElement(); // urls

Would you recommend changing that to <urls><web-urls><url> in general?

vera · 2024-10-25T14:21:57Z

I've now implemented the best option as suggested:

removing prefix from electronic-resource-num
inserting handles and PermaLinks as web-urls
(no change to current behavior) inserting DOIs as related-urls

If DOIs should also be inserted as web-urls, we could also do that.

(FYI, I am out of office next week. :))

qqmyers · 2024-10-25T14:26:49Z

Thanks @vera - can you add a quick release note?

qqmyers

Looks good. I added a release note. Thanks for the extra work with the EndNote format!

vera · 2024-11-04T14:58:03Z

Thanks for adding the release note! @qqmyers

# Conflicts: # src/main/resources/db/migration/V6.4.0.1.sql

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

pdurbin added the Type: Bug a defect label Oct 9, 2024

ofahimIQSS assigned qqmyers Oct 22, 2024

qqmyers assigned vera Oct 22, 2024

cmbz added the FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) label Oct 23, 2024

Merge branch 'develop' into fix/perma-exporters-and-citations

3275eba

# Conflicts: # src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java # src/main/resources/db/migration/V6.3.0.2.sql

fix: fix failing DdiExportUtilTest by updating PermaLink config

07e327c

see IQSS#10775

cmbz added the FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) label Oct 23, 2024

fix: improve PID formatting in EndNote XML citation output

a553458

release note

ae058d1

qqmyers approved these changes Oct 25, 2024

View reviewed changes

qqmyers unassigned qqmyers and vera Oct 25, 2024

cmbz added the FY25 Sprint 10 FY25 Sprint 10 (2024-11-06 - 2024-11-20) label Nov 7, 2024

cmbz added the FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04) label Nov 21, 2024

cmbz added the FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) label Dec 5, 2024

vera and others added 3 commits December 5, 2024 15:30

Merge branch 'develop' into fix/perma-exporters-and-citations

7317ac9

# Conflicts: # src/main/resources/db/migration/V6.4.0.1.sql

Merge branch 'develop' into fix/perma-exporters-and-citations IQSS#10790

cf3ced8

rename sql script IQSS#10790

0d75b88

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 by qqmyers

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

qqmyers commented Oct 22, 2024

vera commented Oct 23, 2024

qqmyers commented Oct 23, 2024 •

edited

Loading

coveralls commented Oct 23, 2024 •

edited

Loading

vera commented Oct 23, 2024

adam3smith commented Oct 23, 2024

vera commented Oct 24, 2024

qqmyers commented Oct 24, 2024

vera commented Oct 24, 2024

vera commented Oct 25, 2024 •

edited

Loading

qqmyers commented Oct 25, 2024

qqmyers left a comment •

edited

Loading

vera commented Nov 4, 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 by qqmyers 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

qqmyers commented Oct 22, 2024

vera commented Oct 23, 2024

qqmyers commented Oct 23, 2024 • edited Loading

coveralls commented Oct 23, 2024 • edited Loading

vera commented Oct 23, 2024

adam3smith commented Oct 23, 2024

vera commented Oct 24, 2024

qqmyers commented Oct 24, 2024

vera commented Oct 24, 2024

vera commented Oct 25, 2024 • edited Loading

qqmyers commented Oct 25, 2024

qqmyers left a comment • edited Loading

Choose a reason for hiding this comment

vera commented Nov 4, 2024

vera commented Aug 22, 2024 •

edited by qqmyers

Loading

vera Aug 23, 2024 •

edited

Loading

vera Aug 22, 2024 •

edited

Loading

vera commented Aug 26, 2024 •

edited

Loading

qqmyers commented Oct 23, 2024 •

edited

Loading

coveralls commented Oct 23, 2024 •

edited

Loading

vera commented Oct 25, 2024 •

edited

Loading

qqmyers left a comment •

edited

Loading