-
Notifications
You must be signed in to change notification settings - Fork 1
GET routes/{route_id} #85
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
|
|
||
| from app.dependencies.auth import require_driver | ||
| from app.models import get_session | ||
| from app.models.route import RouteWithDateRead | ||
| from app.models.route import Route, RouteWithDateRead | ||
| from app.services.implementations.route_service import RouteService | ||
|
|
||
| # Initialize service | ||
|
|
@@ -38,6 +38,35 @@ async def get_routes( | |
| return routes | ||
|
|
||
|
|
||
| @router.get("/{route_id}", response_model=Route, status_code=status.HTTP_200_OK) | ||
| async def get_route( | ||
| route_id: UUID, | ||
| session: AsyncSession = Depends(get_session), | ||
| ) -> Route: | ||
| """ | ||
| Get a route by its unique identifier. | ||
|
|
||
| Parameters: | ||
| route_id (UUID): The unique identifier of the route to delete. | ||
| session (AsyncSession): The database session dependency. | ||
|
|
||
| Authentication: | ||
| Requires the user to be authenticated as a driver. | ||
|
|
||
| Returns: | ||
| None. Responds with HTTP 200 OK on successful get. | ||
|
|
||
| Raises: | ||
| HTTPException: | ||
| - 404 Not Found: If the route with the specified ID does not exist. | ||
| - 500 Server Error | ||
| """ | ||
|
|
||
| # TODO: the auth here does not work, I think this is an auth issue | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove To Do, good catch, and I will make sure to address this soon. |
||
| route = await route_service.get_route(session, route_id) | ||
| return route | ||
|
|
||
|
|
||
| @router.delete("/{route_id}", status_code=status.HTTP_204_NO_CONTENT) | ||
| async def delete_route( | ||
| route_id: UUID, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from datetime import datetime | ||
| from uuid import UUID | ||
|
|
||
| from fastapi import HTTPException | ||
| from sqlalchemy import and_, exists | ||
| from sqlalchemy import select as sql_select | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
@@ -92,6 +93,25 @@ async def get_routes( | |
| for row in rows | ||
| ] | ||
|
|
||
| async def get_route(self, session: AsyncSession, route_id: UUID) -> Route: | ||
| try: | ||
| """Get route by ID""" | ||
| statement = select(Route).where(Route.route_id == route_id) | ||
| result = await session.execute(statement) | ||
| route = result.scalars().first() | ||
|
|
||
| if not route: | ||
| self.logger.error(f"Route with id {route_id} not found") | ||
| raise ValueError(f"Route with id {route_id} not found") | ||
|
|
||
|
Comment on lines
+102
to
+106
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this error handling would raise a 500 Internal Server Error rather than a 404. It would be better to do something like: raise HTTPException( |
||
| return route | ||
| except Exception as error: | ||
| self.logger.exception("Failed to get route %s", route_id) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does %s mean? |
||
| await session.rollback() | ||
| raise HTTPException( | ||
| status_code=500, detail="Failed to retrieve route." | ||
| ) from error | ||
|
|
||
| async def delete_route(self, session: AsyncSession, route_id: UUID) -> bool: | ||
| """Delete route by ID""" | ||
| try: | ||
|
|
@@ -110,4 +130,5 @@ async def delete_route(self, session: AsyncSession, route_id: UUID) -> bool: | |
| except Exception as error: | ||
| self.logger.error(f"Failed to delete route {route_id}: {error!s}") | ||
| await session.rollback() | ||
| # TODO: do we really want to return the raw error | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be good for now! There might be logging that is done at a level above; this just raises an error. |
||
| raise error | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
It's good that you are leaving docstring, but I think that this might be a little too verbose. Just a description, parameter, and returns are fine. Auth should be evident by reading code and raises can be a quick reference to 404 (500 comes default)!