Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 19 additions & 23 deletions backend/app/api/endpoints/cloud_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

"""Shared cloud project endpoints."""

from fastapi import APIRouter, Depends, File, Form, Query, Response, UploadFile, status
from fastapi import APIRouter, Depends, File, Form, Query, UploadFile, status
from sqlalchemy.orm import Session

from app.api.dependencies import get_db
Expand All @@ -25,7 +25,6 @@
CloudProjectMemberCreate,
CloudProjectMemberResponse,
CloudProjectMemberUpdate,
CloudProjectProviderCredentialResponse,
CloudProjectResponse,
CloudProjectUpdate,
LocalBindingCreate,
Expand All @@ -37,6 +36,20 @@
router = APIRouter()


def _project_response(
db: Session, project: object, current_user: User
) -> CloudProjectResponse:
access = cloud_project_service.access(db, int(project.id), current_user.id)
return CloudProjectResponse.model_validate(
{
**project.__dict__,
"current_user_id": current_user.id,
"current_user_name": current_user.user_name,
"access_role": access.role,
}
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.


@router.post(
"", response_model=CloudProjectResponse, status_code=status.HTTP_201_CREATED
)
Expand All @@ -46,7 +59,7 @@ def create_cloud_project(
current_user: User = Depends(get_current_user),
) -> CloudProjectResponse:
project = cloud_project_service.create(db, current_user.id, values)
return CloudProjectResponse.model_validate(project)
return _project_response(db, project, current_user)


@router.get("", response_model=CloudProjectListResponse)
Expand All @@ -56,7 +69,7 @@ def list_cloud_projects(
) -> CloudProjectListResponse:
projects = cloud_project_service.list_accessible(db, current_user.id)
return CloudProjectListResponse(
items=[CloudProjectResponse.model_validate(project) for project in projects]
items=[_project_response(db, project, current_user) for project in projects]
)


Expand All @@ -67,24 +80,7 @@ def get_cloud_project(
current_user: User = Depends(get_current_user),
) -> CloudProjectResponse:
project = cloud_project_service.get(db, project_id, current_user.id)
return CloudProjectResponse.model_validate(project)


@router.get(
"/{project_id}/provider-credential",
response_model=CloudProjectProviderCredentialResponse,
)
def get_cloud_project_provider_credential(
project_id: int,
response: Response,
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> CloudProjectProviderCredentialResponse:
response.headers["Cache-Control"] = "no-store"
token = cloud_project_service.get_provider_credential(
db, project_id, current_user.id
)
return CloudProjectProviderCredentialResponse(token=token)
return _project_response(db, project, current_user)


@router.patch("/{project_id}", response_model=CloudProjectResponse)
Expand All @@ -95,7 +91,7 @@ def update_cloud_project(
current_user: User = Depends(get_current_user),
) -> CloudProjectResponse:
project = cloud_project_service.update(db, project_id, current_user.id, values)
return CloudProjectResponse.model_validate(project)
return _project_response(db, project, current_user)


@router.post(
Expand Down
117 changes: 108 additions & 9 deletions backend/app/api/endpoints/deliveries.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,16 @@

"""Authenticated project TODO and delivery endpoints."""

from fastapi import APIRouter, Depends, File, Form, Query, UploadFile, status
from fastapi import (
APIRouter,
Depends,
File,
Form,
HTTPException,
Query,
UploadFile,
status,
)
from sqlalchemy.orm import Session

from app.api.dependencies import get_db
Expand All @@ -23,6 +32,8 @@
LoopItemAttachmentResponse,
LoopItemCollaboratorCreate,
LoopItemCollaboratorResponse,
LoopItemCommentCreate,
LoopItemCommentResponse,
LoopItemCreate,
LoopItemListResponse,
LoopItemReorder,
Expand All @@ -33,12 +44,22 @@
MyWorkItemResponse,
MyWorkListResponse,
)
from app.services.cloud_projects import cloud_project_service
from app.services.delivery import delivery_service
from app.services.loop_items import loop_item_service
from app.services.loop_items.external_provider import external_loop_item_provider

router = APIRouter()


def _loop_item_response(
db: Session, item: object, current_user: User
) -> LoopItemResponse:
return LoopItemResponse.model_validate(
loop_item_service.response_values(db, item, current_user.id)
)


def _delivery_response(db: Session, delivery: Delivery) -> DeliveryResponse:
return DeliveryResponse.model_validate(
{
Expand Down Expand Up @@ -117,7 +138,7 @@ def find_runtime_task_loop_item(
item = loop_item_service.find_for_runtime_task(
db, current_user.id, device_id, task_id
)
return LoopItemResponse.model_validate(item)
return _loop_item_response(db, item, current_user)


@router.get("/runtime-tasks/cloud-context", response_model=CloudTaskContextResponse)
Expand All @@ -133,8 +154,19 @@ def find_runtime_task_cloud_context(
return CloudTaskContextResponse.model_validate(
{
**binding.__dict__,
"project": project,
"loop_item": item,
"project": {
**project.__dict__,
"current_user_id": current_user.id,
"current_user_name": current_user.user_name,
"access_role": cloud_project_service.access(
db, project.id, current_user.id
).role,
},
"loop_item": (
loop_item_service.response_values(db, item, current_user.id)
if item is not None
else None
),
}
)

Expand Down Expand Up @@ -174,9 +206,27 @@ def list_loop_items(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemListResponse:
project = cloud_project_service.get(db, project_id, current_user.id)
if project.task_provider in {"github", "gitlab"}:
return LoopItemListResponse(
items=[
LoopItemResponse.model_validate(item)
for item in external_loop_item_provider.list(
db, project_id, current_user.id
)
]
)
items = loop_item_service.list(db, project_id, current_user.id)
access = cloud_project_service.access(db, project_id, current_user.id)
return LoopItemListResponse(
items=[LoopItemResponse.model_validate(item) for item in items]
items=[
LoopItemResponse.model_validate(
loop_item_service.response_values(
db, item, current_user.id, access=access
)
)
for item in items
]
)


Expand All @@ -191,8 +241,15 @@ def create_loop_item(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemResponse:
project = cloud_project_service.get(db, project_id, current_user.id)
if project.task_provider in {"github", "gitlab"}:
return LoopItemResponse.model_validate(
external_loop_item_provider.create(
db, project_id, current_user.id, current_user.user_name, values
)
)
item = loop_item_service.create(db, project_id, current_user.id, values)
return LoopItemResponse.model_validate(item)
return _loop_item_response(db, item, current_user)


@router.post(
Expand All @@ -205,9 +262,19 @@ def reorder_loop_items(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemListResponse:
project = cloud_project_service.get(db, project_id, current_user.id)
if project.task_provider in {"github", "gitlab"}:
return LoopItemListResponse(
items=[
LoopItemResponse.model_validate(item)
for item in external_loop_item_provider.list(
db, project_id, current_user.id
)
]
)
Comment on lines +265 to +274

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject or implement external reorder requests.

Lines 265-274 ignore values and only reload the external items, so the endpoint returns success while leaving GitHub/GitLab ordering unchanged. Implement provider-specific ordering or return a clear 4xx response and prevent the caller from offering this action.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/endpoints/deliveries.py` around lines 265 - 274, Update the
external-provider branch in the delivery reorder endpoint to avoid silently
ignoring the submitted values: either implement GitHub/GitLab ordering through
the provider or reject the request with a clear 4xx response. If rejecting,
ensure the corresponding caller/UI does not offer reorder for projects whose
task_provider is "github" or "gitlab".

items = loop_item_service.reorder(db, project_id, current_user.id, values)
return LoopItemListResponse(
items=[LoopItemResponse.model_validate(item) for item in items]
items=[_loop_item_response(db, item, current_user) for item in items]
)


Expand All @@ -217,8 +284,12 @@ def get_loop_item(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemResponse:
if external_loop_item_provider.is_external_item(db, item_id):
return LoopItemResponse.model_validate(
external_loop_item_provider.get(db, item_id, current_user.id)
)
item = loop_item_service.get(db, item_id, current_user.id)
return LoopItemResponse.model_validate(item)
return _loop_item_response(db, item, current_user)


@router.patch("/loop-items/{item_id}", response_model=LoopItemResponse)
Expand All @@ -228,8 +299,30 @@ def update_loop_item(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemResponse:
if external_loop_item_provider.is_external_item(db, item_id):
return LoopItemResponse.model_validate(
external_loop_item_provider.update(db, item_id, current_user.id, values)
)
item = loop_item_service.update(db, item_id, current_user.id, values)
return LoopItemResponse.model_validate(item)
return _loop_item_response(db, item, current_user)


@router.post(
"/loop-items/{item_id}/comments",
response_model=LoopItemCommentResponse,
status_code=status.HTTP_201_CREATED,
)
def add_loop_item_comment(
item_id: str,
values: LoopItemCommentCreate,
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemCommentResponse:
return LoopItemCommentResponse.model_validate(
external_loop_item_provider.add_comment(
db, item_id, current_user.id, values.body
)
)


@router.get(
Expand All @@ -241,6 +334,7 @@ def list_loop_item_attachments(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> list[LoopItemAttachmentResponse]:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make ensure_shadow atomic before using it from these concurrent endpoints.

The helper checks for a shadow and then inserts it in a separate step. Concurrent requests for the same external item can therefore collide on the primary key and return 500.

  • backend/app/api/endpoints/deliveries.py#L337-L337: make shadow creation conflict-safe for attachment listing.
  • backend/app/api/endpoints/deliveries.py#L353-L353: make shadow creation conflict-safe for attachment creation.
  • backend/app/api/endpoints/deliveries.py#L400-L400: make shadow creation conflict-safe for task listing.
  • backend/app/api/endpoints/deliveries.py#L415-L415: make shadow creation conflict-safe for task unbinding.
  • backend/app/api/endpoints/deliveries.py#L430-L430: make shadow creation conflict-safe for task binding.
  • backend/app/api/endpoints/deliveries.py#L446-L446: make shadow creation conflict-safe before delivery creation.
📍 Affects 1 file
  • backend/app/api/endpoints/deliveries.py#L337-L337 (this comment)
  • backend/app/api/endpoints/deliveries.py#L353-L353
  • backend/app/api/endpoints/deliveries.py#L400-L400
  • backend/app/api/endpoints/deliveries.py#L415-L415
  • backend/app/api/endpoints/deliveries.py#L430-L430
  • backend/app/api/endpoints/deliveries.py#L446-L446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/api/endpoints/deliveries.py` at line 337, Make
external_loop_item_provider.ensure_shadow atomic and conflict-safe so concurrent
requests cannot fail on duplicate shadow insertion, preserving its existing
behavior when the shadow already exists. Apply the helper fix to all six call
sites in backend/app/api/endpoints/deliveries.py: lines 337, 353, 400, 415, 430,
and 446; each site requires conflict-safe shadow creation for its respective
endpoint.

attachments = loop_item_service.list_attachments(db, item_id, current_user.id)
return [LoopItemAttachmentResponse.model_validate(item) for item in attachments]

Expand All @@ -256,6 +350,7 @@ def add_loop_item_attachment(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemAttachmentResponse:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)
attachment = loop_item_service.add_attachment(
db,
item_id,
Expand Down Expand Up @@ -302,6 +397,7 @@ def list_loop_item_tasks(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> list[LoopItemTaskBindingResponse]:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)
bindings = loop_item_service.list_task_bindings(db, item_id, current_user.id)
return [LoopItemTaskBindingResponse.model_validate(binding) for binding in bindings]

Expand All @@ -316,6 +412,7 @@ def unbind_loop_item_task(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> None:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)
loop_item_service.unbind_task(db, item_id, values, current_user.id)


Expand All @@ -330,6 +427,7 @@ def bind_loop_item_task(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> LoopItemTaskBindingResponse:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)
binding = loop_item_service.bind_task(db, item_id, values, current_user.id)
return LoopItemTaskBindingResponse.model_validate(binding)

Expand All @@ -345,6 +443,7 @@ def create_delivery(
db: Session = Depends(get_db),
current_user: User = Depends(get_current_user),
) -> DeliveryResponse:
external_loop_item_provider.ensure_shadow(db, item_id, current_user.id)
delivery = delivery_service.create_delivery(db, item_id, current_user.id, values)
return _delivery_response(db, delivery)

Expand Down
7 changes: 7 additions & 0 deletions backend/app/models/delivery.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,13 @@ class LoopNode(Base):
class CloudProject(LoopNode):
__mapper_args__ = {"polymorphic_identity": "project"}

@property
def visibility(self) -> str:
metadata = self.metadata_json
if not isinstance(metadata, dict):
return "private"
return "public" if metadata.get("visibility") == "public" else "private"

@property
def tags(self) -> list[str]:
"""Project-level tag registry stored inside the metadata JSON column."""
Expand Down
Loading
Loading