Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds offline export workflows: frontend UI/routes and API helpers for export request creation, SQL pre-check, and download; backend model/migration for schema_name and TSV support; download/storage flow changes; new serializers/endpoints/permissions; tests; and devops/script tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant Frontend as Frontend App
participant Backend as Backend API
participant Engine as Query Engine
participant Storage as File Storage
User->>Frontend: Open "New Export Request" page
Frontend->>Backend: GET /v1/workflow/export/submission-metadata/
Backend-->>Frontend: submission scope (groups/instances/config)
User->>Frontend: Enter instance/db/schema and SQL
Frontend->>Backend: POST /v1/workflow/export/sqlcheck/
Backend->>Engine: pre_count_check(sql, schema_name)
Engine-->>Backend: row count / validation result
Backend-->>Frontend: return check result
User->>Frontend: Submit export (POST /v1/workflow/)
Frontend->>Backend: create workflow (is_offline_export=1, export_format)
Backend->>Engine: schedule/execute export -> write file
Engine->>Storage: store export file (csv/tsv/...)
Backend-->>Frontend: return workflow id/status
User->>Frontend: Request download
Frontend->>Backend: GET /v1/workflow/{id}/download/
Backend->>Storage: fetch file or generate redirect URL
Storage-->>Backend: file/redirect
Backend-->>Frontend: return blob or redirect URL
Frontend->>User: trigger browser download or navigate to URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
jruszo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
sql/test_offlinedownload.py (1)
483-487: Assert the TSV delimiter, not just the filename.These checks still pass if the new TSV branch writes comma-separated data with a
.tsvextension, so the main regression this feature adds would go unnoticed. Open the generated file and verify at least one tab-delimited row here.Proposed test tightening
tsv_file_name = save_to_format_file( "tsv", result, self.workflow, columns, temp_dir ) self.assertTrue(tsv_file_name.endswith(".tsv")) - self.assertTrue(os.path.exists(os.path.join(temp_dir, tsv_file_name))) + tsv_path = os.path.join(temp_dir, tsv_file_name) + self.assertTrue(os.path.exists(tsv_path)) + with open(tsv_path, newline="", encoding="utf-8") as handle: + rows = list(csv.reader(handle, delimiter="\t")) + self.assertEqual(rows[0], columns) + self.assertEqual(rows[1], ["1", "test1"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/test_offlinedownload.py` around lines 483 - 487, The test only checks the .tsv filename and file existence but not that the file actually uses tabs; update the test around save_to_format_file / tsv_file_name to open the generated file in temp_dir and assert that at least one data row contains tab-separated columns (e.g., contains '\t' between expected column values) and/or does not use commas as the delimiter, so the TSV branch is validated by content rather than just filename.sql_api/tests.py (1)
2245-2296: Use a dedicated export fixture instead of widening the shared workflow one.The baseline
TestWorkflowuser/instance now carries both SQL-submit and export-read/download capabilities. That makes the older non-export workflow tests less sensitive to permission mix-ups between the SQL and export paths. A separate export-only helper/user would keep that matrix explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/tests.py` around lines 2245 - 2296, The shared test fixture is being widened to include both SQL and export permissions (see user.user_permissions.add and the instance tags on self.ins), which masks permission-specific bugs; instead create a dedicated export-only fixture/user and export-only instance (a separate helper setup used by export tests) that only grants export-related permissions (sqlexport_submit, offline_download, menu_sqlexportworkflow and related export tags) and do not add SQL-only permissions (sql_submit, sql_execute, sql_review, etc.) or SQL instance tags; update export tests to use this new export fixture and leave the original workflow fixture (the one constructing self.user, self.ins, WorkflowAuditSetting) unchanged so SQL-only tests remain isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/lib/api.ts`:
- Around line 1386-1395: The blob variant of WorkflowDownloadResult uses
property names that don't match callers; change the variant from { mode: 'blob';
blob: Blob; fileName: string } to use the expected names ({ mode: 'blob'; data:
Blob; filename: string }) so callers in WorkflowsView.vue that access
result.data and result.filename work correctly; update the same shaped
type/returns where else defined/used (the other occurrence noted around the
second definition) and any factory/return sites that construct the blob result
(search for WorkflowDownloadResult, the 'blob' mode, and functions that return
fileName/blob) to produce data and filename instead of blob and fileName.
In `@frontend/src/views/ExportWorkflowCreateView.vue`:
- Around line 55-64: The form and handlers drop the schema-scoped context:
update the reactive form object (the const form declaration) to include a
schemaName (or schema_name) field, propagate that field into the validation
call(s) and the submit payloads referenced around the other ranges (lines
~160-180, 217-235, 286-300), and ensure the same key is used where QueriesView
expects schema_name so exports use the exact schema the user tested; adjust any
validation/submit function signatures and payload construction (e.g., the submit
handler and validate function) to include schemaName and pass it through to the
backend.
- Around line 121-130: The computed canSubmit currently ignores whether the
latest validation reported blocking errors; update the canSubmit computed (the
canSubmit getter) to also require that checkResult.value?.error_count === 0 (or
equivalently that error_count is falsy) in addition to existing checks
(form.workflowName, groupId, instanceId, dbName, sqlContent,
checkResult.value?.syntax_type === 3, and isCheckFresh.value) so the submit
button stays disabled when the most recent check still has errors.
In `@sql_api/api_workflow.py`:
- Around line 752-770: This path currently passes raw db_name into
OffLineDownLoad().pre_count_check and lets SQL/count failures raise 500s; mirror
ExecuteCheck’s behavior by applying the same escaping/normalization used there:
use ExecuteCheckSerializer.validated_data["db_name"] after applying the same
escape/sanitize utility used in ExecuteCheck, call the pre_count_check inside a
try/except that catches DB/SQL errors and re-raises them as a DRF
ValidationError (or the same normalized error type ExecuteCheck uses), and
ensure any error messages returned match ExecuteCheck’s normalized format so
invalid db_name or COUNT(*) failures surface as validation errors instead of
unhandled 500s. Ensure you modify the block around
serializer/instance/export_probe and the call to
OffLineDownLoad().pre_count_check to implement these steps.
In `@sql_api/serializers.py`:
- Around line 1302-1318: The pre_count_check is being called without the chosen
export format, so format-specific validation (e.g., XLSX size limits) is
skipped; fix by making the workflow instance reflect the selected export_format
before validation: after normalizing workflow_data["export_format"], set
instance.export_format = export_format (and/or pass export_format into
sql_export.pre_count_check like sql_export.pre_count_check(workflow=instance,
export_format=export_format) if that parameter exists) so pre_count_check runs
with the correct format context.
In `@sql_api/tests.py`:
- Around line 3419-3435: The test user "blocked_user" must be made the
workflow's engineer/owner so the 403 can only be caused by the missing
offline_download permission; after creating and logging in the user, assign them
as the workflow engineer/owner on export_workflow (e.g., set
export_workflow.engineer = blocked_user or add blocked_user to export_workflow's
engineers/owners collection and save) and ensure they retain
menu_sqlexportworkflow but do NOT have the offline_download permission (remove
or never add Permission.objects.get(codename="offline_download")). Then run the
same GET and assert the 403 to isolate the offline_download gate.
In `@sql/offlinedownload.py`:
- Around line 456-462: The legacy wrapper function offline_file_download
currently accepts client-supplied file_name and workflow_id and bypasses
WorkflowDownload authorization; remove this wrapper or change it to resolve the
Workflow by the provided workflow_id (use the same model lookup logic used by
WorkflowDownload), derive/validate the allowed artifact file_name server-side
(do not accept client-provided file_name), and then call the existing
download_export_file/WorkflowDownload flow so ownership and permission checks
run; ensure you reference offline_file_download, download_export_file and
WorkflowDownload when updating the code.
---
Nitpick comments:
In `@sql_api/tests.py`:
- Around line 2245-2296: The shared test fixture is being widened to include
both SQL and export permissions (see user.user_permissions.add and the instance
tags on self.ins), which masks permission-specific bugs; instead create a
dedicated export-only fixture/user and export-only instance (a separate helper
setup used by export tests) that only grants export-related permissions
(sqlexport_submit, offline_download, menu_sqlexportworkflow and related export
tags) and do not add SQL-only permissions (sql_submit, sql_execute, sql_review,
etc.) or SQL instance tags; update export tests to use this new export fixture
and leave the original workflow fixture (the one constructing self.user,
self.ins, WorkflowAuditSetting) unchanged so SQL-only tests remain isolated.
In `@sql/test_offlinedownload.py`:
- Around line 483-487: The test only checks the .tsv filename and file existence
but not that the file actually uses tabs; update the test around
save_to_format_file / tsv_file_name to open the generated file in temp_dir and
assert that at least one data row contains tab-separated columns (e.g., contains
'\t' between expected column values) and/or does not use commas as the
delimiter, so the TSV branch is validated by content rather than just filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77f159fc-8b7b-439e-9b8b-9f376a6a7338
📒 Files selected for processing (12)
frontend/src/lib/api.tsfrontend/src/router/index.tsfrontend/src/views/ExportWorkflowCreateView.vuefrontend/src/views/QueriesView.vuefrontend/src/views/WorkflowsView.vuesql/models.pysql/offlinedownload.pysql/test_offlinedownload.pysql_api/api_workflow.pysql_api/serializers.pysql_api/tests.pysql_api/urls.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
frontend/src/views/ExportWorkflowCreateView.vue (1)
124-133:⚠️ Potential issue | 🟡 MinorKeep submit disabled when the latest validation is still blocked.
Line 131 only checks
syntax_type === 3. After a fresh validation witherror_count > 0, Line 453 enables the button again even thoughsubmitWorkflow()stops immediately at Lines 284-286. FoldcheckResult.value?.error_count === 0intocanSubmitso the UI does not expose a guaranteed-failure path.Also applies to: 453-456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/ExportWorkflowCreateView.vue` around lines 124 - 133, The computed canSubmit currently requires checkResult.value?.syntax_type === 3 but not that checkResult.value?.error_count === 0, so the UI can enable submission while a fresh validation still reports errors; update the canSubmit computed to also require checkResult.value?.error_count === 0 (in addition to syntax_type and isCheckFresh) so the submit button stays disabled when the latest validation has errors, and ensure the same check is applied where canSubmit is referenced (e.g., places around submitWorkflow and the enablement at the earlier referenced UI logic) so submitWorkflow (which inspects checkResult.error_count) is not exposed by the UI as an immediately-failing path.sql_api/api_workflow.py (1)
756-775:⚠️ Potential issue | 🟠 MajorNormalize export-check inputs and failures the same way as
ExecuteCheck.Line 772 passes the raw
db_name, and Line 774 letspre_count_check()exceptions bubble straight out of the endpoint. A bad database name or count-query failure will still surface here as a 500 even though the regular SQL-check path escapes the name and translates engine errors intoserializers.ValidationError. Reuse that same normalization before returning the export check result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/api_workflow.py` around lines 756 - 775, The endpoint is using raw serializer.validated_data["db_name"] and letting OffLineDownLoad().pre_count_check(workflow=export_probe) raise uncaught exceptions, causing 500s instead of normalized validation errors; update the export path to mirror the ExecuteCheck normalization: sanitize/escape the db_name and schema_name the same way ExecuteCheck does before assigning to export_probe (use the same normalization routine or helper used by ExecuteCheck/ExecuteCheckSerializer), call pre_count_check inside a try/except that catches engine/driver errors and re-raises them as serializers.ValidationError, and then set check_result.rows = check_result.to_dict() as before so the API returns a ValidationError on bad db names or count-query failures; reference ExecuteCheckSerializer, ExecuteCheck normalization helper, export_probe, OffLineDownLoad.pre_count_check, and check_result.rows when making changes.sql/offlinedownload.py (1)
463-469:⚠️ Potential issue | 🔴 CriticalRemove the legacy wrapper or derive the filename server-side.
offline_file_download()still trustsfile_nameandworkflow_idfromrequest.GETand forwards them straight todownload_export_file(). Sincesql/urls.pystill exposes this wrapper, callers can bypassWorkflowDownload's ownership/permission checks and request arbitrary stored artifacts. Resolve the workflow by id, derive the allowed filename from that record, and enforce the same authorization path asWorkflowDownload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/offlinedownload.py` around lines 463 - 469, Remove trusting user-supplied file_name: instead read workflow_id from request.GET, load the Workflow record (e.g., via Workflow.objects.get(pk=workflow_id)) and derive the allowed filename server-side from that record, then call download_export_file with that derived filename; enforce the same authorization/ownership checks used by WorkflowDownload (reuse its permission-checking logic or call into WorkflowDownload helper methods) and return an error if workflow_id is missing/invalid or the user is not authorized. Ensure no direct use of request.GET["file_name"] and handle exceptions for missing workflow or permission denied.sql_api/tests.py (1)
3429-3445:⚠️ Potential issue | 🟡 MinorMake this user the workflow engineer to isolate the
offline_downloadpermission check.The test intends to verify that the
offline_downloadpermission is required for downloading, butblocked_userisn't the workflow engineer and may lack general visibility to the workflow. The 403 response could be coming from visibility checks rather than the intended permission gate.Suggested adjustment
blocked_user.user_permissions.add( Permission.objects.get(codename="menu_sqlexportworkflow") ) blocked_user.resource_group.add(self.res_group.group_id) + export_workflow.engineer = blocked_user.username + export_workflow.engineer_display = blocked_user.display + export_workflow.save(update_fields=["engineer", "engineer_display"]) self._login_as_user(blocked_user.username)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/tests.py` around lines 3429 - 3445, Assign the test user as the workflow engineer so the 403 will reflect the offline_download permission check rather than visibility: set export_workflow.engineer = blocked_user (or the appropriate engineer FK/update API on the Workflow model used in tests) and save the workflow before calling the download endpoint, keeping the user without the offline_download permission so the request is denied for that reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/WorkflowsView.vue`:
- Around line 533-562: downloadSelectedExport leaves downloadSubmitting.value
true when result.mode === 'redirect' because of the early return; before
performing the redirect (inside downloadSelectedExport) explicitly set
downloadSubmitting.value = false (or move the redirect logic after clearing
downloadSubmitting) so the UI button exits the "Preparing download..." state;
reference downloadSelectedExport and the downloadSubmitting reactive variable
when making the change.
In `@sql_api/api_workflow.py`:
- Around line 120-123: The download_available SerializerMethodField currently
returns _workflow_download_available(obj) which only checks workflow state and
file presence; update get_download_available to also check the same permission
predicate used by WorkflowDownload.get() (i.e., user has sql.offline_download
for this workflow/export) so users who can view but not download get
download_available=False; specifically, in get_download_available (and the other
similar uses of _workflow_download_available at the other locations) combine the
existing _workflow_download_available(obj) result with a permission check
against the current request/user's sql.offline_download permission (mirroring
WorkflowDownload.get()) before returning the final boolean.
In `@sql_api/serializers.py`:
- Around line 42-43: EXPORT_FORMAT_CHOICES in serializers.py is out of sync with
sql.models.SqlWorkflow.export_format and sql.offlinedownload.EXPORT_FORMATS
which still include "json" and "xml"; pick one contract and make all layers
match. Either add "json" and "xml" to EXPORT_FORMAT_CHOICES (so serializers.py
contains {"csv","tsv","sql","xlsx","json","xml"}) and use that single constant
from serializers where API validation occurs, or remove "json" and "xml" from
sql.models.SqlWorkflow.export_format and sql.offlinedownload.EXPORT_FORMATS so
all three only allow csv/tsv/sql/xlsx; update any validation/serialization code
that references EXPORT_FORMAT_CHOICES, SqlWorkflow.export_format, or
EXPORT_FORMATS to import/use the same canonical set and run/adjust tests
accordingly.
In `@sql/migrations/0005_alter_sqlworkflow_export_format.py`:
- Around line 8-17: The generated migration has formatting violations; run Black
on the file and reformat the dependencies list and the migrations.AlterField
call for model_name='sqlworkflow' and name='export_format' so the tuple, list
and long CharField argument are wrapped/line-broken per Black rules;
specifically run `black` (or `black --check .` to verify) and commit the
reformatted migration so the dependencies = [...] and the
migrations.AlterField(...) block conform to project style.
In `@sql/migrations/0006_sqlworkflow_schema_name.py`:
- Around line 8-17: The migration AddField for model_name='sqlworkflow' adding
the 'schema_name' CharField is not formatted with Black; run Black on this file
(or the project) to reformat the migration so `black --check` passes (e.g., run
`black sql/migrations/0006_sqlworkflow_schema_name.py`), then update the commit;
ensure the file follows Black style while keeping the AddField call,
model_name='sqlworkflow', and name='schema_name' unchanged.
In `@sql/offlinedownload.py`:
- Around line 147-152: The code dereferences result_set.rows (actual_rows_check)
before checking for errors from check_engine.query, which can raise on error or
empty results; change the logic around check_engine.query / result_set to first
check result_set.error (and that result_set.rows is non-empty) and only then
read result_set.rows[0][0], otherwise set affected_rows = 0 (or a safe default)
and propagate or log the structured validation error; apply the same guard
pattern to the later block that computes affected_rows (lines referenced around
166-174) so both places validate result_set.error and rows length before
accessing elements.
In `@src/docker-compose/docker-compose.local-arm.yml`:
- Line 27: The env var MYSQL_ROOT_HOST is set to "%" which allows root access
from any host; change MYSQL_ROOT_HOST to a scoped value such as "localhost" or
the Docker gateway IP (or remove it to use the image default), and update the
compose setup so the application uses a dedicated non-root DB user/credentials
(create a new user via init SQL or Docker envs) instead of root while keeping
port 3306 published only if necessary; ensure references to MYSQL_ROOT_HOST and
any DB connection envs are updated accordingly.
---
Duplicate comments:
In `@frontend/src/views/ExportWorkflowCreateView.vue`:
- Around line 124-133: The computed canSubmit currently requires
checkResult.value?.syntax_type === 3 but not that checkResult.value?.error_count
=== 0, so the UI can enable submission while a fresh validation still reports
errors; update the canSubmit computed to also require
checkResult.value?.error_count === 0 (in addition to syntax_type and
isCheckFresh) so the submit button stays disabled when the latest validation has
errors, and ensure the same check is applied where canSubmit is referenced
(e.g., places around submitWorkflow and the enablement at the earlier referenced
UI logic) so submitWorkflow (which inspects checkResult.error_count) is not
exposed by the UI as an immediately-failing path.
In `@sql_api/api_workflow.py`:
- Around line 756-775: The endpoint is using raw
serializer.validated_data["db_name"] and letting
OffLineDownLoad().pre_count_check(workflow=export_probe) raise uncaught
exceptions, causing 500s instead of normalized validation errors; update the
export path to mirror the ExecuteCheck normalization: sanitize/escape the
db_name and schema_name the same way ExecuteCheck does before assigning to
export_probe (use the same normalization routine or helper used by
ExecuteCheck/ExecuteCheckSerializer), call pre_count_check inside a try/except
that catches engine/driver errors and re-raises them as
serializers.ValidationError, and then set check_result.rows =
check_result.to_dict() as before so the API returns a ValidationError on bad db
names or count-query failures; reference ExecuteCheckSerializer, ExecuteCheck
normalization helper, export_probe, OffLineDownLoad.pre_count_check, and
check_result.rows when making changes.
In `@sql_api/tests.py`:
- Around line 3429-3445: Assign the test user as the workflow engineer so the
403 will reflect the offline_download permission check rather than visibility:
set export_workflow.engineer = blocked_user (or the appropriate engineer
FK/update API on the Workflow model used in tests) and save the workflow before
calling the download endpoint, keeping the user without the offline_download
permission so the request is denied for that reason.
In `@sql/offlinedownload.py`:
- Around line 463-469: Remove trusting user-supplied file_name: instead read
workflow_id from request.GET, load the Workflow record (e.g., via
Workflow.objects.get(pk=workflow_id)) and derive the allowed filename
server-side from that record, then call download_export_file with that derived
filename; enforce the same authorization/ownership checks used by
WorkflowDownload (reuse its permission-checking logic or call into
WorkflowDownload helper methods) and return an error if workflow_id is
missing/invalid or the user is not authorized. Ensure no direct use of
request.GET["file_name"] and handle exceptions for missing workflow or
permission denied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75997a87-30bd-409e-92c4-72a1cdfeb5e1
📒 Files selected for processing (14)
frontend/src/lib/api.tsfrontend/src/views/ExportWorkflowCreateView.vuefrontend/src/views/QueriesView.vuefrontend/src/views/WorkflowsView.vuesql/migrations/0005_alter_sqlworkflow_export_format.pysql/migrations/0006_sqlworkflow_schema_name.pysql/models.pysql/offlinedownload.pysql/test_offlinedownload.pysql_api/api_workflow.pysql_api/serializers.pysql_api/tests.pysrc/docker-compose/docker-compose.local-arm.ymlsrc/docker/startup.sh
✅ Files skipped from review due to trivial changes (1)
- src/docker/startup.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- sql/models.py
- frontend/src/views/QueriesView.vue
Comment on lines
+533
to
+562
| async function downloadSelectedExport() { | ||
| if (!selectedWorkflowId.value) { | ||
| return | ||
| } | ||
|
|
||
| downloadSubmitting.value = true | ||
| detailError.value = '' | ||
|
|
||
| try { | ||
| const result = await downloadWorkflowExport(selectedWorkflowId.value, requireToken()) | ||
| if (result.mode === 'redirect') { | ||
| window.location.href = result.url | ||
| return | ||
| } | ||
|
|
||
| const objectUrl = window.URL.createObjectURL(result.data) | ||
| const anchor = document.createElement('a') | ||
| anchor.href = objectUrl | ||
| anchor.download = result.filename | ||
| anchor.style.display = 'none' | ||
| document.body.appendChild(anchor) | ||
| anchor.click() | ||
| anchor.remove() | ||
| window.URL.revokeObjectURL(objectUrl) | ||
| } catch (errorValue) { | ||
| detailError.value = toUserFacingMessage(errorValue, 'Failed to download the export artifact.') | ||
| } finally { | ||
| downloadSubmitting.value = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
downloadSubmitting not reset on redirect path.
When result.mode === 'redirect', the early return at line 545 bypasses the finally block, leaving downloadSubmitting as true. This causes the button to remain stuck in the "Preparing download..." state if the browser doesn't navigate away immediately (e.g., if the download triggers a save dialog without navigation).
Proposed fix
try {
const result = await downloadWorkflowExport(selectedWorkflowId.value, requireToken())
if (result.mode === 'redirect') {
+ downloadSubmitting.value = false
window.location.href = result.url
return
}📝 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.
Suggested change
| async function downloadSelectedExport() { | |
| if (!selectedWorkflowId.value) { | |
| return | |
| } | |
| downloadSubmitting.value = true | |
| detailError.value = '' | |
| try { | |
| const result = await downloadWorkflowExport(selectedWorkflowId.value, requireToken()) | |
| if (result.mode === 'redirect') { | |
| window.location.href = result.url | |
| return | |
| } | |
| const objectUrl = window.URL.createObjectURL(result.data) | |
| const anchor = document.createElement('a') | |
| anchor.href = objectUrl | |
| anchor.download = result.filename | |
| anchor.style.display = 'none' | |
| document.body.appendChild(anchor) | |
| anchor.click() | |
| anchor.remove() | |
| window.URL.revokeObjectURL(objectUrl) | |
| } catch (errorValue) { | |
| detailError.value = toUserFacingMessage(errorValue, 'Failed to download the export artifact.') | |
| } finally { | |
| downloadSubmitting.value = false | |
| } | |
| } | |
| async function downloadSelectedExport() { | |
| if (!selectedWorkflowId.value) { | |
| return | |
| } | |
| downloadSubmitting.value = true | |
| detailError.value = '' | |
| try { | |
| const result = await downloadWorkflowExport(selectedWorkflowId.value, requireToken()) | |
| if (result.mode === 'redirect') { | |
| downloadSubmitting.value = false | |
| window.location.href = result.url | |
| return | |
| } | |
| const objectUrl = window.URL.createObjectURL(result.data) | |
| const anchor = document.createElement('a') | |
| anchor.href = objectUrl | |
| anchor.download = result.filename | |
| anchor.style.display = 'none' | |
| document.body.appendChild(anchor) | |
| anchor.click() | |
| anchor.remove() | |
| window.URL.revokeObjectURL(objectUrl) | |
| } catch (errorValue) { | |
| detailError.value = toUserFacingMessage(errorValue, 'Failed to download the export artifact.') | |
| } finally { | |
| downloadSubmitting.value = false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/WorkflowsView.vue` around lines 533 - 562,
downloadSelectedExport leaves downloadSubmitting.value true when result.mode ===
'redirect' because of the early return; before performing the redirect (inside
downloadSelectedExport) explicitly set downloadSubmitting.value = false (or move
the redirect logic after clearing downloadSubmitting) so the UI button exits the
"Preparing download..." state; reference downloadSelectedExport and the
downloadSubmitting reactive variable when making the change.
Comment on lines
+120
to
+123
| download_available = serializers.SerializerMethodField() | ||
|
|
||
| def get_download_available(self, obj): | ||
| return _workflow_download_available(obj) |
Contributor
There was a problem hiding this comment.
Make download_available honor the real download permission.
Line 324 only checks workflow state and file presence, but WorkflowDownload.get() also requires sql.offline_download. Users who can view a finished export but cannot download it will still receive download_available: true, so the SPA will show a button that always 403s. Include the same permission predicate in this flag.
Also applies to: 324-329, 1169-1174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql_api/api_workflow.py` around lines 120 - 123, The download_available
SerializerMethodField currently returns _workflow_download_available(obj) which
only checks workflow state and file presence; update get_download_available to
also check the same permission predicate used by WorkflowDownload.get() (i.e.,
user has sql.offline_download for this workflow/export) so users who can view
but not download get download_available=False; specifically, in
get_download_available (and the other similar uses of
_workflow_download_available at the other locations) combine the existing
_workflow_download_available(obj) result with a permission check against the
current request/user's sql.offline_download permission (mirroring
WorkflowDownload.get()) before returning the final boolean.
Comment on lines
+42
to
43
| EXPORT_FORMAT_CHOICES = {"csv", "tsv", "sql", "xlsx"} | ||
|
|
Contributor
There was a problem hiding this comment.
Keep export-format validation aligned across layers.
These lines now accept only csv/tsv/sql/xlsx, but sql.models.SqlWorkflow.export_format and sql.offlinedownload.EXPORT_FORMATS still allow json and xml. That turns those two formats into API-only failures even though the workflow model and artifact generator still support them. Either expand this set or remove the extra formats everywhere else so the contract is consistent.
Also applies to: 1305-1317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql_api/serializers.py` around lines 42 - 43, EXPORT_FORMAT_CHOICES in
serializers.py is out of sync with sql.models.SqlWorkflow.export_format and
sql.offlinedownload.EXPORT_FORMATS which still include "json" and "xml"; pick
one contract and make all layers match. Either add "json" and "xml" to
EXPORT_FORMAT_CHOICES (so serializers.py contains
{"csv","tsv","sql","xlsx","json","xml"}) and use that single constant from
serializers where API validation occurs, or remove "json" and "xml" from
sql.models.SqlWorkflow.export_format and sql.offlinedownload.EXPORT_FORMATS so
all three only allow csv/tsv/sql/xlsx; update any validation/serialization code
that references EXPORT_FORMAT_CHOICES, SqlWorkflow.export_format, or
EXPORT_FORMATS to import/use the same canonical set and run/adjust tests
accordingly.
Comment on lines
+147
to
152
| result_set = check_engine.query( | ||
| db_name=workflow.db_name, | ||
| sql=count_sql, | ||
| schema_name=schema_name, | ||
| ) | ||
| actual_rows_check = result_set.rows[0][0] |
Contributor
There was a problem hiding this comment.
Check result_set.error before dereferencing rows.
Line 152 assumes the count query returned at least one row, but the explicit error handling does not start until Line 166. When the engine returns an error or an empty result, this path blows up before you can surface a structured validation message. Guard the row access behind the error check and default affected_rows to 0 on failure.
Also applies to: 166-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/offlinedownload.py` around lines 147 - 152, The code dereferences
result_set.rows (actual_rows_check) before checking for errors from
check_engine.query, which can raise on error or empty results; change the logic
around check_engine.query / result_set to first check result_set.error (and that
result_set.rows is non-empty) and only then read result_set.rows[0][0],
otherwise set affected_rows = 0 (or a safe default) and propagate or log the
structured validation error; apply the same guard pattern to the later block
that computes affected_rows (lines referenced around 166-174) so both places
validate result_set.error and rows length before accessing elements.
| - "./mysql/datadir:/var/lib/mysql" | ||
| environment: | ||
| MYSQL_DATABASE: archery | ||
| MYSQL_ROOT_HOST: "%" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the official MySQL Docker image (8.0), what is the security guidance for MYSQL_ROOT_HOST, and is "%" recommended for local development setups?
💡 Result:
The official MySQL Docker image (8.0) uses MYSQL_ROOT_HOST to specify the host for the root user account created during initialization (default is 'localhost', limiting connections to inside the container). Official guidance recommends setting it to a specific host like the Docker gateway IP (e.g., 172.17.0.1) for host access, noting it supports wildcards. Setting to "%" (allowing connections from any host) works as shown in the entrypoint script but is not explicitly recommended in docs, which prioritize security by suggesting specific hosts over broad access. For local development, "%" is commonly used in community examples for convenience when exposing ports, despite general MySQL security advice against '%' in production due to risks.
Citations:
- 1: https://dev.mysql.com/doc/refman/8.1/en/docker-mysql-more-topics.html
- 2: https://hub.docker.com/_/mysql?tab=description
- 3: https://github.com/docker-library/docs/blob/master/mysql/README.md
- 4: Access denied for user 'root'@'192.168.99.1' docker-library/mysql#241
- 5: https://github.com/docker-library/mysql/blob/master/docker-entrypoint.sh
Restrict MySQL root host scope to prevent unnecessary remote access exposure.
MYSQL_ROOT_HOST: "%" permits root login from any host. While the official MySQL Docker image defaults to localhost (container-local only), using a wildcard broadens the attack surface, especially with port 3306 published. Limit root access to a specific host (e.g., "localhost" or Docker gateway IP) or use a dedicated non-root database user for application connectivity.
Suggested change
environment:
MYSQL_DATABASE: archery
- MYSQL_ROOT_HOST: "%"
+ MYSQL_ROOT_HOST: "localhost"
MYSQL_ROOT_PASSWORD: 123456📝 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.
Suggested change
| MYSQL_ROOT_HOST: "%" | |
| environment: | |
| MYSQL_DATABASE: archery | |
| MYSQL_ROOT_HOST: "localhost" | |
| MYSQL_ROOT_PASSWORD: 123456 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/docker-compose/docker-compose.local-arm.yml` at line 27, The env var
MYSQL_ROOT_HOST is set to "%" which allows root access from any host; change
MYSQL_ROOT_HOST to a scoped value such as "localhost" or the Docker gateway IP
(or remove it to use the image default), and update the compose setup so the
application uses a dedicated non-root DB user/credentials (create a new user via
init SQL or Docker envs) instead of root while keeping port 3306 published only
if necessary; ensure references to MYSQL_ROOT_HOST and any DB connection envs
are updated accordingly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "You do not have permission to download export files." | ||
| ) | ||
| except PermissionDenied as exc: | ||
| return JsonResponse({"error": str(exc)}, status=403) |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
sql/offlinedownload.py (1)
150-156:⚠️ Potential issue | 🟠 MajorGuard
result_setbefore readingrows[0][0].Line 155 dereferences
result_set.rows[0][0]before checking engine errors/empty rows, so pre-check can crash instead of returning a structured failure result.Suggested fix
- result_set = check_engine.query( + result_set = check_engine.query( db_name=workflow.db_name, sql=count_sql, schema_name=schema_name, ) - actual_rows_check = result_set.rows[0][0] + actual_rows_check = 0 + if result_set and not result_set.error and result_set.rows: + actual_rows_check = result_set.rows[0][0] @@ - elif result_set.error: + elif not result_set or result_set.error: result = ReviewResult( stage="Auto review failed", errlevel=2, stagestatus="Check failed!", - errormessage=result_set.error, + errormessage=(result_set.error if result_set else "Count query failed"), affected_rows=actual_rows_check, sql=full_sql, )Also applies to: 169-177, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/offlinedownload.py` around lines 150 - 156, The code dereferences result_set.rows[0][0] (variable actual_rows_check) immediately after calling check_engine.query without guarding for errors or empty result sets; update the call sites that read result_set.rows (including the use in actual_rows_check and the similar blocks around lines 169-177 and 199) to first check that result_set is not None, result_set.error (or equivalent error flag) is falsy, and result_set.rows exists and has at least one row and one column before accessing rows[0][0]; if any of those checks fail, return the structured failure result the function expects (preserving existing error messaging flow) instead of letting it raise an IndexError/AttributeError.sql_api/api_workflow.py (2)
120-123:⚠️ Potential issue | 🟠 Major
download_availabledoesn't reflect actual download permission.The field returns
Truewhen the workflow is a finished export with a file, but theWorkflowDownload.get()endpoint also requiressql.offline_downloadpermission. Users who can view but lack download permission will see a download button that always returns 403.Consider passing the request via serializer context and checking the permission:
def get_download_available(self, obj): - return _workflow_download_available(obj) + request = self.context.get("request") + if not request: + return _workflow_download_available(obj) + user = request.user + has_download_perm = user.is_superuser or user.has_perm("sql.offline_download") + return has_download_perm and _workflow_download_available(obj)Note: Ensure the serializer is instantiated with
context={"request": request}where used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/api_workflow.py` around lines 120 - 123, The SerializerMethodField download_available currently returns _workflow_download_available(obj) without checking requester permissions; update get_download_available(self, obj) to read the request from self.context (e.g., request = self.context.get("request")) and combine the existing _workflow_download_available(obj) check with a permission check for "sql.offline_download" (using request.user.has_perm or the same permission check used in WorkflowDownload.get()) so the field only returns True when both the workflow export/file exists and the requesting user has the offline_download permission; also ensure every place the serializer is instantiated passes context={"request": request}.
398-398:⚠️ Potential issue | 🟡 MinorInclude download permission check in detail payload.
Similar to the serializer, this
download_availableflag doesn't reflect whether the current user hassql.offline_downloadpermission. Consider extending the check:- "download_available": _workflow_download_available(workflow), + "download_available": ( + (user.is_superuser or user.has_perm("sql.offline_download")) + and _workflow_download_available(workflow) + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/api_workflow.py` at line 398, The detail payload currently sets "download_available" using _workflow_download_available(workflow) but omits the user's permission check for "sql.offline_download"; update the logic that builds the detail response (where "download_available" is assigned) to also verify the current user's permission (e.g., request.user.has_perm("sql.offline_download")) and only return True if both _workflow_download_available(workflow) and the permission check pass; alternatively modify _workflow_download_available to accept a user argument and use that within the function so callers (including this detail payload) pass request.user when computing download availability.
🧹 Nitpick comments (1)
sql_api/api_workflow.py (1)
782-785: Chain exceptions for better debugging.When re-raising a
ValidationErrorfrom a caught exception, usefrom eto preserve the original traceback context. This helps with debugging production issues.except serializers.ValidationError: raise except Exception as e: - raise serializers.ValidationError({"errors": f"{e}"}) + raise serializers.ValidationError({"errors": f"{e}"}) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql_api/api_workflow.py` around lines 782 - 785, In the exception handling block that catches serializers.ValidationError and a general Exception in sql_api/api_workflow.py (the except serializers.ValidationError / except Exception as e block), re-raise the new serializers.ValidationError using exception chaining (i.e., raise ... from e) so the original traceback is preserved; replace the current raise of serializers.ValidationError({"errors": f"{e}"}) with a chained raise that includes the error message and uses "from e" to maintain context when debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@sql_api/api_workflow.py`:
- Around line 120-123: The SerializerMethodField download_available currently
returns _workflow_download_available(obj) without checking requester
permissions; update get_download_available(self, obj) to read the request from
self.context (e.g., request = self.context.get("request")) and combine the
existing _workflow_download_available(obj) check with a permission check for
"sql.offline_download" (using request.user.has_perm or the same permission check
used in WorkflowDownload.get()) so the field only returns True when both the
workflow export/file exists and the requesting user has the offline_download
permission; also ensure every place the serializer is instantiated passes
context={"request": request}.
- Line 398: The detail payload currently sets "download_available" using
_workflow_download_available(workflow) but omits the user's permission check for
"sql.offline_download"; update the logic that builds the detail response (where
"download_available" is assigned) to also verify the current user's permission
(e.g., request.user.has_perm("sql.offline_download")) and only return True if
both _workflow_download_available(workflow) and the permission check pass;
alternatively modify _workflow_download_available to accept a user argument and
use that within the function so callers (including this detail payload) pass
request.user when computing download availability.
In `@sql/offlinedownload.py`:
- Around line 150-156: The code dereferences result_set.rows[0][0] (variable
actual_rows_check) immediately after calling check_engine.query without guarding
for errors or empty result sets; update the call sites that read result_set.rows
(including the use in actual_rows_check and the similar blocks around lines
169-177 and 199) to first check that result_set is not None, result_set.error
(or equivalent error flag) is falsy, and result_set.rows exists and has at least
one row and one column before accessing rows[0][0]; if any of those checks fail,
return the structured failure result the function expects (preserving existing
error messaging flow) instead of letting it raise an IndexError/AttributeError.
---
Nitpick comments:
In `@sql_api/api_workflow.py`:
- Around line 782-785: In the exception handling block that catches
serializers.ValidationError and a general Exception in sql_api/api_workflow.py
(the except serializers.ValidationError / except Exception as e block), re-raise
the new serializers.ValidationError using exception chaining (i.e., raise ...
from e) so the original traceback is preserved; replace the current raise of
serializers.ValidationError({"errors": f"{e}"}) with a chained raise that
includes the error message and uses "from e" to maintain context when debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c973485-dffc-4bd5-ad94-48290509fbb5
📒 Files selected for processing (8)
frontend/src/views/ExportWorkflowCreateView.vuesql/migrations/0005_alter_sqlworkflow_export_format.pysql/migrations/0006_sqlworkflow_schema_name.pysql/offlinedownload.pysql/test_offlinedownload.pysql_api/api_workflow.pysql_api/serializers.pysql_api/tests.py
✅ Files skipped from review due to trivial changes (1)
- frontend/src/views/ExportWorkflowCreateView.vue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add approved data export workflows to the SPA and
sql_api.This introduces a dedicated SPA export request flow, export-aware workflow list/detail handling, and backend export submission, validation, and download endpoints that reuse the existing SQL workflow approval model.
What Changed
sql_apiexport submission metadata, export SQL validation, and export download endpointsWhy
The product needs larger data exports to run as an approval workflow, similar to DML requests, instead of as direct live-query downloads.
Impact
Users can now submit approved export workflows for
csv,tsv,sql, andxlsx, wait for approval/execution, and download the resulting file from the workflow detail page.Validation
black --check .cd frontend && npm run builddocker exec datamingle-app python manage.py test sql_api.tests.TestWorkflow sql.test_offlinedownloaddocker exec datamingle-app python manage.py makemigrations sql --checkNotes
Untracked local download directories under
src/docker-compose/archery/downloads/were intentionally left out of this PR.Summary by CodeRabbit
New Features
Tests
Chores