-
Notifications
You must be signed in to change notification settings - Fork 501
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
Comments
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. |
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... |
@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? |
@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... |
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.
The text was updated successfully, but these errors were encountered: