-
Notifications
You must be signed in to change notification settings - Fork 30
♻️ common http API interface for long_running_tasks
#7843
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: master
Are you sure you want to change the base?
Changes from all commits
07dbc67
4c689a2
7977cd7
d7f04d1
f50e77d
4cfa96d
421f8cf
e737641
ada5069
7070b66
61daf54
2e752e4
75aa300
d943710
6a6677c
47e5667
e2dfb66
87dec05
ebe6a43
f630a39
757be74
8497191
866656d
2ee9203
d886db0
e7df051
ecd1987
ea17b89
0578e5c
a4660de
70a6921
8374de5
221e051
fd50810
08a65e7
c30f80d
87b6b45
8d894ef
d2dbc95
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 |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
# pylint: disable=too-many-arguments | ||
|
||
|
||
from typing import Annotated | ||
from typing import Annotated, Any | ||
|
||
from fastapi import APIRouter, Depends, status | ||
from models_library.generics import Envelope | ||
|
@@ -54,6 +54,7 @@ def cancel_and_delete_task( | |
@router.get( | ||
"/{task_id}/result", | ||
name="get_task_result", | ||
response_model=Any, | ||
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. enveloped or not? |
||
description="Retrieves the result of a task", | ||
) | ||
def get_task_result( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,12 @@ class TaskResult(BaseModel): | |
error: Any | None | ||
|
||
|
||
class TaskGet(BaseModel): | ||
class TaskGetWithoutHref(BaseModel): | ||
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. this is a bit of a weird naming. |
||
task_id: TaskId | ||
task_name: str | ||
|
||
|
||
class TaskGet(TaskGetWithoutHref): | ||
status_href: str | ||
result_href: str | ||
abort_href: str | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ | |
RQT_LONG_RUNNING_TASKS_CONTEXT_KEY, | ||
) | ||
|
||
# NOTE: figure out how to remove these and expose them differently if possible | ||
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. disguised TODO? who is going to do something about it? |
||
|
||
|
||
def get_tasks_manager(app: web.Application) -> TasksManager: | ||
output: TasksManager = app[APP_LONG_RUNNING_TASKS_MANAGER_KEY] | ||
|
@@ -17,7 +19,3 @@ def get_tasks_manager(app: web.Application) -> TasksManager: | |
def get_task_context(request: web.Request) -> dict[str, Any]: | ||
output: dict[str, Any] = request[RQT_LONG_RUNNING_TASKS_CONTEXT_KEY] | ||
return output | ||
|
||
|
||
def create_task_name_from_request(request: web.Request) -> str: | ||
return f"{request.method} {request.rel_url}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,15 @@ | ||
import logging | ||
from typing import Any | ||
|
||
from aiohttp import web | ||
from common_library.json_serialization import json_dumps | ||
from pydantic import BaseModel | ||
from servicelib.aiohttp import status | ||
from servicelib.aiohttp.rest_responses import create_data_response | ||
|
||
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. relative import |
||
from ...long_running_tasks.errors import TaskNotCompletedError, TaskNotFoundError | ||
from ...long_running_tasks import http_endpoint_responses | ||
from ...long_running_tasks.models import TaskGet, TaskId, TaskStatus | ||
from ...long_running_tasks.task import TrackedTask | ||
from ..requests_validation import parse_request_path_parameters_as | ||
from ._dependencies import get_task_context, get_tasks_manager | ||
|
||
_logger = logging.getLogger(__name__) | ||
routes = web.RouteTableDef() | ||
|
||
|
||
|
@@ -24,24 +21,17 @@ class _PathParam(BaseModel): | |
async def list_tasks(request: web.Request) -> web.Response: | ||
tasks_manager = get_tasks_manager(request.app) | ||
task_context = get_task_context(request) | ||
tracked_tasks: list[TrackedTask] = tasks_manager.list_tasks( | ||
with_task_context=task_context | ||
) | ||
|
||
return web.json_response( | ||
{ | ||
"data": [ | ||
TaskGet( | ||
task_id=t.task_id, | ||
task_name=t.task_name, | ||
status_href=f"{request.app.router['get_task_status'].url_for(task_id=t.task_id)}", | ||
result_href=f"{request.app.router['get_task_result'].url_for(task_id=t.task_id)}", | ||
abort_href=f"{request.app.router['cancel_and_delete_task'].url_for(task_id=t.task_id)}", | ||
) | ||
for t in tracked_tasks | ||
] | ||
}, | ||
dumps=json_dumps, | ||
return create_data_response( | ||
[ | ||
TaskGet( | ||
task_id=t.task_id, | ||
task_name=t.task_name, | ||
status_href=f"{request.app.router['get_task_status'].url_for(task_id=t.task_id)}", | ||
result_href=f"{request.app.router['get_task_result'].url_for(task_id=t.task_id)}", | ||
abort_href=f"{request.app.router['cancel_and_delete_task'].url_for(task_id=t.task_id)}", | ||
) | ||
for t in http_endpoint_responses.list_tasks(tasks_manager, task_context) | ||
] | ||
) | ||
|
||
|
||
|
@@ -51,10 +41,10 @@ async def get_task_status(request: web.Request) -> web.Response: | |
tasks_manager = get_tasks_manager(request.app) | ||
task_context = get_task_context(request) | ||
|
||
task_status: TaskStatus = tasks_manager.get_task_status( | ||
task_id=path_params.task_id, with_task_context=task_context | ||
task_status: TaskStatus = http_endpoint_responses.get_task_status( | ||
tasks_manager, task_context, path_params.task_id | ||
) | ||
return web.json_response({"data": task_status}, dumps=json_dumps) | ||
return create_data_response(task_status) | ||
|
||
|
||
@routes.get("/{task_id}/result", name="get_task_result") | ||
|
@@ -64,37 +54,17 @@ async def get_task_result(request: web.Request) -> web.Response | Any: | |
task_context = get_task_context(request) | ||
|
||
# NOTE: this might raise an exception that will be catched by the _error_handlers | ||
try: | ||
task_result = tasks_manager.get_task_result( | ||
task_id=path_params.task_id, with_task_context=task_context | ||
) | ||
# NOTE: this will fail if the task failed for some reason.... | ||
await tasks_manager.remove_task( | ||
path_params.task_id, with_task_context=task_context, reraise_errors=False | ||
) | ||
return task_result | ||
except (TaskNotFoundError, TaskNotCompletedError): | ||
raise | ||
except Exception: | ||
# the task shall be removed in this case | ||
await tasks_manager.remove_task( | ||
path_params.task_id, with_task_context=task_context, reraise_errors=False | ||
) | ||
raise | ||
return await http_endpoint_responses.get_task_result( | ||
tasks_manager, task_context, path_params.task_id | ||
) | ||
|
||
|
||
@routes.delete("/{task_id}", name="cancel_and_delete_task") | ||
async def cancel_and_delete_task(request: web.Request) -> web.Response: | ||
path_params = parse_request_path_parameters_as(_PathParam, request) | ||
tasks_manager = get_tasks_manager(request.app) | ||
task_context = get_task_context(request) | ||
await tasks_manager.remove_task(path_params.task_id, with_task_context=task_context) | ||
await http_endpoint_responses.remove_task( | ||
tasks_manager, task_context, path_params.task_id | ||
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. why is this one not returning a response? |
||
) | ||
return web.json_response(status=status.HTTP_204_NO_CONTENT) | ||
|
||
|
||
__all__: tuple[str, ...] = ( | ||
"get_tasks_manager", | ||
"TaskId", | ||
"TaskGet", | ||
"TaskStatus", | ||
) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -18,10 +18,10 @@ async def base_long_running_error_handler( | |||||
_: Request, exception: BaseLongRunningError | ||||||
) -> JSONResponse: | ||||||
_logger.debug("%s", exception, stack_info=True) | ||||||
error_fields = dict(code=exception.code, message=f"{exception}") | ||||||
error_fields = {"code": exception.code, "message": f"{exception}"} | ||||||
status_code = ( | ||||||
status.HTTP_404_NOT_FOUND | ||||||
if isinstance(exception, (TaskNotFoundError, TaskNotCompletedError)) | ||||||
if isinstance(exception, TaskNotFoundError | TaskNotCompletedError) | ||||||
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. Using the union operator (|) in an isinstance() call is not valid; please replace it with a tuple, e.g., isinstance(exception, (TaskNotFoundError, TaskNotCompletedError)).
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
else status.HTTP_400_BAD_REQUEST | ||||||
) | ||||||
return JSONResponse(content=jsonable_encoder(error_fields), status_code=status_code) |
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.
Enveloped or not?