-
Notifications
You must be signed in to change notification settings - Fork 11
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
Marekhorst 1434 reimplement context importer module by replacing is lookup with RESTful endpoint #1437
Conversation
…kup with RESTful endpoint WIP. Proper error code handling needs to be introduced once RESTful context API returns an appropriate HTTP error code. Introducing RESTful context API based concepts importer module. New paramater is required to enable RESTful endpoint based import: `import_context_service_location`. If the parameter is not provided then ISLookup-based importer mode will be enabled making `import_islookup_service_location` parameter required in such case.
…kup with RESTful endpoint Proper handling of non-existing context by addressing 404 HTTP code. Adding relevant test cases.
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.
Looks good in general, but there are a couple small, mostly copy-paste, issues.
import static eu.dnetlib.iis.wf.importer.concept.ISLookupServiceBasedConceptImporter.CONCEPT_COUNTER_NAME; | ||
import static eu.dnetlib.iis.wf.importer.concept.ISLookupServiceBasedConceptImporter.PARAM_IMPORT_CONTEXT_IDS_CSV; | ||
import static eu.dnetlib.iis.wf.importer.concept.ISLookupServiceBasedConceptImporter.PORT_OUT_CONCEPTS; |
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.
Imports from wrong class?
* ISLookup mock providing static concept profiles. | ||
* | ||
*/ | ||
private static class MockISLookupFacade implements ContextStreamingFacade { |
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.
Wrong class name?
* ISLookup mock providing static concept profiles. | ||
* | ||
*/ | ||
private static class MockISLookupFacade implements ContextStreamingFacade { |
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.
Wrong class name?
* ISLookup mock providing static concept profiles. | ||
* | ||
*/ | ||
private static class MockISLookupFacade implements ContextStreamingFacade { |
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.
Wrong class name?
* @throws MalformedURLException | ||
*/ | ||
public ContextUrlStreamingFacade(String endpointLocation, | ||
int readTimeout, int connectionTimeout) throws MalformedURLException { |
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.
Doesn't actually throw the exception.
//------------------------ PRIVATE -------------------------- | ||
|
||
private static String buildUrl(String endpointLocation, String contextId) { | ||
return endpointLocation + "/" + contextId; |
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.
In general xxx//yyy
in an URL is not the same as xxx/yyy
, but it seems the exporter API returns the same result in both cases, so maybe it's not that important to worry about it here.
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 agree, the best way would be to rely on a UriTemplate
or RestTemplate
(or even more generic StringSubstitutor
) and provide a template instead of endpointLocation
but it seems it is not available among included dependencies and I think for this particular, rather static case, I wouldn't overcomplicate the final solution.
I'd stick to simple check regarding the trailing slash e.g.:
private static String buildUrl(String endpointLocation, String contextId) {
StringBuilder urlBuilder = new StringBuilder(endpointLocation);
if (!endpointLocation.endsWith(URL_SLASH)) {
urlBuilder.append(URL_SLASH);
}
urlBuilder.append(contextId);
return urlBuilder.toString();
}
and call it a day.
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.
Extracted this method to ContextUrlStreamingFacadeUtils
and prepared a dedicated test class ContextUrlStreamingFacadeUtilsTest
.
…kup with RESTful endpoint Introducing PR related fixes.
This PR introduces an additional support for RESTful streaming endpoint as one another source providing context details in JSON format, alongside already available ISLookup based importer which is still to be maintained until we are ready to dismiss it (#1435).
An appropriate importer module is to be automatically selected relying on
import_context_service_location
parameter availability.