Fix query tool auto execution after session restore - #10055
Fix query tool auto execution after session restore#10055dev-hari-prasad wants to merge 6 commits into
Conversation
- Handle both boolean and string values for is_query_tool - Fix Query Tool tabs being incorrectly detected during restore - Prevent queries from running automatically when restoring a session The issue was caused by is_query_tool only checking for the string 'true'. During session restore the value can come back as a boolean, which caused Query Tool tabs to be treated as View/Edit Data tabs and trigger the auto execution path.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughQueryToolComponent normalizes ChangesQuery Tool Restore and Auto-Execution Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (3)
345-352: ⚡ Quick winConsider normalizing the
restoreparameter lifecycle.The
restoreflag has an inconsistent type lifecycle:
- Starts as boolean
truein saved metaData (context snippet 2:restore: true)- Submitted as string
'true'via form textarea (context snippet 1)- Set to string
'false'here after restoration completes- Checked with loose inequality
!= 'true'on line 390While the loose inequality
!=on line 390 handles both booleantrueand string'true'correctly, explicitly setting it to string'false'rather than booleanfalseadds cognitive overhead. Consider either:
- Normalizing to boolean throughout (set to
falsehere, check with!== trueon line 390), or- Documenting why string
'false'is required for persistence/serialization.The functional update pattern
prev => ({...prev, ...})is correct for React 19.💡 Suggested normalization (if no serialization constraint)
setQtStatePartial(prev => ({ ...prev, editor_disabled: false, params: { ...prev.params, - restore: 'false' + restore: false } }));And on line 390:
- if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore != 'true') { + if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore !== true) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 345 - 352, The restore flag in setQtStatePartial is being set to the string 'false' which mismatches its boolean usage elsewhere (saved metaData uses true and it's checked later with a loose comparison); change the code in the state updater (setQtStatePartial) to set params.restore to the boolean false instead of the string 'false', and update the check that reads the flag (the condition that currently compares against 'true') to use a strict boolean check (e.g., !== true) so the restore lifecycle is normalized to boolean across setQtStatePartial, params.restore, and the restore-checking logic.
917-920: ⚖️ Poor tradeoffReplace brittle string substitution with URL parameter manipulation.
The current implementation directly mutates
panel.metaData.toolUrlusing simple string replacement. This approach is fragile:
- Silent failure: If
'is_query_tool=false'isn't present (unexpected URL format), the replace silently does nothing.- First-match only:
String.replace()without a regex only replaces the first occurrence.- No URL encoding handling: Doesn't handle potential encoding variations.
- Brittle pattern matching: Could theoretically match the substring in other contexts (though unlikely given the URL structure from context snippets).
Additionally, directly mutating
panel.metaDatamay have side effects if the object is shared or tracked elsewhere.🔧 Robust URL parameter manipulation
toggleQueryTool: () => setQtStatePartial((prev)=>{ let panel = qtPanelDocker?.find(qtPanelId); if (panel?.metaData?.toolUrl) { - panel.metaData.toolUrl = panel.metaData.toolUrl.replace('is_query_tool=false', 'is_query_tool=true'); + try { + const url = new URL(panel.metaData.toolUrl, window.location.origin); + url.searchParams.set('is_query_tool', 'true'); + panel.metaData.toolUrl = url.pathname + url.search + url.hash; + } catch (e) { + console.warn('Failed to update is_query_tool parameter:', e); + } } return { ...prev,This approach:
- Properly parses the URL
- Updates the specific parameter
- Handles any URL structure
- Catches malformed URLs
- Preserves all other parameters
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 917 - 920, The current direct string replacement on panel.metaData.toolUrl is brittle; instead parse the toolUrl using the URL/URLSearchParams APIs (inside a try/catch to handle malformed URLs), set the search parameter is_query_tool to "true" via url.searchParams.set('is_query_tool','true'), reconstruct the full URL with url.toString(), and then assign the new string back (preferably by replacing the toolUrl property immutably, e.g. panel.metaData = Object.assign({}, panel.metaData, {toolUrl: newUrl})) so you avoid silent failures, encoding issues, and unintended shared-object side effects; locate this change around the qtPanelDocker?.find(qtPanelId) / panel.metaData.toolUrl code.
141-141: ⚡ Quick winUse consistent equality operators when checking
is_query_tool.The type-flexibility check mixes strict equality (
=== true) and loose equality (== 'true'). While functionally correct here, mixing operators is inconsistent and can obscure intent.Additionally, this pattern only explicitly handles
trueand'true', but URL parameters from context snippets showis_query_tool=falseis also possible (from view-data mode). The current logic treats any non-true/'true'value (including the string'false') as falsy, which works but could be made more explicit.♻️ Proposed fix for consistency
- is_query_tool: params.is_query_tool === true || params.is_query_tool == 'true', + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true',Apply the same fix on line 157:
- _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool == 'true'), + _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool === 'true'),Alternatively, for more explicit boolean coercion:
- is_query_tool: params.is_query_tool === true || params.is_query_tool == 'true', + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true',Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` at line 141, The boolean check for is_query_tool mixes === and == and doesn't explicitly handle string values like 'false'; replace the expression used to derive is_query_tool (currently: params.is_query_tool === true || params.is_query_tool == 'true') with a consistent coercion such as String(params.is_query_tool) === 'true' (or params.is_query_tool === true || params.is_query_tool === 'true') to use strict equality in both branches; update the same expression wherever it appears (the other occurrence that initializes is_query_tool in this component) so both places use the identical, explicit check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 345-352: The restore flag in setQtStatePartial is being set to the
string 'false' which mismatches its boolean usage elsewhere (saved metaData uses
true and it's checked later with a loose comparison); change the code in the
state updater (setQtStatePartial) to set params.restore to the boolean false
instead of the string 'false', and update the check that reads the flag (the
condition that currently compares against 'true') to use a strict boolean check
(e.g., !== true) so the restore lifecycle is normalized to boolean across
setQtStatePartial, params.restore, and the restore-checking logic.
- Around line 917-920: The current direct string replacement on
panel.metaData.toolUrl is brittle; instead parse the toolUrl using the
URL/URLSearchParams APIs (inside a try/catch to handle malformed URLs), set the
search parameter is_query_tool to "true" via
url.searchParams.set('is_query_tool','true'), reconstruct the full URL with
url.toString(), and then assign the new string back (preferably by replacing the
toolUrl property immutably, e.g. panel.metaData = Object.assign({},
panel.metaData, {toolUrl: newUrl})) so you avoid silent failures, encoding
issues, and unintended shared-object side effects; locate this change around the
qtPanelDocker?.find(qtPanelId) / panel.metaData.toolUrl code.
- Line 141: The boolean check for is_query_tool mixes === and == and doesn't
explicitly handle string values like 'false'; replace the expression used to
derive is_query_tool (currently: params.is_query_tool === true ||
params.is_query_tool == 'true') with a consistent coercion such as
String(params.is_query_tool) === 'true' (or params.is_query_tool === true ||
params.is_query_tool === 'true') to use strict equality in both branches; update
the same expression wherever it appears (the other occurrence that initializes
is_query_tool in this component) so both places use the identical, explicit
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13354e15-d117-46d6-9736-8833f3cd5f37
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
- Normalize is_query_tool check with strict equality - Normalize restore flag to use boolean types instead of strings - Refactor fragile toolUrl replacement to use URL API with searchParams
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
293-304:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Restore guard type inconsistency allows execution during restore.
Lines 293 and 300 check
restoreagainst the string'true'using loose equality, but line 390 uses strict inequality against the booleantrue. Whenparams.restorearrives as the string'true'from the URL (the expected format), the guard at line 390 evaluates'true' !== truetotrue, allowing execution to proceed and defeating the restore guard's purpose.Line 350 sets
restore: falseafter restore completes, but this doesn't protect the initial execution check at line 390 when the component mounts withrestore='true'.🛡️ Recommended fix: normalize restore during initialization
Apply this fix at line 141 to normalize
restoreduring state initialization (matching theis_query_toolpattern):is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', + restore: params.restore === true || params.restore === 'true', node_name: retrieveNodeName(selectedNodeInfo),And update line 390 to use strict equality with the normalized boolean:
- if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore !== true) { + if((!qtState.params.is_query_tool || reexecute) && !qtState.params.restore) { eventBus.current.fireEvent(QUERY_TOOL_EVENTS.TRIGGER_EXECUTION, explainObject, macroSQL, executeCursor, executeServerCursor);This ensures consistent boolean handling throughout the restore lifecycle.
Also applies to: 350-350, 390-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 293 - 304, qtState.params.restore is treated inconsistently (string 'true' vs boolean true) which lets restores bypass guards; normalize params.restore to a boolean during state initialization (same approach used for is_query_tool) so qtState.params.restore is true/false, then change the runtime guard that currently checks restore against the string to a strict boolean check (e.g., use qtState.params.restore === true) and ensure restoreToolContent and the subsequent setQtStatePartial({ restore: false }) work with the normalized boolean value.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (2)
919-926: 💤 Low valueConsider validating toolUrl before URL construction.
The URL API approach is solid, but if
panel.metaData.toolUrlhas an unexpected format (e.g., already a full URL with a different origin, or malformed), thetry/catchwill silently log a warning without indicating the root cause. Consider adding a pre-check or enriching the error message to aid debugging.💡 Optional: enrich error logging
try { const url = new URL(panel.metaData.toolUrl, window.location.origin); url.searchParams.set('is_query_tool', 'true'); panel.metaData = Object.assign({}, panel.metaData, {toolUrl: url.pathname + url.search + url.hash}); } catch (e) { - console.warn('Failed to update is_query_tool parameter:', e); + console.warn('Failed to update is_query_tool parameter for toolUrl:', panel.metaData.toolUrl, e); }This helps trace which URL caused the parse failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 919 - 926, The try/catch around new URL(panel.metaData.toolUrl, window.location.origin) should validate and handle edge cases before blindly constructing the URL: check panel.metaData.toolUrl for emptiness and whether it looks like an absolute URL (contains "://" or startsWith("http")), and if absolute use new URL(panel.metaData.toolUrl) while if relative use new URL(panel.metaData.toolUrl, window.location.origin); also enrich the catch to include the offending panel.metaData.toolUrl and window.location.origin and the caught error (e) in the processLogger/console message so you can see the root cause when URL construction fails.
1010-1028: 💤 Low valueMissing PropTypes entry for restore parameter.
The
restoreparameter is used throughout the component (lines 293, 300, 350, 390), but it's not declared in thePropTypesdefinition. Adding it would improve type safety and documentation.📝 Suggested addition
is_query_tool: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]).isRequired, + restore: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]), server_cursor: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 1010 - 1028, Add a missing PropTypes entry for the params.restore prop on QueryToolComponent: update the params PropTypes shape to include a non-required restore property (e.g., params.restore: PropTypes.oneOfType([PropTypes.bool, PropTypes.string])) so the restore prop used in the component (references to params.restore) is declared and type-checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 293-304: qtState.params.restore is treated inconsistently (string
'true' vs boolean true) which lets restores bypass guards; normalize
params.restore to a boolean during state initialization (same approach used for
is_query_tool) so qtState.params.restore is true/false, then change the runtime
guard that currently checks restore against the string to a strict boolean check
(e.g., use qtState.params.restore === true) and ensure restoreToolContent and
the subsequent setQtStatePartial({ restore: false }) work with the normalized
boolean value.
---
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 919-926: The try/catch around new URL(panel.metaData.toolUrl,
window.location.origin) should validate and handle edge cases before blindly
constructing the URL: check panel.metaData.toolUrl for emptiness and whether it
looks like an absolute URL (contains "://" or startsWith("http")), and if
absolute use new URL(panel.metaData.toolUrl) while if relative use new
URL(panel.metaData.toolUrl, window.location.origin); also enrich the catch to
include the offending panel.metaData.toolUrl and window.location.origin and the
caught error (e) in the processLogger/console message so you can see the root
cause when URL construction fails.
- Around line 1010-1028: Add a missing PropTypes entry for the params.restore
prop on QueryToolComponent: update the params PropTypes shape to include a
non-required restore property (e.g., params.restore:
PropTypes.oneOfType([PropTypes.bool, PropTypes.string])) so the restore prop
used in the component (references to params.restore) is declared and
type-checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7576b2cd-c69b-41c5-ae5f-d9230246945d
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (2)
921-923: ⚡ Quick winSimplify the absolute URL detection.
The condition
toolUrl.includes('://') || toolUrl.startsWith('http')could incorrectly classify edge cases. For instance, a URL string"http"(without://) would match the second condition and be treated as absolute, causingnew URL("http")to throw an error.♻️ Proposed fix to use a more robust check
-const isAbsolute = toolUrl.includes('://') || toolUrl.startsWith('http'); +const isAbsolute = toolUrl.includes('://'); const url = isAbsolute ? new URL(toolUrl) : new URL(toolUrl, window.location.origin);The
includes('://')check reliably identifies absolute URLs (http://, https://, file://, etc.) without false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 921 - 923, The absolute-URL detection using isAbsolute (currently toolUrl.includes('://') || toolUrl.startsWith('http')) is too permissive; change the check to rely only on the presence of '://'. Locate the variables toolUrl, isAbsolute, and url in QueryToolComponent.jsx and update isAbsolute to use toolUrl.includes('://') only, then construct url using new URL(toolUrl) when isAbsolute is true and new URL(toolUrl, window.location.origin) otherwise to avoid treating short strings like "http" as absolute.
158-158: ⚡ Quick winConsider reusing the normalized
is_query_toolvalue.The normalization check
params.is_query_tool === true || params.is_query_tool === 'true'is duplicated from line 141. Since theparamsobject (containing the normalizedis_query_tool) is constructed beforeconnection_list, you could reference the already-normalized value to reduce duplication.♻️ Proposed refactor to eliminate duplication
Compute the normalized boolean once before the state object:
+const normalizedParams = { + ...params, + title: _.unescape(params.title), + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', + restore: params.restore === true || params.restore === 'true', + node_name: retrieveNodeName(selectedNodeInfo), + dbname: _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), + server_cursor: preferencesStore.getPreferencesForModule('sqleditor').server_cursor === true, +}; + const [qtState, setQtState] = useState({ preferences: { ... }, is_new_tab: window.location == window.parent?.location, ... - params: { - ...params, - title: _.unescape(params.title), - is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', - restore: params.restore === true || params.restore === 'true', - node_name: retrieveNodeName(selectedNodeInfo), - dbname: _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), - server_cursor: preferencesStore.getPreferencesForModule('sqleditor').server_cursor === true, - }, + params: normalizedParams, connection_list: [{ ... conn_title: getTitle( pgAdmin, null, selectedNodeInfo, true, _.unescape(params.server_name), _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), - _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool === 'true'), + _.unescape(params.role) || _.unescape(params.user), normalizedParams.is_query_tool), ... }],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` at line 158, The duplicated normalization of params.is_query_tool should be removed: use the already-normalized boolean stored on params (or a local variable like isQueryTool set when building params) when constructing connection_list instead of re-evaluating params.is_query_tool === true || params.is_query_tool === 'true'; update the connection_list call in QueryToolComponent.jsx to reference params.is_query_tool (or the local isQueryTool) so the normalized value is computed once and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 921-923: The absolute-URL detection using isAbsolute (currently
toolUrl.includes('://') || toolUrl.startsWith('http')) is too permissive; change
the check to rely only on the presence of '://'. Locate the variables toolUrl,
isAbsolute, and url in QueryToolComponent.jsx and update isAbsolute to use
toolUrl.includes('://') only, then construct url using new URL(toolUrl) when
isAbsolute is true and new URL(toolUrl, window.location.origin) otherwise to
avoid treating short strings like "http" as absolute.
- Line 158: The duplicated normalization of params.is_query_tool should be
removed: use the already-normalized boolean stored on params (or a local
variable like isQueryTool set when building params) when constructing
connection_list instead of re-evaluating params.is_query_tool === true ||
params.is_query_tool === 'true'; update the connection_list call in
QueryToolComponent.jsx to reference params.is_query_tool (or the local
isQueryTool) so the normalized value is computed once and reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8ae2cda-8f88-4f6d-8427-a09d8dbf75b5
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
- Extracted isQueryTool to eliminate duplicate checks - Simplified absolute-URL detection to use '://' only
|
The PR is ready to merge. I have addressed all concerns of CodeRabbit. Pls consider merging this after updating the branch. I have used AI in the last two commit eg. Refactoring: Clean up state initialization and URL checks and Update QueryToolComponent.jsx, but rest assured, I have thoroughly read and understood the changes, made the initial changes myself, and verified everything manually. Pls review the changes @asheshv if any changes are required, let me know. |
| } | ||
| setQtStatePartial({ editor_disabled: false }); | ||
| } else if (qtState.params.restore == 'true') { | ||
| } else if (qtState.params.restore === true) { |
There was a problem hiding this comment.
Changes looks good.
I observed that restoreToolContent() is async but is called without await here, can you fix that also?
| if (panel?.metaData?.toolUrl) { | ||
| try { | ||
| const toolUrl = panel.metaData.toolUrl; | ||
| const isAbsolute = toolUrl.includes('://'); |
There was a problem hiding this comment.
IMO This check is not required
There was a problem hiding this comment.
Pull request overview
Fixes an issue where restored SQL Editor/Query Tool tabs could be misclassified on startup (due to is_query_tool being restored as a boolean), which could lead to unintended query auto-execution.
Changes:
- Normalize
is_query_toolandrestoreto support both boolean and string inputs during initialization. - Improve restore flow by clearing the restore flag after content restoration and adjusting initialization behavior.
- Attempt to persist “promoted to Query Tool” state by updating the panel’s stored
toolUrl.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| //this condition works if user is in View/Edit Data or user does not saved server or tunnel password and disconnected the server and executing the query | ||
| if(!qtState.params.is_query_tool || reexecute) { | ||
| if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore !== true) { |
| url.searchParams.set('is_query_tool', 'true'); | ||
| panel.metaData = Object.assign({}, panel.metaData, {toolUrl: url.toString()}); |
|
I'll address the suggestions and requested changes in a day or a two. |
Await restored content, preserve View/Edit Data execution, and save updated tool URLs.
|
The requested changes and improvements from @mzabuawala and Co-pilto are both adresssed pls review once @asheshv |
|
Thanks @dev-hari-prasad, this is a solid fix for a genuinely nasty bug, a restored I traced the gate at On the existing threads:
Two minor notes, neither blocking:
No changelog entry needed, by the way, we now add those in a batch just before release rather than per-PR. |
|
Is this PR ready for merge, or is there something else I need to address? |
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
Thanks for digging into this, and apologies for the slow review. I've gone through the change in full file context rather than just the diff. The underlying bug is real and nasty, and one of the three behavioural changes here is the right fix for it, but I think the other two are wrong, so I'd hold off merging as it stands.
First, a correction to the premise, because it matters for whoever reads this in a year's time: the params that reach QueryToolComponent are built from request.args and request.form in panel() (web/pgadmin/tools/sqleditor/__init__.py:364), so is_query_tool always arrives as a string and can never be a boolean. The actual mechanism is the promotion path: typing into the editor of a View/Edit Data tab calls promoteToQueryTool() (Query.jsx:390), which flips is_query_tool in memory only, whilst the persisted metaData.toolUrl still carries is_query_tool=false. On restore the tab is therefore treated as View/Edit Data, !qtState.params.is_query_tool is true, and TRIGGER_EXECUTION fires against whatever SQL was restored into the editor. That is the bug, and the toggleQueryTool change is exactly the right fix for it; the is_query_tool and restore normalisation is harmless defensive tidying that does no work today. It would be worth rewriting the PR description accordingly.
Detailed comments are inline, two of which I'd treat as blockers. A few smaller notes that don't attach neatly to a line:
- Anyone upgrading with an already-promoted tab in their saved layout will still have
is_query_tool=falsepersisted, so they'll hit the bug once more after upgrading. Probably acceptable, but worth being aware of. - There are no tests. The interesting behaviour here is docker and layout bound so a full test would be awkward, but a small Jest spec around the URL rewriting would be cheap, and would have caught the first of the two blockers below.
- CI is green (32 passed, 2 skipped); the
BLOCKEDmerge state is purely the unresolved review threads.
No release notes entry is needed, incidentally: we now batch those up shortly before a release rather than doing one per PR.
| const toolUrl = panel.metaData.toolUrl; | ||
| const url = new URL(toolUrl, window.location.origin); | ||
| url.searchParams.set('is_query_tool', 'true'); | ||
| panel.metaData = Object.assign({}, panel.metaData, {toolUrl: url.toString()}); |
There was a problem hiding this comment.
This one is a blocker, I'm afraid. new URL(toolUrl, window.location.origin) followed by url.toString() always yields an absolute URL, so metaData.toolUrl changes from /sqleditor/panel/123?... to http://127.0.0.1:<port>/sqleditor/panel/123?..., and that is what gets persisted into the saved layout.
The Desktop runtime defaults to fixedPort: false (runtime/src/js/pgadmin.js:24) and therefore calls getAvailablePort(0), so the port is different on every launch. At the next start-up, ToolForm will POST its action at a dead origin and the tab simply won't come back: cross-origin, no session, no CSRF token. That breaks tool tab restore in precisely the flow this PR is trying to repair, and Desktop mode is where the issue was reported from.
Note that this is also why dropping the earlier isAbsolute check made no difference either way: both variants ended in toString(). Keeping it relative is enough:
panel.metaData = {...panel.metaData, toolUrl: url.pathname + url.search};| }); | ||
| //this condition works if user is in View/Edit Data or user does not saved server or tunnel password and disconnected the server and executing the query | ||
| if(!qtState.params.is_query_tool || reexecute) { | ||
| if(!qtState.params.is_query_tool || (reexecute && qtState.params.restore !== true)) { |
There was a problem hiding this comment.
The other blocker. As written this clause is either dead code or an active regression, and never useful.
On the mount path (line 445) initializeQueryTool() is called with no arguments, so reexecute is always false and the new clause cannot affect the reported bug at all.
The only caller that passes reexecute=true is REINIT_QT_CONNECTION, fired from ResultSet.jsx:259 and :268 after the 428 password prompt and after the CRYPTKEY_MISSING master password prompt. That listener is registered once inside the mount-only effect at line 447 (dependency array []), so it holds render one's qtState, in which params.restore is true for any restored tab and stays that way for the lifetime of the component. The restore: false that restoreToolContent now writes lands in state that this particular closure will never see.
The net effect is that on every session-restored Query Tool tab, for as long as that tab is open, re-entering a password after a dropped connection will reconnect and then silently not run the query, which is a fairly unpleasant thing to debug from a bug report.
I'd just drop the clause: the toggleQueryTool change is what actually fixes the reported issue.
| connection_list: qtState.connection_list, | ||
| current_file: qtState.current_file, | ||
| toggleQueryTool: () => setQtStatePartial((prev)=>{ | ||
| let panel = qtPanelDocker?.find(qtPanelId); |
There was a problem hiding this comment.
Two smaller points on this block.
Firstly, the docker lookup, the panel.metaData mutation and the saveLayout() call all happen inside the setQtStatePartial((prev)=>{...}) updater. Updater functions are required to be pure, and React is free to invoke them more than once; there's no StrictMode in this codebase today so it won't misbehave right now, but it's a trap for whoever turns it on. Moving the whole block above the setQtStatePartial call costs nothing.
Secondly, if qtPanelDocker?.find(qtPanelId) returns undefined, say because the panel lives in the other workspace's docker, the promotion silently isn't persisted and nothing is logged; only a URL parse failure warns. A console.warn on the miss would save someone a long afternoon.
| }; | ||
|
|
||
| const getSQLScript = () => { | ||
| const getSQLScript = async () => { |
There was a problem hiding this comment.
Minor: getSQLScript is now async and awaits restoreToolContent(), but its only caller (line 444) still doesn't await it, so the await buys nothing as things stand. Either await it there too, or leave the function synchronous.
What this fixes
Fixes an issue where Query Tool tabs are restored from a previous session can/start automatically executing their queries when pgAdmin starts.
What was happening
While looking into this, I found that
is_query_toolwas only being checked against the string value 'true'`.During the session restoration process the value can be restored as a boolean instead. When that happens, the tab gets treated as a view/edit data tab instead of a Query toool tab and can end up going through the auto execution logic during initialization. Which can lead to descurtive and unwanted queries to run against the database and this is not what a user would expect.
Fix
This change addresses the restore flow in a few places:
is_query_toolis_query_toolstate in the saved metadataTesting
PS: You will need to build the app with the changes made in this branch inorder to see this in effect otherwise the offical build will emit the same bevhiour as descirbied in the issue #10031.
Fixes #10031
Summary by CodeRabbit
trueor the string"true".