From 7eb8a501e7e72d60e97655daaed5e5a962c52dd8 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 22:30:35 +0100 Subject: [PATCH 1/3] Revert the Query Tool execute option checkboxes when changing an option fails. #10600 The request was sent from inside the state updater, so the revert in its catch handler had no effect, server cursor's context value was never reverted, and a 200 response with status false was treated as success. The request is now sent outside the updater, both failure cases revert the checkbox (and server cursor), and execute option requests are sent one at a time with the menu items disabled whilst a change is in flight, so that concurrent read-modify-writes of the session's transaction object cannot overwrite each other. --- .../js/components/sections/MainToolBar.jsx | 81 +++++++++--- .../sqleditor/MainToolBarRestore.spec.js | 118 +++++++++++++++++- 2 files changed, 178 insertions(+), 21 deletions(-) diff --git a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx index 04908e2e2ff..bb84d51e40f 100644 --- a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx +++ b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx @@ -50,7 +50,14 @@ function changeQueryExecutionSettings(type, api, transId, value) { let url = url_for(`sqleditor.${type}`, { 'trans_id': transId, }); - return api.post(url, JSON.stringify(value)); + return api.post(url, JSON.stringify(value)).then((res)=>{ + // The server answers 200 with status false when it cannot check the + // transaction or connection, in which case nothing was changed. + if(res?.data?.data?.status === false) { + throw new Error(res.data.data.result || gettext('Failed to change the execute option.')); + } + return res; + }); } // Execute options that also live on the server side transaction object, so @@ -107,6 +114,13 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const [pendingExecOptions, setPendingExecOptions] = useState(null); // The number of restored execute options still being sent to the server. const [applyingExecOptions, setApplyingExecOptions] = useState(0); + /* Each execute option request rewrites the whole transaction object kept in + the session, so they are sent one at a time, chained on this promise, so + that a later request cannot overwrite an earlier one's change. */ + const execOptionsQueueRef = React.useRef(Promise.resolve()); + // Whether an execute option changed from the menu is still being sent. + const [changingExecOption, setChangingExecOption] = useState(false); + const changingExecOptionRef = React.useRef(false); const {isSaveToolDataEnabled} = useApplicationState() ?? {}; /* Menu button refs */ const saveAsMenuRef = React.useRef(null); @@ -168,22 +182,53 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT explain(true); }, [explain]); + const sendExecOption = useCallback((option, value)=>{ + // Start from the queue so that a synchronous failure is handled too. + const request = execOptionsQueueRef.current + .then(()=>changeQueryExecutionSettings(option, queryToolCtx.api, queryToolCtx.params.trans_id, value)); + execOptionsQueueRef.current = request.catch(()=>{/* handled by the caller */}); + return request; + }, []); + const checkMenuClick = useCallback((e)=>{ - setCheckedMenuItems((prev)=>{ - let newVal = !prev[e.value]; - if (e.value === 'server_cursor') { - queryToolCtx.updateServerCursor({server_cursor: newVal}); + if(EXECUTION_OPTIONS.includes(e.value)) { + if(changingExecOptionRef.current) { + return; } - if(e.value === 'auto_commit' || e.value === 'auto_rollback' || e.value === 'server_cursor') { - changeQueryExecutionSettings(e.value, queryToolCtx.api, queryToolCtx.params.trans_id, newVal) - .catch ((error)=>{ - newVal = prev[e.value]; - eventBus.fireEvent(QUERY_TOOL_EVENTS.HANDLE_API_ERROR, error, { - checkTransaction: true, - }); + const option = e.value; + const oldVal = Boolean(checkedMenuItemsRef.current[option]); + const newVal = !oldVal; + const setOption = (val)=>{ + setCheckedMenuItems((prev)=>({...prev, [option]: val})); + if(option === 'server_cursor') { + queryToolCtx.updateServerCursor({server_cursor: val}); + } + }; + changingExecOptionRef.current = true; + setChangingExecOption(true); + setOption(newVal); + sendExecOption(option, newVal) + .catch((error)=>{ + setOption(oldVal); + // Save the reverted value so the tab state matches the server. + if(isSaveToolDataEnabled?.('sqleditor')) { + eventBus.fireEvent(QUERY_TOOL_EVENTS.TRIGGER_SAVE_QUERY_TOOL_DATA); + } + eventBus.fireEvent(QUERY_TOOL_EVENTS.HANDLE_API_ERROR, error, { + checkTransaction: true, }); + }) + .finally(()=>{ + changingExecOptionRef.current = false; + setChangingExecOption(false); + }); + if(isSaveToolDataEnabled?.('sqleditor')) { + eventBus.fireEvent(QUERY_TOOL_EVENTS.TRIGGER_SAVE_QUERY_TOOL_DATA); } - + return; + } + setCheckedMenuItems((prev)=>{ + let newVal = !prev[e.value]; let otherVars = {}; if (e.value === 'explain_serialize_binary' && newVal) { otherVars = { 'explain_serialize_text': false }; @@ -242,9 +287,7 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const options = Object.entries(pendingExecOptions); setApplyingExecOptions((prev)=>prev + options.length); options.forEach(([option, value])=>{ - // Start from a promise so that a synchronous failure is handled too. - Promise.resolve() - .then(()=>changeQueryExecutionSettings(option, queryToolCtx.api, queryToolCtx.params.trans_id, value)) + sendExecOption(option, value) .catch((error)=>{ setCheckedMenuItems((prev)=>({...prev, [option]: !value})); if(option === 'server_cursor') { @@ -759,11 +802,11 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT label={gettext('Execute Options Menu')} > {gettext('Auto commit?')} + disabled={changingExecOption} onClick={checkMenuClick}>{gettext('Auto commit?')} {gettext('Auto rollback on error?')} + disabled={changingExecOption} onClick={checkMenuClick}>{gettext('Auto rollback on error?')} {gettext('Use server cursor?')} + disabled={changingExecOption} onClick={checkMenuClick}>{gettext('Use server cursor?')} { act(() => { eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false, auto_rollback: true}); }); - await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(1)); await waitFor(() => expectGated(true)); - // Still disabled until every option has reached the server. + // Still disabled until every option has reached the server, and the + // options are sent one at a time. await act(async () => { resolvers[0]({data: {}}); }); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(2)); expectGated(true); await act(async () => { resolvers[1]({data: {}}); }); @@ -327,3 +329,115 @@ describe('MainToolBar toolbar options restore', () => { expect(queryToolCtx.api.post).not.toHaveBeenCalled(); }); }); + +describe('MainToolBar execute options menu', () => { + let eventBus, queryToolCtx; + const ThemedMainToolBar = withTheme(MainToolBar); + + const renderToolBar = () => { + const containerRef = React.createRef(); + return render( + + + +
+ {/* noop */}} + onManageMacros={()=>{/* noop */}} onAddToMacros={()=>{/* noop */}} /> +
+ + + + ); + }; + + const menuItem = (label) => document.querySelector(`li[data-label="${label}"]`); + + const openMenuAndClick = async (label) => { + if(!menuItem(label)) { + await act(async () => { + fireDomEvent.click(document.querySelector('button[data-label="Execute options"]')); + }); + await waitFor(() => expect(menuItem(label)).not.toBeNull()); + } + await act(async () => { fireDomEvent.click(menuItem(label)); }); + }; + + beforeEach(() => { + eventBus = new EventBus(); + queryToolCtx = { + params: {trans_id: 1234, is_query_tool: true, macros: []}, + preferences: {sqleditor: {...sqleditorPrefs}, editor: {}}, + api: {post: jest.fn(() => Promise.resolve({data: {data: {status: true}}}))}, + modal: {}, + toolbarOptionsRef: {current: null}, + updateServerCursor: jest.fn(), + }; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('sends a changed execute option to the server', async () => { + renderToolBar(); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + await openMenuAndClick('Use server cursor?'); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledWith('/mock/sqleditor.server_cursor/1234', 'true')); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current.server_cursor).toBe(true)); + expect(queryToolCtx.updateServerCursor).toHaveBeenLastCalledWith({server_cursor: true}); + }); + + it('reverts an execute option when the request fails', async () => { + queryToolCtx.api.post = jest.fn(() => Promise.reject(new Error('failed'))); + const fireEvent = jest.spyOn(eventBus, 'fireEvent'); + renderToolBar(); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + await openMenuAndClick('Use server cursor?'); + await waitFor(() => { + expect(fireEvent).toHaveBeenCalledWith(QUERY_TOOL_EVENTS.HANDLE_API_ERROR, expect.any(Error), {checkTransaction: true}); + }); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current.server_cursor).toBe(false)); + expect(queryToolCtx.updateServerCursor).toHaveBeenLastCalledWith({server_cursor: false}); + expect(menuItem('Use server cursor?')).toHaveAttribute('data-checked', 'false'); + fireEvent.mockRestore(); + }); + + it('reverts an execute option when the server returns status false', async () => { + queryToolCtx.api.post = jest.fn(() => Promise.resolve({data: {data: {status: false, result: 'Not connected'}}})); + const fireEvent = jest.spyOn(eventBus, 'fireEvent'); + renderToolBar(); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + await openMenuAndClick('Auto commit?'); + await waitFor(() => { + expect(fireEvent).toHaveBeenCalledWith(QUERY_TOOL_EVENTS.HANDLE_API_ERROR, + expect.objectContaining({message: 'Not connected'}), {checkTransaction: true}); + }); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current.auto_commit).toBe(true)); + fireEvent.mockRestore(); + }); + + it('disables the execute options whilst a change is being sent', async () => { + let resolvePost; + queryToolCtx.api.post = jest.fn(() => new Promise((resolve) => { resolvePost = resolve; })); + renderToolBar(); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + await openMenuAndClick('Auto commit?'); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(1)); + ['Auto commit?', 'Auto rollback on error?', 'Use server cursor?'].forEach((label) => { + expect(menuItem(label)).toHaveAttribute('aria-disabled', 'true'); + }); + + // A click whilst the request is in flight is ignored. + await act(async () => { fireDomEvent.click(menuItem('Auto rollback on error?')); }); + expect(queryToolCtx.api.post).toHaveBeenCalledTimes(1); + + await act(async () => { resolvePost({data: {data: {status: true}}}); }); + await waitFor(() => expect(menuItem('Auto rollback on error?')).not.toHaveAttribute('aria-disabled', 'true')); + expect(queryToolCtx.toolbarOptionsRef.current.auto_commit).toBe(false); + expect(queryToolCtx.toolbarOptionsRef.current.auto_rollback).toBe(false); + }); +}); From 83911ef7241ba8af27fb9b5a28d35a7fecaff4e1 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 23:27:18 +0100 Subject: [PATCH 2/3] Send Query Tool execute options to the current transaction. sendExecOption and checkMenuClick are memoised once, so they captured the first render's Query Tool context and, after a connection change replaced the transaction, kept sending execute option changes (and server cursor updates) to the old, dropped transaction id. Read the context from a ref at the moment each queued request is sent instead. --- .../js/components/sections/MainToolBar.jsx | 12 ++++- .../sqleditor/MainToolBarRestore.spec.js | 50 +++++++++++++++++-- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx index bb84d51e40f..761dd1b1e53 100644 --- a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx +++ b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx @@ -91,6 +91,10 @@ export function getChangedExecutionOptions(restored, current) { export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddToMacros}) { const eventBus = useContext(QueryToolEventsContext); const queryToolCtx = useContext(QueryToolContext); + /* A connection change replaces the transaction, so callbacks that outlive + a render read the current context from here rather than capturing it. */ + const queryToolCtxRef = React.useRef(queryToolCtx); + queryToolCtxRef.current = queryToolCtx; const queryToolConnCtx = useContext(QueryToolConnectionContext); const modalId = MODAL_DIALOGS.QT_CONFIRMATIONS; @@ -184,8 +188,12 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const sendExecOption = useCallback((option, value)=>{ // Start from the queue so that a synchronous failure is handled too. + // Read the context when the request is sent, as it may be queued. const request = execOptionsQueueRef.current - .then(()=>changeQueryExecutionSettings(option, queryToolCtx.api, queryToolCtx.params.trans_id, value)); + .then(()=>{ + const {api, params} = queryToolCtxRef.current; + return changeQueryExecutionSettings(option, api, params.trans_id, value); + }); execOptionsQueueRef.current = request.catch(()=>{/* handled by the caller */}); return request; }, []); @@ -201,7 +209,7 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const setOption = (val)=>{ setCheckedMenuItems((prev)=>({...prev, [option]: val})); if(option === 'server_cursor') { - queryToolCtx.updateServerCursor({server_cursor: val}); + queryToolCtxRef.current.updateServerCursor({server_cursor: val}); } }; changingExecOptionRef.current = true; diff --git a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js index 855f6446d40..f8250feb8b0 100644 --- a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js +++ b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js @@ -101,7 +101,15 @@ describe('MainToolBar toolbar options restore', () => { ); queryToolCtx.params.is_query_tool = isQueryTool; const result = render(ui(connected)); - return {...result, setConnected: (conn) => result.rerender(ui(conn))}; + return { + ...result, + setConnected: (conn, ctx) => { + if(ctx) { + queryToolCtx = ctx; + } + result.rerender(ui(conn)); + }, + }; }; beforeEach(() => { @@ -160,6 +168,21 @@ describe('MainToolBar toolbar options restore', () => { expect(queryToolCtx.api.post).toHaveBeenCalledWith('/mock/sqleditor.server_cursor/1234', 'true'); }); + it('sends restored execute options to the transaction current when connected', async () => { + const {setConnected} = renderToolBar(false); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false}); + }); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current.auto_commit).toBe(false)); + + // The transaction is replaced before the pending option is sent. + setConnected(true, {...queryToolCtx, params: {...queryToolCtx.params, trans_id: 5678}}); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(1)); + expect(queryToolCtx.api.post).toHaveBeenCalledWith('/mock/sqleditor.auto_commit/5678', 'false'); + }); + it('reverts and saves a restored execute option the server rejects', async () => { queryToolCtx.api.post = jest.fn(() => Promise.reject(new Error('failed'))); const fireEvent = jest.spyOn(eventBus, 'fireEvent'); @@ -336,8 +359,8 @@ describe('MainToolBar execute options menu', () => { const renderToolBar = () => { const containerRef = React.createRef(); - return render( - + const ui = (ctx) => ( +
@@ -348,6 +371,8 @@ describe('MainToolBar execute options menu', () => { ); + const result = render(ui(queryToolCtx)); + return {...result, setContext: (ctx) => result.rerender(ui(ctx))}; }; const menuItem = (label) => document.querySelector(`li[data-label="${label}"]`); @@ -388,6 +413,25 @@ describe('MainToolBar execute options menu', () => { expect(queryToolCtx.updateServerCursor).toHaveBeenLastCalledWith({server_cursor: true}); }); + it('sends execute options to the current transaction after a connection change', async () => { + const {setContext} = renderToolBar(); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + // A connection change replaces the transaction with a new one. + const newCtx = { + ...queryToolCtx, + params: {...queryToolCtx.params, trans_id: 5678}, + updateServerCursor: jest.fn(), + }; + setContext(newCtx); + + await openMenuAndClick('Use server cursor?'); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledWith('/mock/sqleditor.server_cursor/5678', 'true')); + expect(queryToolCtx.api.post).not.toHaveBeenCalledWith('/mock/sqleditor.server_cursor/1234', expect.anything()); + expect(newCtx.updateServerCursor).toHaveBeenLastCalledWith({server_cursor: true}); + expect(queryToolCtx.updateServerCursor).not.toHaveBeenCalled(); + }); + it('reverts an execute option when the request fails', async () => { queryToolCtx.api.post = jest.fn(() => Promise.reject(new Error('failed'))); const fireEvent = jest.spyOn(eventBus, 'fireEvent'); From 41baa179ef9cecbaf8dd88a44f163b8b9b37bbb4 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Fri, 16 Oct 2026 01:19:44 +0100 Subject: [PATCH 3/3] Update the Query Tool context ref in a layout effect rather than during render. --- .../sqleditor/static/js/components/sections/MainToolBar.jsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx index 761dd1b1e53..1a7b2646eca 100644 --- a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx +++ b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx @@ -94,7 +94,9 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT /* A connection change replaces the transaction, so callbacks that outlive a render read the current context from here rather than capturing it. */ const queryToolCtxRef = React.useRef(queryToolCtx); - queryToolCtxRef.current = queryToolCtx; + React.useLayoutEffect(()=>{ + queryToolCtxRef.current = queryToolCtx; + }, [queryToolCtx]); const queryToolConnCtx = useContext(QueryToolConnectionContext); const modalId = MODAL_DIALOGS.QT_CONFIRMATIONS;