LCORE-463: Added missing error handling #420
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded try/except around feedback JSON file write to catch and log OSError/IOError, re-raising on failure. Introduced unit tests validating permission-denied behavior using non-writable path and different payload variants. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Endpoint as store_feedback
participant FS as FileSystem
Client->>Endpoint: store_feedback(payload, path)
rect rgba(220,235,255,0.5)
note right of Endpoint: Attempt to persist JSON
Endpoint->>FS: write(path, json)
alt write succeeds
FS-->>Endpoint: ok
Endpoint-->>Client: return success (log info)
else OSError/IOError
FS-->>Endpoint: error
note right of Endpoint: Log error with target path
Endpoint-->>Client: raise OSError/IOError
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
lcore-463-missing-error-handling
branch
2 times, most recently
from
783c672 to
ab75c32
Compare
|
@CodeRabbit fullreview |
|
@tisnik I'll conduct a full review of the PR changes for LCORE-463: Added missing error handling. ✅ Actions performedFull review triggered. |
![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 (2)
src/app/endpoints/feedback.py (2)
143-145: Prevent client payload from overriding server-controlled fields (user_id, timestamp).With the current merge order, user-supplied keys in
feedbackcan overrideuser_idandtimestamp, compromising integrity. Ensure server fields win.Apply this diff:
- current_time = str(datetime.now(UTC)) - data_to_store = {"user_id": user_id, "timestamp": current_time, **feedback} + current_time = datetime.now(UTC).isoformat() + # Server-controlled fields must not be overridden by client input + data_to_store = {**feedback, "user_id": user_id, "timestamp": current_time}
110-118: Avoid leaking internal exception details to API clients.Returning
str(e)(which can include filesystem paths, errno, etc.) in the 500 response is an information disclosure risk. Keep details in server logs; return a generic error to clients.Apply this diff:
- 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), }, ) from e
🧹 Nitpick comments (3)
src/app/endpoints/feedback.py (2)
148-153: Good addition; tighten logging and simplify exception type.Catching OSError is sufficient in Python 3 (IOError aliases to OSError). Also, log the stack trace for easier debugging.
Apply this diff:
- try: - with open(feedback_file_path, "w", encoding="utf-8") as feedback_file: - json.dump(data_to_store, feedback_file) - except (OSError, IOError) as e: - logger.error("Failed to store feedback at %s: %s", feedback_file_path, e) - raise + try: + with open(feedback_file_path, "w", encoding="utf-8") as feedback_file: + json.dump(data_to_store, feedback_file) + except OSError as e: + logger.exception("Failed to store feedback at %s", feedback_file_path) + raise
138-142: Also handle directory creation errors with logging context.
mkdircan fail (permissions, read-only FS, invalid path). Mirror the write-path handling so failures are logged with the target directory.Apply this diff:
- storage_path.mkdir(parents=True, exist_ok=True) + try: + storage_path.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.exception("Failed to prepare feedback storage directory %s", storage_path) + raisetests/unit/app/endpoints/test_feedback.py (1)
131-155: Optional: Verify server-enforced fields override any client-supplied duplicates.If you adopt the merge-order fix, add a regression test to ensure a client cannot spoof
user_id/timestamp.Example test to add near this block:
def test_store_feedback_overrides_spoofed_fields(mocker): configuration.user_data_collection_configuration.feedback_storage = "fake-path" mocker.patch("builtins.open", mocker.mock_open()) mocker.patch("app.endpoints.feedback.Path", return_value=mocker.MagicMock()) mocker.patch("app.endpoints.feedback.get_suid", return_value="fake-uuid") mock_json = mocker.patch("app.endpoints.feedback.json") user_id = "real_user_id" payload = {"user_id": "spoofed", "timestamp": "1970-01-01T00:00:00Z"} store_feedback(user_id, payload) args, kwargs = mock_json.dump.call_args stored = args[0] assert stored["user_id"] == user_id assert stored["timestamp"] != "1970-01-01T00:00:00Z"
📜 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)
src/app/endpoints/feedback.py(1 hunks)tests/unit/app/endpoints/test_feedback.py(1 hunks)
tisnik
force-pushed
the
lcore-463-missing-error-handling
branch
from
ab75c32 to
ffb1c29
Compare
1 participant
Description
LCORE-463: Added missing error handling
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Bug Fixes
Tests