Complete mongo db integration for delete /books/{id} (#9)

-Refactored the DELETE endpoint to use a MongoDB helper for atomic deletes
via `find_one_and_update`. Improved error handling to return 400 on invalid
ObjectId and 404 for missing resources.
-Added comprehensive tests for happy path, idempotency, non-existent records,
and invalid ID formats. 
-Updated OpenAPI spec and ensured 100% Pylint coverage.

---------

Co-authored-by: Copilot <[email protected]>

Author: codesungrape

.gitignore

@@ -14,4 +14,5 @@ htmlcov/
 
 
 # VSextension
-HTTP_scripts/
+HTTP_scripts/
+HTTP_request_examples

app/datastore/mongo_helper.py

@@ -1,5 +1,6 @@
 """Module containing pymongo helper functions."""
 
+from bson.objectid import ObjectId
 from pymongo.cursor import Cursor
 
 
@@ -62,3 +63,21 @@ def find_books(collection, query_filter=None, projection=None, limit=None) -> Cu
     if limit is not None and limit > 0:
         cursor = cursor.limit(limit)
     return cursor
+
+
+def delete_book_by_id(book_collection, book_id):
+    """
+    Soft delete book with given id
+    Returns: The original document if found and updated, otherwise None.
+    """
+    # Convert string ID to ObjectId
+    object_id_to_update = ObjectId(book_id)
+
+    # UPDATE operation
+    update_operation = {"$set": {"state": "deleted"}}
+
+    filter_query = {"_id": object_id_to_update, "state": {"$ne": "deleted"}}
+
+    result = book_collection.find_one_and_update(filter_query, update_operation)
+
+    return result

app/routes.py

@@ -2,13 +2,13 @@
 
 import copy
 
-from bson.objectid import ObjectId
+from bson.objectid import InvalidId, ObjectId
 from flask import jsonify, request
 from pymongo.errors import ConnectionFailure
 from werkzeug.exceptions import HTTPException, NotFound
 
 from app.datastore.mongo_db import get_book_collection
-from app.datastore.mongo_helper import insert_book_to_mongo
+from app.datastore.mongo_helper import delete_book_by_id, insert_book_to_mongo
 from app.services.book_service import fetch_active_books, format_books_for_api
 from app.utils.api_security import require_api_key
 from app.utils.helper import append_hostname
@@ -179,14 +179,19 @@ def delete_book(book_id):
         """
         Soft delete a book by setting its state to 'deleted' or return error if not found.
         """
-        if not books:
-            return jsonify({"error": "Book collection not initialized"}), 500
+        try:
+            book_collection = get_book_collection()
+            if book_collection is None:
+                return jsonify({"error": "Book collection not initialized"}), 500
 
-        for book in books:
-            if book.get("id") == book_id:
-                book["state"] = "deleted"
-                return "", 204
-        return jsonify({"error": "Book not found"}), 404
+            delete_result = delete_book_by_id(book_collection, book_id)
+
+            if delete_result is None:
+                return jsonify({"error": "Book not found"}), 404
+
+            return "", 204
+        except InvalidId:
+            return jsonify({"error": "Invalid Book ID format"}), 400
 
     # ----------- PUT section ------------------
 

openapi.yml

@@ -350,7 +350,7 @@ paths:
       summary: Delete a book by ID
       security:
         - ApiKeyAuth: []
-      description: Soft deletes a book by setting its state to 'deleted'.
+      description: Soft deletes a book by setting its state to 'deleted'. This operation is idempotent.
       operationId: deleteBookById
       parameters:
         - name: book_id
@@ -365,9 +365,24 @@ paths:
         '204':
           description: Book deleted successfully.
           content: {}
+        '400':
+          description: Bad Request. The provided book_id is not in a valid 24-character hexadecimal format.
+          content:
+            application/json:
+              schema:
+                # You have a great SimpleError component we can reuse!
+                $ref: '#/components/schemas/SimpleError'
+              example:
+                error: "Invalid Book ID format"
         '401':
           $ref: '#/components/responses/Unauthorized' 
         '404':
-          $ref: '#/components/responses/NotFound' 
+          description: A book with the specified ID was not found or was already deleted.
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/SimpleError'
+              example:
+                error: "Book not found"
         '500':
           $ref: '#/components/responses/InternalServerError' 

tests/test_api_security.py

@@ -196,12 +196,23 @@ def test_delete_book_fails_with_invalid_api_key(client):
 
 def test_delete_book_succeeds_with_valid_api_key(client, monkeypatch):
     """
-    WHEN a DELETE request is made with a valid API key
-    THEN it should return 200 OK (or appropriate response)
+    GIVEN a valid API key
+    WHEN a DELETE request is made to a valid book ID
+    THEN the response should be 204 No Content
     """
-    monkeypatch.setattr("app.routes.books", [{"id": "some-id"}])
+    # Define what a successful result from the DB helper is
+    successful_db_result = {"_id": "some-id", "state": "active"}
+
+    monkeypatch.setattr(
+        "app.routes.delete_book_by_id", lambda collection, book_id: successful_db_result
+    )
+
+    monkeypatch.setattr("app.routes.get_book_collection", lambda: "a fake collection")
+
+    # Act
+    valid_oid_string = "635c02a7a5f6e1e2b3f4d5e6"
+    response = client.delete(f"/books/{valid_oid_string}", headers=HEADERS["VALID"])
 
-    response = client.delete("/books/some-id", headers=HEADERS["VALID"])
     assert response.status_code == 204
 
 

tests/test_app.py

@@ -428,7 +428,9 @@ def test_get_book_returns_specified_book(
         assert response_data["author"] == "Kent Beck"
 
 
-def test_get_book_with_invalid_id_format_returns_400(client, db_setup):  # pylint: disable=unused-argument
+def test_get_book_with_invalid_id_format_returns_400(
+    client, db_setup
+):  # pylint: disable=unused-argument
     # Arrange
     # an ID that is clearly not a valid MongoDB ObjectId
     invalid_book_id = "this-is-not-a-valid-id"
@@ -444,6 +446,7 @@ def test_get_book_with_invalid_id_format_returns_400(client, db_setup):  # pylin
     expected_error = {"error": "Invalid book ID format"}
     assert response.get_json() == expected_error
 
+
 def test_get_book_not_found_returns_404(client, monkeypatch):
     """
     WHEN given a well-formed but non-existent ObjectId,
@@ -508,18 +511,35 @@ def test_invalid_urls_return_404(client):
 
 # ------------------------ Tests for DELETE --------------------------------------------
 
+VALID_OID_STRING = "635c02a7a5f6e1e2b3f4d5e6"
+
 
 def test_book_is_soft_deleted_on_delete_request(client):
-    with patch("app.routes.books", books_database):
-        # Send DELETE request with valid API header
-        book_id = "1"
-        headers = {"X-API-KEY": "test-key-123"}
-        response = client.delete(f"/books/{book_id}", headers=headers)
+    """
+    GIVEN a valid book ID and API key
+    WHEN a DELETE request is made
+    THEN the view function should call the database helper correctly and return 204.
+
+    This test verifies the integration between the Flask route and the data layer.
+    """
+    with patch("app.routes.delete_book_by_id") as mock_delete_helper:
+        # Arrange
+        # Configure the mock to simulate a successful deletion
+        mock_delete_helper.return_value = {"_id": VALID_OID_STRING}
+
+        # Mock get_book_collection to avoid a real DB connection
+        with patch("app.routes.get_book_collection", return_value="fake_collection"):
+            # --- Act ---
+            # Send the DELETE request using a valid API header.
+            headers = {"X-API-KEY": "test-key-123"}
+            response = client.delete(f"/books/{VALID_OID_STRING}", headers=headers)
 
         assert response.status_code == 204
-        assert response.data == b""
-        # check that the book's state has changed to deleted
-        assert books_database[0]["state"] == "deleted"
+        mock_delete_helper.assert_called_once()
+        mock_delete_helper.assert_called_once_with(
+            "fake_collection",  # The (mocked) collection object
+            VALID_OID_STRING,  # The ID passed from the URL
+        )
 
 
 def test_delete_empty_book_id(client):
@@ -531,19 +551,51 @@ def test_delete_empty_book_id(client):
 
 
 def test_delete_invalid_book_id(client):
-    headers = {"X-API-KEY": "test-key-123"}
-    response = client.delete("/books/12341234", headers=headers)
-    assert response.status_code == 404
+    """
+    GIVEN a malformed book ID (not a valid ObjectId format)
+    WHEN a DELETE request is made
+    THEN the response should be 400 InvalidId Error.
+    """
+    invalid_id = "1234-this-is-not-a-valid-id"
+
+    # Mock get_book_collection to avoid a real DB connection
+    with patch("app.routes.get_book_collection", return_value="fake_collection"):
+        # --- Act ---
+        # Send the DELETE request using a valid API header.
+        headers = {"X-API-KEY": "test-key-123"}
+        response = client.delete(f"/books/{invalid_id}", headers=headers)
+
+    assert response.status_code == 400
     assert response.content_type == "application/json"
-    assert "Book not found" in response.get_json()["error"]
+    response_data = response.get_json()
+    assert "error" in response_data
+    assert "Invalid Book ID format" in response_data["error"]
 
 
 def test_book_database_is_initialized_for_delete_book_route(client):
-    with patch("app.routes.books", None):
+    with patch("app.routes.get_book_collection") as mock_get_collection:
+        mock_get_collection.return_value = None
+
         headers = {"X-API-KEY": "test-key-123"}
-        response = client.delete("/books/1", headers=headers)
+        response = client.delete(f"/books/{VALID_OID_STRING}", headers=headers)
+
         assert response.status_code == 500
-        assert "Book collection not initialized" in response.get_json()["error"]
+        response_data = response.get_json()
+        assert "error" in response_data
+        assert "Book collection not initialized" in response_data["error"]
+
+
+def test_returns_404_if_helper_function_result_is_none(client):
+    with patch("app.routes.delete_book_by_id") as mock_delete_book:
+        mock_delete_book.return_value = None
+
+        headers = {"X-API-KEY": "test-key-123"}
+        response = client.delete(f"/books/{VALID_OID_STRING}", headers=headers)
+
+        assert response.status_code == 404
+        response_data = response.get_json()
+        assert "error" in response_data
+        assert "Book not found" in response_data["error"]
 
 
 # ------------------------ Tests for PUT --------------------------------------------

tests/test_mongo_helper.py

@@ -2,7 +2,10 @@
 
 from unittest.mock import MagicMock
 
-from app.datastore.mongo_helper import find_books, insert_book_to_mongo
+from bson import ObjectId
+
+from app.datastore.mongo_helper import (delete_book_by_id, find_books,
+                                        insert_book_to_mongo)
 
 
 def test_insert_book_to_mongo_calls_insert_one():
@@ -77,3 +80,54 @@ def test_find_books_applies_limit_when_provided():
 
     # 3. Check that the function returned the FINAL cursor from the chain.
     assert result_cursor == mock_limited_cursor
+
+
+def test_delete_book_by_id_happy_path():
+    """Given a valid book_id, soft deletes the book"""
+    # Arrange
+    valid_id = ObjectId()
+    fake_book_id_str = str(valid_id)
+    fake_book_from_db = {
+        "_id": valid_id,
+        "title": "A Mocked Book",
+        "author": "The Mockist",
+        "synopsis": "A tale of fakes and stubs.",
+        "state": "active",
+    }
+
+    mock_collection = MagicMock()
+    mock_collection.find_one_and_update.return_value = fake_book_from_db
+
+    # Act
+    result = delete_book_by_id(mock_collection, fake_book_id_str)
+
+    # Assert
+    mock_collection.find_one_and_update.assert_called_once()
+    assert result == fake_book_from_db
+    # Was the method called with the EXACT right arguments?
+    expected_filter = {"_id": valid_id, "state": {"$ne": "deleted"}}
+    expected_update = {"$set": {"state": "deleted"}}
+    mock_collection.find_one_and_update.assert_called_once_with(
+        expected_filter, expected_update
+    )
+
+
+def test_soft_delete_already_deleted_book_returns_none():
+    # Arrange
+    valid_id = ObjectId()
+    fake_book_id_str = str(valid_id)
+
+    mock_collection = MagicMock()
+    mock_collection.find_one_and_update.return_value = None
+
+    # Act
+    result = delete_book_by_id(mock_collection, fake_book_id_str)
+
+    # Assert
+    assert result is None
+    mock_collection.find_one_and_update.assert_called_once()
+    expected_filter = {"_id": valid_id, "state": {"$ne": "deleted"}}
+    expected_update = {"$set": {"state": "deleted"}}
+    mock_collection.find_one_and_update.assert_called_with(
+        expected_filter, expected_update
+    )