refactor: Tesk app #197
refactor: Tesk app #197
Conversation
Reviewer's Guide by SourceryThis pull request refactors the Tesk application by introducing a new TeskApp class that extends the Foca framework. The changes consolidate the configuration loading and server initialization logic into this new class, simplifying the main application entry point. Additionally, a new custom exception ConfigNotFoundError is added to handle missing configuration files, and a minor formatting issue is fixed in the TesServiceInfo class. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JaeAeich - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
tesk/app.py
Outdated
|
|
||
| from connexion import FlaskApp | ||
| from foca import Foca | ||
| from werkzeug.exceptions import InternalServerError |
There was a problem hiding this comment.
suggestion: Consider using a more specific exception
Using InternalServerError might be too generic. Consider defining a custom exception or using a more specific one to better handle different error scenarios.
| from werkzeug.exceptions import InternalServerError | |
| from werkzeug.exceptions import HTTPException | |
| class CustomServerError(HTTPException): | |
| code = 500 | |
| description = 'A custom internal server error occurred' |
tesk/app.py
Outdated
| try: | ||
| TeskApp().run() | ||
| except Exception as error: | ||
| logger.exception("An error occurred while running the application.") |
There was a problem hiding this comment.
suggestion: Log the actual error message
Consider including the actual error message in the log for better debugging. For example: logger.exception(f"An error occurred while running the application: {error}").
| logger.exception("An error occurred while running the application.") | |
| logger.exception(f"An error occurred while running the application: {error}") |
| @@ -13,6 +13,11 @@ | |||
| NotFound, | |||
| ) | |||
|
|
|||
|
|
|||
| class ConfigNotFoundError(FileNotFoundError): | |||
There was a problem hiding this comment.
suggestion: Consider adding more context to the custom exception
It might be useful to add more context or attributes to the ConfigNotFoundError to provide additional information about the error, such as the expected location of the config file.
| class ConfigNotFoundError(FileNotFoundError): | |
| class ConfigNotFoundError(FileNotFoundError): | |
| def __init__(self, filepath): | |
| super().__init__(f"Configuration file not found: {filepath}") | |
| self.filepath = filepath |
tesk/tesk_app.py
Outdated
| @final | ||
| def run(self) -> None: | ||
| """Run the application.""" | ||
| _environment = self.conf.server.environment or "production" |
There was a problem hiding this comment.
suggestion: Consider using a constant for default environment
Using a constant for the default environment value (e.g., DEFAULT_ENVIRONMENT = "production") can improve readability and maintainability.
| _environment = self.conf.server.environment or "production" | |
| DEFAULT_ENVIRONMENT = "production" | |
| _environment = self.conf.server.environment or DEFAULT_ENVIRONMENT |
tesk/tesk_app.py
Outdated
| """Run the application.""" | ||
| _environment = self.conf.server.environment or "production" | ||
| logger.info(f"Running application in {_environment} environment...") | ||
| _debug = self.conf.server.debug or False |
There was a problem hiding this comment.
suggestion: Consider using a constant for default debug value
Using a constant for the default debug value (e.g., DEFAULT_DEBUG = False) can improve readability and maintainability.
| _debug = self.conf.server.debug or False | |
| DEFAULT_DEBUG = False | |
| _debug = self.conf.server.debug or DEFAULT_DEBUG |
tesk/tesk_app.py
Outdated
| ConfigNotFoundError: If the configuration file is not found. | ||
| """ | ||
| logger.info("Loading configuration path...") | ||
| if config_path_env := os.getenv("TESK_FOCA_CONFIG_FILE"): |
There was a problem hiding this comment.
🚨 suggestion (security): Consider adding validation for the environment variable
It might be useful to add validation to ensure that the environment variable TESK_FOCA_CONFIG_FILE points to a valid file path.
| if config_path_env := os.getenv("TESK_FOCA_CONFIG_FILE"): | |
| if config_path_env := os.getenv("TESK_FOCA_CONFIG_FILE"): | |
| config_file_path = Path(config_path_env).resolve() | |
| if not config_file_path.is_file(): | |
| raise ConfigNotFoundError(f"Configuration file not found at {config_file_path}") | |
| self.config_file = config_file_path |
| from the environment variable `TESK_FOCA_CONFIG_PATH` if set, or from | ||
| the default path if not. It raises a `FileNotFoundError` if the | ||
| configuration file is not found. | ||
| from tesk.tesk_app import TeskApp |
There was a problem hiding this comment.
issue (complexity): Consider reverting to the original structure while incorporating the new error handling logic.
The new code introduces several complexities and regressions:
-
Loss of Configuration Flexibility: The original code allowed for dynamic configuration loading from an environment variable or a default path. This flexibility is lost in the new code, making it less adaptable to different deployment environments.
-
Error Handling: The new code introduces a try-except block that catches all exceptions and raises an
InternalServerError. This is less specific and can obscure the root cause of issues, making debugging more difficult. The original code raised a specificFileNotFoundErrorif the configuration file was missing, which is more informative. -
Code Readability and Documentation: The original code had detailed docstrings explaining the purpose and behavior of the
init_appfunction. This documentation is missing in the new code, making it harder for future developers to understand the code's intent and functionality. -
Modularity: The original code was modular, with a clear separation of concerns. The
init_appfunction was responsible for initializing the app, while themainfunction was responsible for running it. The new code combines these responsibilities, reducing modularity and making the code harder to maintain.
Consider reverting to the original structure while incorporating the new error handling logic in a way that retains these benefits. Here is a suggested approach:
"""API server entry point."""
import logging
import os
from pathlib import Path
from connexion import FlaskApp
from foca import Foca
from werkzeug.exceptions import InternalServerError
logger = logging.getLogger(__name__)
def init_app() -> FlaskApp:
"""Initialize and return the FOCA app.
This function initializes the FOCA app by loading the configuration
from the environment variable `TESK_FOCA_CONFIG_PATH` if set, or from
the default path if not. It raises a `FileNotFoundError` if the
configuration file is not found.
Returns:
FlaskApp: A Connexion application instance.
Raises:
FileNotFoundError: If the configuration file is not found.
"""
# Determine the configuration path
config_path_env = os.getenv("TESK_FOCA_CONFIG_PATH")
if config_path_env:
config_path = Path(config_path_env).resolve()
else:
config_path = (Path(__file__).parents[1] / "deployment" / "config.yaml").resolve()
# Check if the configuration file exists
if not config_path.exists():
raise FileNotFoundError(f"Config file not found at: {config_path}")
foca = Foca(
config_file=config_path,
)
return foca.create_app()
def main() -> None:
"""Run FOCA application."""
try:
app = init_app()
app.run(port=app.port)
except Exception as error:
logger.exception("An error occurred while running the application.")
raise InternalServerError(
"An error occurred while running the application."
) from error
if __name__ == "__main__":
main()This version retains the flexibility, error specificity, and modularity of the original code while incorporating the new error handling logic.
There was a problem hiding this comment.
I'm still not too sure about this PR. IMO the improvements made here (minus the error handling issues) would make more sense in FOCA. Until that is the case, I don't see the immediate benefits on this code base, relative to the old approach.
Would you care to explain (ideally in the PR description) why you think this change is necessary?
tesk/api/ga4gh/tes/models.py
Outdated
| @@ -469,7 +469,7 @@ class TesServiceInfo(Service): | |||
| "service", | |||
| example=["VmSize"], | |||
| ) | |||
| type: Optional[TesServiceType] = None # type: ignore | |||
| type: Optional[TesServiceType] = None # type: ignore | |||
There was a problem hiding this comment.
yeah I thought so, but since #196 was majorly written using datamodel plugin, I didn't do it there as this error popped up when I started writing this. See the mypy tests are passign there.
There was a problem hiding this comment.
Except for the env vars, nothing in this module is specific to TESK. Consider leaving the original entry point for now and integrating this new app class in FOCA instead - this will reduce the boilerplate in all of our apps.
There was a problem hiding this comment.
OK, please review my reasoning again, and close this is you feel it might be irrelevent, I'll have to change the logic of the endpoints a lil bit so I'll address other reviews of the PR after merging/closing this.
SInce the app will be handeling reading and storing config, I think it would be better if that is done at the instatntiation only and not when the endpoint it called, specifically service-info. While writing that endpoint, I could would;ve had to use |
I suppose the use case is valid - given that config parsing is tied to the FOCA app creation in the current FOCA version, it makes the config not easily accessible outside of app context. However, I think the proper way of handling this is to decouple the config parsing from the app creation in FOCA and provide an easy entry point to the config in FOCA itself (basically what you did but generalized in FOCA). Apps using FOCA would then use two steps to create the app: 1. Parse the config; 2. Create the app based on the parsed config. That will address your use case without having to put this code in each repo and without having to parse the config twice. So I propose to close this and consider reusing (parts of) the code in FOCA after the Connexion 3 upgrade. |
Please check changes in #198 |
Summary
All the configs and commonly used attributes that are parsed at runtime can be reused with this abstraction, eg the
configvalue.Summary by Sourcery
This pull request refactors the Tesk application by introducing a new TeskApp class that extends the Foca framework for initializing and running the application. It also adds a custom ConfigNotFoundError for better error handling when configuration files are missing.