csv download on public dashboard working #1196
Conversation
WalkthroughExtracted CSV streaming into a reusable generator Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PrivateAPI as Private Endpoint
participant PublicAPI as Public Endpoint
participant StreamFn as stream_chart_data_csv()
participant Service as charts_service
rect rgb(230,240,255)
Note over PrivateAPI,PublicAPI: CSV Download Flows (auth/validation in endpoints)
end
Client->>PrivateAPI: request with auth + payload
PrivateAPI->>PrivateAPI: validate schema & resolve org_warehouse
PrivateAPI->>StreamFn: call stream_chart_data_csv(org_warehouse, payload)
StreamFn->>Service: get_chart_data_table_preview(page=1..N)
Service-->>StreamFn: page response (headers + rows)
StreamFn->>Client: yield CSV chunk(s)
StreamFn->>StreamFn: close buffer
Client->>PublicAPI: request token + chart_id + payload
PublicAPI->>PublicAPI: validate token, verify chart ownership, resolve org_warehouse
PublicAPI->>StreamFn: call stream_chart_data_csv(org_warehouse, payload)
StreamFn->>Service: get_chart_data_table_preview(page=1..N)
Service-->>StreamFn: page response
StreamFn->>Client: yield CSV chunk(s)
StreamFn->>StreamFn: close buffer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1196 +/- ##
==========================================
- Coverage 51.10% 50.98% -0.12%
==========================================
Files 97 97
Lines 11798 11824 +26
==========================================
Hits 6029 6029
- Misses 5769 5795 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ddpui/api/public_api.py (1)
964-969: Improve exception handling and logging.The exception handling could be improved in several ways:
- Use
logger.exception: This provides full stack traces, which are essential for debugging production issues.- Add exception chaining: Use
from eto preserve the exception chain for better debugging.- Consistent error messages: The error message includes
str(e)which can expose internal details to public users.Apply this diff to improve exception handling:
except Dashboard.DoesNotExist: logger.warning(f"Public CSV download failed - dashboard not found for token: {token}") - raise HttpError(404, "Dashboard not found or no longer public") + raise HttpError(404, "Dashboard not found or no longer public") from None except Exception as e: - logger.error(f"Public CSV download error for chart {chart_id}: {str(e)}") - raise HttpError(500, f"CSV download failed: {str(e)}") + logger.exception(f"Public CSV download error for chart {chart_id}") + raise HttpError(500, "CSV download failed") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ddpui/api/charts_api.py(1 hunks)ddpui/api/public_api.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ddpui/api/charts_api.py (6)
ddpui/schemas/chart_schema.py (1)
ChartDataPayload(57-92)ddpui/core/charts/charts_service.py (1)
get_chart_data_table_preview(1145-1215)ddpui/utils/custom_logger.py (4)
warning(49-53)info(25-29)error(31-35)exception(43-47)ddpui/auth.py (1)
has_permission(30-51)ddpui/models/org_user.py (1)
OrgUser(67-88)ddpui/models/org.py (1)
OrgWarehouse(128-149)
ddpui/api/public_api.py (4)
ddpui/schemas/chart_schema.py (1)
ChartDataPayload(57-92)ddpui/models/visualization.py (1)
Chart(34-67)ddpui/models/org.py (1)
OrgWarehouse(128-149)ddpui/api/charts_api.py (1)
stream_chart_data_csv(790-856)
🪛 Ruff (0.14.2)
ddpui/api/charts_api.py
824-824: Local variable header_written is assigned to but never used
Remove assignment to unused variable header_written
(F841)
854-854: Use explicit conversion flag
Replace with conversion flag
(RUF010)
854-854: Redundant exception object included in logging.exception call
(TRY401)
856-856: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
ddpui/api/public_api.py
913-913: Unused function argument: request
(ARG001)
935-935: Abstract raise to an inner function
(TRY301)
940-940: Abstract raise to an inner function
(TRY301)
962-962: Consider moving this statement to an else block
(TRY300)
966-966: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
967-967: Do not catch blind exception: Exception
(BLE001)
968-968: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
968-968: Use explicit conversion flag
Replace with conversion flag
(RUF010)
969-969: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
969-969: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: checks (3.10, 6)
🔇 Additional comments (4)
ddpui/api/charts_api.py (1)
859-888: LGTM! Clean refactoring.The refactoring successfully extracts CSV streaming logic into a reusable function while maintaining proper authentication, authorization, and access control in the endpoint. The filename generation and response configuration are appropriate.
ddpui/api/public_api.py (3)
6-6: LGTM! Necessary imports for CSV streaming.The new imports are all required for the CSV download endpoint functionality.
Also applies to: 11-12, 32-32
913-913: Note:requestparameter required by framework.While static analysis flags
requestas unused, it's required by the Django Ninja framework for all endpoint handlers. This is a false positive that can be safely ignored.
912-969: LGTM! Proper authentication and authorization for public CSV downloads.The endpoint correctly validates public dashboard access via token, ensures the chart belongs to the dashboard's organization, and reuses the common CSV streaming function. The filename generation and response configuration are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ddpui/api/charts_api.py (2)
790-854: Well-structured streaming implementation with proper resource management.The CSV streaming function correctly addresses previous review feedback by using a
finallyblock to ensure the StringIO buffer is closed and preserving the exception chain withfrom error. The pagination logic and memory-efficient buffer management are well-implemented.Consider adding a return type hint for better type safety and IDE support:
+from typing import Generator + -def stream_chart_data_csv(org_warehouse, payload: ChartDataPayload, page_size=5000): +def stream_chart_data_csv( + org_warehouse, payload: ChartDataPayload, page_size=5000 +) -> Generator[str, None, None]:
857-886: Clean refactoring with proper authentication and authorization flow.The endpoint correctly delegates CSV streaming to the shared function while maintaining appropriate access controls and warehouse validation before initiating the stream.
Minor inconsistency in error message wording at line 866 compared to line 554:
- raise HttpError(403, "Access to schema denied") + raise HttpError(403, "Access denied to schema")This matches the wording used in the
get_chart_dataendpoint at line 554.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/api/charts_api.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ddpui/api/charts_api.py (4)
ddpui/schemas/chart_schema.py (1)
ChartDataPayload(57-92)ddpui/core/charts/charts_service.py (1)
get_chart_data_table_preview(1145-1215)ddpui/models/org_user.py (1)
OrgUser(67-88)ddpui/models/org.py (1)
OrgWarehouse(128-149)
⏰ 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: checks (3.10, 6)
Summary by CodeRabbit