Skip to content

test: add publisher field - #137

Closed
vikram-dagger wants to merge 1 commit into
mainfrom
update-api-publisher
Closed

test: add publisher field#137
vikram-dagger wants to merge 1 commit into
mainfrom
update-api-publisher

Conversation

@vikram-dagger

@vikram-dagger vikram-dagger commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Greptile Summary

The PR adds a required publisher column to the SQLAlchemy Book model.

  • Adds publisher as a non-null String(255) database field.
  • Does not update creation schemas, persistence calls, or existing database schemas.

Confidence Score: 3/5

This PR is not safe to merge until book creation supplies publisher and existing databases receive a schema migration.

The required column makes current create requests fail on fresh schemas, while deployments with an existing books table will query a column that create_all does not add.

Files Needing Attention: models.py

Important Files Changed

Filename Overview
models.py Adds a required publisher column, but current inserts omit it and existing databases receive no corresponding migration.

Reviews (1): Last reviewed commit: "test: add publisher field" | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Comment thread models.py
id: Mapped[int] = mapped_column(primary_key=True, index=True)
title: Mapped[str] = mapped_column(String(255), index=True)
author: Mapped[str] = mapped_column(String(255))
publisher: Mapped[str] = mapped_column(String(255), nullable=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Required publisher breaks inserts

When any book is created, BookIn and the repository constructor supply only title and author, so the required publisher column receives no value and the request fails with an integrity error returned as HTTP 400.

Comment thread models.py
id: Mapped[int] = mapped_column(primary_key=True, index=True)
title: Mapped[str] = mapped_column(String(255), index=True)
author: Mapped[str] = mapped_column(String(255))
publisher: Mapped[str] = mapped_column(String(255), nullable=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Publisher column lacks migration

If a deployment already has a books table from an earlier release, create_all does not add the new publisher column, so subsequent ORM reads and writes reference a nonexistent column and fail.

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