Feature/flask smorest subcategory #20
Conversation
WalkthroughPluralizes Subcategory endpoints across docs and tests, registers a new “/subcategories” blueprint, migrates Subcategory CRUD and relationship routes into app/migrated_routes/subcategory.py with JWT-protected mutations and pagination, adds a Subcategory input schema, and removes prior Subcategory routes from app/routes.py. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant F as Flask App
participant SB as Subcategory Blueprint
participant DB as Database
rect rgb(240,248,255)
note right of C: Read operations
C->>F: GET /subcategories
F->>SB: Route dispatch
SB->>DB: Query all subcategories
DB-->>SB: Rows
SB-->>F: 200 JSON {subcategories:[...]}
F-->>C: 200
end
rect rgb(245,255,240)
note right of C: Auth-required mutations
C->>F: POST /subcategories (JWT, body)
F->>SB: Validate JWT, parse body
SB->>DB: Validate refs, insert subcategory
DB-->>SB: OK or IntegrityError
alt Unique violation
SB-->>F: 409 Conflict
else Success
SB-->>F: 201 Created {id,...}
end
F-->>C: 201/409
end
rect rgb(255,250,240)
note right of C: Updates and deletion
C->>F: PUT /subcategories/{id} (JWT)
F->>SB: Dispatch + validate
SB->>DB: Update + assoc links
DB-->>SB: OK or IntegrityError
SB-->>F: 200 or 409
F-->>C: 200/409
C->>F: DELETE /subcategories/{id} (JWT)
F->>SB: Dispatch
SB->>DB: Delete row
DB-->>SB: OK
SB-->>F: 204 No Content
F-->>C: 204
end
rect rgb(248,248,255)
note right of C: Related collections
C->>F: GET /subcategories/{id}/products?page=n
F->>SB: Dispatch
SB->>DB: Query + paginate
DB-->>SB: Page results
SB-->>F: 200 {items, page, pages}
F-->>C: 200
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/routes.py (6)
190-191: Improve error handling.The bare
exceptclause catches all exceptions, including system exits. Additionally, the generic error message doesn't aid debugging or provide actionable feedback to clients.Apply this diff to improve error handling:
- except: - return "Error occured", 500 + except IntegrityError: + return jsonify({'error': 'Product name already exists'}), 409 + except Exception as e: + app.logger.error(f"Error creating product: {str(e)}") + return jsonify({'error': 'Failed to create product'}), 500
279-281: Risk of duplicate subcategory associations.Using
extendadds subcategories to the existing collection without clearing it first. Repeated updates with the same subcategory IDs will create duplicate associations.Apply this diff to replace rather than extend:
if sc_ids is not None: + product.subcategories = [] subcategories = Subcategory.query.filter(Subcategory.id.in_(sc_ids)) product.subcategories.extend(subcategories)
283-283: Use 200 status code for successful updates.Status code 201 is for resource creation. Updates should return 200.
Apply this diff:
- return jsonify(product.to_json()), 201 + return jsonify(product.to_json()), 200
284-285: Improve error handling.The bare
exceptclause catches all exceptions, including system exits. The generic error message doesn't aid debugging.Apply this diff to improve error handling:
- except: - return "Error occured", 500 + except IntegrityError: + return jsonify({'error': 'Product name already exists'}), 409 + except Exception as e: + app.logger.error(f"Error updating product: {str(e)}") + return jsonify({'error': 'Failed to update product'}), 500
320-321: Improve error handling.The bare
exceptclause catches all exceptions, including system exits. The generic error message doesn't aid debugging.Apply this diff:
- except: - return "Error occured", 500 + except Exception as e: + app.logger.error(f"Error deleting product: {str(e)}") + return jsonify({'error': 'Failed to delete product'}), 500
409-410: Improve error handling.The bare
exceptclause catches all exceptions, including system exits. The generic error message doesn't aid debugging.Apply this diff:
- except: - return "Error occured", 500 + except Exception as e: + app.logger.error(f"Error retrieving subcategories: {str(e)}") + return jsonify({'error': 'Failed to retrieve subcategories'}), 500
🧹 Nitpick comments (3)
app/migrated_routes/subcategory.py (2)
45-58: Optional: return a stable ordering for list endpoints.For deterministic tests and clients, order results (e.g., by id).
- return {"subcategories": Subcategory.query.all()} + return {"subcategories": Subcategory.query.order_by(Subcategory.id.asc()).all()}
296-298: Prefer materializing related collections and ordering explicitly.Returning a dynamic query relies on Marshmallow iterating the query; return a list with stable order.
- return {"categories": subcategory.categories} + return { + "categories": subcategory.categories.order_by(Category.id.asc()).all() + }tests/test_subcategory.py (1)
42-51: Replace DB-specific exception in test_create_subcategory_duplicate_name: in tests/test_subcategory.py:42-51, avoid asserting sqlite3.IntegrityError (ties tests to SQLite). Instead, handle uniqueness violations at the HTTP layer (assert a 409 Conflict JSON response) or at minimum catch SQLAlchemy’s IntegrityError without referencing sqlite3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(3 hunks)app/__init__.py(1 hunks)app/migrated_routes/subcategory.py(1 hunks)app/routes.py(1 hunks)app/schemas.py(1 hunks)tests/conftest.py(1 hunks)tests/test_relationships.py(3 hunks)tests/test_subcategory.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
app/schemas.py (1)
app/models.py (1)
Subcategory(96-113)
app/routes.py (1)
app/models.py (2)
Subcategory(96-113)Product(116-134)
app/migrated_routes/subcategory.py (2)
app/models.py (3)
Category(77-93)Product(116-134)Subcategory(96-113)app/schemas.py (6)
CategoriesOut(12-13)PaginationArgs(75-76)ProductsOut(71-72)SubcategoriesOut(41-42)SubcategoryIn(45-63)SubcategoryOut(36-38)
tests/test_relationships.py (1)
tests/conftest.py (5)
create_category(54-63)create_product(82-93)create_subcategory(67-78)client(14-21)create_authenticated_headers(43-50)
tests/test_subcategory.py (1)
tests/conftest.py (3)
create_subcategory(67-78)create_authenticated_headers(43-50)client(14-21)
🪛 Ruff (0.13.1)
app/schemas.py
54-54: Unused method argument: kwargs
(ARG002)
61-61: Unused method argument: data_key
(ARG002)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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
🔇 Additional comments (9)
app/__init__.py (1)
16-19: LGTM! Blueprint registration follows existing patterns.The subcategory blueprint import and registration correctly mirrors the category blueprint pattern, with the appropriate pluralized URL prefix "/subcategories".
tests/conftest.py (1)
67-78: LGTM! Test helper correctly updated to pluralized endpoint.The
create_subcategoryfixture now targets the new/subcategoriesendpoint, consistent with the API migration and matching the pattern used in thecreate_categoryfixture.app/schemas.py (1)
45-64: LGTM! Schema implementation is consistent with existing patterns.The
SubcategoryInschema correctly mirrors theCategoryInstructure with appropriate field definitions, validation, and preprocessing. The inclusion of bothcategoriesandproductsfields aligns with the bidirectional relationships in the Subcategory model.Note: The static analysis hints about unused parameters (
kwargs,data_key) are false positives—these are part of Marshmallow's callback signatures and should be ignored.README.md (2)
67-67: LGTM! Product fetch endpoint correctly updated.The subcategory products endpoint now uses the pluralized path
/subcategories/<subcategory_id: int>/products, consistent with the API migration.
126-147: LGTM! Subcategory documentation comprehensively updated.All Subcategory endpoints have been correctly updated to use the pluralized
/subcategoriesprefix. The documentation structure mirrors the Category section and properly indicates which endpoints require authentication.tests/test_subcategory.py (1)
58-58: LGTM: pluralized endpoints and status codes are consistent.GET/PUT/DELETE paths under /subcategories and 200/204 expectations align with the new blueprint.
Also applies to: 65-69, 85-85, 88-88, 100-100, 102-104, 118-118, 139-139, 163-164
tests/test_relationships.py (1)
215-215: LGTM: relationship routes moved to /subcategories/... and pagination assertions updated.Paths and expected codes match the new API surface.
Also applies to: 218-225, 229-230, 232-243, 267-269
app/migrated_routes/subcategory.py (1)
32-44: Constraint‐name matching is reliable under existing naming conventions; no changes required.app/routes.py (1)
7-7: LGTM! Import changes align with the migration.The removal of
Categoryand retention ofSubcategoryis correct.Subcategoryis still needed here for Product-Subcategory relationship management in the remaining Product endpoints.
| from flask.views import MethodView | ||
| from flask_jwt_extended import jwt_required | ||
| from flask_smorest import Blueprint, abort | ||
| from psycopg2.errors import UniqueViolation |
There was a problem hiding this comment.
Avoid hard dependency on psycopg2 in SQLite/dev environments.
Importing psycopg2.errors will crash if psycopg2 isn’t installed. Guard the import and feature-detect at runtime.
Apply:
-from psycopg2.errors import UniqueViolation
+try:
+ from psycopg2.errors import UniqueViolation as PGUniqueViolation
+except Exception: # psycopg2 not installed or different driver
+ PGUniqueViolation = () # so isinstance(..., PGUniqueViolation) is always FalseAnd update usages:
- if (
- isinstance(ie.orig, UniqueViolation)
+ if (
+ isinstance(ie.orig, PGUniqueViolation)
and getattr(ie.orig, "diag", None)
and ie.orig.diag.constraint_name
== SubcategoryCollection._NAME_UNIQUE_CONSTRAINT.name
):…and similarly in put().
🤖 Prompt for AI Agents
In app/migrated_routes/subcategory.py around line 4, the direct import "from
psycopg2.errors import UniqueViolation" creates a hard dependency that will
crash in environments without psycopg2; wrap the import in a try/except
ImportError (or ModuleNotFoundError) and assign a fallback (e.g.,
UniqueViolation = None or a generic Exception subclass) so the module can import
in SQLite/dev setups. Then, in the post() and put() handlers, change the error
handling to first check if UniqueViolation is not None before using it in an
except clause (or catch Exception and inspect getattr(err, '__class__', None) or
err.pgcode/name to detect unique-violation semantics), so you only rely on
psycopg2-specific exception classes when psycopg2 is actually available. Ensure
behavior is unchanged when psycopg2 is present and that code paths gracefully
handle the fallback in dev.
There was a problem hiding this comment.
already discussed in #18
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch PR #18 details and comments to understand the prior discussion
gh pr view 18 --json title,body,comments --jq '{title, body, comments: [.comments[] | {author: .author.login, body: .body}]}'
Length of output: 13165
@piyush-jaiswal, understood! Since you've already addressed this pattern in #18 and are applying it consistently here, I'll note that decision. Thanks for the clarification.
| subcategory = Subcategory.query.get_or_404(id) | ||
|
|
||
| products = subcategory.products.order_by(Product.id.asc()).paginate( | ||
| page=page, per_page=SubcategoryProducts._PER_PAGE, error_out=False | ||
| ) | ||
|
|
||
| return {"products": products} |
There was a problem hiding this comment.
Bug: returning Pagination object breaks schema; return items.
ProductsOut expects a list; Pagination isn’t a list/iterable for Marshmallow. Return products.items.
Apply:
- return {"products": products}
+ return {"products": products.items}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subcategory = Subcategory.query.get_or_404(id) | |
| products = subcategory.products.order_by(Product.id.asc()).paginate( | |
| page=page, per_page=SubcategoryProducts._PER_PAGE, error_out=False | |
| ) | |
| return {"products": products} | |
| subcategory = Subcategory.query.get_or_404(id) | |
| products = subcategory.products.order_by(Product.id.asc()).paginate( | |
| page=page, per_page=SubcategoryProducts._PER_PAGE, error_out=False | |
| ) | |
| return {"products": products.items} |
🤖 Prompt for AI Agents
In app/migrated_routes/subcategory.py around lines 333 to 339, the handler
currently returns the Flask-SQLAlchemy Pagination object which breaks the
Marshmallow ProductsOut schema; instead return the actual list by replacing the
Pagination return with the paginated.items list (e.g., return {"products":
products.items}) so the serializer receives a plain list. Ensure any clients
that need pagination metadata still get it separately if required.
There was a problem hiding this comment.
it works. tests pass and manually verified as well
There was a problem hiding this comment.
@piyush-jaiswal Great to hear that the fix works correctly! Thanks for verifying both through tests and manual testing. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| def test_update_subcategory_adds_linked_categories_and_products(self, create_authenticated_headers, create_category, create_product, create_subcategory): | ||
| headers = create_authenticated_headers() | ||
| category = create_category("UC1", headers=headers).get_json() | ||
| product = create_product("UP1", headers=headers).get_json() | ||
| subcategory = create_subcategory("U_SC", categories=[category["id"]], products=[product["id"]], headers=headers).get_json() | ||
|
|
||
| with pytest.raises(IntegrityError) as ie_c: | ||
| self.client.put( | ||
| f"/subcategories/{subcategory['id']}", | ||
| json={"categories": [category["id"]]}, | ||
| headers=headers, | ||
| ) | ||
| with pytest.raises(IntegrityError) as ie_p: | ||
| self.client.put( | ||
| f"/subcategories/{subcategory['id']}", | ||
| json={"products": [product["id"]]}, | ||
| headers=headers, | ||
| ) | ||
| with pytest.raises(IntegrityError) as ie_cp: | ||
| self.client.put( | ||
| f"/subcategories/{subcategory['id']}", | ||
| json={"categories": [category["id"]], "products": [product["id"]]}, | ||
| headers=headers, | ||
| ) | ||
|
|
||
| assert isinstance(ie_c.value.orig, sqlite3.IntegrityError) | ||
| assert isinstance(ie_p.value.orig, sqlite3.IntegrityError) | ||
| assert isinstance(ie_cp.value.orig, sqlite3.IntegrityError) | ||
| assert "UNIQUE constraint failed" in str(ie_c.value.orig) | ||
| assert "UNIQUE constraint failed" in str(ie_p.value.orig) | ||
| assert "UNIQUE constraint failed" in str(ie_cp.value.orig) | ||
|
|
||
| assert self._subcategory_category_ids(subcategory["id"]) == [category["id"]] | ||
| assert self._subcategory_product_ids(subcategory["id"]) == [product["id"]] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Re-link tests depend on raw IntegrityError; consider dual-assert for Postgres.
On SQLite this raises IntegrityError; on Postgres the API maps to 409. If CI ever runs on Postgres, gate by engine or assert 409 in an alternate test.
🏁 Script executed:
#!/bin/bash
# Check engine in config to decide if tests should expect exceptions or HTTP 409
rg -n 'SQLALCHEMY_DATABASE_URI' -C2 appLength of output: 239
🏁 Script executed:
#!/bin/bash
rg -n "IntegrityError" -C3 appLength of output: 3605
🏁 Script executed:
#!/bin/bash
sed -n '100,140p' app/migrated_routes/subcategory.py
sed -n '210,260p' app/migrated_routes/subcategory.pyLength of output: 3113
Ensure consistent 409 response for duplicate links on all backends
In app/migrated_routes/subcategory.py’s update handler (around the IntegrityError catch in the PUT method), also catch sqlite3.IntegrityError on UNIQUE constraint failures and abort(409) to match the Postgres UniqueViolation path—or update the tests to assert an HTTP 409 rather than a raw IntegrityError.
There was a problem hiding this comment.
already discussed in #18
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch PR #18 details to understand the prior discussion context
gh pr view 18 --repo piyush-jaiswal/ecommerce-rest-api --json title,body,comments --jq '{title, body, comments: [.comments[] | {author: .author.login, body: .body}]}'
Length of output: 13165
@piyush-jaiswal Got it! I can see that PR #18 established the pattern of catching IntegrityError in tests for UNIQUE constraint violations (e.g., test_update_category_adds_linked_subcategories). This PR consistently applies the same approach to subcategory operations. Thanks for the clarification!
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation