Draft architecture for LORIS API calls within LORIS-MRI Python

[maximemulder]

This is a draft architecture to call the LORIS API from within the LORIS-MRI Python code (issue #1147), using the API calls from the DICOM upload API PR (Loris#9154). The code is not fully working yet (TODO: error handling, type conversions), but it presents the broad lines of the architecture:

There is an Api class used as the canonical way to make API calls. It stores the LORIS URL and an API token, and can be passed around to any code that needs to make API calls. It has a method api.call(...) that allows to call any API route with unstructured values (route as a string, returns the response without parsing it).

There is a python.lib.api namespace that contains specialized wrappers for each API route. These wrappers allow to use the API in a structured way, by passing the route arguments directly as function arguments, and by returning structured objects.

Unresolved question, in which namespace to put this Api class ?

• lib.api (which this PR does, EDIT: this is a bad choice because there is a name clash with the lib.api namespace) ?
• lib.dataclass.api (EDIT: I prefer this option) ?
• lib.api.api ?

Unresolved question, how to structure the objects returned by the API calls ?

• Manually in the classes constructors (which this PR does) ?
• Manually in static methods (examples: from_object, from_json, from_dict) ?
• Automatically using a library (I have heard of dataclasses-json and pydantic)

Unresolved question, how to structure the API wrapper files ?

• One file per route, with the route function and its types (which this PR does).
• Different files or namespaces for types and routes (examples: lib.api.routes, lib.api.types) ?
• Something else ?

The (which this PR does) mentions are just here as an indication, I am not particularly advocating for these options over others.

[cmadjar]

@maximemulder I just realized that for HBCD, I do call the API and I added the following in database_config.py:

api = {
    'host'     : 'https://10.31.82.81',
    'version'  : 'v0.0.3',
    'username' : 'admin',
    'password' : 'XXXXXXXX',
}

I am only using the API to create issues in the issue tracker and this is how the token is generated:

        api_cred = self.config_file.api
        try:
            self.jwtoken = json.loads(
                requests.post(
                    url=os.path.join(api_cred['host'], 'api', api_cred['version'], 'login'),
                    json={'username': api_cred['username'], 'password': api_cred['password']},
                    verify=False
                ).content.decode('ascii')
            )['token']
        except (ValueError, KeyError)  as err:
            config_filename = loris_getopt_obj.options_dict['profile']['value']
            self.hbcd_log_obj.log_error_and_exit(
                message=f"Could not connect to the API using information stored in {config_filename}. "
                        f"Please verify them and try again.",
                exit_code=lib.exitcode.PROFILE_FAILURE
            )

and then self.jwtoken is being use to do API calls to create an issue

        url = os.path.join(self.config_file.api['host'], 'issue_tracker', 'Edit')
        try:
            response = requests.post(
                url=url,
                headers={'Authorization': 'Bearer %s' % self.jwtoken},
                verify=False,
                data=self.issue_dict
            )
            issue_created = response.json()['issueID']
        except ValueError as err:
            self.hbcd_log_obj.log_error_and_exit(
                message=f"Failed creating issue via API with URL {url} and data dictionary {self.issue_dict}."
                        f" Error was:\n{err}",
                exit_code=lib.exitcode.INSERT_FAILURE
            )
        self.hbcd_log_obj.log_info(f"Successfully create issue number {issue_created}")

That system works well for HBCD. Wondering if that could work for your case as well.

[maximemulder]

Hhhhm, interesting, there are definitely some parts that I don't have in my code (why verify=False ?). Using os.path.join to join the parts of an URL technically works but is probably a little unorthodox IMO 😅. In any case, what I am most interested in in my PR is the bigger picture, like what the abstractions are and how the files are organized. There is also the topic of the config. Adding API information in the config seems to be what I will also do, however, I will need to have the user as an argument of the script, as there may be several users using the script.

[maximemulder]

Summary of today's design meeting

This PR design is fine. The answers to the unanswered questions are the following:

• Put the api.py file in the lib.dataclass namespace to avoid conflict with the lib.api namespace.
• Keep the types in the route files for now.
• Have a default API version in the Api class (EDIT: I actually think this is a bad idea now, the user should know which API version they want to use for each call they make, having a default might lead to unintended behaviour).
• Be careful about nullable types in the structured API.
• Do not use a library to structure the API responses.

I looked into Swagger codegen in more details. It certainly generates a lot of the boilerplate to us, which is more practical to create the API client, but the generated code seems to be of lesser quality, and notably does not seem to have type hints (big no-no for me).

A final not really related to this PR, but another one that should arrive next week, is how to pass user credentials to a script that calls the API. One solution is simply to store a single user's credentials in the config file, as @cmadjar does it. However, I need a script which can be ran by multiple users, and the options we considered for the password are these ones:

• Use a CLI argument (likely unsecure)
• Use an environment variable (liked by @laemtl and others)
• Use the standard input (more cumbersome to use)
• Use a specific config file located at a hardcoded path somewhere in the Linux user home directory.
• Use a specific config file passed as a CLI argument.

I will ask Sylvain about this as he might have some insights or an opinion.

[maximemulder]

After some discussion with Rida, we concluded the script should live at the BIC and not LORIS-MRI, so not planned for now.