ldraney/pal-e-api
Archived
1
0
Fork 0
Code Issues 15 Pull requests 1 Projects Releases Packages Wiki Activity Actions

fix: add partial index declarations to models for alembic check #266

Open
forgejo_admin wants to merge 1 commit from 265-fix-add-partial-index-declarations-to-mo into main
pull from: 265-fix-add-partial-index-declarations-to-mo
merge into: ldraney:main
Conversation 2 Commits 1 Files changed 1 +13
forgejo_admin commented 2026-04-12 17:17:17 +00:00
Contributor
Copy link

Summary

Add missing partial index declarations to SQLAlchemy models so alembic check stops flagging them as needing removal. Model-only change — no new migration.

Changes

  • src/pal_e_docs/models.py — added text to sqlalchemy imports, added ix_blocks_block_type_mermaid partial index to Block __table_args__, added __table_args__ with ix_notes_note_type_architecture partial index to Note model

Test Plan

  • python -c "from pal_e_docs.models import Block, Note; print('OK')" imports cleanly
  • ruff check and ruff format pass
  • CI migration-test step (alembic check) should pass

Review Checklist

  • No new migration file created
  • Index names match existing DB indexes from migration u1p2q3r4s5t6
  • text imported from sqlalchemy
  • ruff check and format pass
  • Model imports cleanly

Related Notes

None — standalone CI fix.

Related

Closes #265

  • CI Pipeline #93 failure (migration-test step)
## Summary Add missing partial index declarations to SQLAlchemy models so `alembic check` stops flagging them as needing removal. Model-only change — no new migration. ## Changes - `src/pal_e_docs/models.py` — added `text` to sqlalchemy imports, added `ix_blocks_block_type_mermaid` partial index to Block `__table_args__`, added `__table_args__` with `ix_notes_note_type_architecture` partial index to Note model ## Test Plan - `python -c "from pal_e_docs.models import Block, Note; print('OK')"` imports cleanly - `ruff check` and `ruff format` pass - CI migration-test step (`alembic check`) should pass ## Review Checklist - [x] No new migration file created - [x] Index names match existing DB indexes from migration u1p2q3r4s5t6 - [x] `text` imported from sqlalchemy - [x] ruff check and format pass - [x] Model imports cleanly ## Related Notes None — standalone CI fix. ## Related Closes #265 - CI Pipeline #93 failure (migration-test step)
Add Index declarations for ix_blocks_block_type_mermaid and
ix_notes_note_type_architecture so alembic autogenerate no longer
flags them as needing removal. No new migration — indexes already
exist in DB via migration u1p2q3r4s5t6.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-12 17:17:56 +00:00
Author
Contributor
Copy link

QA Review

Diff Analysis

  • 1 file changed, 13 additions, 0 deletions — minimal and focused
  • text correctly added to sqlalchemy imports (already had Index)
  • ix_notes_note_type_architecture partial index added to Note via new __table_args__ tuple — trailing comma present, correct form
  • ix_blocks_block_type_mermaid partial index added to Block __table_args__ — inserted before the UniqueConstraint, clean placement
  • No new migration files — model-only change as intended
  • postgresql_where=text(...) usage is correct for declaring partial indexes in SQLAlchemy

Checks

  • Index names match migration u1p2q3r4s5t6
  • No migration file added
  • text import present
  • ruff check and format confirmed passing
  • Model imports cleanly verified
  • Branch naming convention: 265-fix-add-partial-index-declarations-to-mo matches issue #265

Concerns

None.

VERDICT: APPROVED

## QA Review ### Diff Analysis - 1 file changed, 13 additions, 0 deletions — minimal and focused - `text` correctly added to sqlalchemy imports (already had `Index`) - `ix_notes_note_type_architecture` partial index added to Note via new `__table_args__` tuple — trailing comma present, correct form - `ix_blocks_block_type_mermaid` partial index added to Block `__table_args__` — inserted before the UniqueConstraint, clean placement - No new migration files — model-only change as intended - `postgresql_where=text(...)` usage is correct for declaring partial indexes in SQLAlchemy ### Checks - [x] Index names match migration u1p2q3r4s5t6 - [x] No migration file added - [x] `text` import present - [x] ruff check and format confirmed passing - [x] Model imports cleanly verified - [x] Branch naming convention: `265-fix-add-partial-index-declarations-to-mo` matches issue #265 ### Concerns None. VERDICT: APPROVED
forgejo_admin commented 2026-04-12 17:20:29 +00:00
Author
Contributor
Copy link

PR #266 Review

DOMAIN REVIEW

Stack: Python / SQLAlchemy / Alembic (pal-e-docs models)

