Skip to content

Conversation

@piyush-jaiswal
Copy link
Owner

@piyush-jaiswal piyush-jaiswal commented Sep 27, 2025

Summary by CodeRabbit

  • New Features

    • Clear 409 Conflict responses with descriptive messages when creating/updating categories with duplicate names or duplicate category–subcategory links.
  • Bug Fixes

    • Failed updates no longer alter existing category–subcategory links, preventing unintended changes; duplicate-link attempts now return 409.
  • Refactor

    • Product retrieval for category pages now uses relationship-based filtering for more reliable results.
  • Tests

    • Added test coverage ensuring integrity constraints trigger errors and preserve existing associations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

Walkthrough

Adds integrity-aware error handling to category create/update flows (catching UNIQUE violations for Category.name and category-subcategory links), caches the single-column UniqueConstraint for Category.name, switches product retrieval to relationship-based filtering, and adds a test asserting UNIQUE constraint behavior when linking subcategories.

Changes

Cohort / File(s) Summary
Category routes: integrity handling & query updates
app/migrated_routes/category.py
- Added @staticmethod _get_name_unique_constraint() and class attribute _NAME_UNIQUE_CONSTRAINT to locate/cache the single-column UniqueConstraint on Category.name.
- Imported psycopg2.errors.UniqueViolation, sqlalchemy.UniqueConstraint, sqlalchemy.exc.IntegrityError.
- Wrapped db.session.add()/commit() in try/except to catch IntegrityError; on UniqueViolation for Category.name or for the category_subcategory PK returns 409 with specific messages and rolls back.
- Replaced explicit join-based product queries with relationship-based filtering: Product.subcategories.any(Subcategory.categories.any(id=id)).
Tests: relationship uniqueness behavior
tests/test_relationships.py
- Added sqlite3 and IntegrityError imports.
- New test test_update_category_adds_linked_subcategories asserting that adding an already-linked subcategory raises IntegrityError (UNIQUE violation) and that the original category-subcategory linkage remains intact.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant API as CategoryCollection
  participant DB as SQLAlchemy Session

  Client->>API: POST/PUT Category (name, subcategories)
  API->>DB: session.add(instance) / commit()
  alt Commit succeeds
    DB-->>API: commit OK
    API-->>Client: 200/201
  else IntegrityError with UniqueViolation on name
    DB-->>API: IntegrityError(UniqueViolation)
    API->>DB: session.rollback()
    API-->>Client: 409 Conflict (duplicate category name)
  else IntegrityError with UniqueViolation on category_subcategory pk
    DB-->>API: IntegrityError(UniqueViolation)
    API->>DB: session.rollback()
    API-->>Client: 409 Conflict (duplicate category-subcategory link)
  end
Loading
sequenceDiagram
  autonumber
  actor Client
  participant API as CategoryProducts.get
  participant ORM as SQLAlchemy ORM

  Client->>API: GET /categories/:id/products
  API->>ORM: Product.query.filter(Product.subcategories.any(Subcategory.categories.any(id=:id)))
  ORM-->>API: product list
  API-->>Client: 200 with products
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my whiskers, patch and bind,
Names kept single, no duplicates to find.
Links that clash get gently unmade,
Commits roll back, no mess is paid.
Hop on — the schema's neat and kind. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Feature/db error handling category” directly highlights the introduction of database error handling for category operations, which is the primary focus of the changeset, and does so in a concise, noise-free manner.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ 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-error-handling-category

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5a815a and 43ba76b.

📒 Files selected for processing (2)
  • app/migrated_routes/category.py (5 hunks)
  • tests/test_relationships.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_relationships.py (2)
tests/conftest.py (4)
  • create_authenticated_headers (43-50)
  • create_category (54-63)
  • create_subcategory (67-78)
  • client (14-21)
app/migrated_routes/category.py (1)
  • put (153-215)
app/migrated_routes/category.py (1)
app/models.py (3)
  • Category (77-93)
  • Product (116-134)
  • Subcategory (96-113)

@piyush-jaiswal
Copy link
Owner Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

✅ Actions performed

Full review triggered.

1 similar comment
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
tests/test_relationships.py (2)

1-2: Backend-coupled import ties the test to SQLite

Importing sqlite3 for assertions below makes the test engine-specific. Consider asserting engine-agnostic invariants (HTTP 409 when API maps integrity errors) or parametrize by backend.

If you plan to run tests on Postgres too, add a variant that expects a 409 response instead of a raw DB exception.


102-113: Test asserts driver-specific error; consider a backend-agnostic variant

This expects sqlite3.IntegrityError and “UNIQUE constraint failed”. On Postgres, the route maps duplicate links to HTTP 409 (after fixes). Add a complementary test that exercises the API behavior (409) and keep this as SQLite-only if desired.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5a815a and 43ba76b.

