Skip to content

review-fix: automated review of PR #24#27

Closed
vikram-blaxel wants to merge 2 commits into
mainfrom
review-fix/pr-24-aa85ac57
Closed

review-fix: automated review of PR #24#27
vikram-blaxel wants to merge 2 commits into
mainfrom
review-fix/pr-24-aa85ac57

Conversation

@vikram-blaxel
Copy link
Copy Markdown
Owner

Automated review fixes for PR #24.

All 10 files are staged. Here is the structured summary:


Security Fixes

  1. models.py — ISBN field schema mismatch (Critical): Added isbn: str with Field(pattern=...) regex validation to BookIn and added isbn: str to BookOut, closing the NOT NULL / missing-field IntegrityError that crashed every book creation.
  2. routers.py — Raw exception leakage (Warning): Replaced detail=str(e) with generic messages ("An error occurred creating the book" / "An error occurred retrieving books"); added logger.exception(...) for server-side logging; used raise ... from e for proper exception chaining.
  3. routers.py — Broad except Exception (Warning): Narrowed both except Exception clauses to except SQLAlchemyError.
  4. routers.py — Unbounded limit parameter (Warning): Added Query(default=10, ge=1, le=100) to cap the GET /books/ limit parameter at 100.
  5. Dockerfile — Container runs as root with trace logging (Warning): Added RUN useradd -m appuser + USER appuser before CMD; changed --log-level trace to --log-level warning.
  6. docker-compose.yml — PostgreSQL exposed to all interfaces (Warning): Changed port binding from "5432:5432" to "127.0.0.1:5432:5432".
  7. dependencies.pyprint() for errors (Informational): Replaced print(f"Error initializing...") with logger.error(...).
  8. models.pyString(255) for ISBN (Informational/Warning): Reduced to String(20) to match the max ISBN-13-with-hyphens length and added unique=True.

Code Quality Fixes

  1. repositories.pycreate_book missing isbn (Critical): Updated Book(title=..., author=...)Book(title=..., author=..., isbn=book.isbn).
  2. repositories.pyupdate_book missing isbn (Critical): Added db_book.isbn = book.isbn to the update block.
  3. db.sql — Missing isbn column (Critical): Added isbn VARCHAR(20) UNIQUE NOT NULL; also removed the erroneous UNIQUE constraints from title and author.
  4. main.py — Redundant dependencies=[Depends(get_db)] (Warning): Removed the double session-open from app.include_router(...).
  5. conftest.py — Unused imports (Warning/Informational): Removed inspect and text from from sqlalchemy import ...; renamed TestingSessionLocal to testing_session_local (snake_case).
  6. models.py — Unnecessary pass in Base (Informational): Removed the redundant pass statement.

Test Additions

  1. test_main.pyTEST_BOOKS updated (Critical): Added "isbn" key to both entries so BookIn(**TEST_BOOKS[n]) no longer raises ValidationError.
  2. test_main.py — ISBN assertions added (Warning): Added assert book.isbn == ... in test_create_book and test_update_book.
  3. test_main.py — Restored commented assertion (Warning): Uncommented assert len(books) >= 2 in test_get_books.
  4. test_main.py — New TestBookRoutes class (Critical): Added 10 HTTP-layer integration tests covering: successful create/read/update/delete cycle, 422 for invalid ISBN, 400 on DB error during create, 500 on DB error during list, 404 for all three not-found paths, and the full CRUD happy-path end-to-end flow.
  5. Unused import create_engine removed from test_main.py.

Skipped Items

  1. requirements.txt — Unpinning dependencies (Warning): Not pinned to exact versions. Pinning requires knowing the exact installed versions in this environment and is a deployment/operations concern outside the scope of a code fix; pip-audit reported no known CVEs against the current versions.
  2. routers.py — No authentication/authorization (Warning): Adding OAuth2/JWT is a substantial feature that would require new infrastructure (token issuance, secret management) and would require modifying test intent. Skipped per "do not introduce new dependencies unless strictly necessary."
  3. main.py — Swagger/ReDoc UI exposed (Informational): Disabling /docs and /redoc in production is a deployment configuration decision; left enabled for developer usability.
  4. Dockerfile — Use exact digest image (Informational): python:3.11.9-slim pinning is a build-reproducibility concern; skipped to avoid potentially breaking the existing build pipeline.

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