Feature/flask smorest product #23
Conversation
WalkthroughAdds a new Flask-Smorest product blueprint registered at /products, removes legacy /product routes, introduces Product schemas and request args, updates tests and documentation to use pluralized endpoints, and adds integrity/validation handling and JWT protection for mutating product operations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant App as Flask App
participant PB as Product Blueprint
participant DB as Database
rect rgba(230,245,255,0.6)
note over Client,App: Query products (public)
Client->>App: GET /products?name=...
App->>PB: ProductCollection.get(name,page)
PB->>DB: SELECT products (filter + paginate)
DB-->>PB: rows
PB-->>App: 200 {"products":[...]}
App-->>Client: 200 OK
end
rect rgba(240,255,240,0.6)
note over Client,App: Create product (authenticated)
Client->>App: POST /products (JWT, body)
App->>PB: ProductCollection.post(data)
PB->>DB: validate subcategories, INSERT product, INSERT links
alt unique name conflict
DB-->>PB: IntegrityError
PB-->>App: 409 Conflict
App-->>Client: 409
else success
DB-->>PB: new product
PB-->>App: 201 Created
App-->>Client: 201
end
end
rect rgba(255,250,230,0.6)
note over Client,App: Update product (authenticated)
Client->>App: PUT /products/{id} (JWT, body)
App->>PB: ProductById.put(id,data)
PB->>DB: update product and links
alt name/link conflict
DB-->>PB: IntegrityError
PB-->>App: 409
App-->>Client: 409
else success
DB-->>PB: updated product
PB-->>App: 200 OK
App-->>Client: 200
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_product.py (1)
47-117: Update tests to expect 409 responses for duplicate productsThe product endpoints will start translating unique-constraint violations into HTTP 409, even on SQLite. These tests currently expect the DB
IntegrityErrorto bubble up and will fail after the fix. Please rewrite them to issue the request, assertresponse.status_code == 409, and validate the error payload instead of wrapping the call inpytest.raises(IntegrityError).
📜 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/product.py(1 hunks)app/routes.py(1 hunks)app/schemas.py(1 hunks)tests/conftest.py(1 hunks)tests/test_product.py(8 hunks)tests/test_relationships.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_relationships.py (1)
tests/conftest.py (4)
client(14-21)create_authenticated_headers(43-55)create_product(87-98)create_subcategory(72-83)
app/routes.py (1)
app/models.py (1)
User(18-62)
tests/test_product.py (1)
tests/conftest.py (3)
create_product(87-98)client(14-21)create_authenticated_headers(43-55)
app/schemas.py (1)
app/models.py (1)
Product(116-134)
app/migrated_routes/product.py (2)
app/models.py (2)
Product(116-134)Subcategory(96-113)app/schemas.py (6)
NameArgs(98-99)PaginationArgs(102-103)ProductIn(75-95)ProductOut(66-68)ProductsOut(71-72)SubcategoriesOut(41-42)
🪛 Ruff (0.13.2)
app/schemas.py
84-84: Unused method argument: kwargs
(ARG002)
93-93: Unused method argument: data_key
(ARG002)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
| try: | ||
| db.session.add(product) | ||
| db.session.commit() | ||
| except IntegrityError as ie: | ||
| db.session.rollback() | ||
| if ( | ||
| isinstance(ie.orig, UniqueViolation) | ||
| and ie.orig.diag.constraint_name | ||
| == ProductCollection._NAME_UNIQUE_CONSTRAINT.name | ||
| ): | ||
| abort(409, message="Product with this name already exists") | ||
| raise |
There was a problem hiding this comment.
Return 409 on SQLite unique violations instead of leaking 500s
Both post and put handlers only translate unique violations when ie.orig is a psycopg2 UniqueViolation. Under the in-memory SQLite DB used in tests (and commonly in local runs), ie.orig is a sqlite3.IntegrityError, so these blocks fall through to raise. That bubbles the DB error, yielding a 500 instead of the intended 409 and forcing the tests to pytest.raises(IntegrityError). Please broaden the handling to cover SQLite by inspecting ie.orig (e.g., isinstance(..., sqlite3.IntegrityError) plus message matching) before aborting with the appropriate 409 message for duplicate product names or duplicate product-subcategory links.
Also applies to: 236-250
🤖 Prompt for AI Agents
In app/migrated_routes/product.py around lines 128-139 (and similarly at
236-250), the except block only treats psycopg2 UniqueViolation as a duplicate
and re-raises other DB errors (causing 500s under SQLite); update the exception
handling to also detect SQLite unique constraint violations by checking if
ie.orig is an instance of sqlite3.IntegrityError and/or by matching its message
(e.g., "UNIQUE constraint failed" or the specific constraint/column names) and,
when it indicates the Product name or product-subcategory unique constraint,
call abort(409, message="Product with this name already exists") or the
appropriate 409 message; keep the existing rollback, preserve behavior for other
errors by re-raising, and apply the same change to the put/post handling at
lines 236-250.
| if name := data.get("name"): | ||
| product.name = name | ||
| if description := data.get("description"): | ||
| product.description = description | ||
|
|
||
| with db.session.no_autoflush: | ||
| if sc_ids := data.get("subcategories"): | ||
| subcategories = Subcategory.query.filter( | ||
| Subcategory.id.in_(sc_ids) | ||
| ).all() | ||
| if len(subcategories) != len(sc_ids): | ||
| abort(422, message="One or more subcategories not present") | ||
| product.subcategories.extend(subcategories) | ||
|
|
There was a problem hiding this comment.
Let updates apply even when values are empty
if name := data.get("name") / if description := data.get("description") skip assignments when the payload supplies an empty string or explicit null, so clients cannot clear or blank-out these fields. Switch to key checks instead:
- if name := data.get("name"):
- product.name = name
- if description := data.get("description"):
- product.description = description
+ if "name" in data:
+ product.name = data["name"]
+ if "description" in data:
+ product.description = data["description"](Validation already forbids empty names; description should accept empty/None.)
📝 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.
| if name := data.get("name"): | |
| product.name = name | |
| if description := data.get("description"): | |
| product.description = description | |
| with db.session.no_autoflush: | |
| if sc_ids := data.get("subcategories"): | |
| subcategories = Subcategory.query.filter( | |
| Subcategory.id.in_(sc_ids) | |
| ).all() | |
| if len(subcategories) != len(sc_ids): | |
| abort(422, message="One or more subcategories not present") | |
| product.subcategories.extend(subcategories) | |
| if "name" in data: | |
| product.name = data["name"] | |
| if "description" in data: | |
| product.description = data["description"] | |
| with db.session.no_autoflush: | |
| if sc_ids := data.get("subcategories"): | |
| subcategories = Subcategory.query.filter( | |
| Subcategory.id.in_(sc_ids) | |
| ).all() | |
| if len(subcategories) != len(sc_ids): | |
| abort(422, message="One or more subcategories not present") | |
| product.subcategories.extend(subcategories) |
| def test_update_product_adds_linked_subcategories( | ||
| self, create_authenticated_headers, create_product, create_subcategory | ||
| ): | ||
| subcategory1 = create_subcategory("UPS1").get_json() | ||
| subcategory2 = create_subcategory("UPS2").get_json() | ||
| product = create_product( | ||
| "UP", "desc", subcategories=[subcategory1["id"], subcategory2["id"]] | ||
| ).get_json() | ||
|
|
||
| with pytest.raises(IntegrityError) as ie: | ||
| self.client.put( | ||
| f"/products/{product['id']}", | ||
| json={"subcategories": [subcategory1["id"]]}, | ||
| headers=create_authenticated_headers(), | ||
| ) | ||
|
|
||
| assert isinstance(ie.value.orig, sqlite3.IntegrityError) | ||
| assert "UNIQUE constraint failed" in str(ie.value.orig) | ||
| assert self._product_subcategory_ids(product["id"]) == sorted( | ||
| [subcategory1["id"], subcategory2["id"]] | ||
| ) |
There was a problem hiding this comment.
Assert on HTTP 409 instead of bubbling IntegrityError
After we surface duplicate-link violations as proper 409 responses (see product blueprint review), this pytest.raises(IntegrityError) expectation will start failing. Please switch the test to call the endpoint normally, assert response.status_code == 409, and verify the JSON error payload rather than capturing the DB exception.
🤖 Prompt for AI Agents
In tests/test_relationships.py around lines 178 to 198, the test currently
expects a database IntegrityError to be raised when updating a product with a
duplicate subcategory link; update it to perform a normal HTTP PUT call and
assert response.status_code == 409, then parse response.get_json() (or
response.json) and assert the error payload contains the expected
message/structure for duplicate-link violations, and finally confirm the
product's subcategory links were not changed by checking
self._product_subcategory_ids(product["id"]) remains the same.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/migrated_routes/product.py (3)
128-139: SQLite unique violations still return 500 instead of 409.The exception handling only detects
psycopg2.UniqueViolation. Under SQLite (used in tests and local development),ie.origis asqlite3.IntegrityError, causing the code toraiseand leak a 500 error instead of returning 409. Importsqlite3and broaden the check:+import sqlite3 + from flask.views import MethodViewThen update the exception handling:
except IntegrityError as ie: db.session.rollback() if ( - isinstance(ie.orig, UniqueViolation) + (isinstance(ie.orig, UniqueViolation) + or (isinstance(ie.orig, sqlite3.IntegrityError) + and "UNIQUE constraint" in str(ie.orig))) and ie.orig.diag.constraint_name == ProductCollection._NAME_UNIQUE_CONSTRAINT.name ): abort(409, message="Product with this name already exists")Note: For SQLite, you'll need to parse the constraint name from the error message or use a different approach since
ie.orig.diagis psycopg2-specific.
236-250: Same SQLite handling gap exists in PUT.This exception block has the same issue as the POST handler: SQLite
IntegrityErrorinstances fall through toraise, returning 500 instead of 409. Apply the same fix pattern as suggested for lines 128-139.
221-224: Walrus operator still blocks empty name updates.Line 221 uses
if name := data.get("name")which skips the assignment whennameis an empty string. While validation prevents empty names, the check should use key presence for consistency. Line 223 correctly uses"description" in data, which allows clearing the description field.Apply this diff to make name handling consistent:
- if name := data.get("name"): - product.name = name + if "name" in data: + product.name = data["name"]
🧹 Nitpick comments (3)
app/schemas.py (1)
84-84: Consider accepting**_ifkwargsis intentionally unused.The
kwargsparameter is unused but required by the@pre_loaddecorator signature. If you don't need it, replace with**_to signal intentional non-use:- def strip_strings(self, data, **kwargs): + def strip_strings(self, data, **_):app/migrated_routes/product.py (2)
31-42: Constraint discovery could fail silently.
_get_name_unique_constraint()usesnext()without a default, so if the constraint isn't found (e.g., schema mismatch, migration issue), it raisesStopIterationduring class definition, preventing the entire module from loading. Consider adding error handling:@staticmethod def _get_name_unique_constraint(): name_col = Product.__table__.c.name - return next( - con - for con in Product.__table__.constraints - if isinstance(con, UniqueConstraint) - and len(con.columns) == 1 - and con.columns.contains_column(name_col) - ) + try: + return next( + con + for con in Product.__table__.constraints + if isinstance(con, UniqueConstraint) + and len(con.columns) == 1 + and con.columns.contains_column(name_col) + ) + except StopIteration: + raise RuntimeError("Product.name unique constraint not found in schema")
233-233:.extend()can create duplicates if called repeatedly.Line 233 uses
product.subcategories.extend(subcategories)which appends to the existing relationships. If a client sends a PUT request with the same subcategory IDs multiple times, or if subcategories already exist, this creates duplicate associations (until the unique constraint at commit catches them). Consider replacing the collection or checking for existing associations first:- product.subcategories.extend(subcategories) + # Replace existing subcategories or merge carefully + existing_ids = {sc.id for sc in product.subcategories} + new_subcategories = [sc for sc in subcategories if sc.id not in existing_ids] + product.subcategories.extend(new_subcategories)Or clarify in documentation that PUT subcategories is additive, not a replacement operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/migrated_routes/product.py(1 hunks)app/schemas.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/migrated_routes/product.py (2)
app/models.py (2)
Product(116-134)Subcategory(96-113)app/schemas.py (6)
NameArgs(98-99)PaginationArgs(102-103)ProductIn(75-95)ProductOut(66-68)ProductsOut(71-72)SubcategoriesOut(41-42)
app/schemas.py (1)
app/models.py (1)
Product(116-134)
🪛 Ruff (0.13.2)
app/schemas.py
84-84: Unused method argument: kwargs
(ARG002)
93-93: Unused method argument: data_key
(ARG002)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
app/schemas.py (2)
83-90: None checks now properly guard.strip()calls.The pre_load hook now correctly checks for
Nonebefore calling.strip()on bothnameanddescription, preventingAttributeErrorwhen clients sendnullvalues. This resolves the concern raised in the previous review.
92-95: Inline validation message is acceptable here.While the static analyzer flags the inline string in
ValidationError, this simple validation case doesn't warrant a custom exception class. The message is clear and concise for a basic length check.app/migrated_routes/product.py (2)
80-141: POST operation properly validates and creates products.The POST handler correctly:
- Requires JWT authentication
- Validates the product name is non-empty via schema
- Checks all subcategory IDs exist before creating associations
- Handles unique name conflicts with 409
- Returns 201 with the created product
254-281: DELETE operation is straightforward and correct.The delete handler properly requires JWT, fetches the product with 404 on missing, and commits the deletion. The
passive_deletes=Trueon the relationship ensures cascade behavior is handled at the database level.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests