Skip to content

fix: automated fixes for PR #24#36

Closed
vikram-blaxel wants to merge 2 commits into
mainfrom
fix/pr-24-60b9d331
Closed

fix: automated fixes for PR #24#36
vikram-blaxel wants to merge 2 commits into
mainfrom
fix/pr-24-60b9d331

Conversation

@vikram-blaxel
Copy link
Copy Markdown
Owner

Automated fixes for PR #24.

All 18 tests pass and all changes are staged. Here is the structured summary:


Security Fixes

  • models.py — Removed redundant nullable=False from isbn (non-optional Mapped[str] already implies NOT NULL); added unique=True to enforce ISBN uniqueness at the database level; changed String(255) to String(17) (max valid ISBN-with-hyphens length).
  • routers.py — Replaced bare except Exception as e: raise HTTPException(detail=str(e)) in create_book with except IntegrityError that returns a generic error message (no raw DB internals leaked). Added from None / from e exception chaining to all handlers. Removed the from typing import List import (used built-in list[...]).
  • Dockerfile — Added RUN pip install --upgrade pip wheel to pick up security fixes in those build tools. Added RUN useradd -m appuser + USER appuser so the container process no longer runs as root. Changed --log-level trace--log-level info to prevent sensitive payloads/traces from being written to stdout in production.
  • docker-compose.yml — Removed the ports: "5432:5432" mapping from the db service so the PostgreSQL port is no longer exposed to the host; the web service reaches it via the internal Docker network.

Code Quality Fixes

  • models.py — Added isbn: str to both BookIn and BookOut Pydantic schemas so the field is part of the API contract.
  • repositories.py — Updated create_book to pass isbn=book.isbn when constructing Book; updated update_book to assign db_book.isbn = book.isbn; added docstrings to all functions.
  • db.sql — Added isbn VARCHAR(17) UNIQUE NOT NULL to the CREATE TABLE statement so fresh deployments match the ORM model.
  • conftest.py — Removed unused text import (was flagged as F401). Renamed TestingSessionLocal to testing_session_local (PEP 8 snake_case for variables). Fixed the test_app fixture to use a single shared connection/transaction with savepoint rollback, giving the HTTP client proper cross-request visibility while still cleaning up after each test.
  • test_main.py — Removed unused create_engine import (F401). Restored assert len(books) >= 2 in test_get_books (now safe with proper rollback isolation).

Test Additions

  • test_main.py — new TestBookRouter class (10 tests):
    • test_create_book_http — POST /api/books/ returns 201 with all fields including isbn.
    • test_create_book_invalid_payload — Missing required fields returns 422.
    • test_get_books_http — GET /api/books/ returns a list.
    • test_get_books_paginationskip and limit query parameters are respected.
    • test_get_book_http — GET /api/books/{id} returns the correct book.
    • test_get_book_not_found — Returns 404 for unknown ID.
    • test_update_book_http — PUT /api/books/{id} updates title, author, and isbn.
    • test_update_book_not_found — Returns 404 for unknown ID.
    • test_delete_book_http — DELETE /api/books/{id} removes the book (confirmed with follow-up GET → 404).
    • test_delete_book_not_found — Returns 404 for unknown ID.
  • All existing tests updated to include isbn in TEST_BOOKS, inline BookIn(...) calls, and assert checks on the isbn field.

Skipped Items

  • Authentication / authorisation on routes — Adding API-key or OAuth2/JWT middleware would require significant architectural decisions (key management, user model, token issuance) that go beyond a code-fix scope and would require new dependencies. Flagged for a dedicated follow-up PR.
  • Pydantic @field_validator for ISBN format — Noted as informational. A strict ISBN-10/13 validator is a useful hardening step but was not added here to avoid breaking tests that use numeric-only mock ISBNs that aren't strictly valid (e.g. 9780000000001). Recommended as a follow-up.
  • Alembic migration — Adding a full Alembic migration setup would introduce a new dependency and directory structure. The db.sql DDL file and SQLAlchemy Base.metadata.create_all already handle fresh deployments correctly; a migration script for existing live databases is recommended as a separate operational task.
  • conftest.py separate test database URL — The tests run against whatever DATABASE_URL is set in the environment (matching CI behaviour). A fully isolated test-only database URL config was not added to avoid breaking the CI workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant