-
Notifications
You must be signed in to change notification settings - Fork 530
Fixing ror v2 api access for cvoc #11762
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
base: develop
Are you sure you want to change the base?
Conversation
|
||
} else { | ||
curPath = ((JsonObject) curPath).get(pathParts[index]); | ||
if (NumberUtils.isCreatable(pathParts[index])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth handling the case where "0" is returned as a string too? This code assumes anything that can be a number is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say a number is a number. If you construct a JSON object then you shouldn't use a number as a string value for a key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me rephrase - the code currently assumes that if the value is interpretable as a number, we must be parsing an array, whereas if the value is a json object, the keys would have to be strings. I don't know how often we'll encounter such things (objects with keys "1", "2", "3"), but it seems like it might be cleaner/safer to see if the value can be a number && the curPath is a json array, you're new code is invoked, otherwise it tries to deal with it as an object with a string key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ffritze - apologies for letting this sit. I'm suggesting the following change: 5cd4ecf . If that's acceptable to you, can you include it, and update your branch from IQSS/develop (as done in develop...GlobalDataverseCommunityConsortium:dataverse:fix_ror-v2_api_cvoc) so our build system will be able to run it and we can plan on including it in Dataverse 6.9? (FWIW: I wasn't able to make a commit or PR against your branch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qqmyers Thanks a lot. I will test it in our infrastructure. Currently we have a maintenance phase so I will postpone it until the beginning of next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qqmyers I can now confirm that your additional code in this branch is working as expected. This pull request is ready for production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ffritze great! We were leaving you assigned to this PR until you confirmed this. I'll remove you. Have a good weekend!
I think this is a useful update but I'm not sure that ROR v2 needs it: in gdcc/dataverse-external-vocab-support#46 I'm suggesting that we just get the whole array with "params": ["/name"] This is consistent with what we do with Skosmos, etc. where we get all the variants and names in different languages if they exist. |
Another ROR specific note - I'm not sure 0 is always the best to display )if you do want only one name) - in the PR the new ror.js script looks for the entry with the "ror_display" tag, which is what shows on their website. |
Yes, having the array was my intent. That means in the exports like OAI_ORE, you'll see:```
|
Waiting on conflict resolution |
FWIW: I'm reconsidering where using /names and retrieving the array is the best option here. While I think some export formats would benefit from being able to show all the options, our current DataCiteXML assumes that the value is a string, and, prior to #11782, getting an array would break things. |
FYI: I created #11793 and added a generic release note that says there are enhancements to the ext. vocab mechanism, which should cover this PR as well (assuming both go into Dataverse 6.9 (which we want so that the additions are available before ROR's v1 API gets shut off). |
223461d
to
3cf657f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I'll approve this so it moves to Ready for QA. @ofahimIQSS - make sure @ffritze has completed his testing before you merge.
What this PR does / why we need it:
The ROR-API has changed during the last weeks. In the future ROR API v1 will be deprecated.
The old search path for the termName in the old CVOC ROR script was "/value"
In version 2 of the ROR API this will no longer work. So the new search path in the cvocConf.json should be
""termName": {
"pattern": "{0}",
"params": [
"/names/0/value"
]
},"
Unfortunately the current implementation in DV 6.7.1 cannot handle the "0" index. It reads it as a String but it must be an Integer number regarding the JSON output from ROR V2.
This pull request provides a fix the specific implementation and it can handle now both String and Number as values for the ROR JSON output.
In cvocJson.conf after applying this pull request. Your can use the new ROR API endpoint: "https://api.ror.org/organizations/{0}"
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
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:
This pull request is only necessary for the ROR API V2.