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

Marekhorst 1434 reimplement context importer module by replacing is lookup with RESTful endpoint #1437

Conversation

marekhorst
Copy link
Member

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.

…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.
@marekhorst marekhorst requested a review from mpol October 11, 2023 11:52
@marekhorst marekhorst self-assigned this Oct 11, 2023
@marekhorst marekhorst changed the title Marekhorst 1434 reimplement context importer module by replacing is lookup with res tful endpoint Marekhorst 1434 reimplement context importer module by replacing is lookup with RESTful endpoint Oct 11, 2023
Copy link
Contributor

@mpol mpol left a 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.

Comment on lines 6 to 8
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;
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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;
Copy link
Contributor

@mpol mpol Oct 12, 2023

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.

Copy link
Member Author

@marekhorst marekhorst Oct 13, 2023

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.

Copy link
Member Author

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.
@marekhorst marekhorst closed this Oct 13, 2023
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

Successfully merging this pull request may close these issues.

2 participants