Add DDL workflow support and restore datamingle-app#19
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a refactored workflow submission and management system across the frontend and backend. It adds a new workflow creation UI component, restructures workflow API types and endpoints, enhances the MySQL execution engine for direct DDL execution, and updates access control and permission handling for workflow operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Frontend as WorkflowsCreateView
participant API as Backend API
participant DB as Database
User->>Frontend: Load workflow creation page
Frontend->>API: fetchWorkflowMetadata()
API->>DB: Query resource groups & instances
DB-->>API: Metadata records
API-->>Frontend: Metadata payload
Frontend-->>User: Render form with options
User->>Frontend: Select resource group, instance, database
Frontend->>Frontend: Filter instance options by group
Frontend->>API: Fetch databases for instance
API->>DB: Query instance databases
DB-->>API: Database list
API-->>Frontend: Database options
Frontend-->>User: Update form fields
User->>Frontend: Enter SQL & click "Check SQL"
Frontend->>API: checkWorkflowSql(payload)
API->>API: Parse & validate SQL
API-->>Frontend: Syntax/warnings/errors & column list
Frontend-->>User: Display check results
User->>Frontend: Fill remaining fields & submit
Frontend->>API: createWorkflow(payload)
API->>DB: Insert workflow record
DB-->>API: Workflow created with ID
API-->>Frontend: Success response with workflow ID
Frontend->>User: Navigate to workflow list & show new workflow
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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: 3
🧹 Nitpick comments (3)
sql_api/api_workflow.py (3)
762-769: Consider usingnotinstead ofis Falsefor boolean check.While
is Falseis explicit, it's unconventional for boolean returns. Ifcan_reviewreturns a truthy/falsy value (not strictly boolean),is Falsewould only match explicitFalse. Usingnotis more idiomatic.♻️ Proposed simplification
- if ( - Audit.can_review(request.user, workflow_id, WorkflowType.SQL_REVIEW) - is False - ): + if not Audit.can_review(request.user, workflow_id, WorkflowType.SQL_REVIEW):🤖 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 762 - 769, Replace the strict identity check against False with an idiomatic boolean negation: instead of comparing the result of Audit.can_review(request.user, workflow_id, WorkflowType.SQL_REVIEW) using "is False", use "not" to test falsiness; keep the same behavior that when the user cannot review the fetched SqlWorkflow (obtained via get_object_or_404 with workflow_id) you raise PermissionDenied with the same message.
963-966: Preserve exception chain when re-raising AuditException.Similar to the earlier exception handling, use
raise ... from eto maintain the traceback chain for debugging.♻️ Proposed fix
try: workflow_audit_detail = auditor.operate(action, user, data["audit_remark"]) except AuditException as e: - raise serializers.ValidationError({"errors": f"Operation failed, {str(e)}"}) + raise serializers.ValidationError({"errors": f"Operation failed, {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 963 - 966, The except block that catches AuditException should preserve the original exception chain when re-raising as a serializers.ValidationError: change the raise to include "from e" so the traceback is maintained (i.e., when handling AuditException from auditor.operate(action, user, data["audit_remark"]) replace the current raise serializers.ValidationError(...) with a re-raise that uses "from e" to keep the original AuditException in the chain).
463-466: Broad exception catch may expose internal details and loses traceback context.Catching bare
Exceptionand re-raising loses the exception chain. Consider:
- Catching more specific exceptions when possible
- Using
raise ... from eto preserve context for debugging♻️ Proposed fix
check_result = check_engine.execute_check(db_name=db_name, sql=full_sql) - except Exception as e: - raise serializers.ValidationError({"errors": f"{e}"}) + except Exception as 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 463 - 466, The current broad except Exception block around check_engine.escape_string and check_engine.execute_check swallows the original traceback and may expose internals; update the handler to catch the most specific errors possible (e.g., the DB/client error thrown by check_engine and input errors like ValueError) instead of Exception, and when re-raising use "raise serializers.ValidationError({'errors': str(e)}) from e" to preserve the exception chain; reference the check_engine.escape_string, check_engine.execute_check call sites and the serializers.ValidationError raise so you update those exact places and add specific exception types with "raise ... from e" fallback.
🤖 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/WorkflowsCreateView.vue`:
- Around line 25-28: Concurrent fetchInstanceResources() calls can complete out
of order and overwrite state such as databaseOptions and the loading flags
(metadataLoading, databasesLoading, checkLoading, submitting). Fix it by adding
a per-call sentinel (e.g., incrementing requestId or using an AbortController
stored on the component) before each fetchInstanceResources() invocation, pass
that token into the async flow, and when a response/error arrives only update
databaseOptions and loading refs if the token matches the latest stored token
(or the request wasn't aborted); ensure any previous pending request is aborted
or ignored. Apply the same pattern for all places that call
fetchInstanceResources() (including the other block covering lines 155-180) so
only the most recent response mutates state.
- Around line 102-108: buildCheckSignature currently omits groupId and is
recomputed after the async sql-check, allowing stale in-flight results to be
treated as current; compute the signature once per request (capture it before
any await) and include form.groupId in buildCheckSignature (in addition to
form.instanceId, form.dbName, form.sqlContent), then pass that request-bound
signature/token through the async sql-check call and only accept/store the
response if the returned signature matches the captured one; also ensure the
invalidation logic that watches group/target/sql changes uses the same
signature/token so submission gating (the check-before-submit code paths)
rejects results that don’t match the current signature.
In `@sql/engines/mysql.py`:
- Around line 819-852: The helper _direct_workflow_statements currently falls
back to raw sql when review_content is malformed or when review rows are
filtered out, and it also ignores review verdicts (errlevel) — change it to
“fail closed”: if json.loads(review_content) raises (TypeError, JSONDecodeError)
return [] immediately; if any review row is unreadable (not list/dict) or any
parsed ReviewResult has errlevel == 2 (rejected/high-risk) return []
immediately; otherwise build and return the reviewed statements as before;
ensure execute_check and execute_direct_workflow behavior is preserved by
returning an empty list when reviews are unusable or rejected so execution is
blocked.
---
Nitpick comments:
In `@sql_api/api_workflow.py`:
- Around line 762-769: Replace the strict identity check against False with an
idiomatic boolean negation: instead of comparing the result of
Audit.can_review(request.user, workflow_id, WorkflowType.SQL_REVIEW) using "is
False", use "not" to test falsiness; keep the same behavior that when the user
cannot review the fetched SqlWorkflow (obtained via get_object_or_404 with
workflow_id) you raise PermissionDenied with the same message.
- Around line 963-966: The except block that catches AuditException should
preserve the original exception chain when re-raising as a
serializers.ValidationError: change the raise to include "from e" so the
traceback is maintained (i.e., when handling AuditException from
auditor.operate(action, user, data["audit_remark"]) replace the current raise
serializers.ValidationError(...) with a re-raise that uses "from e" to keep the
original AuditException in the chain).
- Around line 463-466: The current broad except Exception block around
check_engine.escape_string and check_engine.execute_check swallows the original
traceback and may expose internals; update the handler to catch the most
specific errors possible (e.g., the DB/client error thrown by check_engine and
input errors like ValueError) instead of Exception, and when re-raising use
"raise serializers.ValidationError({'errors': str(e)}) from e" to preserve the
exception chain; reference the check_engine.escape_string,
check_engine.execute_check call sites and the serializers.ValidationError raise
so you update those exact places and add specific exception types with "raise
... from e" fallback.
🪄 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: 22c27261-1374-4d03-8da2-3f3d51e8943f
📒 Files selected for processing (16)
frontend/src/lib/api.tsfrontend/src/router/index.tsfrontend/src/views/WorkflowsCreateView.vuefrontend/src/views/WorkflowsView.vuesql/engines/mysql.pysql/engines/oracle.pysql/engines/test_mysql.pysql/engines/tests.pysql/utils/sql_review.pysql/utils/tests.pysql_api/api_user.pysql_api/api_workflow.pysql_api/serializers.pysql_api/tests.pysql_api/urls.pysrc/docker/Dockerfile.local-arm
Comment on lines
+25
to
+28
| const metadataLoading = ref(false) | ||
| const databasesLoading = ref(false) | ||
| const checkLoading = ref(false) | ||
| const submitting = ref(false) |
Contributor
There was a problem hiding this comment.
Ignore out-of-order database lookups.
Switching instances quickly can leave multiple fetchInstanceResources() calls in flight. An older response can then overwrite databaseOptions, clear the spinner, or surface an error for the wrong instance.
🧭 Proposed fix
const metadataLoading = ref(false)
const databasesLoading = ref(false)
const checkLoading = ref(false)
const submitting = ref(false)
+const databaseRequestId = ref(0)
@@
watch(
() => form.instanceId,
async (instanceId) => {
+ const requestId = ++databaseRequestId.value
form.dbName = ''
databaseOptions.value = []
if (!instanceId) {
+ databasesLoading.value = false
return
}
@@
try {
const payload = await fetchInstanceResources(
Number(instanceId),
'database',
requireToken(),
)
+ if (requestId !== databaseRequestId.value) {
+ return
+ }
databaseOptions.value = payload.result
} catch (errorValue) {
+ if (requestId !== databaseRequestId.value) {
+ return
+ }
formError.value = toUserFacingMessage(errorValue, 'Failed to load databases for the selected instance.')
} finally {
- databasesLoading.value = false
+ if (requestId === databaseRequestId.value) {
+ databasesLoading.value = false
+ }
}
},
)Also applies to: 155-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/WorkflowsCreateView.vue` around lines 25 - 28, Concurrent
fetchInstanceResources() calls can complete out of order and overwrite state
such as databaseOptions and the loading flags (metadataLoading,
databasesLoading, checkLoading, submitting). Fix it by adding a per-call
sentinel (e.g., incrementing requestId or using an AbortController stored on the
component) before each fetchInstanceResources() invocation, pass that token into
the async flow, and when a response/error arrives only update databaseOptions
and loading refs if the token matches the latest stored token (or the request
wasn't aborted); ensure any previous pending request is aborted or ignored.
Apply the same pattern for all places that call fetchInstanceResources()
(including the other block covering lines 155-180) so only the most recent
response mutates state.
Comment on lines
+102
to
+108
| function buildCheckSignature() { | ||
| return JSON.stringify([ | ||
| form.instanceId, | ||
| form.dbName, | ||
| form.sqlContent, | ||
| ]) | ||
| } |
Contributor
There was a problem hiding this comment.
Make the SQL-check signature request-bound.
Line 230 recomputes the signature after the await, and buildCheckSignature() omits groupId even though Lines 132-137 invalidate the check on group changes. If the user changes the group/target/SQL while the request is in flight, a stale result can be stamped as current and Lines 261-263 will allow submission without a fresh review. The API still accepts workflow creation without a prior sqlcheck, so this client-side gate needs to fail closed.
🛡️ Proposed fix
function buildCheckSignature() {
return JSON.stringify([
+ form.groupId,
form.instanceId,
form.dbName,
form.sqlContent,
])
}
@@
async function runSqlCheck() {
formError.value = ''
@@
+ const requestSignature = buildCheckSignature()
checkLoading.value = true
try {
- checkResult.value = await checkWorkflowSql(
+ const result = await checkWorkflowSql(
{
instance_id: Number(form.instanceId),
db_name: form.dbName.trim(),
full_sql: form.sqlContent,
},
requireToken(),
)
- lastCheckedSignature.value = buildCheckSignature()
+ if (requestSignature !== buildCheckSignature()) {
+ return
+ }
+ checkResult.value = result
+ lastCheckedSignature.value = requestSignature
} catch (errorValue) {
+ if (requestSignature !== buildCheckSignature()) {
+ return
+ }
formError.value = toUserFacingMessage(errorValue, 'Failed to run SQL check.')
} finally {
checkLoading.value = false
}
}Also applies to: 132-137, 221-230, 261-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/WorkflowsCreateView.vue` around lines 102 - 108,
buildCheckSignature currently omits groupId and is recomputed after the async
sql-check, allowing stale in-flight results to be treated as current; compute
the signature once per request (capture it before any await) and include
form.groupId in buildCheckSignature (in addition to form.instanceId,
form.dbName, form.sqlContent), then pass that request-bound signature/token
through the async sql-check call and only accept/store the response if the
returned signature matches the captured one; also ensure the invalidation logic
that watches group/target/sql changes uses the same signature/token so
submission gating (the check-before-submit code paths) rejects results that
don’t match the current signature.
Comment on lines
+819
to
+852
| def _direct_workflow_statements(self, workflow): | ||
| """Prefer reviewed statements so direct execution matches the checked SQL.""" | ||
| review_content = workflow.sqlworkflowcontent.review_content or "[]" | ||
| statements = [] | ||
|
|
||
| try: | ||
| review_rows = json.loads(review_content) | ||
| except (TypeError, json.JSONDecodeError): | ||
| review_rows = [] | ||
|
|
||
| for row in review_rows: | ||
| if isinstance(row, list): | ||
| review_row = ReviewResult(inception_result=row) | ||
| elif isinstance(row, dict): | ||
| review_row = ReviewResult(**row) | ||
| else: | ||
| continue | ||
|
|
||
| statement = remove_comments(review_row.sql, db_type="mysql").strip() | ||
| if statement: | ||
| statements.append(statement.rstrip(";")) | ||
|
|
||
| if statements: | ||
| return statements | ||
|
|
||
| normalized_sql = sqlparse.format( | ||
| workflow.sqlworkflowcontent.sql_content, | ||
| strip_comments=True, | ||
| ) | ||
| return [ | ||
| statement.strip().rstrip(";") | ||
| for statement in sqlparse.split(normalized_sql) | ||
| if statement.strip() | ||
| ] |
Contributor
There was a problem hiding this comment.
Fail closed when reviewed statements are blocked or unreadable.
execute_check() marks rejected/high-risk rows with errlevel = 2, but this helper currently executes every review_row.sql regardless. It also falls back to raw sql_content whenever review_content is malformed or filters down to nothing, which bypasses the reviewed statement set. Returning [] here is safer because execute_direct_workflow() already converts that into a failed execution.
🔒 Proposed fix
def _direct_workflow_statements(self, workflow):
"""Prefer reviewed statements so direct execution matches the checked SQL."""
- review_content = workflow.sqlworkflowcontent.review_content or "[]"
+ review_content = workflow.sqlworkflowcontent.review_content
statements = []
+ has_review_content = bool(review_content)
- try:
- review_rows = json.loads(review_content)
- except (TypeError, json.JSONDecodeError):
- review_rows = []
+ if has_review_content:
+ try:
+ review_rows = json.loads(review_content)
+ except (TypeError, json.JSONDecodeError):
+ return []
+ else:
+ review_rows = []
for row in review_rows:
if isinstance(row, list):
review_row = ReviewResult(inception_result=row)
elif isinstance(row, dict):
review_row = ReviewResult(**row)
else:
continue
+ if int(getattr(review_row, "errlevel", 0) or 0) >= 2:
+ return []
statement = remove_comments(review_row.sql, db_type="mysql").strip()
if statement:
statements.append(statement.rstrip(";"))
- if statements:
+ if has_review_content:
return statements🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/engines/mysql.py` around lines 819 - 852, The helper
_direct_workflow_statements currently falls back to raw sql when review_content
is malformed or when review rows are filtered out, and it also ignores review
verdicts (errlevel) — change it to “fail closed”: if json.loads(review_content)
raises (TypeError, JSONDecodeError) return [] immediately; if any review row is
unreadable (not list/dict) or any parsed ReviewResult has errlevel == 2
(rejected/high-risk) return [] immediately; otherwise build and return the
reviewed statements as before; ensure execute_check and execute_direct_workflow
behavior is preserved by returning an empty list when reviews are unusable or
rejected so execution is blocked.
This was referenced Apr 2, 2026
This was referenced Apr 17, 2026
This was referenced Apr 27, 2026
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.
What changed
This PR adds end-to-end DDL workflow support alongside the existing SQL workflow flow.
datamingle-appcx_Oraclein the local ARM environmentWhy
The repo previously supported SQL workflow behavior mainly through backend endpoints and an incomplete SPA placeholder. DDL tickets needed the same submit, approve, and execute lifecycle as DML requests, plus a usable frontend and a direct execution path for MySQL-family DDL.
The local container rename and test-environment fixes were also needed so validation could run cleanly in the expected
datamingle-appcontainer.Impact
datamingle-appValidation
docker exec datamingle-app python manage.py testnpm run buildSummary by CodeRabbit
Release Notes
New Features
Improvements