-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature: external sources #2560
Conversation
…lDataService and used this in the ExternalDataRestRepositories for an accurate pagination on the search methods
…st DataProvider and fixed checkstyle
… single metadatafield would get lost due to sorting issues
This PR it's working as expected witch regards the RestContract. But I think it will be useful to have the Entry's context at EntryValue level, so I've created this PR: DSpace/RestContract#88 to address that need. |
dspace-api/src/main/java/org/dspace/external/provider/ExternalDataProvider.java
Show resolved
Hide resolved
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.
Overall, this PR works as expected. I've some improvements proposal that can be address in different and subsequent pull requests.
One of those improvements is related with the unavailability of the external source service. I've done some tests and forced an error and got an Internal Server Error on the REST payload. Should the client interpret this error or should a generic error be thrown?
Thanks for this @KevinVdV and others!
dspace-api/src/main/java/org/dspace/external/provider/ExternalDataProvider.java
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/external/provider/impl/OrcidV2AuthorDataProvider.java
Show resolved
Hide resolved
throw new IllegalArgumentException("The maximum number of results to retrieve cannot exceed 100."); | ||
} | ||
|
||
String searchPath = "search?q=" + URLEncoder.encode(query) + "&start=" + start + "&rows=" + limit; |
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.
We have cases when the submitter knows the ORCID ID of the author. In those cases it's useful to ignore que ORCID search and simply do the record fetch. Otherwise, the query will return millions of results and we will do unnecessary requests, resulting in a delayed output for the client.
If we identify that the query value is an ORCID ID, then we firstly do simple checkdigit validation to ensure we have a valid ORCID ID. And if valid, then perform the record fetch.
How it works:
ORCID-ID: 0000-0003-1234-4321
stripped(ORCID-ID): 0000000312344321
baseDigits: 000000031234432
generateOrcidCheckDigit(baseDigits): 1
(stripped(ORCID-ID).equals(baseDigits+generateOrcidCheckDigit(baseDigits))): true
/* Generates check digit as per ISO 7064 11,2.
*
*/
private static String generateOrcidCheckDigit(String baseDigits) {
int total = 0;
for (int i = 0; i < baseDigits.length(); i++) {
int digit = Character.getNumericValue(baseDigits.charAt(i));
total = (total + digit) * 2;
}
int remainder = total % 11;
int result = (12 - remainder) % 11;
return result == 10 ? "X" : String.valueOf(result);
}
It can be considered in another PR as an improvement of Orcid Person provider.
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.
Agreed, this does seem like something to consider as a followup to streamline the requests to ORCID
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.
Thank you @KevinVdV and @benbosman !
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.
@KevinVdV : I gave this a code review today. Mostly it's looking good, but I do have some (mostly minor) implementation concerns in inline comments below.
dspace-api/src/main/java/org/dspace/app/sherpa/SHERPAResponse.java
Outdated
Show resolved
Hide resolved
throw new IllegalArgumentException("The maximum number of results to retrieve cannot exceed 100."); | ||
} | ||
|
||
String searchPath = "search?q=" + URLEncoder.encode(query) + "&start=" + start + "&rows=" + limit; |
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.
Agreed, this does seem like something to consider as a followup to streamline the requests to ORCID
dspace-api/src/main/java/org/dspace/external/provider/impl/SherpaJournalDataProvider.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/mock/MockMetadataValue.java
Outdated
Show resolved
Hide resolved
dspace-server-webapp/src/main/java/org/dspace/app/rest/ExternalSourcesRestController.java
Outdated
Show resolved
Hide resolved
...server-webapp/src/main/java/org/dspace/app/rest/repository/ExternalSourceRestRepository.java
Outdated
Show resolved
Hide resolved
…sourceController instead of a custom 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.
@KevinVdV and @Raf-atmire : This is looking better. Thanks for the updates. I've got a few more inline comments though (all minor) as a few new mistakes were accidentally introduced.
dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/MockMetadataConverter.java
Outdated
Show resolved
Hide resolved
…or external sources feature
…erty in external-services.xml
@tdonohue We have processed your feedback, merged with latest master to add projection support. So this is ready for another review. |
…nfiguration value
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.
👍 Code/PR looks good to me now. Thanks for all the hard work @KevinVdV and @Raf-atmire . Just waiting on Travis to succeed before we merge it.
This is the REST API development for DSpace/RestContract#74
In this PR we introduce the new concept "external sources". These external sources will be the backbone for authority control & metadata imports using third party API's.
You can interact with external sources by using the "ExternalDataService" interface. The implementation of this interface is just a wrapper around a list of ExternalDataProvider objects. The ExternalDataProvider is an interface that offers search methods (find one / find by query), the implementations of these interfaces are the ones that are actually doing to the calls to the third party sources (SHERPA/RoMEO, ORCID, Library of Congress, ...)
The ExternalDataProvider implementations are configured in the config/spring/api/external-services.xml file.
The ExternalDataProvider will return results in the generic "ExternalDataObject" class.