Add db empty check constraints and migration

[piyush-jaiswal]

Summary by CodeRabbit

• Bug Fixes

  • Prevents saving empty or whitespace-only values for user emails and passwords, and for category, subcategory, and product names, improving data integrity and form validation.

• Chores

  • Database schema updated to enforce non-empty trimmed strings on the affected fields via a reversible migration.

[coderabbitai]

Walkthrough

Added 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

Cohort / File(s)	Summary
Model constraints app/models.py	Added ConstraintFactory.non_empty_string(column_name) returning a SQLAlchemy CheckConstraint using trim(column) != ''. Updated __table_args__ to apply these checks to User (email, email_normalized, password_hash), Category (name), Subcategory (name), and Product (name). Also imported CheckConstraint.
Alembic migration migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py	New migration (revision='911b11318ef1', down_revision='27b56cc8451c') adding CHECK constraints enforcing trim(column) <> '' for the same columns in upgrade(), and removing them in downgrade() using batch_op.

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

I twitch my nose at empty strings tonight,
Trim and tuck them out of sight.
Constraints stitched neat, rows stand tall—
Migrations hum and answer the call.
A rabbit hops, schema set just right.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Add db empty check constraints and migration" concisely and accurately summarizes the primary change — adding database check constraints to prevent empty strings and the accompanying Alembic migration. It directly reflects the updates in app/models.py (ConstraintFactory and table_args changes) and the new migration file. The wording is clear and focused, though capitalization and the phrase "empty check constraints" could be slightly tightened for grammar.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/db-constraint-empty-strings

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f023f7 and 1881c2a.

📒 Files selected for processing (1)

• migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• migrations/versions/911b11318ef1_add_constraints_for_empty_strings.py

⏰ 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)

• GitHub Check: deploy-preview

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 809a062 and 4f023f7.

📒 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.