diff --git a/.gitignore b/.gitignore index 727b8ee..ba2d6f7 100644 --- a/.gitignore +++ b/.gitignore @@ -14,4 +14,5 @@ htmlcov/ # VSextension -HTTP_scripts/ \ No newline at end of file +HTTP_scripts/ +HTTP_request_examples \ No newline at end of file diff --git a/app/datastore/mongo_helper.py b/app/datastore/mongo_helper.py index e9be332..3fc57ec 100644 --- a/app/datastore/mongo_helper.py +++ b/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 diff --git a/app/routes.py b/app/routes.py index e56f6da..d9d24a8 100644 --- a/app/routes.py +++ b/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 ------------------ diff --git a/openapi.yml b/openapi.yml index 901fa91..a889cc8 100644 --- a/openapi.yml +++ b/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' diff --git a/tests/test_api_security.py b/tests/test_api_security.py index dee2a59..cceeca1 100644 --- a/tests/test_api_security.py +++ b/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 diff --git a/tests/test_app.py b/tests/test_app.py index b0160ad..ae78dc7 100644 --- a/tests/test_app.py +++ b/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 -------------------------------------------- diff --git a/tests/test_mongo_helper.py b/tests/test_mongo_helper.py index aafb96b..f546117 100644 --- a/tests/test_mongo_helper.py +++ b/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 + )