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.

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

DataCite alreadyExists() doesn't check correctly for no PID case #5554

Closed
qqmyers opened this issue Feb 14, 2019 · 4 comments
Closed

DataCite alreadyExists() doesn't check correctly for no PID case #5554

qqmyers opened this issue Feb 14, 2019 · 4 comments
Assignees

Comments

@qqmyers
Copy link
Member

qqmyers commented Feb 14, 2019

Correcting a bug I think I introduced in #5427: a DvObject with no PID currently has a non-null GlobalIdentifier with an empty value. The alreadyExists class just checks for null.

@pdurbin
Copy link
Member

pdurbin commented Feb 15, 2019

As @qqmyers says, his pull request #5555 adds an additional check for empty string. Seems fine. I moved it to QA.

I strikes me as somewhat odd that the method is called "alreadyExists" (returns true or false) but when no identifier exists, it doesn't log "doesn't exist". Instead it logs "No identifier sent." Sent where? I assume the code was refactored at some point. Not a big deal but I thought I'd mention it. This isn't anything Jim added in his pull request.

@pdurbin pdurbin removed their assignment Feb 15, 2019
@qqmyers
Copy link
Member Author

qqmyers commented Feb 15, 2019

I'm guessing that this was/is a sanity check - e.g. it doesn't make sense to call alreadyExists() if there's no PID being sent (to this method)... It must be that some upstream code does though, otherwise there wouldn't have been an issue when DataCite changed its return value for a check on an empty string...

@kcondon kcondon self-assigned this Feb 15, 2019
@kcondon
Copy link
Contributor

kcondon commented Feb 15, 2019

@qqmyers As a practical matter, how would one test this? Put another way, what possible problem would not fixing this cause? It sounds like maybe it would not correctly detect an already existing PID because the check is for null and in some cases it returned an empty string? How would I simulate that: create a duplicate/collision situation and see whether it correctly detects it?

@qqmyers
Copy link
Member Author

qqmyers commented Feb 15, 2019

@kcondon - I'm not sure. I guess the scenarios (publishing a first version?) that were causing the DataCite failures in Dec/Jan would still exercise this and, if you repeated those, you should see the "No identifier sent." message in the log now. Since DataCite reverted, this PR doesn't change the final outcome, but if they ever make the same mistake, this will stop Dataverse from calling them and potentially getting the answer that yes, the empty DOI exists...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants