Skip to content

fix: automated fixes for PR #24#34

Closed
vikram-blaxel wants to merge 2 commits into
mainfrom
fix/pr-24-786accc7
Closed

fix: automated fixes for PR #24#34
vikram-blaxel wants to merge 2 commits into
mainfrom
fix/pr-24-786accc7

Conversation

@vikram-blaxel
Copy link
Copy Markdown
Owner

Automated fixes for PR #24.

20/20 tests pass. All changes are staged. Here is the structured summary:


Security Fixes

  1. .gitignore — Added .env and *.db so the secrets file and local SQLite databases are no longer tracked by git. Also added .env.example as a safe template.

  2. .env — Added DATABASE_URL=sqlite:///./dev.db for local development (the CI workflow overrides this with a real PostgreSQL URL via env injection). The old .env never had this key, which caused all test runs outside CI to fail.

  3. models.py — ISBN format validation via @field_validator in BookIn: accepts only 10- or 13-digit strings (optionally hyphen-separated), rejecting arbitrary strings before they reach the database.

  4. routers.py — Sanitized error messages: create_book now catches IntegrityError separately (returns HTTP 409 with a generic message) and wraps the fallback Exception with a generic "Unable to create book." message; get_books now returns "Internal server error." rather than str(e), preventing schema/stack-trace leakage. All raise … from e chaining added (W0707).

  5. Dockerfile — Non-root user: Added RUN useradd -m appuser + USER appuser before CMD so the container process no longer runs as root. Changed --log-level trace--log-level info to stop verbose request/header logging in production.


Code Quality Fixes

  1. models.py

    • Added isbn: str to both BookIn and BookOut.
    • Narrowed isbn column to String(17) and added unique=True.
    • Removed unnecessary pass from Base class body (W0107).
    • Fixed blank-line spacing between Book class and # Pydantic models comment (PEP 8).
    • Added import re and Annotated (used by validator).
  2. repositories.py

    • create_book: now passes isbn=book.isbn when constructing models.Book.
    • update_book: now sets db_book.isbn = book.isbn alongside title and author.
  3. routers.py

    • Moved from typing import List to the top (PEP 8 / C0411).
    • Added from sqlalchemy.exc import IntegrityError for specific exception handling.
  4. db.sql — Updated CREATE TABLE books to include isbn VARCHAR(17) NOT NULL UNIQUE and corrected column lengths/constraints to match the ORM model.

  5. conftest.py — Removed unused inspect and text imports (F401). Replaced the old savepoint-based test_db/test_app fixtures (which leaked state under SQLite) with per-test fresh in-memory SQLite databases using StaticPool, providing true isolation without requiring a live PostgreSQL instance.


Test Additions

test_main.py — Complete overhaul:

  • Added isbn field to all TEST_BOOKS constants and all BookIn(...) call-sites.
  • Added assert book.isbn == expected_isbn assertions to test_create_book, test_get_book, and test_update_book.
  • Restored assert len(books) >= 2 in test_get_books (now safe due to per-test DB isolation).
  • Removed unused create_engine import (F401 ruff).
  • Added TestHTTPEndpoints class (12 new tests) covering every router endpoint:
    • test_create_book_http — POST 201, fields including isbn returned
    • test_get_books_http — GET 200 returns list
    • test_get_book_http — GET 200 includes isbn
    • test_get_book_not_found_http — GET 404
    • test_update_book_http — PUT 200 updates all fields including isbn
    • test_update_book_not_found_http — PUT 404
    • test_delete_book_http — DELETE 200, confirms deletion via subsequent GET 404
    • test_delete_book_not_found_http — DELETE 404
    • test_create_book_invalid_isbn_http — POST 422 for invalid ISBN format
    • test_create_book_missing_fields_http — POST 422 for missing required fields
    • test_get_books_pagination_http — GET with limit and skip boundary values
    • test_create_book_duplicate_isbn_http — POST 409 for duplicate ISBN

Skipped Items

  • Authentication/authorization on endpoints — Deferred. Adding OAuth2/JWT would require significant new infrastructure (auth middleware, token issuance, user model) and would likely break existing tests. This is a pre-existing issue flagged as a warning, not introduced by the current PR.
  • Pinning all dependencies to exact versions — Deferred. Pinning requires pip freeze output against a known-good environment and a lock-file strategy; doing this blind could introduce incompatibilities. The current packages have no known CVEs per pip-audit.
  • requirements.txt pinning — Same reason as above; no new vulnerabilities introduced.

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