Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2b2da99
fix: remove unnecessary slash in perma PIDs in DDI exporters
vera Aug 22, 2024
c8cdb8b
fix: remove doi property for perma PIDs in BibTeX citation
vera Aug 22, 2024
ccf74f9
fix: fix PID formatting in EndNote XML citation
vera Aug 22, 2024
34a795c
fix: use correct PID separator in DDI exporters (WIP)
vera Aug 22, 2024
825d503
fix: use correct PID separator in DDI exporters (WIP)
vera Aug 23, 2024
b8a068e
fix: use correct PID separator in DDI exporters
vera Aug 23, 2024
70031f3
fix: use GlobalId methods to create PID URLs/strings in DdiExportUtil
vera Aug 26, 2024
f6e1b06
fix: set separator when creating new dataset
vera Aug 27, 2024
46de8fb
test: add tests for DDI exports for datasets with PermaLinks
vera Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/main/java/edu/harvard/iq/dataverse/DataCitation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,11 +293,13 @@ public void writeAsBibtexCitation(OutputStream os) throws IOException {
out.write("version = {");
out.write(version);
out.write("},\r\n");
out.write("doi = {");
out.write(persistentId.getAuthority());
out.write("/");
out.write(persistentId.getIdentifier());
out.write("},\r\n");
if("doi".equals(persistentId.getProtocol())) {
out.write("doi = {");
out.write(persistentId.getAuthority());
out.write("/");
out.write(persistentId.getIdentifier());
out.write("},\r\n");
}
out.write("url = {");
out.write(persistentId.asURL());
out.write("}\r\n");
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@vera vera Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

xmlw.writeCharacters(electResourceNum);
xmlw.writeEndElement();
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoulder just becomes part of the identifier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vera vera Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

String pidUri = pid;
//Some tests don't send real PIDs - don't try to get their URL form
if(!pidUri.equals("null:null/null")) {
Expand Down Expand Up @@ -344,7 +344,7 @@ private static void writeDocDescElement (XMLStreamWriter xmlw, DatasetDTO datase
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 + ("perma".equals(persistentAgency) ? "" : "/") + persistentId);
xmlw.writeEndElement(); // IDNo
xmlw.writeEndElement(); // titlStmt
xmlw.writeStartElement("distStmt");
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/edu/harvard/iq/dataverse/DataCitationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public void testToEndNoteString_withTitleAndAuthor() throws ParseException {
"<edition>V1</edition>" +
"<publisher>LibraScholar</publisher>" +
"<urls><related-urls><url>https://doi.org/10.5072/FK2/LK0D1H</url></related-urls></urls>" +
"<electronic-resource-num>doi/10.5072/FK2/LK0D1H</electronic-resource-num>" +
"<electronic-resource-num>doi:10.5072/FK2/LK0D1H</electronic-resource-num>" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"</record>" +
"</records>" +
"</xml>";
Expand Down Expand Up @@ -295,7 +295,7 @@ public void testToEndNoteString_withoutTitleAndAuthor() throws ParseException {
"<edition>V1</edition>" +
"<publisher>LibraScholar</publisher>" +
"<urls><related-urls><url>https://doi.org/10.5072/FK2/LK0D1H</url></related-urls></urls>" +
"<electronic-resource-num>doi/10.5072/FK2/LK0D1H</electronic-resource-num>" +
"<electronic-resource-num>doi:10.5072/FK2/LK0D1H</electronic-resource-num>" +
"</record>" +
"</records>" +
"</xml>";
Expand Down
Loading