Update feedback responses to match road core #381
Conversation
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
WalkthroughUpdates align the feedback endpoint’s OpenAPI docs with implementation: refined 200/401/403 responses, removed 400, and added a 500 error. Introduces a reusable ErrorResponse schema/model and wires it into the endpoint’s documented responses. Runtime behavior stays the same; changes are documentation and schema additions. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Feedback API (/v1/feedback)
participant Store as Storage
Client->>API: POST feedback
API->>Store: Save feedback
alt Success
Store-->>API: OK
API-->>Client: 200 FeedbackResponse
else Unauthorized
API-->>Client: 401 UnauthorizedResponse
else Forbidden
API-->>Client: 403 Forbidden
else Storage error
Store-->>API: Failure
API-->>Client: 500 ErrorResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ 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 comments)
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: 0
🔭 Outside diff range comments (2)
src/app/endpoints/feedback.py (1)
96-105: Avoid leaking internal exception details in client-facing 500s; include stack trace in logs.Returning str(e) in the HTTP response can leak filesystem paths or provider errors. Log the exception with stack trace, but send a generic cause to clients.
- except Exception as e: - logger.error("Error storing user feedback: %s", e) + except Exception as e: + logger.exception("Error storing user feedback") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail={ "response": "Error storing user feedback", - "cause": str(e), + "cause": "Internal server error", }, ) from edocs/openapi.json (1)
285-356: Standardize “Missing or invalid credentials” responses to use 401
There are two operations in docs/openapi.json that currently return 400 for unauthorized requests while others use 401 with the same description. For consistency, update these to 401:• docs/openapi.json:137–140 (change
"400": { … }to"401": { … })
• docs/openapi.json:641–644 (change"400": { … }to"401": { … })
🧹 Nitpick comments (6)
src/models/responses.py (2)
475-497: Docstring scope + example polish (grammar, UUID, vendor URL).
- Docstring says “for query endpoint” but this model is used across endpoints (feedback 500). Make it generic.
- Grammar: “Error while validation question” → “Error while validating question”.
- Example UUID “1237-…” isn’t a valid UUID shape; use the existing example UUID used elsewhere in this file.
- Prefer avoiding vendor-specific URLs in public examples; replace with a generic upstream hostname.
Apply:
-class ErrorResponse(BaseModel): - """Model representing error response for query endpoint.""" +class ErrorResponse(BaseModel): + """Generic error response payload used across endpoints.""" @@ - "examples": [ - { - "detail": { - "response": "Error while validation question", - "cause": "Failed to handle request to https://bam-api.res.ibm.com/v2/text", - }, - }, - { - "detail": { - "response": "Error retrieving conversation history", - "cause": "Invalid conversation ID 1237-e89b-12d3-a456-426614174000", - }, - }, - ] + "examples": [ + { + "detail": { + "response": "Error while validating question", + "cause": "Failed to handle upstream request to https://example.com/api", + }, + }, + { + "detail": { + "response": "Error retrieving conversation history", + "cause": "Invalid conversation ID 123e4567-e89b-12d3-a456-426614174000", + }, + }, + ]
475-497: Consider deduplicating error payload models.NotAvailableResponse and ErrorResponse have identical shapes (detail: dict[str, str]) with different examples. Consider unifying to a single ErrorResponse (with multiple example sets) to reduce duplication and keep schemas consistent.
src/app/endpoints/feedback.py (2)
28-44: Responses mapping alignment: LGTM; minor wording nit (“cannot”).
- 200 now returns FeedbackResponse: good.
- 401/403 descriptions updated: good.
- 500 added with ErrorResponse: good.
- Nit: prefer “cannot” over “can not” for consistency.
- 500: { - "description": "User feedback can not be stored", + 500: { + "description": "User feedback cannot be stored", "model": ErrorResponse, },
120-132: Validate and guard feedback storage path (prevent accidental writes to CWD).When feedback_storage is unset, Path("") resolves to the current working directory. Consider validating the config and failing fast with 500 instead of writing into CWD.
- storage_path = Path( - configuration.user_data_collection_configuration.feedback_storage or "" - ) + storage_root = configuration.user_data_collection_configuration.feedback_storage + if not storage_root: + logger.error("Feedback storage path is not configured") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail={ + "response": "Error storing user feedback", + "cause": "Feedback storage path is not configured", + }, + ) + storage_path = Path(storage_root)I can update the OpenAPI example text for the 500 to match this cause if you adopt it.
docs/openapi.json (2)
305-343: Feedback endpoint responses updated correctly; nit on “cannot.”The 200/401/403 updates and 500 with ErrorResponse align with the code. Please change “User feedback can not be stored” to “User feedback cannot be stored” for consistency.
- "description": "User feedback can not be stored", + "description": "User feedback cannot be stored",
1193-1223: ErrorResponse schema: description scope + example polish (grammar, UUID).
- Description says “for query endpoint” but it’s reused (feedback 500). Make it generic.
- Grammar: “Error while validation question” → “Error while validating question”.
- Fix example UUID to the commonly used valid sample.
- "ErrorResponse": { + "ErrorResponse": { "properties": { @@ - "title": "ErrorResponse", - "description": "Model representing error response for query endpoint.", + "title": "ErrorResponse", + "description": "Generic error response payload used across endpoints.", "examples": [ { "detail": { - "cause": "Failed to handle request to https://bam-api.res.ibm.com/v2/text", - "response": "Error while validation question" + "cause": "Failed to handle upstream request to https://example.com/api", + "response": "Error while validating question" } }, { "detail": { - "cause": "Invalid conversation ID 1237-e89b-12d3-a456-426614174000", + "cause": "Invalid conversation ID 123e4567-e89b-12d3-a456-426614174000", "response": "Error retrieving conversation history" } } ] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/openapi.json(4 hunks)src/app/endpoints/feedback.py(2 hunks)src/models/responses.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app/endpoints/feedback.py (1)
src/models/responses.py (4)
ErrorResponse(475-497)FeedbackResponse(189-212)UnauthorizedResponse(271-285)ForbiddenResponse(288-300)
⏰ 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: e2e_tests
🔇 Additional comments (1)
src/app/endpoints/feedback.py (1)
14-19: Import addition looks correct.Importing ErrorResponse to wire the 500 schema is aligned with the OpenAPI changes.
2 participants
Description
Updates the responses to match what was originally in Road Core: https://github.com/road-core/service/blob/9d65d15a4d1dec47e5aac15ee86fef39db975006/ols/app/endpoints/feedback.py#L93-L110
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation