Return only product data by name and add test #12
Conversation
WalkthroughEndpoint switched from a path parameter to a query parameter for product lookup; handler signature changed accordingly, returns the raw product JSON (no derived categories), validates presence of name (400) and returns 404 if not found; tests added to cover URL-encoded names and 404 behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Router as Flask Router
participant Handler as get_product_by_name
participant DB as Database
Client->>Router: GET /product?name=<encoded>
Router->>Handler: call get_product_by_name()
Handler->>Handler: name = request.args.get('name')
alt name missing
Handler-->>Client: 400 Bad Request
else name present
Handler->>DB: Query Product by name
alt Found
DB-->>Handler: Product object
Handler-->>Client: 200 JSON (product.to_json())
else Not found
DB-->>Handler: None
Handler-->>Client: 404 Not Found
end
end
note over Handler: Internal exceptions propagate as 5xx
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes.py (1)
884-910: Route ambiguity: '/product/string:name' conflicts with '/product/int:p_id' and reserved paths.As written, GET /product/string:name will:
- Never handle purely numeric names (e.g., “12345”), because '/product/int:p_id' matches first.
- Also shadow reserved paths on GET (e.g., '/product/create' will look up name="create").
This is a real functional bug for numeric product names and a maintainability footgun. Use a disambiguated path for name lookups.
Apply this minimal, backwards-safe change:
-@app.route('/product/<string:name>', methods=['GET']) +@app.route('/product/by-name/<path:name>', methods=['GET']) def get_product_by_name(name): ... - return jsonify(product.to_json()), 200 + return jsonify(product.to_json()), 200And update the test call sites accordingly:
- get_resp = self.client.get(f"/product/{encoded_name}") + get_resp = self.client.get(f"/product/by-name/{encoded_name}") @@ - not_found_resp = self.client.get(f"/product/{encoded_name}") + not_found_resp = self.client.get(f"/product/by-name/{encoded_name}")This avoids clashes with numeric IDs and allows names containing “/” (since path:... supports it) when combined with the test-side safe='' quoting. If you want to keep forbidding “/” in names, use string:... but still disambiguate with a prefix: '/product/by-name/string:name'.
🧹 Nitpick comments (3)
tests/test_product.py (1)
119-137: Add a couple of edge-case cases (optional).Consider parametrizing additional names to harden behavior: non-ASCII (e.g., “Café”), plus signs (“C++”), percent signs (“20% off”), and purely numeric names (“12345”). The last one currently collides with the ID route (details below).
Would you like me to add a parameterized test covering those cases?
app/routes.py (2)
906-910: Minor: prefer first_or_404 for brevity.If using Flask-SQLAlchemy, you can simplify the lookup+abort pattern.
Apply this diff:
- product = Product.query.filter(Product.name == name).first() - if product is None: - abort(404) - return jsonify(product.to_json()), 200 + product = Product.query.filter_by(name=name).first_or_404() + return jsonify(product.to_json()), 200
884-910: Index/uniqueness on Product.name (optional).Since we’re introducing a primary access path by name, consider adding a DB unique index on Product.name for fast lookups and to enforce uniqueness. It also enables returning 409 on duplicate creation instead of 500 for better API semantics (similar to the user email flow).
If you want, I can draft an Alembic migration to add the index/constraint and adjust create_product to return 409 on IntegrityError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/routes.py(1 hunks)tests/test_product.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_product.py (3)
app/routes.py (1)
create_product(700-751)tests/conftest.py (1)
client(15-22)tests/test_subcategory.py (2)
TestSubcategory(7-199)test_get_subcategory_products_empty(189-199)
app/routes.py (1)
app/models.py (3)
to_json(71-76)to_json(87-92)to_json(103-109)
🔇 Additional comments (2)
tests/test_product.py (1)
119-133: Solid URL-encoding test and field assertions.Good coverage for fetching by a URL-encoded product name and validating id, name, and description. Mirrors existing patterns in this suite.
app/routes.py (1)
906-910: Return-only product JSON is correct and consistent.The simplification to return product.to_json() directly with a 200 and 404 on miss is clean and aligns with the ID-based getter.
tests/test_product.py
Outdated
| encoded_name = quote(product_name) | ||
| get_resp = self.client.get(f"/product/{encoded_name}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote slashes too: use quote(..., safe='') to avoid accidental path splits.
urllib.parse.quote leaves “/” unescaped by default. If a product name ever contains “/”, the generated path will split into multiple segments and not hit this route. Encode everything by setting safe=''.
Apply this diff:
- encoded_name = quote(product_name)
+ encoded_name = quote(product_name, safe='')
@@
- encoded_name = quote("Non Existent Product")
+ encoded_name = quote("Non Existent Product", safe='')Also applies to: 134-135
🤖 Prompt for AI Agents
In tests/test_product.py around lines 126-127 and also 134-135 the call to
urllib.parse.quote uses the default safe characters which leaves "/" unescaped;
update both calls to quote(product_name, safe='') so slashes and all reserved
characters are percent-encoded, ensuring the product name does not split the
path when building the request URL.
There was a problem hiding this comment.
I pushed a fix in commit a4d32d6, please review it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the specific commit a4d32d6 to see what changes were made
git show a4d32d6 --name-only
# Show the actual diff for the commit
git show a4d32d6
# Also check the current state of the file around the relevant lines
rg -n -A2 -B2 "quote.*product" tests/test_product.py
Length of output: 3306
@piyush-jaiswal Excellent solution! 🎉
You took a much better approach than what I originally suggested. Instead of just fixing the URL encoding issue, you addressed the root cause by changing from path parameters to query parameters (/product?name=... instead of /product/{name}).
This is superior because:
- Eliminates encoding complexity - No need for
urllib.parse.quote()at all - Better handles special characters - Query parameters naturally handle
/, spaces, Unicode, etc. - More RESTful - Using query params for filtering is a common pattern
- Added validation - Proper 400 error for missing query parameter
- Enhanced testing - Great use of
@pytest.mark.parametrizewith various special character test cases
The commit also shows you updated both the route handler and the Swagger documentation appropriately. This is a much cleaner and more robust solution than percent-encoding everything in the URL path.
(\ /)
( ._. )
o_("_")("_")
Great problem solving! 🌟
🧬 Code Graph Analysis Results
- File: app/routes.py, lines 700-751
- Snippet:
def create_product():
"""
Create Product
---
tags:
- Product
description: Create a new product.
security:
- access_token: []
requestBody:
required: true
description: name - Name of the product <br> description - Description of the product (optional) <br> subcategories - Array of subcategory ids (optional)
content:
application/json:
schema:
type: object
required:
- name
properties:
name:
type: string
description:
type: string
subcategories:
type: array
items:
type: integer
responses:
201:
description: Product created successfully.
400:
description: Invalid input.
500:
description: Error occurred.
"""
if not request.json:
abort(400)
try:
product = Product(
name=request.json.get('name'),
description=request.json.get('description')
)
sc_ids = request.json.get('subcategories')
if sc_ids is not None:
subcategories = Subcategory.query.filter(Subcategory.id.in_(sc_ids))
product.subcategories.extend(subcategories)
db.session.add(product)
db.session.commit()
return jsonify(product.to_json()), 201
except:
return "Error occured", 500
- File: app/models.py, lines 95-109
- Snippet:
class Product(db.Model):
__tablename__ = 'product'
id = db.Column(db.Integer, primary_key=True)
name = db.Column(db.String(200), nullable=False, unique=True)
description = db.Column(db.String(500))
created_at = db.Column(db.DateTime, nullable=False ,default=datetime.utcnow)
subcategories = db.relationship("Subcategory", secondary=subcategory_product, back_populates="products", lazy='dynamic', passive_deletes=True)
def to_json(self):
return {
'id': self.id,
'name': self.name,
'description': self.description,
'created_at': self.created_at,
}
- File: tests/utils.py, lines 6-10
- Snippet:
def verify_token_error_response(response, expected_code, status_code=401):
assert response.status_code == status_code
data = response.get_json()
assert "error" in data
assert data["code"] == expected_code
- File: tests/utils.py, lines 17-22
- Snippet:
def get_expired_token_headers(app_context, id=1):
with app_context:
token = create_access_token(
identity=str(id), expires_delta=timedelta(seconds=-1)
)
return get_auth_header(token)
- File: tests/utils.py, lines 25-26
- Snippet:
def get_invalid_token_headers():
return get_auth_header("invalid.token.format")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@coderabbitai full review |
|
@coderabbitai resume |
✅ Actions performedFull review triggered. |
|
@piyush-jaiswal I'll review your fix right away. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/test_product.py (2)
1-1: Remove unused import (Ruff F401).
quoteis no longer used since the test now relies onquery_string(which handles encoding). Drop the import to satisfy lint and avoid confusion with the older path-param approach.-from urllib.parse import quote
141-142: Add explicit 400 tests for missing/blank query parameter.You already cover 404. Add coverage for
namemissing andname=""to lock in the 400 behavior.Proposed test additions (outside current hunk):
def test_get_product_by_name_missing_param_returns_400(self): resp = self.client.get("/product") # no query param assert resp.status_code == 400 def test_get_product_by_name_blank_param_returns_400(self): resp = self.client.get("/product", query_string={"name": ""}) assert resp.status_code == 400app/routes.py (3)
886-905: Document the 400 response in the OpenAPI block.The handler aborts with 400 when
nameis missing, but the docstring doesn’t list it. Add a 400 response for accuracy.responses: description: Product retrieved successfully. + 400: + description: Missing required query parameter 'name'. description: Product not found. description: Error occurred.
906-909: Treat whitespace-only names as missing.Currently
" "passes thenot namecheck (it doesn’t), leading to a 404 instead of 400. Trim first to keep input validation consistent.- name = request.args.get('name') - if not name: + name = request.args.get('name', type=str) + if name is not None: + name = name.strip() + if not name: abort(400, description="Missing required query parameter 'name'")
910-913: Optional: make name lookup case-insensitive (without wildcard pitfalls).If product names should match regardless of case, prefer
lower()equality overiliketo avoid%/_acting as wildcards (e.g., “20% off”).Within this function:
- product = Product.query.filter(Product.name == name).first() + from sqlalchemy import func + product = ( + Product.query + .filter(func.lower(Product.name) == name.lower()) + .first() + )Schema/index recommendation (outside this file): ensure
Product.namehasunique=Trueandindex=Trueto support fast exact-name lookups and enforce uniqueness at the DB layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/routes.py(2 hunks)tests/test_product.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_product.py (1)
tests/test_subcategory.py (1)
TestSubcategory(7-199)
🪛 Ruff (0.12.2)
tests/test_product.py
1-1: urllib.parse.quote imported but unused
Remove unused import: urllib.parse.quote
(F401)
🔇 Additional comments (3)
tests/test_product.py (2)
119-127: Good parametrization for tricky names.Covers spaces,
/, Unicode,+, and%cases. Usingquery_stringis the right call—Flask handles encoding correctly.
128-140: Positive-path assertions look solid.Validates id/name/description and ensures the endpoint returns the raw product JSON as intended.
app/routes.py (1)
884-914: LGTM: Query-param lookup with clear 400/404 semantics.Switching to
GET /product?name=...simplifies routing, and returningproduct.to_json()aligns with “only product data” intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_product.py (2)
1-1: Remove unused import (ruff F401).
quoteis no longer used after switching to the query parameter approach.Apply this diff:
-from urllib.parse import quote
119-143: Good special-character coverage; add assertions to prove “only product data” and a missing-param test.
- Add negative assertions to ensure no derived fields (e.g., categories/subcategories) are returned.
- Add a test that omits the
namequery param to assert a 400, matching the route behavior.Apply this diff to strengthen the existing test:
prod_data = get_resp.get_json() assert prod_data["id"] == p_id assert prod_data["name"] == name assert prod_data["description"] == "desc" + assert "categories" not in prod_data + assert "subcategories" not in prod_dataAdd this test method inside
TestProduct(outside the parametrized test):def test_get_product_by_name_missing_param(self): resp = self.client.get("/product") # no query_string assert resp.status_code == 400app/routes.py (2)
884-913: Document the new 400 response in the Swagger block.You now return 400 for missing
name, but the docstring doesn’t list it.Apply this diff within the docstring:
responses: description: Product retrieved successfully. + 400: + description: Missing required query parameter 'name'. description: Product not found. description: Error occurred.
906-913: Optional: simplify lookup with first_or_404 and filter_by.Shrinks boilerplate and keeps semantics identical.
Apply this diff:
- name = request.args.get('name') + name = request.args.get('name') if not name: abort(400, description="Missing required query parameter 'name'") - product = Product.query.filter(Product.name == name).first() - if product is None: - abort(404) - return jsonify(product.to_json()), 200 + product = Product.query.filter_by(name=name).first_or_404() + return jsonify(product.to_json()), 200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/routes.py(2 hunks)tests/test_product.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/test_product.py
1-1: urllib.parse.quote imported but unused
Remove unused import: urllib.parse.quote
(F401)
🔇 Additional comments (1)
app/routes.py (1)
884-913: Switch to query param is clean and aligns with tests.Endpoint now correctly reads
namefromrequest.args, returns 400 on missing, 404 when not found, and only product JSON. Matches PR intent and improves robustness for special chars.
✅ Actions performedReviews resumed. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Summary by CodeRabbit
Refactor
Tests