#283: Improve health endpoint with missing required conditions #291
Conversation
…ing-required-conditions
|
Hi @RobyBen can you check failing pipelines? I think you don't have your development environment configured with linting and pre-commit too. For more info check global set up docshttps://antoniomrtz.github.io/SpotifyElectron/developer/SETUP/ and set tup backend docs |
| @router.get("/", summary="Health Check Endpoint") | ||
| def get_health() -> Response: | ||
| """Validates if the app has launched correctly | ||
| async def check_database_connection() -> Dict[str, str]: |
There was a problem hiding this comment.
This logic belongs to a service layer more than a controller one. Move functions that handle logic to health_service
|
|
||
| class HealthCheckResponse(BaseModel): | ||
| status: str |
There was a problem hiding this comment.
Instead of status:str we can create a Enum that encapsulates different status. With that we can make the code more reusable and concise
| service_health = check_song_service() | ||
| auth_health = check_auth_config() | ||
|
|
||
| health_details = { |
| status_code = HTTP_200_OK if is_healthy else HTTP_503_SERVICE_UNAVAILABLE | ||
|
|
||
| if not is_healthy: | ||
| logger.warning(f"Health check failed: {health_details}") |
There was a problem hiding this comment.
I dont think this is needed, we're going to return 503 if healthcheck failed
AntonioMrtz
changed the title
…_schema. I think I fixed it, it should work if I understand it correctly | [AntonioMrtz#283]
…ing-required-conditions
…ing-required-conditions
|
Hi @RobyBen , let me know if you need something :). Feel free to request a review again whenever you feel ready. |
…ing-required-conditions
|
Can you check my commit? |
Hi @RobyBen I will check it ASAP. Thanks for your work :) |
There was a problem hiding this comment.
Can we first fix the errors detected by the pipeline:
- Run ruff format and check fix https://antoniomrtz.github.io/SpotifyElectron/developer/backend/Linting-%26-Formatting/
- Update OpenAPI https://antoniomrtz.github.io/SpotifyElectron/developer/utils/OpenAPI/
…conditions' of https://github.com/RobyBen/SpotifyElectron into feat/283-Improve-health-endpoint-with-missing-required-conditions
|
The OpenAPI part I mentioned here is not completed :(. It should generate changes in openapi.json. The pipeline is running |
|
I commited the change, please take a look |
|
Perfect @RobyBen , pipelines are working correctly. I will check the code ASAP :) |
| environment (AppEnvironmentMode): The current environment mode. | ||
| connection_uri (str): The URI for connecting to the database. | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
There's no need to add docs for returning None. The extension njpwerner.autodocstring handles it like that and I think Pycharm too
| """ | ||
| from pymongo import MongoClient |
There was a problem hiding this comment.
The purpose of this class is not having any initialization of the Database until we know if tests will be run or we're in production mode. database_connection_class contains the logic associated to a concrete database, being a mock connectior or a real one. We're also abstracting the logic behind the DatabaseConnection classes, DatabaseConnectionManager is only managing that instance and showing it to the rest of the app, concrete logic should be inside DatabaseConnection classes :). Feel free to ask if something isn't explained correctly
| cls.client = MongoClient(connection_uri) | ||
|
|
||
| @classmethod | ||
| def ping_database(cls) -> bool: |
There was a problem hiding this comment.
We should move this to the concrete implementations of database connection, such as Prod and Testing, included in the same folder
| logger = SpotifyElectronLogger(LOGGING_HEALTH).getLogger() | ||
|
|
||
|
|
||
| def check_database_connection() -> dict[str, str]: |
There was a problem hiding this comment.
This can return None if the if statement is not met. We should change ping database to throw an exception if ping couldn't be done instead of returning a bool. I still feel like this func belongs more to a service layer.
| "message": f"SongServiceProvider check failed: {str(exception)}", | ||
| } | ||
| else: | ||
| if service is None: |
There was a problem hiding this comment.
This can be done in a single return x if y else z statement
Description
Improve health endpoint with missing required conditions
Commit type
feat: indicates the addition of a new feature or functionality to the project.Issue
#283
Solution
Add health end points
Proposed Changes
Potential Impact
info about problems in project
Screenshots
None
Additional Tasks
Tests.
Assigned
@AntonioMrtz