Correctness of partial index declarations:

  • sqlalchemy.text (lowercase) is the correct import for SQL text clauses in postgresql_where. Verified it is added to the existing import block alphabetically between Text and UniqueConstraint.
  • Index("ix_notes_note_type_architecture", "note_type", postgresql_where=text("note_type = 'architecture'")) -- correct partial index syntax. Single-column index on note_type filtered to architecture rows.
  • Index("ix_blocks_block_type_mermaid", "block_type", postgresql_where=text("block_type = 'mermaid'")) -- same pattern, correct.
  • Both postgresql_where clauses use text() wrappers, which is required for Alembic's autogenerate comparison to recognize them as matching the DB state.

Model integrity:

  • Note had no prior __table_args__. The new tuple is well-formed (trailing comma present in the tuple via the closing paren placement).
  • Block's existing __table_args__ preserved: ix_blocks_note_id_position, ix_blocks_anchor_id, and uq_blocks_note_id_anchor_id all untouched. New index inserted before the UniqueConstraint, which is fine -- ordering in the tuple is cosmetic.

No migration created: Only models.py modified (1 file, +13 lines, -0 lines). No new file in alembic/versions/. This is the correct approach -- these indexes already exist in the DB; the models just need to declare them so alembic check sees parity.

Alembic check compatibility: With these declarations in place, alembic check will see the partial indexes in both the model metadata and the DB, eliminating the false-positive drift detection that caused CI pipeline #93 to fail.

BLOCKERS

None.

NITS

  1. The PR checklist references migration u1p2q3r4s5t6 as the source of these indexes, but that migration revision ID does not appear in the local alembic/versions/ directory (local checkout may be behind remote main). Not blocking, but worth confirming the migration exists on the remote main branch for traceability.

SOP COMPLIANCE

  • Branch named after issue: 265-fix-add-partial-index-declarations-to-mo (follows {issue-number}-{kebab-case-purpose})
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present
  • Related references issue: Closes #265 + CI Pipeline #93
  • No secrets committed
  • No scope creep -- single file, model-only change

PROCESS OBSERVATIONS

Minimal-risk CI fix. Change failure risk is near zero -- this is a model metadata declaration only (no DDL, no data migration, no runtime behavior change). The fix unblocks CI for all subsequent PRs, so deployment frequency impact is positive.

VERDICT: APPROVED

## PR #266 Review ### DOMAIN REVIEW **Stack**: Python / SQLAlchemy / Alembic (pal-e-docs models) **Correctness of partial index declarations:** - `sqlalchemy.text` (lowercase) is the correct import for SQL text clauses in `postgresql_where`. Verified it is added to the existing import block alphabetically between `Text` and `UniqueConstraint`. - `Index("ix_notes_note_type_architecture", "note_type", postgresql_where=text("note_type = 'architecture'"))` -- correct partial index syntax. Single-column index on `note_type` filtered to architecture rows. - `Index("ix_blocks_block_type_mermaid", "block_type", postgresql_where=text("block_type = 'mermaid'"))` -- same pattern, correct. - Both `postgresql_where` clauses use `text()` wrappers, which is required for Alembic's autogenerate comparison to recognize them as matching the DB state. **Model integrity:** - Note had no prior `__table_args__`. The new tuple is well-formed (trailing comma present in the tuple via the closing paren placement). - Block's existing `__table_args__` preserved: `ix_blocks_note_id_position`, `ix_blocks_anchor_id`, and `uq_blocks_note_id_anchor_id` all untouched. New index inserted before the UniqueConstraint, which is fine -- ordering in the tuple is cosmetic. **No migration created:** Only `models.py` modified (1 file, +13 lines, -0 lines). No new file in `alembic/versions/`. This is the correct approach -- these indexes already exist in the DB; the models just need to declare them so `alembic check` sees parity. **Alembic check compatibility:** With these declarations in place, `alembic check` will see the partial indexes in both the model metadata and the DB, eliminating the false-positive drift detection that caused CI pipeline #93 to fail. ### BLOCKERS None. ### NITS 1. The PR checklist references migration `u1p2q3r4s5t6` as the source of these indexes, but that migration revision ID does not appear in the local `alembic/versions/` directory (local checkout may be behind remote main). Not blocking, but worth confirming the migration exists on the remote main branch for traceability. ### SOP COMPLIANCE - [x] Branch named after issue: `265-fix-add-partial-index-declarations-to-mo` (follows `{issue-number}-{kebab-case-purpose}`) - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present - [x] Related references issue: Closes #265 + CI Pipeline #93 - [x] No secrets committed - [x] No scope creep -- single file, model-only change ### PROCESS OBSERVATIONS Minimal-risk CI fix. Change failure risk is near zero -- this is a model metadata declaration only (no DDL, no data migration, no runtime behavior change). The fix unblocks CI for all subsequent PRs, so deployment frequency impact is positive. ### VERDICT: APPROVED
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/pal-e-api!266
No description provided.