delete object and atomic action remove fix: return 204

mahenzon · mahenzon · commit d9d5baa61241 · 2023-09-06T13:53:04.000+03:00
diff --git a/fastapi_jsonapi/api.py b/fastapi_jsonapi/api.py
@@ -212,35 +212,39 @@ def _register_get_resource_detail(self, path: str):
         )
 
     def _register_patch_resource_detail(self, path: str):
-        detail_response_example = {
+        update_response_example = {
             status.HTTP_200_OK: {"model": self.detail_response_schema},
         }
         self._router.add_api_route(
             # TODO: variable path param name (set default name on DetailView class)
             # TODO: trailing slash (optional)
             path=path + "/{obj_id}",
             tags=self._tags,
-            responses=detail_response_example | self.default_error_responses,
+            responses=update_response_example | self.default_error_responses,
             methods=["PATCH"],
             summary=f"Patch object `{self.type_}` by id",
             endpoint=self._create_patch_resource_detail_view(),
             name=self.get_endpoint_name("update", "detail"),
         )
 
     def _register_delete_resource_detail(self, path: str):
-        detail_response_example = {
-            status.HTTP_200_OK: {"model": self.detail_response_schema},
+        delete_response_example = {
+            status.HTTP_204_NO_CONTENT: {
+                "description": "If a server is able to delete the resource,"
+                " the server MUST return a result with no data",
+            },
         }
         self._router.add_api_route(
             # TODO: variable path param name (set default name on DetailView class)
             # TODO: trailing slash (optional)
             path=path + "/{obj_id}",
             tags=self._tags,
-            responses=detail_response_example | self.default_error_responses,
+            responses=delete_response_example | self.default_error_responses,
             methods=["DELETE"],
             summary=f"Delete object `{self.type_}` by id",
             endpoint=self._create_delete_resource_detail_view(),
             name=self.get_endpoint_name("delete", "detail"),
+            status_code=status.HTTP_204_NO_CONTENT,
         )
 
     def _create_pagination_query_params(self) -> List[Parameter]:
diff --git a/fastapi_jsonapi/atomic/atomic.py b/fastapi_jsonapi/atomic/atomic.py
@@ -3,7 +3,7 @@
     Type,
 )
 
-from fastapi import APIRouter, Request
+from fastapi import APIRouter, Request, Response, status
 
 from fastapi_jsonapi.atomic.atomic_handler import AtomicViewHandler
 from fastapi_jsonapi.atomic.schemas import (
@@ -33,7 +33,10 @@ async def view_atomic(
             request=request,
             operations_request=operations_request,
         )
-        return await atomic_handler.handle()
+        result = await atomic_handler.handle()
+        if result:
+            return result
+        return Response(status_code=status.HTTP_204_NO_CONTENT)
 
     def _register_view(self):
         self.router.add_api_route(
diff --git a/fastapi_jsonapi/atomic/atomic_handler.py b/fastapi_jsonapi/atomic/atomic_handler.py
@@ -54,7 +54,9 @@ async def prepare_operations(self) -> List[OperationBase]:
         prepared_operations: List[OperationBase] = []
 
         for operation in self.operations_request.operations:
-            jsonapi = RoutersJSONAPI.all_jsonapi_routers[operation.data.type]
+            operation_type = operation.ref and operation.ref.type or operation.data and operation.data.type
+            assert operation_type
+            jsonapi = RoutersJSONAPI.all_jsonapi_routers[operation_type]
 
             dependencies_result: Dict[str, Any] = await self.handle_view_dependencies(
                 jsonapi=jsonapi,
@@ -63,14 +65,15 @@ async def prepare_operations(self) -> List[OperationBase]:
                 action=operation.op,
                 request=self.request,
                 jsonapi=jsonapi,
+                ref=operation.ref,
                 data=operation.data,
                 data_layer_view_dependencies=dependencies_result,
             )
             prepared_operations.append(one_operation)
 
         return prepared_operations
 
-    async def handle(self) -> Union[AtomicResponseDict, AtomicResultResponse]:
+    async def handle(self) -> Union[AtomicResponseDict, AtomicResultResponse, None]:
         prepared_operations = await self.prepare_operations()
         results = []
 
@@ -84,9 +87,16 @@ async def handle(self) -> Union[AtomicResponseDict, AtomicResultResponse]:
             previous_dl = dl
             response = await operation.handle(dl=dl)
             # response.data.id
-            results.append({"data": response.data})
+            if response:
+                results.append({"data": response.data})
 
         if previous_dl:
             await previous_dl.atomic_end(success=success)
 
-        return {"atomic:results": results}
+        if results:
+            return {"atomic:results": results}
+
+        """
+        if all results are empty,
+        the server MAY respond with 204 No Content and no document.
+        """
diff --git a/fastapi_jsonapi/atomic/prepared_atomic_operation.py b/fastapi_jsonapi/atomic/prepared_atomic_operation.py
@@ -1,12 +1,12 @@
 from __future__ import annotations
 
 from dataclasses import dataclass
-from typing import TYPE_CHECKING, Any, Dict, Type
+from typing import TYPE_CHECKING, Any, Dict, Optional, Type
 
 from fastapi import Request
 
 from fastapi_jsonapi import RoutersJSONAPI
-from fastapi_jsonapi.atomic.schemas import AtomicOperationAction, OperationDataType
+from fastapi_jsonapi.atomic.schemas import AtomicOperationAction, AtomicOperationRef, OperationDataType
 
 if TYPE_CHECKING:
     from fastapi_jsonapi.data_layers.base import BaseDataLayer
@@ -19,6 +19,7 @@
 class OperationBase:
     jsonapi: RoutersJSONAPI
     view: ViewBase
+    ref: Optional[AtomicOperationRef]
     data: OperationDataType
     data_layer_view_dependencies: Dict[str, Any]
 
@@ -28,6 +29,7 @@ def prepare(
         action: str,
         request: Request,
         jsonapi: RoutersJSONAPI,
+        ref: Optional[AtomicOperationRef],
         data: OperationDataType,
         data_layer_view_dependencies: Dict[str, Any],
     ) -> "OperationBase":
@@ -49,6 +51,7 @@ def prepare(
         return operation_cls(
             jsonapi=jsonapi,
             view=view,
+            ref=ref,
             data=data,
             data_layer_view_dependencies=data_layer_view_dependencies,
         )
@@ -81,9 +84,10 @@ async def handle(self, dl: BaseDataLayer):
 class OperationUpdate(DetailOperationBase):
     async def handle(self, dl: BaseDataLayer):
         data_in = self.jsonapi.schema_in_patch(data=self.data)
+        obj_id = self.ref and self.ref.id or self.data and self.data.id
         response = await self.view.process_update_object(
             dl=dl,
-            obj_id=data_in.data.id,
+            obj_id=obj_id,
             data_update=data_in.data,
         )
         return response
@@ -93,9 +97,19 @@ class OperationRemove(DetailOperationBase):
     async def handle(
         self,
         dl: BaseDataLayer,
-    ):
-        response = await self.view.process_delete_object(
+    ) -> None:
+        """
+        Todo: fix atomic delete
+         Deleting Resources
+           An operation that deletes a resource
+           MUST target that resource
+           through the operation’s ref or href members,
+           but not both.
+
+        :param dl:
+        :return:
+        """
+        await self.view.process_delete_object(
             dl=dl,
-            obj_id=self.data.id,
+            obj_id=self.ref and self.ref.id,
         )
-        return response
diff --git a/fastapi_jsonapi/atomic/schemas.py b/fastapi_jsonapi/atomic/schemas.py
@@ -121,8 +121,8 @@ class AtomicOperation(BaseModel):
         description="a meta object that contains non-standard meta-information about the operation",
     )
 
-    @root_validator
-    def validate_operation(cls, values: dict):
+    @classmethod
+    def _validate_one_of_ref_or_href(cls, values: dict):
         """
         An operation object MAY contain either of the following members,
         but not both, to specify the target of the operation: (ref, href)
@@ -148,6 +148,32 @@ def validate_operation(cls, values: dict):
         # TODO: pydantic V2
         raise ValueError(msg)
 
+    @root_validator
+    def validate_operation(cls, values: dict):
+        """
+        :param values:
+        :return:
+        """
+        cls._validate_one_of_ref_or_href(values=values)
+        op = values.get("op")
+        ref: Optional[AtomicOperationRef] = values.get("ref")
+        if op == AtomicOperationAction.remove:
+            if not ref:
+                msg = f"ref should be present for action {op.value!r}"
+                raise ValueError(msg)
+            # when updating / removing item, ref [l]id has to be present
+            if not (ref.id or ref.lid):
+                msg = f"id or local id has to be present for action {op.value!r}"
+                raise ValueError(msg)
+
+        data: OperationDataType = values.get("data")
+        operation_type = ref and ref.type or data and data.type
+        if not operation_type:
+            msg = "Operation has to be in ref or in data"
+            raise ValueError(msg)
+
+        return values
+
 
 class AtomicOperationRequest(BaseModel):
     operations: List[AtomicOperation] = Field(
diff --git a/fastapi_jsonapi/views/detail_view.py b/fastapi_jsonapi/views/detail_view.py
@@ -73,18 +73,16 @@ async def handle_delete_resource(
         self,
         obj_id: str,
         **extra_view_deps,
-    ):
+    ) -> None:
         dl: "BaseDataLayer" = await self.get_data_layer(extra_view_deps)
-        return await self.process_delete_object(dl=dl, obj_id=obj_id)
+        await self.process_delete_object(dl=dl, obj_id=obj_id)
 
     async def process_delete_object(
         self,
         dl: "BaseDataLayer",
         obj_id: str,
-    ):
+    ) -> None:
         view_kwargs = {dl.url_id_field: obj_id}
         db_object = await dl.get_object(view_kwargs=view_kwargs, qs=self.query_params)
 
         await dl.delete_object(db_object, view_kwargs)
-
-        return self._build_detail_response(db_object)
diff --git a/tests/schemas.py b/tests/schemas.py
@@ -70,7 +70,7 @@ class Config:
 # User Bio Schemas ⬇️
 
 
-class UserBioBaseSchema(BaseModel):
+class UserBioAttributesBaseSchema(BaseModel):
     """UserBio base schema."""
 
     class Config:
@@ -83,7 +83,7 @@ class Config:
     keys_to_ids_list: Dict[str, List[int]] = None
 
 
-class UserBioSchema(UserBioBaseSchema):
+class UserBioSchema(UserBioAttributesBaseSchema):
     """UserBio item schema."""
 
     id: int
diff --git a/tests/test_api/test_api_sqla_with_includes.py b/tests/test_api/test_api_sqla_with_includes.py
@@ -32,7 +32,7 @@
     PostCommentAttributesBaseSchema,
     SelfRelationshipSchema,
     UserAttributesBaseSchema,
-    UserBioBaseSchema,
+    UserBioAttributesBaseSchema,
     UserInSchemaAllowIdOnPost,
     UserPatchSchema,
     UserSchema,
@@ -689,7 +689,7 @@ async def test_create_object_with_relationship_and_fetch_include(
     ):
         create_user_bio_body = {
             "data": {
-                "attributes": UserBioBaseSchema(
+                "attributes": UserBioAttributesBaseSchema(
                     birth_city=fake.word(),
                     favourite_movies=fake.sentence(),
                     keys_to_ids_list={"foobar": [1, 2, 3], "spameggs": [2, 3, 4]},
@@ -1289,7 +1289,7 @@ async def test_fail_to_bind_relationship_with_constraint(
         patch_user_bio_body = {
             "data": {
                 "id": user_1_bio.id,
-                "attributes": UserBioBaseSchema.from_orm(user_1_bio).dict(),
+                "attributes": UserBioAttributesBaseSchema.from_orm(user_1_bio).dict(),
                 "relationships": {
                     "user": {
                         "data": {
@@ -1579,16 +1579,8 @@ async def test_delete_object_and_fetch_404(
     ):
         url = app.url_path_for("get_user_detail", obj_id=user_1.id)
         res = await client.delete(url)
-        assert res.status_code == status.HTTP_200_OK, res.text
-        assert res.json() == {
-            "data": {
-                "attributes": UserAttributesBaseSchema.from_orm(user_1),
-                "id": str(user_1.id),
-                "type": "user",
-            },
-            "jsonapi": {"version": "1.0"},
-            "meta": None,
-        }
+        assert res.status_code == status.HTTP_204_NO_CONTENT, res.text
+        assert res.content == b""
 
         res = await client.get(url)
         assert res.status_code == status.HTTP_404_NOT_FOUND, res.text
diff --git a/tests/test_atomic/test_create_objects.py b/tests/test_atomic/test_create_objects.py
@@ -10,7 +10,7 @@
 
 from tests.misc.utils import fake
 from tests.models import User, UserBio
-from tests.schemas import UserAttributesBaseSchema, UserBioBaseSchema
+from tests.schemas import UserAttributesBaseSchema, UserBioAttributesBaseSchema
 
 pytestmark = mark.asyncio
 
@@ -205,7 +205,7 @@ async def test_create_bio_with_relationship_to_user_to_one(
         async_session: AsyncSession,
         user_1: User,
     ):
-        user_bio = UserBioBaseSchema(
+        user_bio = UserBioAttributesBaseSchema(
             birth_city=fake.city(),
             favourite_movies=fake.sentence(),
         )
@@ -241,7 +241,7 @@ async def test_create_bio_with_relationship_to_user_to_one(
         result_bio_data = results[0]
         res: Result = await async_session.execute(stmt_bio)
         user_bio_created: UserBio = res.scalar_one()
-        assert user_bio == UserBioBaseSchema.from_orm(user_bio_created)
+        assert user_bio == UserBioAttributesBaseSchema.from_orm(user_bio_created)
         assert result_bio_data == {
             "data": {
                 "attributes": user_bio.dict(),
diff --git a/tests/test_atomic/test_delete_objects.py b/tests/test_atomic/test_delete_objects.py
diff --git a/tests/test_atomic/test_mixed_atomic.py b/tests/test_atomic/test_mixed_atomic.py
diff --git a/tests/test_atomic/test_update_objects.py b/tests/test_atomic/test_update_objects.py