📒 Files selected for processing (2)
  • app/migrated_routes/category.py (5 hunks)
  • tests/test_relationships.py (2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
app/migrated_routes/category.py

116-116: Use raise without specifying exception name

Remove exception name

(TRY201)


213-213: Use raise without specifying exception name

Remove exception name

(TRY201)

🔇 Additional comments (3)
app/migrated_routes/category.py (3)

105-116: Postgres 409 path unreachable; compare against resolved DB constraint names and re‑raise correctly

Comparing to CategoryCollection._NAME_UNIQUE_CONSTRAINT.name won’t match Postgres’ actual constraint (e.g., category_name_key). Also prefer bare raise to preserve traceback. This mirrors an earlier review note.

         try:
             db.session.add(category)
             db.session.commit()
         except IntegrityError as ie:
             db.session.rollback()
-            if (
-                isinstance(ie.orig, UniqueViolation)
-                and ie.orig.diag.constraint_name
-                == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name
-            ):
+            if (
+                isinstance(getattr(ie, "orig", None), UniqueViolation)
+                and getattr(getattr(ie.orig, "diag", None), "constraint_name", None)
+                in CategoryCollection._resolve_name_unique_constraint_names()
+            ):
                 abort(409, message="Category with this name already exists")
-            raise ie
+            raise

203-214: Duplicate link 409 path also unreachable on Postgres; resolve PK name via inspector and use bare re‑raise

category_subcategory.primary_key.name is typically None unless explicitly named; Postgres reports something like category_subcategory_pkey. Resolve it from the DB and compare against diag.constraint_name. This mirrors an earlier review note.

         try:
             db.session.commit()
         except IntegrityError as ie:
             db.session.rollback()
-            if (
-                isinstance(ie.orig, UniqueViolation)
-                and ie.orig.diag.constraint_name
-                == category_subcategory.primary_key.name
-            ):
-                abort(409, message="Category and subcategory already linked")
-            raise ie
+            if isinstance(getattr(ie, "orig", None), UniqueViolation):
+                pk_name = category_subcategory.primary_key.name
+                if not pk_name:
+                    pk_info = inspect(db.engine).get_pk_constraint(category_subcategory.name)
+                    pk_name = pk_info.get("name")
+                if getattr(getattr(ie.orig, "diag", None), "constraint_name", None) == pk_name:
+                    abort(409, message="Category and subcategory already linked")
+            raise

315-317: Approve code changes: relationship-based filtering reads clearly; nested any() is idiomatic, and the category_subcategory table already defines a composite primary key on (category_id, subcategory_id), supporting FKs, plus an index.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5a815a and 43ba76b.

📒 Files selected for processing (2)
  • app/migrated_routes/category.py (5 hunks)
  • tests/test_relationships.py (2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
app/migrated_routes/category.py

116-116: Use raise without specifying exception name

Remove exception name

(TRY201)


213-213: Use raise without specifying exception name

Remove exception name

(TRY201)

🔇 Additional comments (3)
app/migrated_routes/category.py (3)

31-43: Import-time TypeError: staticmethod invoked inside the class body

At Line 42 you call _get_name_unique_constraint() while the class body is still executing. Because Line 31 decorates it with @staticmethod, the name in the class namespace is a staticmethod descriptor, so CategoryCollection fails to import with TypeError: 'staticmethod' object is not callable. Beyond that, caching the ORM UniqueConstraint instance won’t help you match the runtime constraint name on Postgres anyway (it stays None when the column was declared with unique=True). Please replace this with a lazily evaluated inspector-based helper that returns the real constraint names.

-from sqlalchemy import UniqueConstraint, exists
+from functools import lru_cache
+from sqlalchemy import UniqueConstraint, exists, inspect
...
-    @staticmethod
-    def _get_name_unique_constraint():
-        name_col = Category.__table__.c.name
-        return next(
-            con
-            for con in Category.__table__.constraints
-            if isinstance(con, UniqueConstraint)
-            and len(con.columns) == 1
-            and con.columns.contains_column(name_col)
-        )
-
-    _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint()
+    @staticmethod
+    @lru_cache(maxsize=1)
+    def _name_unique_constraint_names():
+        insp = inspect(db.engine)
+        return tuple(
+            uc["name"]
+            for uc in insp.get_unique_constraints(Category.__tablename__)
+            if uc.get("name") and set(uc.get("column_names") or []) == {"name"}
+        )

105-116: Duplicate-name 409 branch is still unreachable on Postgres

Line 113 compares ie.orig.diag.constraint_name against _NAME_UNIQUE_CONSTRAINT.name, but—as noted previously—that attribute is None for constraints produced by Column(unique=True). In Postgres the driver reports category_name_key, so this equality never succeeds and we still leak the IntegrityError as a 500. After adopting the inspector helper above, rework this block to compare against the resolved constraint names (use getattr to guard against engines that lack .diag). Also re-raise with bare raise to preserve the original traceback.

-            if (
-                isinstance(ie.orig, UniqueViolation)
-                and ie.orig.diag.constraint_name
-                == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name
-            ):
+            constraint_name = getattr(getattr(ie.orig, "diag", None), "constraint_name", None)
+            if (
+                isinstance(ie.orig, UniqueViolation)
+                and constraint_name in CategoryCollection._name_unique_constraint_names()
+            ):
                 abort(409, message="Category with this name already exists")
-            raise ie
+            raise

203-213: Association duplicate guard never fires because the PK name is None

category_subcategory.primary_key.name (Line 210) returns None unless the association table’s primary key was explicitly named—Postgres emits category_subcategory_pkey, so this branch is dead and the update endpoint still returns 500. Resolve the actual PK name via inspect(db.engine).get_pk_constraint(category_subcategory.name)["name"] (cache it) and compare against that result. Guard access to .diag just like in the name-constraint case, and use bare raise afterward.

-            if (
-                isinstance(ie.orig, UniqueViolation)
-                and ie.orig.diag.constraint_name
-                == category_subcategory.primary_key.name
-            ):
+            pk_name = category_subcategory.primary_key.name
+            if not pk_name:
+                pk_name = inspect(db.engine).get_pk_constraint(category_subcategory.name)["name"]
+            constraint_name = getattr(getattr(ie.orig, "diag", None), "constraint_name", None)
+            if isinstance(ie.orig, UniqueViolation) and constraint_name == pk_name:
                 abort(409, message="Category and subcategory already linked")
-            raise ie
+            raise

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
app/migrated_routes/category.py (2)

105-116: Make duplicate-name detection portable across drivers (fallback to sqlstate 23505).

If tests/dev use SQLite, ie.orig won’t be UniqueViolation; adding a sqlstate fallback keeps the 409 path consistent without coupling to psycopg2 type checks.

Apply:

-            if (
-                isinstance(ie.orig, UniqueViolation)
-                and ie.orig.diag.constraint_name
-                == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name
-            ):
+            if (
+                (isinstance(ie.orig, UniqueViolation)
+                 or getattr(getattr(ie.orig, "diag", None), "sqlstate", None) == "23505")
+                and getattr(getattr(ie.orig, "diag", None), "constraint_name", None)
+                == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name
+            ):
                 abort(409, message="Category with this name already exists")

Based on learnings.


4-6: Driver-specific import is fine; consider a lightweight portability guard.

Using psycopg2.errors.UniqueViolation ties behavior to Postgres. With the sqlstate fallback in handlers, portability improves without removing this import.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43ba76b and 819a812.

📒 Files selected for processing (1)
  • app/migrated_routes/category.py (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-27T20:27:42.172Z
Learnt from: piyush-jaiswal
PR: piyush-jaiswal/ecommerce-rest-api#18
File: app/migrated_routes/category.py:203-213
Timestamp: 2025-09-27T20:27:42.172Z
Learning: In this ecommerce REST API codebase, SQLAlchemy naming conventions are configured in app/__init__.py with PostgreSQL-compatible patterns, ensuring that association table primary keys like category_subcategory.primary_key.name return actual constraint names (e.g., "category_subcategory_pkey") rather than None, making IntegrityError constraint name comparisons work correctly.

Applied to files:

  • app/migrated_routes/category.py
🧬 Code graph analysis (1)
app/migrated_routes/category.py (1)
app/models.py (3)
  • Category (77-93)
  • Product (116-134)
  • Subcategory (96-113)
🔇 Additional comments (2)
app/migrated_routes/category.py (2)

315-317: LGTM: Relationship-based filtering is clearer than manual joins.

This reads well and should generate efficient EXISTS subqueries with proper FK/PK indexes.


31-42: Blocker: @staticmethod call inside class body is not callable (TypeError at import time).

Calling a @staticmethod within the class body invokes the staticmethod descriptor, which isn’t callable and raises at import time. Call the underlying function via .__func__ (or move the assignment after the class definition).

Apply:

-    _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint()
+    _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint.__func__()

Likely an incorrect or invalid review comment.

@piyush-jaiswal
Copy link
Owner Author

@coderabbitai resolve

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

✅ Actions performed

Comments resolved.

@piyush-jaiswal piyush-jaiswal merged commit 8dc3ebb into master Sep 28, 2025
3 checks passed
@piyush-jaiswal piyush-jaiswal deleted the feature/db-error-handling-category branch September 28, 2025 06:40
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.

2 participants