Add db empty check constraints and migration #16
Conversation
WalkthroughAdded a ConstraintFactory providing TRIM-based non-empty CHECK constraints and applied them to several string columns in User, Category, Subcategory, and Product models; added an Alembic migration to create and drop those constraints. Changes
Sequence Diagram(s)Not applicable — changes are schema-level constraints and an Alembic migration (no runtime control-flow changes). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py (2)
23-37: Unify SQL expression style with models to avoid autogen churn.Models use "TRIM(...) != ''" while the migration uses "trim(...) <> ''". Align both sides (pick one) so Alembic autogenerate doesn’t keep proposing diffs due to textual mismatches.
1-1: Polish migration comments/docstring.Minor grammar fixes for clarity.
-"""add constraints for empty strings +"""Add constraints against empty strings @@ -# migrations generated manually! Alembic did not these detect changes. +# Migration generated manually; Alembic did not detect these changes.Also applies to: 18-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models.py(5 hunks)migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py(1 hunks)
🔇 Additional comments (4)
migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py (1)
23-37: Constraints look correct and are applied/dropped in proper order.The DDL is sound and the downgrade mirrors upgrade. Nice use of batch_alter_table.
Also applies to: 41-57
app/models.py (3)
3-3: Import change LGTM.Bringing in CheckConstraint is appropriate.
26-30: Consider also enforcing NOT NULL at DB (already set).Since nullable=False is present on these columns, the TRIM check will never see NULLs. This is good; just calling it out as aligned.
11-16: Naming convention present — confirm 'ck' mapping or make constraint names table‑scoped.app/init.py defines naming_convention and MetaData(naming_convention=...), see app/init.py:25-33. Confirm the mapping contains a "ck" entry that includes %(table_name)s (e.g. "ck": "%(table_name)s_%(constraint_name)s"). If that exists and matches your migration names, keep the simple ConstraintFactory(non_empty_string(column_name)) and ensure names align with migrations; otherwise change ConstraintFactory to accept table_name and emit table-qualified names (e.g. f'{table_name}_{column_name}_non_empty_check') and update table_args as in the suggested diff.
Summary by CodeRabbit
Bug Fixes
Chores