Skip to content

Commit

Permalink
fix: API docs Task and improve consistency
Browse files Browse the repository at this point in the history 
Fix documentation about the type of task result in the
`/api/v1/tasks`.

The type was moved to `Dict[str, Any]`

It adds more consistency on task result in the API.
Verifying the `message` and `status` and `error` (in case of FAILURE)

Signed-off-by: Kairo Araujo <kairo.araujo@testifysec.com>
  • Loading branch information
@kairoaraujo
kairoaraujo committed Jan 31, 2024
1 parent db8ee39 commit 5e9f876
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 59 deletions.
50 changes: 14 additions & 36 deletions docs/swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,30 +1516,6 @@
],
"title": "TargetsInfo"
},
"TaskDetails": {
"properties": {
"message": {
"type": "string",
"title": "Message",
"description": "Result detail description"
},
"error": {
"type": "string",
"title": "Error",
"description": "If the task status result is `False` shows an error message"
},
"details": {
"type": "object",
"title": "Details",
"description": "Any releavant information from task"
}
},
"type": "object",
"required": [
"message"
],
"title": "TaskDetails"
},
"TaskName": {
"type": "string",
"enum": [
Expand All @@ -1557,6 +1533,16 @@
},
"TaskResult": {
"properties": {
"message": {
"type": "string",
"title": "Message",
"description": "Result detail description"
},
"error": {
"type": "string",
"title": "Error",
"description": "If the task status result is `False` shows an error message"
},
"status": {
"type": "boolean",
"title": "Status",
Expand All @@ -1577,21 +1563,14 @@
"description": "Last time task was updated"
},
"details": {
"allOf": [
{
"$ref": "#/components/schemas/TaskDetails"
}
],
"type": "object",
"title": "Details",
"description": "Relevant result details"
}
},
"type": "object",
"required": [
"status",
"task",
"last_update",
"details"
"message"
],
"title": "TaskResult"
},
Expand Down Expand Up @@ -1629,14 +1608,13 @@
"description": "The Celery task state. Note: It isn't the task result status.\n\n`PENDING`: Task state is unknown (assumed pending since you know the id).\n\n`RECEIVED`: Task received by a worker (only used in events).\n\n`STARTED`: Task started by a worker.\n\n`RUNNING`: Task is running.\n\n`SUCCESS`: Task succeeded.\n\n`FAILURE`: Task failed.\n\n`ERRORED`: Task errored. RSTUF identified an error while processing the task.\n\n`REVOKED`: Task revoked.\n\n`REJECTED`: Task was rejected (only used in events).\n\n"
},
"result": {
"anyOf": [
{},
"allOf": [
{
"$ref": "#/components/schemas/TaskResult"
}
],
"title": "Result",
"description": "Task result details (state `SUCCESS` uses schema `TaskResult)"
"description": "Task result if available."
}
},
"type": "object",
Expand Down
50 changes: 32 additions & 18 deletions repository_service_tuf_api/tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import enum
from datetime import datetime
from typing import Any, Dict, Optional, Union
from typing import Any, Dict, Optional

from celery import states
from pydantic import BaseModel, Field
Expand Down Expand Up @@ -42,25 +42,23 @@ class GetParameters(BaseModel):
task_id: str


class TaskDetails(BaseModel):
class TaskResult(BaseModel):
message: str = Field(description="Result detail description")
error: Optional[str] = Field(
description=(
"If the task status result is `False` shows an error message"
)
)
details: Optional[Dict[str, Any]] = Field(
description="Any releavant information from task"
)


class TaskResult(BaseModel):
status: bool = Field(
status: Optional[bool] = Field(
description="Task result status. `True` Success | `False` Failure"
)
task: TaskName = Field(description="Task name by worker")
last_update: datetime = Field(description="Last time task was updated")
details: TaskDetails = Field(description="Relevant result details")
task: Optional[TaskName] = Field(description="Task name by worker")
last_update: Optional[datetime] = Field(
description="Last time task was updated"
)
details: Optional[Dict[str, Any]] = Field(
description="Relevant result details"
)


class TasksData(BaseModel):
Expand All @@ -81,10 +79,8 @@ class TasksData(BaseModel):
"`REJECTED`: Task was rejected (only used in events).\n\n"
)
)
result: Optional[Union[Any, TaskResult]] = Field(
description=(
"Task result details (state `SUCCESS` uses schema `TaskResult)"
)
result: Optional[TaskResult] = Field(
description=("Task result if available.")
)


Expand Down Expand Up @@ -133,13 +129,31 @@ def get(task_id: str) -> Response:
task_state = task.state
task_result = task.result

# Celery FAILURE task, we include the task result (exception) as an error
# and default message as critical failure executing the task.
if isinstance(task.result, Exception):
task_result = {"message": str(task.result)}
elif task_state == TaskState.SUCCESS and not task_result.get(
task_result = {
"message": "Critical failure executing the task.",
"error": str(task.result),
"status": False,
}

# There are Celery states without result (i.e. PENDING, RECEIVED, STARTED)
if task_result is None or task_result.get("message") is None:
task_result = {"status": False, "message": "No message available."}

# If the task state is SUCCESS and the task result is False we considere
# it an errored task.
if task_state == TaskState.SUCCESS and not task_result.get(
"status", False
):
task_state = TaskState.ERRORED

# In RSTUF Worker the 'message' key is mandatory, but in case of it still
# return a task result without a message we considere it an errored task
if task_state == TaskState.SUCCESS and task_result.get("message") is None:
task_result = {"message": "No message returned by the task."}

Check warning on line 155 in repository_service_tuf_api/tasks.py

View check run for this annotation

Codecov / codecov/patch

repository_service_tuf_api/tasks.py#L155 

Added line #L155 was not covered by tests

return Response(
data=TasksData(task_id=task_id, state=task_state, result=task_result),
message="Task state.",
Expand Down
20 changes: 15 additions & 5 deletions tests/unit/api/test_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_get(self, test_client, monkeypatch):

def test_get_result_is_exception(self, test_client, monkeypatch):
mocked_task_result = pretend.stub(
state="SUCCESS", result=ValueError("Failed to load")
state="FAILURE", result=ValueError("Failed to load")
)
mocked_repository_metadata = pretend.stub(
AsyncResult=pretend.call_recorder(lambda t: mocked_task_result)
Expand All @@ -81,8 +81,12 @@ def test_get_result_is_exception(self, test_client, monkeypatch):
assert test_response.json() == {
"data": {
"task_id": "test_id",
"state": "SUCCESS",
"result": {"message": "Failed to load"},
"state": "FAILURE",
"result": {
"message": "Critical failure executing the task.",
"error": "Failed to load",
"status": False,
},
},
"message": "Task state.",
}
Expand Down Expand Up @@ -153,7 +157,10 @@ def test_get_result_success_with_empty_result(
"data": {
"task_id": "test_id",
"state": "ERRORED",
"result": {},
"result": {
"status": False,
"message": "No message available.",
},
},
"message": "Task state.",
}
Expand Down Expand Up @@ -183,7 +190,10 @@ def test_get_result_failure_with_empty_result(
"data": {
"task_id": "test_id",
"state": "FAILURE",
"result": {},
"result": {
"status": False,
"message": "No message available.",
},
},
"message": "Task state.",
}
Expand Down

0 comments on commit 5e9f876

Please sign in to comment.