From 8daf40128cefbc7ecd7be0609cce770003d98380 Mon Sep 17 00:00:00 2001 From: Ashesh Vashi Date: Fri, 5 Jun 2026 18:52:39 +0530 Subject: [PATCH 1/3] fix(datagridview): correct silent-failure bugs in DataGridView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four upstream bugs that silently no-op'd, exposed by aggressive review of the SchemaView/DataGridView code: - Feature priority comparator typo: `f1.priorty` is undefined, so the < comparison was always false and features never sorted. - Row className: `classList.join[' ']` bracket-accesses `Array.prototype.join` (returns undefined), so every grid row rendered with no CSS classes. - Reorder hover class: `classList?.append(...)` — Arrays have no `.append` method; the optional-chain silently no-op'd, so the hover row highlight was never applied during drag-reorder. - Inherited-column lock: `inSchemaWithColumnCheck` short-circuited on `isNew(state)` before reading `inheritedfrom`. In a child table CREATE flow, inherited columns arrive with `attnum` undefined (so `isNew` returns true) — the function returned false (not disabled), making Name and datatype editable on columns that should mirror the parent table. New regression tests pin each fix. --- .../tables/columns/static/js/column.ui.js | 18 ++-- .../DataGridView/features/feature.js | 2 +- .../DataGridView/features/reorder.jsx | 5 +- .../static/js/SchemaView/DataGridView/row.jsx | 2 +- .../SchemaView/feature_register.spec.js | 48 +++++++++ .../no_bracket_on_prototype_method.spec.js | 102 ++++++++++++++++++ .../schema_ui_files/column.ui.spec.js | 39 +++++++ 7 files changed, 206 insertions(+), 10 deletions(-) create mode 100644 web/regression/javascript/SchemaView/feature_register.spec.js create mode 100644 web/regression/javascript/SchemaView/no_bracket_on_prototype_method.spec.js diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js index 062464c504f..76ff8f1c42d 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js @@ -88,17 +88,21 @@ export default class ColumnSchema extends BaseUISchema { } if(this.nodeInfo && ('schema' in this.nodeInfo)) { - if(this.isNew(state)) { - return false; - } - - // We will disable control if it's system columns - // inheritedfrom check is useful when we use this schema in table node - // inheritedfrom has value then we should disable it + // Parent-table inheritance locks the column in BOTH create and + // edit modes — its Name and datatype mirror the parent. The + // `isNew(state)` shortcut below must NOT run first, otherwise + // inherited columns become editable on a fresh child-table + // dialog. (`OF TYPE` inheritance uses `inheritedfromtype` and is + // handled by the dedicated `editable` callback further down — + // not by this method.) if (!isEmptyString(state.inheritedfrom)){ return true; } + if(this.isNew(state)) { + return false; + } + // ie: it's position is less than 1 return !(!_.isUndefined(state.attnum) && state.attnum > 0); } diff --git a/web/pgadmin/static/js/SchemaView/DataGridView/features/feature.js b/web/pgadmin/static/js/SchemaView/DataGridView/features/feature.js index 5c06ea42c3d..2bb34999ebc 100644 --- a/web/pgadmin/static/js/SchemaView/DataGridView/features/feature.js +++ b/web/pgadmin/static/js/SchemaView/DataGridView/features/feature.js @@ -67,7 +67,7 @@ function addToSortedList(_list, _item, _comparator = (a, b) => (a < b)) { _list.splice(idx, 0, _item); } -const featurePriorityCompare = (f1, f2) => (f1.priorty < f2.priority); +const featurePriorityCompare = (f1, f2) => (f1.priority < f2.priority); export function register(cls) { diff --git a/web/pgadmin/static/js/SchemaView/DataGridView/features/reorder.jsx b/web/pgadmin/static/js/SchemaView/DataGridView/features/reorder.jsx index aaa7ebee9be..768edec0970 100644 --- a/web/pgadmin/static/js/SchemaView/DataGridView/features/reorder.jsx +++ b/web/pgadmin/static/js/SchemaView/DataGridView/features/reorder.jsx @@ -156,7 +156,10 @@ export default class Reorder extends Feature { preview(rowRef); if (index == this.hoverIndex) { - classList?.append('DataGridView-tableRowHovered'); + // `classList` is an Array (see `let classList = []` in row.jsx) + // — Array.prototype has no `.append`, so `.append` would + // silently no-op via the optional chain. + classList?.push('DataGridView-tableRowHovered'); } row.dragHandlerId = handlerId; diff --git a/web/pgadmin/static/js/SchemaView/DataGridView/row.jsx b/web/pgadmin/static/js/SchemaView/DataGridView/row.jsx index 27b24ede15e..48a402f9522 100644 --- a/web/pgadmin/static/js/SchemaView/DataGridView/row.jsx +++ b/web/pgadmin/static/js/SchemaView/DataGridView/row.jsx @@ -58,7 +58,7 @@ export function DataGridRow({row, isResizing}) { diff --git a/web/regression/javascript/SchemaView/feature_register.spec.js b/web/regression/javascript/SchemaView/feature_register.spec.js new file mode 100644 index 00000000000..9ce54c808b2 --- /dev/null +++ b/web/regression/javascript/SchemaView/feature_register.spec.js @@ -0,0 +1,48 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Tests for the feature-priority-ordering contract on +// DataGridView/features/feature.js's `register()`. Features are added to +// the module-scoped sorted list via a comparator that checks +// `priority`. A typo on either side of that comparator silently breaks +// the order (the comparator returns `undefined < N` which is always +// false) and features end up in import-registration order rather than +// in declared-priority order. + +import Feature, { + register, + FeatureSet, +} from '../../../pgadmin/static/js/SchemaView/DataGridView/features/feature'; + +class HighPriorityFeature extends Feature { + static priority = 999; +} +class LowPriorityFeature extends Feature { + static priority = 1; +} + +describe('DataGridView features — register() priority ordering', () => { + test('register sorts classes by static priority (low priority first)', () => { + // Register HIGH first then LOW. If the comparator works correctly, + // LOW (priority=1) must end up at a lower index than HIGH + // (priority=999) regardless of registration order. + register(HighPriorityFeature); + register(LowPriorityFeature); + + const fs = new FeatureSet(); + const lowIdx = fs.features + .findIndex((f) => f.constructor === LowPriorityFeature); + const highIdx = fs.features + .findIndex((f) => f.constructor === HighPriorityFeature); + + expect(lowIdx).toBeGreaterThanOrEqual(0); + expect(highIdx).toBeGreaterThanOrEqual(0); + expect(lowIdx).toBeLessThan(highIdx); + }); +}); diff --git a/web/regression/javascript/SchemaView/no_bracket_on_prototype_method.spec.js b/web/regression/javascript/SchemaView/no_bracket_on_prototype_method.spec.js new file mode 100644 index 00000000000..478b5b6dba7 --- /dev/null +++ b/web/regression/javascript/SchemaView/no_bracket_on_prototype_method.spec.js @@ -0,0 +1,102 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Guard against a recurring typo class found during a PEM audit: +// using SQUARE brackets to "call" a prototype method that's a +// function: +// +// classList.join[' '] // wrong: property access on `join`, undefined +// memDeps.push[newProps] // wrong: property access on `push`, undefined +// +// Both expressions evaluate to `undefined`, silently no-op, and survive +// type checking because JS allows arbitrary property access. They are +// almost always a typo for the function call form `.join(' ')` or +// `.push(newProps)`. +// +// We don't have an ESLint rule for this yet, so a regression test that +// walks the SchemaView source tree is the next best thing. + +import fs from 'fs'; +import path from 'path'; + +const SCHEMA_VIEW_DIR = path.resolve( + __dirname, + '..', + '..', + '..', + 'pgadmin', + 'static', + 'js', + 'SchemaView' +); + +const HOT_METHODS = [ + 'push', 'pop', 'shift', 'unshift', + 'join', 'map', 'filter', 'forEach', 'reduce', 'reduceRight', + 'concat', 'slice', 'splice', 'sort', 'reverse', + 'some', 'every', 'find', 'findIndex', 'flat', 'flatMap', + 'indexOf', 'lastIndexOf', 'includes', +]; + +// `.method[` anywhere in the source is the smoke signal. +const ANTI_PATTERN = new RegExp( + String.raw`\.(?:${HOT_METHODS.join('|')})\s*\[`, + 'g' +); + +const collectFiles = (dir) => { + const entries = fs.readdirSync(dir, { withFileTypes: true }); + return entries.flatMap((e) => { + const p = path.join(dir, e.name); + if (e.isDirectory()) return collectFiles(p); + if (/\.(jsx?|tsx?)$/.test(e.name)) return [p]; + return []; + }); +}; + +describe('SchemaView source — bracket-on-prototype-method anti-pattern', () => { + test('no .push[...] / .join[...] / .map[...] etc. anywhere under SchemaView', () => { + const files = collectFiles(SCHEMA_VIEW_DIR); + const offenders = []; + + for (const file of files) { + const src = fs.readFileSync(file, 'utf8'); + // Reset regex state and scan line-by-line for better error + // messages. + const lines = src.split(/\r?\n/); + lines.forEach((line, i) => { + if (line.includes('eslint-disable')) return; // explicit opt-out + // Skip pure single-line comments — the test catches its own + // documentation otherwise. Block-comment detection would need + // a real lexer; the convention "code comes before trailing //" + // is enough. + const trimmed = line.trim(); + if (trimmed.startsWith('//') || trimmed.startsWith('*')) return; + // Strip trailing line comments before matching. + const codeOnly = line.split(/\/\//)[0]; + ANTI_PATTERN.lastIndex = 0; + if (ANTI_PATTERN.test(codeOnly)) { + offenders.push(`${path.relative(SCHEMA_VIEW_DIR, file)}:${i + 1} ${trimmed}`); + } + }); + } + + if (offenders.length > 0) { + const msg = [ + 'Bracket-on-prototype-method anti-pattern found ' + + '(likely typo for a function call):', + ...offenders.map((o) => ' ' + o), + '', + 'If this is intentional (legitimate property access on a function', + 'object), add an `eslint-disable` marker on the line.', + ].join('\n'); + throw new Error(msg); + } + }); +}); diff --git a/web/regression/javascript/schema_ui_files/column.ui.spec.js b/web/regression/javascript/schema_ui_files/column.ui.spec.js index cc99104e044..f24c433b508 100644 --- a/web/regression/javascript/schema_ui_files/column.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/column.ui.spec.js @@ -133,6 +133,45 @@ describe('ColumnSchema', ()=>{ expect(schemaObj.inSchemaWithColumnCheck(state)).toBe(false); }); + // The case above sets both `attnum=-1` AND `inheritedfrom` before + // checking the inherited path, which masked the real bug: in CREATE + // mode (no attnum) for a child table inheriting from a parent, the + // `isNew(state) → return false` shortcut fired BEFORE the + // `inheritedfrom` check, leaving Name/datatype editable on columns + // that should mirror the parent. + it('inSchemaWithColumnCheck — inherited column in CREATE mode is ' + + 'disabled (the `inheritedfrom` check must run before the ' + + 'create-mode shortcut, not after it)', ()=>{ + schemaObj.nodeInfo = {schema: {}}; + + // Sanity: fresh non-inherited column in create mode is editable. + let fresh = {}; + expect(schemaObj.isNew(fresh)).toBe(true); + expect(schemaObj.inSchemaWithColumnCheck(fresh)).toBe(false); + + // BUG CASE: child table inherits from parent. Inherited columns + // arrive with `inheritedfrom` set; `attnum` is undefined (no + // server-side identity yet → isNew=true). They should be disabled + // in BOTH create and edit modes (Name/datatype mirror the parent). + let inheritedInCreate = { inheritedfrom: 140391 }; + expect(schemaObj.isNew(inheritedInCreate)).toBe(true); + expect(schemaObj.inSchemaWithColumnCheck(inheritedInCreate)).toBe(true); + + // Companion: same column in EDIT mode — also disabled. + let inheritedInEdit = { inheritedfrom: 140391, attnum: 5 }; + expect(schemaObj.isNew(inheritedInEdit)).toBe(false); + expect(schemaObj.inSchemaWithColumnCheck(inheritedInEdit)).toBe(true); + + // An `OF TYPE`-inherited column carries `inheritedfromtype`, NOT + // `inheritedfrom` — that case is editable (the cltype field's + // dedicated `editable` callback unwraps it). + // `inSchemaWithColumnCheck` only locks `inheritedfrom`; this assert + // pins that contract. + let ofType = { inheritedfromtype: 16384 }; + expect(schemaObj.isNew(ofType)).toBe(true); + expect(schemaObj.inSchemaWithColumnCheck(ofType)).toBe(false); + }); + it('editableCheckForTable', ()=>{ let state = {}; schemaObj.nodeInfo = {}; From 5f7977ee11f3023f60a88309fbf4cae407da1070 Mon Sep 17 00:00:00 2001 From: Ashesh Vashi Date: Fri, 5 Jun 2026 18:52:56 +0530 Subject: [PATCH 2/3] feat(schemaview): introduce deferredDepChange protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the queue-based deferredDepChange protocol so schemas can run async work (confirm dialogs, server fetches) atomically across a state change, with first-class error surfacing. Core infrastructure: - `SchemaState/reducer.js`: compose actionObj (type/path/value/ depChange + _.cloneDeep(oldState)) and append resolved callbacks to data.__deferred__. - `hooks/useSchemaState.js`: `drainDeferredQueue` processes queued promises in order; rejections surface via pgAdmin.Browser.notifier.error (falls back to console.error when the notifier is unavailable). Drain useEffect depends on the __deferred__ ref identity rather than .length so replaced entries reliably re-trigger. - `DepListener.js`: prefix-match guard uses delimiter-terminated joins so 'shared' doesn't match 'shared_username'. Caches joined source/dest keys for the hot match path. addDepListener deep-copies the source path so a caller mutating its own array later doesn't poison the listener. - `utils/listenDepChanges.js`: full JSDoc contract for depChange and deferredDepChange callbacks — return shape, when to opt out (return undefined), side-effect rule, actionObj fields, drain error-surfacing contract. Tests: - dep_listener.spec.js: prefix-match isolation, defensive snapshot, listener lifecycle. - reducer_deferred.spec.js: queue mechanism, actionObj composition. - deferred_drain.spec.js: serial processing, callback delta application, notifier.error on rejection, console.error fallback. - drain_useeffect_race.spec.jsx: deterministic race regression test for the ref-vs-length useEffect dependency fix. --- .../static/js/SchemaView/DepListener.js | 74 +++-- .../js/SchemaView/SchemaState/reducer.js | 9 +- .../js/SchemaView/hooks/useSchemaState.js | 91 +++++- .../js/SchemaView/utils/listenDepChanges.js | 46 +++ .../SchemaView/deferred_drain.spec.js | 121 ++++++++ .../SchemaView/dep_listener.spec.js | 265 ++++++++++++++++++ .../SchemaView/drain_useeffect_race.spec.jsx | 104 +++++++ .../SchemaView/reducer_deferred.spec.js | 94 +++++++ 8 files changed, 763 insertions(+), 41 deletions(-) create mode 100644 web/regression/javascript/SchemaView/deferred_drain.spec.js create mode 100644 web/regression/javascript/SchemaView/dep_listener.spec.js create mode 100644 web/regression/javascript/SchemaView/drain_useeffect_race.spec.jsx create mode 100644 web/regression/javascript/SchemaView/reducer_deferred.spec.js diff --git a/web/pgadmin/static/js/SchemaView/DepListener.js b/web/pgadmin/static/js/SchemaView/DepListener.js index 9c271462269..2bd9d0ec1e7 100644 --- a/web/pgadmin/static/js/SchemaView/DepListener.js +++ b/web/pgadmin/static/js/SchemaView/DepListener.js @@ -8,26 +8,49 @@ ////////////////////////////////////////////////////////////// import _ from 'lodash'; +// Join a path array the same way the filter predicates need to compare: +// segments separated by '|', terminated with an extra '|' so a listener +// registered on ['shared'] doesn't false-match a currPath of +// ['shared_username']. Equivalent to `_.join(arr.concat(['']), '|')` but +// avoids the array allocation that was visible in the hot path. +const _joinPath = (arr) => arr.join('|') + '|'; export class DepListener { constructor() { this._depListeners = []; + // True iff at least one registered listener has a defCallback. Lets + // getDeferredDepChange short-circuit when there's no deferred work + // possible — common in synthetic schemas and the dominant case in + // typical dialogs. + this._hasDefCallback = false; } /* Will keep track of the dependent fields and there callbacks */ addDepListener(source, dest, callback, defCallback) { this._depListeners = this._depListeners || []; + // Defensive shallow copy of the source path. The cached _sourceKey + // is already a string snapshot and isn't affected by post- + // registration mutation, but `source` itself is passed to the + // callback at dispatch time — a caller that re-uses and mutates + // the array would silently corrupt later callback invocations. + const sourceCopy = Array.from(source); this._depListeners.push({ - source: source, + source: sourceCopy, dest: dest, callback: callback, - defCallback: defCallback + defCallback: defCallback, + // Pre-compute the source's joined form so the per-dispatch filters + // in getDepChange / getDeferredDepChange don't re-join + re-allocate + // for every listener on every keystroke. + _sourceKey: _joinPath(sourceCopy), }); + if (defCallback) this._hasDefCallback = true; } removeDepListener(dest) { this._depListeners = _.filter(this._depListeners, (l)=>!_.join(l.dest, '|').startsWith(_.join(dest, '|'))); + this._hasDefCallback = this._depListeners.some((l) => l.defCallback); } _getListenerData(state, listener, actionObj) { @@ -59,37 +82,36 @@ export class DepListener { /* If this comes from deferred change */ if(actionObj.listener?.callback) { state = this._getListenerData(state, actionObj.listener, actionObj); - } else { - // adding a extra item in path to avoid incorrect matching like shared and shared_username - let allListeners = _.filter(this._depListeners, (entry)=>_.join(currPath.concat(['']), '|').startsWith(_.join(entry.source.concat(['']), '|'))); - if(allListeners) { - for(const listener of allListeners) { - state = this._getListenerData(state, listener, actionObj); - } + return state; + } + // Compare against each listener using the pre-computed _sourceKey, + // which already encodes the trailing-'|' prefix-match protection. + const currKey = _joinPath(currPath); + for(const listener of this._depListeners) { + if(listener.callback && currKey.startsWith(listener._sourceKey)) { + state = this._getListenerData(state, listener, actionObj); } } return state; } getDeferredDepChange(currPath, state, actionObj) { - let deferredList = []; - let allListeners = _.filter(this._depListeners, (entry) => _.join( - currPath, '|' - ).startsWith(_.join(entry.source, '|'))); - - if(allListeners) { - for(const listener of allListeners) { - if(listener.defCallback) { - let thePromise = this._getDefListenerPromise(state, listener, actionObj); - if(thePromise) { - deferredList.push({ - action: actionObj, - promise: thePromise, - listener: listener, - }); - } - } + // Common case: nothing in the registry has a defCallback. Bail + // before touching any listener entries. + if(!this._hasDefCallback) return []; + const deferredList = []; + const currKey = _joinPath(currPath); + for(const listener of this._depListeners) { + if(!listener.defCallback) continue; + if(!currKey.startsWith(listener._sourceKey)) continue; + const thePromise = this._getDefListenerPromise(state, listener, actionObj); + if(thePromise) { + deferredList.push({ + action: actionObj, + promise: thePromise, + listener: listener, + }); } } return deferredList; diff --git a/web/pgadmin/static/js/SchemaView/SchemaState/reducer.js b/web/pgadmin/static/js/SchemaView/SchemaState/reducer.js index 4892d217427..170509da97a 100644 --- a/web/pgadmin/static/js/SchemaView/SchemaState/reducer.js +++ b/web/pgadmin/static/js/SchemaView/SchemaState/reducer.js @@ -67,7 +67,14 @@ export const sessDataReducer = (state, action) => { deferredList = getDeferredDepChange( action.path, _.cloneDeep(data), state, action ); - data.__deferred__ = deferredList || []; + // APPEND rather than replace — multiple SET_VALUEs in the same + // React batch each contribute their deferred promises to the queue. + // Replacing the array would lose still-pending promises from the + // previous action. The drain useEffect in useSchemaState then + // processes the full list and `CLEAR_DEFERRED_QUEUE` empties it. + if (deferredList && deferredList.length > 0) { + data.__deferred__ = (data.__deferred__ || []).concat(deferredList); + } break; case SCHEMA_STATE_ACTIONS.ADD_ROW: diff --git a/web/pgadmin/static/js/SchemaView/hooks/useSchemaState.js b/web/pgadmin/static/js/SchemaView/hooks/useSchemaState.js index 2bd8a0194a7..73af2d11992 100644 --- a/web/pgadmin/static/js/SchemaView/hooks/useSchemaState.js +++ b/web/pgadmin/static/js/SchemaView/hooks/useSchemaState.js @@ -9,6 +9,8 @@ import { useEffect, useReducer } from 'react'; import _ from 'lodash'; +import gettext from 'sources/gettext'; +import pgAdmin from 'sources/pgadmin'; import { prepareData } from '../common'; import { @@ -17,6 +19,72 @@ import { sessDataReducer, } from '../SchemaState'; +/** + * Drain a list of deferred-dep queue items. + * + * Each item is `{action, promise, listener}` produced by + * `DepListener.getDeferredDepChange`. We wait for each promise to settle + * and then dispatch a DEFERRED_DEPCHANGE action carrying the resolved + * callback. Two protocol guards: + * + * 1. The resolved value MUST be a function (the callback that returns + * the data delta). If it's anything else, we warn and skip — this + * catches the common protocol mistake of resolving with a data + * object directly. + * 2. Rejected promises surface to the user through + * `pgAdmin.Browser.notifier.error` so a backend failure can't + * leave the dialog in a half-applied state with no feedback. + * Schemas that want graceful in-place recovery should resolve + * with a reset callback rather than rejecting. + * + * Exported so it can be unit-tested without rendering a full SchemaView. + */ +export const drainDeferredQueue = (items, dispatch) => { + items.forEach((item) => { + Promise.resolve(item.promise).then( + (resFunc) => { + if (typeof resFunc !== 'function') { + // Protocol violation: the schema author resolved with + // something other than a (tmpstate) => deltaObj callback. + // Loud console.error so it trips dev/QA test suites; not a + // notifier toast because this is a code bug, not a runtime + // failure the end user can act on. + console.error( + 'deferredDepChange promise must resolve to a callback function; ' + + 'got %o. The dispatch is skipped — see useSchemaState ' + + 'drainDeferredQueue for the protocol.', + resFunc, + ); + return; + } + dispatch({ + type: SCHEMA_STATE_ACTIONS.DEFERRED_DEPCHANGE, + path: item.action.path, + depChange: item.action.depChange, + listener: { + ...item.listener, + callback: resFunc, + }, + }); + }, + (err) => { + const msg = err?.message || String(err) || 'unknown error'; + const userMsg = gettext('Dependent update failed: ') + msg; + const notifier = pgAdmin?.Browser?.notifier; + if (typeof notifier?.error === 'function') { + notifier.error(userMsg); + } else { + // Notifier unavailable (very early init, isolated test + // harness, etc.). Surface to console.error rather than + // silently dropping the rejection — the latter is the bug + // this drainer exists to prevent. + console.error('deferredDepChange:', userMsg, err); + } + }, + ); + }); +}; + export const useSchemaState = ({ schema, getInitData, immutableData, onDataChange, viewHelperProps, @@ -117,20 +185,15 @@ export const useSchemaState = ({ type: SCHEMA_STATE_ACTIONS.CLEAR_DEFERRED_QUEUE, }); - items.forEach((item) => { - item.promise.then((resFunc) => { - sessDispatch({ - type: SCHEMA_STATE_ACTIONS.DEFERRED_DEPCHANGE, - path: item.action.path, - depChange: item.action.depChange, - listener: { - ...item.listener, - callback: resFunc, - }, - }); - }); - }); - }, [sessData.__deferred__?.length]); + drainDeferredQueue(items, sessDispatch); + // Depend on the array reference rather than its length. With React + // automatic batching the queue's length can round-trip through 0 in + // the same commit (CLEAR followed by a fresh APPEND), and a + // length-based dep would compare equal across renders and miss the + // second drain. The reducer creates a new __deferred__ array on + // every dispatch, so ref-equality changes whenever the queue does; + // the early `length == 0` return keeps the no-op case free. + }, [sessData.__deferred__]); state.reload = reload; state.reset = resetData; diff --git a/web/pgadmin/static/js/SchemaView/utils/listenDepChanges.js b/web/pgadmin/static/js/SchemaView/utils/listenDepChanges.js index ed59c1e9dfe..4951ab481fc 100644 --- a/web/pgadmin/static/js/SchemaView/utils/listenDepChanges.js +++ b/web/pgadmin/static/js/SchemaView/utils/listenDepChanges.js @@ -13,6 +13,52 @@ import _ from 'lodash'; import { evalFunc } from 'sources/utils'; +/** + * Wires a field's `depChange` and `deferredDepChange` callbacks into the + * SchemaState dependency tracker so they fire when the field's own value + * or any of its declared `deps` change. + * + * ## `depChange(state, source, topState, actionObj) => deltaObj | undefined` + * + * Synchronous. Return a partial-state object to merge into the field's + * local data; `undefined` means "no change". Runs inline during the + * reducer dispatch, so it must be cheap and side-effect free. + * + * ## `deferredDepChange(state, source, topState, actionObj) => Promise | undefined` + * + * Asynchronous follow-up. Use this for work that needs a confirmation + * dialog, a network round-trip, or any other Promise-bound result. + * + * The contract: + * + * 1. Return **`undefined`** to opt out — nothing is queued, no + * Promise is constructed. Use this when the trigger doesn't apply + * (wrong `source`, no actual change, nothing to do). + * 2. Otherwise return a Promise that **always settles**: + * - On success, resolve with a callback `(tmpstate) => deltaObj`. + * The callback is invoked at drain time against the latest + * state and must return a delta object only — it must NOT + * mutate `tmpstate` or any captured input state. + * - On failure, prefer resolving with a recovery callback that + * resets any "in-progress" flag and surface the error via + * `pgAdmin.Browser.notifier.error(...)` from inside the + * Promise body. Rejecting is permitted as a safety net — the + * drainer routes rejections to `notifier.error` so the user + * sees a generic message — but per-schema recovery gives a + * better UX than a generic toast. + * 3. Side effects (notifier dialogs, schema-level mutations like + * `setOperClassOptions`) belong **inside the Promise body before + * resolving** — not inside the returned callback. Exception: when + * the side effect's input legitimately depends on `tmpstate` + * (drain-time state, e.g. merging fetched columns into whatever + * the user has typed since the deferred work was queued), the + * side effect may live in the callback. Treat the exception as + * a smell: prefer to compute the result before resolve when you + * can. + * + * A Promise that never resolves leaks into `data.__deferred__` forever + * and is the bug pattern this protocol exists to prevent. + */ export const listenDepChanges = ( accessPath, field, schemaState, setRefreshKey ) => { diff --git a/web/regression/javascript/SchemaView/deferred_drain.spec.js b/web/regression/javascript/SchemaView/deferred_drain.spec.js new file mode 100644 index 00000000000..10b0997e0e4 --- /dev/null +++ b/web/regression/javascript/SchemaView/deferred_drain.spec.js @@ -0,0 +1,121 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Tests that the deferredDepChange queue drains follow the protocol: +// - Rejected promises don't silently disappear; they're surfaced +// to the user via pgAdmin.Browser.notifier.error. +// - When the resolved value isn't a function, we warn (console) and +// skip the dispatch instead of submitting a broken listener. + +import { drainDeferredQueue } from + '../../../pgadmin/static/js/SchemaView/hooks/useSchemaState'; +import pgAdmin from '../fake_pgadmin'; + +describe('drainDeferredQueue — protocol guards', () => { + let warnSpy, notifierSpy; + beforeEach(() => { + warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + notifierSpy = jest.spyOn(pgAdmin.Browser.notifier, 'error') + .mockImplementation(() => {}); + }); + afterEach(() => { + warnSpy.mockRestore(); + notifierSpy.mockRestore(); + }); + + test('dispatches DEFERRED_DEPCHANGE when promise resolves to a function', async () => { + const dispatch = jest.fn(); + const cb = () => ({}); + const item = { + action: { path: ['x'], depChange: () => {} }, + listener: { source: ['x'], dest: ['x'] }, + promise: Promise.resolve(cb), + }; + drainDeferredQueue([item], dispatch); + await item.promise; + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledTimes(1); + const call = dispatch.mock.calls[0][0]; + expect(call.type).toBe('deferred_depchange'); + expect(call.listener.callback).toBe(cb); + }); + + test('SKIPS dispatch and console.errors when promise resolves to a NON-function', async () => { + // Protocol violation: schema author resolved with a data object + // instead of a callback. We surface this loudly via console.error + // (not warn) so it trips test suites and is more likely to be + // caught in dev/QA. Notifier toast would be wrong here — this is + // a code bug, not a user-actionable failure. + console.error.mockImplementation(() => {}); + const dispatch = jest.fn(); + const item = { + action: { path: ['x'], depChange: () => {} }, + listener: { source: ['x'], dest: ['x'] }, + promise: Promise.resolve({ x: 1 }), + }; + drainDeferredQueue([item], dispatch); + await item.promise; + await Promise.resolve(); + expect(dispatch).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalled(); + expect(console.error.mock.calls[0][0]) + .toMatch(/must resolve to a callback function/); + // Clear so the global afterEach doesn't trip on our intentional + // error invocation. + console.error.mockClear(); + }); + + test('surfaces rejected promises to the user via notifier.error', async () => { + const dispatch = jest.fn(); + const err = new Error('boom'); + const item = { + action: { path: ['x'], depChange: () => {} }, + listener: { source: ['x'], dest: ['x'] }, + promise: Promise.reject(err), + }; + drainDeferredQueue([item], dispatch); + // Wait for the rejection's .catch chain to fire. + await new Promise((res) => setTimeout(res, 0)); + expect(dispatch).not.toHaveBeenCalled(); + expect(notifierSpy).toHaveBeenCalledTimes(1); + // The error message should include the original error's text. + expect(notifierSpy.mock.calls[0][0]).toMatch(/boom/); + }); + + test('falls back to console.error when notifier is unavailable', async () => { + // Edge case: pgAdmin.Browser.notifier could be missing during + // very early init or in test harnesses that haven't installed it. + // The rejection still needs to be surfaced — silent no-op would + // recreate the bug this commit fixed. + // setup-jest.js already spies console.error; mock it for this test + // and clear before the global afterEach asserts non-call. + console.error.mockImplementation(() => {}); + const savedNotifier = pgAdmin.Browser.notifier; + pgAdmin.Browser.notifier = undefined; + + try { + const dispatch = jest.fn(); + const item = { + action: { path: ['x'], depChange: () => {} }, + listener: { source: ['x'], dest: ['x'] }, + promise: Promise.reject(new Error('boom-no-notifier')), + }; + drainDeferredQueue([item], dispatch); + await new Promise((res) => setTimeout(res, 0)); + expect(console.error).toHaveBeenCalled(); + expect(console.error.mock.calls[0].join(' ')) + .toMatch(/boom-no-notifier/); + } finally { + pgAdmin.Browser.notifier = savedNotifier; + // Reset so the global afterEach doesn't trip on our intentional + // error invocation. + console.error.mockClear(); + } + }); +}); diff --git a/web/regression/javascript/SchemaView/dep_listener.spec.js b/web/regression/javascript/SchemaView/dep_listener.spec.js new file mode 100644 index 00000000000..b97aa6daa19 --- /dev/null +++ b/web/regression/javascript/SchemaView/dep_listener.spec.js @@ -0,0 +1,265 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Tests for DepListener — both synchronous getDepChange and the async +// getDeferredDepChange path. Includes the prefix-match safety rule +// (matching "shared" should NOT also match "shared_username"). + +import { DepListener } from + '../../../pgadmin/static/js/SchemaView/DepListener'; + +describe('DepListener — prefix-match protection', () => { + test('getDepChange does NOT match `shared` listener when currPath is `shared_username`', () => { + const d = new DepListener(); + const cb = jest.fn(() => ({})); + d.addDepListener(['shared'], ['shared'], cb); + + d.getDepChange(['shared_username'], { shared: 'x' }, {}); + + expect(cb).not.toHaveBeenCalled(); + }); + + test('getDeferredDepChange does NOT match `shared` listener when currPath is `shared_username`', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['shared'], ['shared'], null, defCb); + + const result = d.getDeferredDepChange(['shared_username'], { shared: 'x' }, {}); + + expect(defCb).not.toHaveBeenCalled(); + expect(result).toEqual([]); + }); + + test('getDeferredDepChange DOES match when source is a true prefix (`shared` matches `shared.sub`)', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['shared'], ['shared'], null, defCb); + + const result = d.getDeferredDepChange(['shared', 'sub'], { shared: { sub: 1 } }, {}); + + expect(defCb).toHaveBeenCalledTimes(1); + expect(result).toHaveLength(1); + }); + + test('getDeferredDepChange matches exact same path', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['shared'], ['shared'], null, defCb); + + const result = d.getDeferredDepChange(['shared'], { shared: 'x' }, {}); + + expect(defCb).toHaveBeenCalledTimes(1); + expect(result).toHaveLength(1); + }); +}); + +// Behaviors the planned optimization needs to preserve. These run GREEN +// against the current implementation (characterization) and must remain +// GREEN after the refactor. +describe('DepListener — callback-vs-defCallback dispatch', () => { + test('getDepChange invokes only the sync callback, never defCallback', () => { + const d = new DepListener(); + const cb = jest.fn(() => ({})); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['x'], cb, defCb); + + d.getDepChange(['a'], { a: 1 }, {}); + + expect(cb).toHaveBeenCalledTimes(1); + expect(defCb).not.toHaveBeenCalled(); + }); + + test('getDeferredDepChange invokes only defCallback, never sync callback', () => { + const d = new DepListener(); + const cb = jest.fn(() => ({})); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['x'], cb, defCb); + + const result = d.getDeferredDepChange(['a'], { a: 1 }, {}); + + expect(defCb).toHaveBeenCalledTimes(1); + expect(cb).not.toHaveBeenCalled(); + expect(result).toHaveLength(1); + }); + + test('getDeferredDepChange returns [] when no listener has a defCallback (sync-only registrations)', () => { + const d = new DepListener(); + const cb1 = jest.fn(() => ({})); + const cb2 = jest.fn(() => ({})); + d.addDepListener(['a'], ['x'], cb1); + d.addDepListener(['b'], ['y'], cb2); + + const result = d.getDeferredDepChange(['a'], { a: 1 }, {}); + + expect(result).toEqual([]); + expect(cb1).not.toHaveBeenCalled(); + expect(cb2).not.toHaveBeenCalled(); + }); + + test('getDeferredDepChange returns [] when there are no listeners at all', () => { + const d = new DepListener(); + const result = d.getDeferredDepChange(['anything'], {}, {}); + expect(result).toEqual([]); + }); + + test('skipping a non-matching listener does not block a later matching one', () => { + const d = new DepListener(); + const defCbA = jest.fn(() => Promise.resolve(() => ({}))); + const defCbB = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['x'], null, defCbA); + d.addDepListener(['b'], ['y'], null, defCbB); + + const result = d.getDeferredDepChange(['b'], { a: 1, b: 2 }, {}); + + expect(defCbA).not.toHaveBeenCalled(); + expect(defCbB).toHaveBeenCalledTimes(1); + expect(result).toHaveLength(1); + }); + + test('multiple matching listeners all fire, in registration order', () => { + const d = new DepListener(); + const order = []; + const defCb1 = jest.fn(() => { order.push(1); return Promise.resolve(() => ({})); }); + const defCb2 = jest.fn(() => { order.push(2); return Promise.resolve(() => ({})); }); + d.addDepListener(['shared'], ['x'], null, defCb1); + d.addDepListener(['shared'], ['y'], null, defCb2); + + const result = d.getDeferredDepChange(['shared'], { shared: 1 }, {}); + + expect(result).toHaveLength(2); + expect(order).toEqual([1, 2]); + }); + + test('defCallback that returns falsy is skipped (no entry pushed to deferredList)', () => { + const d = new DepListener(); + const defCb = jest.fn(() => undefined); + d.addDepListener(['a'], ['x'], null, defCb); + + const result = d.getDeferredDepChange(['a'], { a: 1 }, {}); + + expect(defCb).toHaveBeenCalledTimes(1); + expect(result).toEqual([]); + }); +}); + +describe('DepListener — early-bail when no defCallbacks registered', () => { + // Builds a `source` array that traps access only AFTER registration + // completes. Lets the implementation legitimately pre-compute join + // keys at add time, while flagging any subsequent walk that touches + // the array during getDeferredDepChange. + const makeTrippedSource = (segments) => { + const target = [...segments]; + let armed = false; + const proxy = new Proxy(target, { + get(t, prop) { + if (armed && (prop === 'concat' || prop === 'join' + || prop === 'length' || /^\d+$/.test(prop))) { + throw new Error( + 'listener.source was iterated during getDeferredDepChange ' + + 'despite having no defCallback' + ); + } + return t[prop]; + }, + }); + return { proxy, arm: () => { armed = true; } }; + }; + + test('getDeferredDepChange does not iterate listener.source when no defCallbacks are registered', () => { + const d = new DepListener(); + const t = makeTrippedSource(['a']); + d.addDepListener(t.proxy, ['x'], jest.fn(() => ({}))); + t.arm(); + + const result = d.getDeferredDepChange(['anything'], {}, {}); + expect(result).toEqual([]); + }); + + test('after removing the last defCallback listener, the early-bail engages', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['onlydef'], null, defCb); + d.removeDepListener(['onlydef']); + + const t = makeTrippedSource(['b']); + d.addDepListener(t.proxy, ['syncOnly'], jest.fn(() => ({}))); + t.arm(); + + const result = d.getDeferredDepChange(['anything'], {}, {}); + expect(result).toEqual([]); + }); +}); + +describe('DepListener — source-array mutation isolation', () => { + // Pins the defensive-copy contract: a caller that re-uses (and + // mutates) its source array after `addDepListener` should not be + // able to corrupt the listener's source path (which is passed to + // the callback). The cached _sourceKey is already a string snapshot + // and resistant to mutation; this test guards listener.source too. + test('mutating the caller-side source array after registration does not affect listener.source seen by the callback', () => { + const d = new DepListener(); + const source = ['a', 'b']; + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(source, ['dest'], null, defCb); + + // Mutate the original array AFTER registration. + source.push('mutated'); + source[0] = 'tampered'; + + d.getDeferredDepChange(['a', 'b'], {}, {}); + expect(defCb).toHaveBeenCalledTimes(1); + // The second arg to defCb is listener.source. The defensive copy + // means it still shows the originally-registered path, not the + // mutated one. + const [, sourceArg] = defCb.mock.calls[0]; + expect(sourceArg).toEqual(['a', 'b']); + }); +}); + +describe('DepListener — removeDepListener', () => { + test('removeDepListener drops only listeners whose dest is prefixed by the given path', () => { + const d = new DepListener(); + const defCbKeep = jest.fn(() => Promise.resolve(() => ({}))); + const defCbDrop = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['keep'], null, defCbKeep); + d.addDepListener(['a'], ['drop', 'sub'], null, defCbDrop); + + d.removeDepListener(['drop']); + + const result = d.getDeferredDepChange(['a'], { a: 1 }, {}); + expect(defCbKeep).toHaveBeenCalledTimes(1); + expect(defCbDrop).not.toHaveBeenCalled(); + expect(result).toHaveLength(1); + }); + + test('removeDepListener leaves the deferred path functional when remaining listeners still have defCallbacks', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['drop'], null, defCb); + d.addDepListener(['b'], ['keep'], null, defCb); + + d.removeDepListener(['drop']); + + const result = d.getDeferredDepChange(['b'], { b: 1 }, {}); + expect(result).toHaveLength(1); + }); + + test('after removing the last defCallback-bearing listener, getDeferredDepChange returns []', () => { + const d = new DepListener(); + const defCb = jest.fn(() => Promise.resolve(() => ({}))); + d.addDepListener(['a'], ['onlydef'], null, defCb); + d.addDepListener(['b'], ['syncOnly'], jest.fn(() => ({}))); + + d.removeDepListener(['onlydef']); + + const result = d.getDeferredDepChange(['a'], { a: 1 }, {}); + expect(result).toEqual([]); + expect(defCb).not.toHaveBeenCalled(); + }); +}); diff --git a/web/regression/javascript/SchemaView/drain_useeffect_race.spec.jsx b/web/regression/javascript/SchemaView/drain_useeffect_race.spec.jsx new file mode 100644 index 00000000000..f715ee922ed --- /dev/null +++ b/web/regression/javascript/SchemaView/drain_useeffect_race.spec.jsx @@ -0,0 +1,104 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Deterministic test for the drain useEffect's dep-array race: +// when two SET_VALUE dispatches land in the same React batch, the +// reducer's `data.__deferred__` array may end the batch at the same +// length it started, even though new items were appended after the +// drainer's CLEAR ran. A length-based dep array compares equal +// across renders and the second drain never fires. +// +// The fix uses the array REFERENCE as the dep. The reducer creates a +// new __deferred__ array on every dispatch (via _.cloneDeep at entry + +// `.concat(...)` in the APPEND), so ref-equality changes whenever the +// queue does and the effect re-runs. + +import React, { useReducer, useEffect } from 'react'; +import { render, act } from '@testing-library/react'; +import { + SCHEMA_STATE_ACTIONS, + sessDataReducer, +} from '../../../pgadmin/static/js/SchemaView/SchemaState'; + +// Mirror of the drain useEffect from useSchemaState.js with an +// injectable spy so the test can count invocations per render commit. +// The spy receives `dispatch` so the test can simulate a synchronous +// follow-up SET_VALUE (the racey case where the effect's CLEAR and a +// new SET_VALUE batch into the same commit, round-tripping length). +const useDrain = (sessData, drainSpy, dispatch) => { + useEffect(() => { + const items = sessData.__deferred__ || []; + if (items.length === 0) return; + // Mirror production order: dispatch CLEAR first, THEN run the + // drainer (which may synchronously dispatch a follow-up SET_VALUE + // — that's how the racey case lands CLEAR+SET_VALUE in one + // batch with the length round-tripping through 0). + dispatch({ type: SCHEMA_STATE_ACTIONS.CLEAR_DEFERRED_QUEUE }); + drainSpy(items, dispatch); + }, [sessData.__deferred__]); +}; + +const Harness = React.forwardRef(({ drainSpy }, ref) => { + const [sessData, dispatch] = useReducer(sessDataReducer, { + name: '', other: '', __changeId: 0, + }); + useDrain(sessData, drainSpy, dispatch); + React.useImperativeHandle(ref, () => ({ dispatch, sessData }), [sessData]); + return null; +}); +Harness.displayName = 'Harness'; + +const makeAction = (path, value, tag) => ({ + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path, value, + // The reducer reads `action.deferredDepChange` to populate the queue; + // we stub it to return a single tagged item per dispatch. + deferredDepChange: () => [{ + action: { path, depChange: () => {} }, + listener: { source: path, dest: path }, + promise: Promise.resolve(() => ({})), + tag, + }], +}); + +describe('drain useEffect — batched-dispatch race', () => { + test('drain-time CLEAR + synchronous SET_VALUE batched together: both items reach the drain spy', async () => { + // Engineers the exact race the ref-based dep array fixes: + // render N: __deferred__ = [first] (length 1) + // effect fires, drainSpy called with [first] + // drainSpy synchronously dispatches a follow-up SET_VALUE + // => SET_VALUE appends 'second' + // effect then dispatches CLEAR + // render N+1 commits both: CLEAR (→ []) + SET_VALUE (→ [second]) + // final __deferred__ = [second] (length 1) + // PREVIOUS length was 1, CURRENT length is 1 — length-dep sees + // no change, effect skips, 'second' never drains. + const drainSpy = jest.fn((items, dispatch) => { + // Only inject the follow-up on the first call so the test + // terminates (otherwise we'd recurse). + if (items.some((i) => i.tag === 'first')) { + dispatch(makeAction(['other'], 'b', 'second')); + } + }); + const ref = React.createRef(); + render(); + + await act(async () => { + ref.current.dispatch(makeAction(['name'], 'a', 'first')); + }); + // Let any cascaded effects flush. + await act(async () => { await Promise.resolve(); }); + + const seenTags = drainSpy.mock.calls + .flatMap((call) => call[0]) + .map((item) => item.tag); + expect(seenTags).toContain('first'); + expect(seenTags).toContain('second'); + }); +}); diff --git a/web/regression/javascript/SchemaView/reducer_deferred.spec.js b/web/regression/javascript/SchemaView/reducer_deferred.spec.js new file mode 100644 index 00000000000..ef7757ca013 --- /dev/null +++ b/web/regression/javascript/SchemaView/reducer_deferred.spec.js @@ -0,0 +1,94 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Tests for the sessDataReducer's interaction with the deferred-dep +// queue. The reducer must APPEND new deferred items to data.__deferred__ +// rather than replacing the queue — otherwise two SET_VALUE actions +// that fire in the same React batch can lose the first action's pending +// promise(s) before the drain useEffect runs. + +import { + SCHEMA_STATE_ACTIONS, + sessDataReducer, +} from '../../../pgadmin/static/js/SchemaView/SchemaState'; + +describe('sessDataReducer — deferred queue accumulation', () => { + const initial = { name: '', other: '', __changeId: 0 }; + + // A trivial deferredDepChange that returns a unique-tagged promise so + // we can identify which actions produced which items. + const makeDefDepChange = (tag) => + (_currPath, _newState, _action) => [{ tag, promise: Promise.resolve(() => ({})) }]; + // Note: the reducer's getDeferredDepChange (top of reducer.js) calls + // action.deferredDepChange(currPath, newState, {type, path, value, + // depChange, oldState}). The return value is what becomes + // __deferred__. The reducer itself doesn't care about the inner shape + // — only that it's an array — so we can stuff in tagged sentinels. + + test('SET_VALUE installs the deferred list in __deferred__', () => { + const action = { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['name'], value: 'a', + deferredDepChange: makeDefDepChange('first'), + }; + const next = sessDataReducer(initial, action); + expect(next.__deferred__).toHaveLength(1); + expect(next.__deferred__[0].tag).toBe('first'); + }); + + test('a second SET_VALUE APPENDS to __deferred__ instead of replacing', () => { + // Simulate two synchronous SET_VALUEs in the same React batch: the + // first leaves a deferred item; the second must preserve it. + const after1 = sessDataReducer(initial, { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['name'], value: 'a', + deferredDepChange: makeDefDepChange('first'), + }); + expect(after1.__deferred__).toHaveLength(1); + + const after2 = sessDataReducer(after1, { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['other'], value: 'b', + deferredDepChange: makeDefDepChange('second'), + }); + expect(after2.__deferred__).toHaveLength(2); + expect(after2.__deferred__.map((i) => i.tag)).toEqual(['first', 'second']); + }); + + test('SET_VALUE with no deferredDepChange leaves the existing queue alone', () => { + const after1 = sessDataReducer(initial, { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['name'], value: 'a', + deferredDepChange: makeDefDepChange('first'), + }); + expect(after1.__deferred__).toHaveLength(1); + + // No deferredDepChange — should not clobber the queue. + const after2 = sessDataReducer(after1, { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['other'], value: 'b', + }); + expect(after2.__deferred__).toHaveLength(1); + expect(after2.__deferred__[0].tag).toBe('first'); + }); + + test('CLEAR_DEFERRED_QUEUE empties __deferred__', () => { + const after1 = sessDataReducer(initial, { + type: SCHEMA_STATE_ACTIONS.SET_VALUE, + path: ['name'], value: 'a', + deferredDepChange: makeDefDepChange('first'), + }); + expect(after1.__deferred__).toHaveLength(1); + + const cleared = sessDataReducer(after1, { + type: SCHEMA_STATE_ACTIONS.CLEAR_DEFERRED_QUEUE, + }); + expect(cleared.__deferred__).toHaveLength(0); + }); +}); From 1340cbc18df5c45662b41550fe1a65913d44ff13 Mon Sep 17 00:00:00 2001 From: Ashesh Vashi Date: Fri, 5 Jun 2026 18:53:15 +0530 Subject: [PATCH 3/3] fix(schemas): align deferredDepChange callbacks with new protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five schemas adopt the queue-based deferredDepChange contract, fixing real bugs along the way: - exclusion_constraint.ui.js (amname): opt out when amname is unchanged or columns is empty — pre-fix code queued a 'clear columns' confirm dialog unconditionally, warning about clearing an empty collection on the very first selection. - index.ui.js (amname): explicit `return undefined` opt-out in place of the no-op Promise.resolve(()=>{}) round-trip. - table.ui.js (typname + coll_inherits): typname opt-out when unchanged with stale-ofTypeTables guard (`typeTable?.oftype_columns ?? []`); changeColumnOptions hoisted out of the resolved callback per protocol JSDoc; coll_inherits handles same-length swap (previous add/remove branches missed it); stale-OID guard in REMOVE branch prevents the `col.inheritedid != undefined` filter from dropping local user-added columns. - foreign_table.ui.js (inherits): same swap + stale-OID guard. - azure_schema.ui.js (is_authenticating): orphan-Promise fix — the previous version returned a Promise that never resolved in the non-match branch, leaving a permanent __deferred__ entry. Source path compared against `source[source.length - 1]` (arrays, not strings). Auth failure surfaces to the user via notifier and resets is_authenticating + clears stale auth_code on failure. New `*.deferred.spec.js` for each schema, plus contract updates to the existing exclusion_constraint.ui.spec.js and table.ui.spec.js. --- .../static/js/foreign_table.ui.js | 87 ++++---- .../static/js/exclusion_constraint.ui.js | 18 +- .../tables/indexes/static/js/index.ui.js | 46 ++--- .../schemas/tables/static/js/table.ui.js | 178 ++++++++--------- .../misc/cloud/static/js/azure_schema.ui.js | 52 +++-- .../azure_schema.deferred.spec.js | 130 ++++++++++++ .../exclusion_constraint.deferred.spec.js | 149 ++++++++++++++ .../exclusion_constraint.ui.spec.js | 14 +- .../foreign_table.deferred.spec.js | 187 ++++++++++++++++++ .../schema_ui_files/index.ui.deferred.spec.js | 110 +++++++++++ .../schema_ui_files/table.ui.deferred.spec.js | 175 ++++++++++++++++ .../schema_ui_files/table.ui.spec.js | 11 +- 12 files changed, 947 insertions(+), 210 deletions(-) create mode 100644 web/regression/javascript/schema_ui_files/azure_schema.deferred.spec.js create mode 100644 web/regression/javascript/schema_ui_files/exclusion_constraint.deferred.spec.js create mode 100644 web/regression/javascript/schema_ui_files/foreign_table.deferred.spec.js create mode 100644 web/regression/javascript/schema_ui_files/index.ui.deferred.spec.js create mode 100644 web/regression/javascript/schema_ui_files/table.ui.deferred.spec.js diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui.js index 33b28df1ee5..cf36e14ae74 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui.js @@ -129,60 +129,47 @@ export default class ForeignTableSchema extends BaseUISchema { options: obj.fieldOptions.tables, optionsLoaded: (res)=>obj.inheritedTableList=res, deferredDepChange: (state, source, topState, actionObj)=>{ - return new Promise((resolve)=>{ - // current table list and previous table list - let newColInherits = state.inherits || []; - let oldColInherits = actionObj.oldState.inherits || []; - - let tabName; - let tabColsResponse; - - // Add columns logic - // If new table is added in list - if(newColInherits.length > 1 && newColInherits.length > oldColInherits.length) { - // Find newly added table from current list - tabName = _.difference(newColInherits, oldColInherits); - tabColsResponse = obj.getColumns({attrelid: this.getTableOid(tabName[0])}); - } else if (newColInherits.length == 1) { - // First table added - tabColsResponse = obj.getColumns({attrelid: this.getTableOid(newColInherits[0])}); - } - - if(tabColsResponse) { - tabColsResponse.then((res)=>{ - resolve((tmpstate)=>{ - let finalCols = res.map((col)=>obj.columnsObj.getNewData(col)); - finalCols = [...tmpstate.columns, ...finalCols]; - return { - adding_inherit_cols: false, - columns: finalCols, - }; - }); - }); - } + const newColInherits = state.inherits || []; + const oldColInherits = actionObj.oldState.inherits || []; + const added = _.difference(newColInherits, oldColInherits); + const removed = _.difference(oldColInherits, newColInherits); + + // REMOVE takes precedence: any removed parent means stale + // columns to clean up. Covers pure shrink AND same-length + // swap (e.g. multi-select replace) — without this branch a + // swap would leave the removed parent's columns sitting in + // the grid until the user did something else. + if(removed.length > 0) { + const removeOid = this.getTableOid(removed[0]); + // Guard: if inheritedTableList is stale and the removed + // table can't be resolved to an OID, opt out. Filtering on + // `inheritedid != undefined` would silently drop local + // user-added columns (`null == undefined` in JS). + if(removeOid == null) return undefined; + return Promise.resolve((tmpstate)=>({ + adding_inherit_cols: false, + columns: (tmpstate.columns || []) + .filter((col)=>col.inheritedid != removeOid), + })); + } - // Remove columns logic - let removeOid; - if(newColInherits.length > 0 && newColInherits.length < oldColInherits.length) { - // Find deleted table from previous list - tabName = _.difference(oldColInherits, newColInherits); - removeOid = this.getTableOid(tabName[0]); - } else if (oldColInherits.length === 1 && newColInherits.length < 1) { - // We got last table from list - tabName = oldColInherits[0]; - removeOid = this.getTableOid(tabName); - } - if(removeOid) { - resolve((tmpstate)=>{ - let finalCols = tmpstate.columns; - _.remove(tmpstate.columns, (col)=>col.inheritedid==removeOid); + // Pure ADD: list grew without any removals. + if(added.length > 0) { + const fetchOid = this.getTableOid(added[0]); + if(fetchOid == null) return undefined; + return obj.getColumns({attrelid: fetchOid}).then((res)=>( + (tmpstate)=>{ + const fetched = res.map((col)=>obj.columnsObj.getNewData(col)); return { adding_inherit_cols: false, - columns: finalCols + columns: [...(tmpstate.columns || []), ...fetched], }; - }); - } - }); + } + )); + } + + // Lists are equivalent — no work to do. + return undefined; }, }, { diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui.js index 451caa339a9..dc6058b3cc2 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui.js @@ -284,6 +284,16 @@ export default class ExclusionConstraintSchema extends BaseUISchema { type: 'select', group: gettext('Definition'), options: this.fieldOptions.amname, deferredDepChange: (state, source, topState, actionObj)=>{ + // Opt out cleanly when nothing would change: amname unchanged + // or no columns to wipe. Previously this queued a confirmation + // dialog unconditionally. + // actionObj.oldState is guaranteed by the reducer to be the + // pre-dispatch clone; no need for optional chaining (and the + // cancel branch below accesses it unconditionally). + if(state.amname === actionObj.oldState.amname + || !state.columns?.length) { + return undefined; + } return new Promise((resolve)=>{ pgAdmin.Browser.notifier.confirm( gettext('Change access method?'), @@ -311,11 +321,9 @@ export default class ExclusionConstraintSchema extends BaseUISchema { })); }, function() { - resolve(()=>{ - return { - amname: actionObj.oldState.amname, - }; - }); + resolve(()=>({ + amname: actionObj.oldState.amname, + })); } ); }); diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js index 9753327152e..ff7ba9c0305 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js @@ -476,35 +476,25 @@ export default class IndexSchema extends BaseUISchema { }; }, deferredDepChange: (state, source, topState, actionObj) => { - const setColumns = (resolve)=>{ - resolve(()=>{ - state.columns.splice(0, state.columns?.length); - return { - columns: state.columns, - }; - }); - }; - if((state.amname != actionObj?.oldState.amname) && state.columns?.length > 0) { - return new Promise((resolve)=>{ - pgAdmin.Browser.notifier.confirm( - gettext('Warning'), - gettext('Changing access method will clear columns collection. Do you want to continue?'), - function () { - setColumns(resolve); - }, - function() { - resolve(()=>{ - state.amname = actionObj?.oldState.amname; - return { - amname: state.amname, - }; - }); - } - ); - }); - } else { - return Promise.resolve(()=>{/*This is intentional (SonarQube)*/}); + // No-op when amname didn't change or there's nothing to clear. + // Returning undefined opts out of the deferred queue. + // actionObj.oldState is guaranteed by the reducer to be the + // pre-dispatch clone; no need for optional chaining (and the + // cancel branch below accesses it unconditionally). + if (state.amname === actionObj.oldState.amname + || !state.columns?.length) { + return undefined; } + return new Promise((resolve)=>{ + pgAdmin.Browser.notifier.confirm( + gettext('Warning'), + gettext('Changing access method will clear columns collection. Do you want to continue?'), + // Confirmed — clear the columns collection. + () => resolve(() => ({ columns: [] })), + // Cancelled — revert amname to its previous value. + () => resolve(() => ({ amname: actionObj.oldState.amname })), + ); + }); }, }, { diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js index 8c678ba4b04..9484b870243 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js @@ -596,79 +596,57 @@ export default class TableSchema extends BaseUISchema { } }, deferredDepChange: (state, source, topState, actionObj)=>{ - return new Promise((resolve)=>{ - // current table list and previous table list - let newColInherits = state.coll_inherits || []; - let oldColInherits = actionObj.oldState.coll_inherits || []; + const newColInherits = state.coll_inherits || []; + const oldColInherits = actionObj.oldState.coll_inherits || []; + const added = _.difference(newColInherits, oldColInherits); + const removed = _.difference(oldColInherits, newColInherits); - let tabName; - let tabColsResponse; - - // Add columns logic - // If new table is added in list - if(newColInherits.length > 1 && newColInherits.length > oldColInherits.length) { - // Find newly added table from current list - tabName = _.difference(newColInherits, oldColInherits); - tabColsResponse = obj.getColumns({tid: this.getTableOid(tabName[0])}); - } else if (newColInherits.length == 1) { - // First table added - tabColsResponse = obj.getColumns({tid: this.getTableOid(newColInherits[0])}); - } - - if(tabColsResponse) { - tabColsResponse.then((res)=>{ - resolve((tmpstate)=>{ - let finalCols = res.map((col)=>obj.columnsSchema.getNewData(col)); - let currentSelectedCols = []; - if (!_.isEmpty(tmpstate.columns)){ - currentSelectedCols = tmpstate.columns; - } - let colNameList = []; - tmpstate.columns.forEach((col=>{ - colNameList.push(col.name); - })); - for (let col of Object.values(finalCols)) { - if(!colNameList.includes(col.name)){ - currentSelectedCols.push(col); - } - } - - if (!_.isEmpty(currentSelectedCols)){ - finalCols = currentSelectedCols; - } - - obj.changeColumnOptions(finalCols); - return { - adding_inherit_cols: false, - columns: finalCols, - }; - }); - }); - } + // REMOVE takes precedence: any removed parent means stale + // columns to clean up. Covers pure shrink AND same-length + // swap — without this branch a swap would leave the removed + // parent's columns sitting in the grid. + if(removed.length > 0) { + const removeOid = this.getTableOid(removed[0]); + // Guard: if inheritedTableList is stale and the removed + // table can't be resolved to an OID, opt out. Filtering on + // `inheritedid != undefined` would silently drop local + // user-added columns (`null == undefined` in JS). + if(removeOid == null) return undefined; + return Promise.resolve((tmpstate)=>{ + const finalCols = (tmpstate.columns || []) + .filter((col)=>col.inheritedid != removeOid); + obj.changeColumnOptions(finalCols); + return { + adding_inherit_cols: false, + columns: finalCols, + }; + }); + } - // Remove columns logic - let removeOid; - if(newColInherits.length > 0 && newColInherits.length < oldColInherits.length) { - // Find deleted table from previous list - tabName = _.difference(oldColInherits, newColInherits); - removeOid = this.getTableOid(tabName[0]); - } else if (oldColInherits.length === 1 && newColInherits.length < 1) { - // We got last table from list - tabName = oldColInherits[0]; - removeOid = this.getTableOid(tabName); - } - if(removeOid) { - resolve((tmpstate)=>{ - let finalCols = tmpstate.columns; - _.remove(tmpstate.columns, (col)=>col.inheritedid==removeOid); + // Pure ADD: list grew without any removals. + if(added.length > 0) { + const fetchOid = this.getTableOid(added[0]); + if(fetchOid == null) return undefined; + return obj.getColumns({tid: fetchOid}).then((res)=>( + (tmpstate)=>{ + const fetched = res.map((col)=>obj.columnsSchema.getNewData(col)); + const existing = tmpstate.columns || []; + const existingNames = new Set(existing.map((c)=>c.name)); + const finalCols = [ + ...existing, + ...fetched.filter((c)=>!existingNames.has(c.name)), + ]; obj.changeColumnOptions(finalCols); return { adding_inherit_cols: false, - columns: finalCols + columns: finalCols, }; - }); - } - }); + } + )); + } + + // Lists are equivalent — no work to do. + return undefined; }, }, { @@ -784,49 +762,47 @@ export default class TableSchema extends BaseUISchema { obj.ofTypeTables = res; }, deferredDepChange: (state, source, topState, actionObj)=>{ - const setColumns = (resolve)=>{ - let finalCols = []; - if(!isEmptyString(state.typname)) { - let typeTable = _.find(obj.ofTypeTables||[], (t)=>t.label==state.typname); - finalCols = typeTable.oftype_columns; - } - resolve(() => { - obj.changeColumnOptions(finalCols); - return { - columns: finalCols, - primary_key: [], - foreign_key: [], - exclude_constraint: [], - unique_constraint: [], - partition_keys: [], - partitions: [], - }; - }); - }; + // No change — opt out of the deferred queue. + if(state.typname == actionObj.oldState.typname) return undefined; + + // finalCols depends only on closure-captured state and + // obj.ofTypeTables — not on tmpstate. Compute it once here so + // the schema-level side effect (changeColumnOptions) can run + // BEFORE resolve, matching the protocol's "side effects in the + // Promise body, callbacks return pure deltas" rule. + let finalCols = []; + if(!isEmptyString(state.typname)) { + // ofTypeTables can be empty or stale (loaded options not + // yet refreshed). Guard against an undefined lookup so the + // callback returns an empty column list instead of throwing + // into the deferred-queue drain. + const typeTable = _.find(obj.ofTypeTables||[], (t)=>t.label==state.typname); + finalCols = typeTable?.oftype_columns ?? []; + } + const deltaCb = ()=>({ + columns: finalCols, + primary_key: [], + foreign_key: [], + exclude_constraint: [], + unique_constraint: [], + partition_keys: [], + partitions: [], + }); if(!isEmptyString(state.typname) && isEmptyString(actionObj.oldState.typname)) { return new Promise((resolve)=>{ pgAdmin.Browser.notifier.confirm( gettext('Remove column definitions?'), gettext('Changing \'Of type\' will remove column definitions.'), - function () { - setColumns(resolve); + ()=>{ + obj.changeColumnOptions(finalCols); + resolve(deltaCb); }, - function() { - resolve(()=>{ - return { - typname: null, - }; - }); - } + ()=>resolve(()=>({typname: null})), ); }); - } else if(state.typname != actionObj.oldState.typname) { - return new Promise((resolve)=>{ - setColumns(resolve); - }); - } else { - return Promise.resolve(()=>{/*This is intentional (SonarQube)*/}); } + obj.changeColumnOptions(finalCols); + return Promise.resolve(deltaCb); }, }, { diff --git a/web/pgadmin/misc/cloud/static/js/azure_schema.ui.js b/web/pgadmin/misc/cloud/static/js/azure_schema.ui.js index 89f79733121..c500d5dade7 100644 --- a/web/pgadmin/misc/cloud/static/js/azure_schema.ui.js +++ b/web/pgadmin/misc/cloud/static/js/azure_schema.ui.js @@ -142,21 +142,43 @@ class AzureCredSchema extends BaseUISchema { type: '', deps:['auth_btn'], deferredDepChange: (state, source)=>{ - return new Promise((resolve, reject)=>{ - if(source == 'auth_btn' && state.auth_type == 'interactive_browser_credential' && state.is_authenticating ) { - obj.fieldOptions.getAuthCode() - .then((res)=>{ - resolve(()=>{ - return { - is_authenticating: false, - auth_code: res.data.data.user_code, - }; - }); - }) - .catch((err)=>{ - reject(err instanceof Error ? err : Error(gettext('Something went wrong'))); - }); - } + // Opt out of the queue cleanly when the trigger doesn't match + // — returning undefined skips this listener entirely. The + // previous version returned a Promise that never resolved in + // this branch, leaving a permanent orphan in data.__deferred__. + // + // `source` is the path of the field that changed (an array). + // Compare against the last segment so the guard works for both + // a top-level field path (['auth_btn']) and a nested + // embedding (['some_parent', 'auth_btn']). + const trigger = Array.isArray(source) ? source[source.length - 1] : source; + if (trigger !== 'auth_btn' + || state.auth_type !== 'interactive_browser_credential' + || !state.is_authenticating) { + return undefined; + } + return new Promise((resolve)=>{ + obj.fieldOptions.getAuthCode() + .then((res)=>{ + resolve(()=>({ + is_authenticating: false, + auth_code: res.data.data.user_code, + })); + }) + .catch((err)=>{ + // Surface the failure to the user AND reset both + // is_authenticating (so the UI unblocks) and any stale + // auth_code from a prior successful attempt (otherwise + // the user sees "still authenticated" UI alongside the + // failure toast, which is misleading). + const msg = err?.response?.data?.errormsg + || err?.message + || gettext('Something went wrong'); + pgAdmin.Browser.notifier.error( + gettext('Azure authentication failed: ') + msg + ); + resolve(()=>({ is_authenticating: false, auth_code: null })); + }); }); }, }, diff --git a/web/regression/javascript/schema_ui_files/azure_schema.deferred.spec.js b/web/regression/javascript/schema_ui_files/azure_schema.deferred.spec.js new file mode 100644 index 00000000000..d6c97373381 --- /dev/null +++ b/web/regression/javascript/schema_ui_files/azure_schema.deferred.spec.js @@ -0,0 +1,130 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Characterization + contract tests for AzureCredSchema's +// is_authenticating deferredDepChange. + +import { AzureCredSchema } from + '../../../pgadmin/misc/cloud/static/js/azure_schema.ui'; +import pgAdmin from '../fake_pgadmin'; + +const getIsAuthenticatingField = (cred) => { + const field = cred.baseFields.find((f) => f.id === 'is_authenticating'); + expect(field).toBeDefined(); + expect(typeof field.deferredDepChange).toBe('function'); + return field.deferredDepChange.bind(cred); +}; + +describe('AzureCredSchema is_authenticating deferredDepChange', () => { + let cred, defChange, authCodeMock; + + beforeEach(() => { + authCodeMock = jest.fn(); + cred = new AzureCredSchema(null, { getAuthCode: () => authCodeMock() }); + defChange = getIsAuthenticatingField(cred); + }); + + // Source is passed by the schema framework as an *array* (the path + // to the field that changed) — e.g. ['auth_btn'] for a top-level + // field, or ['parent', 'auth_btn'] when the schema is embedded in a + // nested context. The earlier tests passed a bare string which + // happened to compare equal-by-coercion against 'auth_btn' for a + // single-element array but doesn't reflect production shape. + + // ---- Happy path (characterization — must hold before AND after refactor) - + + test('matching trigger + getAuthCode resolves → callback returns auth_code delta', async () => { + authCodeMock.mockResolvedValue({ + data: { data: { user_code: 'ABC-123' } }, + }); + const result = defChange( + { auth_type: 'interactive_browser_credential', is_authenticating: true }, + ['auth_btn'], + ); + expect(result).toBeInstanceOf(Promise); + const cb = await result; + expect(typeof cb).toBe('function'); + expect(cb()).toEqual({ is_authenticating: false, auth_code: 'ABC-123' }); + }); + + test('matching trigger when embedded in a nested path (source ends in auth_btn) still proceeds', async () => { + // Regression: the legacy guard `source != "auth_btn"` worked by + // single-element-array coercion. With a nested source like + // ['parent', 'auth_btn'], the array coerces to 'parent,auth_btn' + // and the guard fired wrongly, opting out of the auth flow. The + // fix compares against the LAST path segment. + authCodeMock.mockResolvedValue({ + data: { data: { user_code: 'XYZ-789' } }, + }); + const result = defChange( + { auth_type: 'interactive_browser_credential', is_authenticating: true }, + ['parent', 'auth_btn'], + ); + expect(result).toBeInstanceOf(Promise); + const cb = await result; + expect(cb()).toEqual({ is_authenticating: false, auth_code: 'XYZ-789' }); + }); + + test('matching trigger + getAuthCode rejects → recovers via notifier.error + resolves with reset callback', async () => { + // Updated contract: instead of rejecting (which the drain swallows + // into console.error with zero user feedback and leaves + // is_authenticating stuck true), the schema now surfaces the error + // via the notifier and resolves with a callback that resets + // is_authenticating so the UI unblocks. + const errorSpy = jest.spyOn(pgAdmin.Browser.notifier, 'error') + .mockImplementation(() => {}); + authCodeMock.mockRejectedValue(new Error('upstream failure')); + + const result = defChange( + { auth_type: 'interactive_browser_credential', is_authenticating: true }, + ['auth_btn'], + ); + expect(result).toBeInstanceOf(Promise); + + const cb = await result; + expect(typeof cb).toBe('function'); + // Also clears any stale auth_code from a prior successful attempt. + // Otherwise the user would see "still authenticated" UI alongside + // the failure toast, which is misleading. + expect(cb()).toEqual({ is_authenticating: false, auth_code: null }); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy.mock.calls[0][0]).toMatch(/upstream failure/); + + errorSpy.mockRestore(); + }); + + // ---- NEW contract: non-matching trigger returns undefined -------------- + + test('source other than auth_btn → returns undefined (was a hung Promise)', () => { + const result = defChange( + { auth_type: 'interactive_browser_credential', is_authenticating: true }, + ['some_other_field'], + ); + expect(result).toBeUndefined(); + expect(authCodeMock).not.toHaveBeenCalled(); + }); + + test('auth_type not interactive_browser_credential → returns undefined', () => { + const result = defChange( + { auth_type: 'service_principal_credential', is_authenticating: true }, + ['auth_btn'], + ); + expect(result).toBeUndefined(); + expect(authCodeMock).not.toHaveBeenCalled(); + }); + + test('is_authenticating false → returns undefined', () => { + const result = defChange( + { auth_type: 'interactive_browser_credential', is_authenticating: false }, + ['auth_btn'], + ); + expect(result).toBeUndefined(); + expect(authCodeMock).not.toHaveBeenCalled(); + }); +}); diff --git a/web/regression/javascript/schema_ui_files/exclusion_constraint.deferred.spec.js b/web/regression/javascript/schema_ui_files/exclusion_constraint.deferred.spec.js new file mode 100644 index 00000000000..076f2ce1f51 --- /dev/null +++ b/web/regression/javascript/schema_ui_files/exclusion_constraint.deferred.spec.js @@ -0,0 +1,149 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Characterization + contract tests for the amname deferredDepChange +// on ExclusionConstraintSchema. Reachable paths: +// 1) amname unchanged -> no-op (new) +// 2) amname changed, columns empty -> no-op (new) +// 3) amname changed -> btree, user confirms -> exColumnSchema +// gets btree operClass options + sort defaults, +// delta {columns: []} +// 4) amname changed -> other, user confirms -> exColumnSchema +// gets empty operClass options + no-sort defaults, +// delta {columns: []} +// 5) amname changed, user cancels -> delta +// {amname: oldValue}, exColumnSchema NOT mutated + +import ExclusionConstraintSchema from + '../../../pgadmin/browser/server_groups/servers/databases/schemas/tables/constraints/exclusion_constraint/static/js/exclusion_constraint.ui'; +import pgAdmin from '../fake_pgadmin'; + +const makeSchema = () => { + const operClassOptions = [{ label: 'oc1', value: 'oc1' }]; + const schema = new ExclusionConstraintSchema( + { + columns: () => Promise.resolve([]), + amname: () => Promise.resolve([]), + spcname: () => Promise.resolve([]), + getOperClass: () => operClassOptions, + getOperator: () => Promise.resolve([]), + }, + {}, + ); + return schema; +}; + +const getAmnameDeferred = (schema) => { + const field = schema.fields.find((f) => f.id === 'amname'); + expect(field).toBeDefined(); + expect(typeof field.deferredDepChange).toBe('function'); + return field.deferredDepChange.bind(schema); +}; + +describe('ExclusionConstraintSchema.amname deferredDepChange', () => { + let schema, deferredDepChange, confirmSpy; + let setOperClassSpy, changeDefaultsSpy; + + beforeEach(() => { + schema = makeSchema(); + deferredDepChange = getAmnameDeferred(schema); + confirmSpy = jest.spyOn(pgAdmin.Browser.notifier, 'confirm'); + confirmSpy.mockClear(); + setOperClassSpy = jest.spyOn(schema.exColumnSchema, 'setOperClassOptions') + .mockImplementation(() => {}); + changeDefaultsSpy = jest.spyOn(schema.exColumnSchema, 'changeDefaults') + .mockImplementation(() => {}); + }); + afterEach(() => { + confirmSpy.mockRestore(); + setOperClassSpy.mockRestore(); + changeDefaultsSpy.mockRestore(); + }); + + // ---- No-op paths (new contract) ---------------------------------------- + + test('no-op: amname unchanged → returns undefined', () => { + const state = { amname: 'btree', columns: [{ column: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(result).toBeUndefined(); + expect(confirmSpy).not.toHaveBeenCalled(); + }); + + test('no-op: amname changed but columns empty → returns undefined', () => { + const state = { amname: 'gist', columns: [] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(result).toBeUndefined(); + expect(confirmSpy).not.toHaveBeenCalled(); + }); + + // ---- Confirm paths (characterization) ---------------------------------- + + test('confirm with amname=btree → exColumnSchema gets btree operClass + sort defaults, delta {columns: []}', async () => { + const state = { amname: 'btree', columns: [{ column: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'gist' }, + }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + const [, , confirmCb] = confirmSpy.mock.calls[0]; + + confirmCb(); + const cb = await result; + expect(cb()).toEqual({ columns: [] }); + expect(setOperClassSpy).toHaveBeenCalled(); + expect(changeDefaultsSpy).toHaveBeenCalledWith({ + order: true, + nulls_order: true, + is_sort_nulls_applicable: true, + }); + expect(schema.exColumnSchema.amname).toBe('btree'); + }); + + test('confirm with amname=gist → exColumnSchema gets empty operClass + no-sort defaults, delta {columns: []}', async () => { + const state = { amname: 'gist', columns: [{ column: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + const [, , confirmCb] = confirmSpy.mock.calls[0]; + + confirmCb(); + const cb = await result; + expect(cb()).toEqual({ columns: [] }); + expect(setOperClassSpy).toHaveBeenCalledWith([]); + expect(changeDefaultsSpy).toHaveBeenCalledWith({ + order: false, + nulls_order: false, + is_sort_nulls_applicable: false, + }); + expect(schema.exColumnSchema.amname).toBe('gist'); + }); + + // ---- Cancel path (characterization) ------------------------------------ + + test('cancel → delta {amname: oldValue}, exColumnSchema NOT mutated', async () => { + const state = { amname: 'gist', columns: [{ column: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + const [, , , cancelCb] = confirmSpy.mock.calls[0]; + + cancelCb(); + const cb = await result; + expect(cb()).toEqual({ amname: 'btree' }); + expect(setOperClassSpy).not.toHaveBeenCalled(); + expect(changeDefaultsSpy).not.toHaveBeenCalled(); + // Input state's amname is NOT mutated. + expect(state.amname).toBe('gist'); + }); +}); diff --git a/web/regression/javascript/schema_ui_files/exclusion_constraint.ui.spec.js b/web/regression/javascript/schema_ui_files/exclusion_constraint.ui.spec.js index 94918371f18..a2617efa24a 100644 --- a/web/regression/javascript/schema_ui_files/exclusion_constraint.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/exclusion_constraint.ui.spec.js @@ -203,8 +203,10 @@ describe('ExclusionConstraintSchema', ()=>{ it('btree', (done)=>{ confirmSpy.mockClear(); - let state = {amname: 'btree'}; - let deferredPromise = deferredDepChange(state); + let state = {amname: 'btree', columns: [{column: 'c1'}]}; + let deferredPromise = deferredDepChange(state, null, null, { + oldState: { amname: 'gist' }, + }); deferredPromise.then((depChange)=>{ expect(schemaObj.exColumnSchema.setOperClassOptions).toHaveBeenCalledWith(operClassOptions); expect(depChange()).toEqual({ @@ -218,8 +220,10 @@ describe('ExclusionConstraintSchema', ()=>{ it('not btree', (done)=>{ confirmSpy.mockClear(); - let state = {amname: 'gist'}; - let deferredPromise = deferredDepChange(state); + let state = {amname: 'gist', columns: [{column: 'c1'}]}; + let deferredPromise = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); deferredPromise.then((depChange)=>{ expect(schemaObj.exColumnSchema.setOperClassOptions).toHaveBeenCalledWith([]); expect(depChange()).toEqual({ @@ -233,7 +237,7 @@ describe('ExclusionConstraintSchema', ()=>{ it('press no', (done)=>{ confirmSpy.mockClear(); - let state = {amname: 'gist'}; + let state = {amname: 'gist', columns: [{column: 'c1'}]}; let deferredPromise = deferredDepChange(state, null, null, { oldState: { amname: 'btree', diff --git a/web/regression/javascript/schema_ui_files/foreign_table.deferred.spec.js b/web/regression/javascript/schema_ui_files/foreign_table.deferred.spec.js new file mode 100644 index 00000000000..45737abd6d7 --- /dev/null +++ b/web/regression/javascript/schema_ui_files/foreign_table.deferred.spec.js @@ -0,0 +1,187 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Characterization + contract tests for the inherits deferredDepChange +// on ForeignTableSchema. Reachable paths: +// add : new list grew -> fetch columns, +// append fetched cols to tmpstate.columns +// rm : new list shrank -> drop columns +// whose inheritedid == removed table's oid +// noop : new == old (same shape or both empty) -> return undefined +// +// The new contract additionally requires: +// - no input-state mutation +// - opt-out path returns undefined (not a hanging Promise) + +import ForeignTableSchema from + '../../../pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/static/js/foreign_table.ui'; + +const makeSchema = (getColumnsMock) => { + const schema = new ForeignTableSchema( + () => null, () => null, + getColumnsMock, + { role: [], schema: [], foreignServers: [], tables: [] }, + ); + schema.inheritedTableList = [ + { label: 'a', value: 1 }, + { label: 'b', value: 2 }, + { label: 'c', value: 3 }, + ]; + // Make columnsObj.getNewData deterministic and observable. + jest.spyOn(schema.columnsObj, 'getNewData') + .mockImplementation((col) => ({ ...col, inheritedid: col.inheritedid })); + return schema; +}; + +const getDeferred = (schema) => { + const field = schema.fields.find((f) => f.id === 'inherits'); + expect(field).toBeDefined(); + expect(typeof field.deferredDepChange).toBe('function'); + return field.deferredDepChange.bind(schema); +}; + +describe('ForeignTableSchema.inherits deferredDepChange', () => { + let schema, deferredDepChange, getColumnsMock; + + beforeEach(() => { + getColumnsMock = jest.fn(); + schema = makeSchema(getColumnsMock); + deferredDepChange = getDeferred(schema); + }); + + // ---- ADD paths (characterization) -------------------------------------- + + test('first table added → fetches columns, callback appends to tmpstate.columns', async () => { + getColumnsMock.mockResolvedValue([{ name: 'col_x', inheritedid: 1 }]); + const state = { inherits: [1] }; + const result = deferredDepChange(state, null, null, { + oldState: { inherits: [] }, + }); + const cb = await result; + expect(getColumnsMock).toHaveBeenCalledWith({ attrelid: 1 }); + const delta = cb({ columns: [{ name: 'existing', inheritedid: null }] }); + expect(delta.adding_inherit_cols).toBe(false); + expect(delta.columns).toEqual([ + { name: 'existing', inheritedid: null }, + { name: 'col_x', inheritedid: 1 }, + ]); + }); + + test('additional table added → fetches columns for the newly added id', async () => { + getColumnsMock.mockResolvedValue([{ name: 'col_y', inheritedid: 2 }]); + const state = { inherits: [1, 2] }; + const result = deferredDepChange(state, null, null, { + oldState: { inherits: [1] }, + }); + const cb = await result; + expect(getColumnsMock).toHaveBeenCalledWith({ attrelid: 2 }); + const delta = cb({ columns: [{ name: 'col_x', inheritedid: 1 }] }); + expect(delta.columns).toEqual([ + { name: 'col_x', inheritedid: 1 }, + { name: 'col_y', inheritedid: 2 }, + ]); + }); + + // ---- REMOVE paths (characterization, plus no-mutation contract) -------- + + test('one of several removed → drops columns with that inheritedid, without mutating tmpstate.columns', async () => { + const state = { inherits: [1] }; + const result = deferredDepChange(state, null, null, { + oldState: { inherits: [1, 2] }, + }); + const cb = await result; + const tmpCols = [ + { name: 'col_x', inheritedid: 1 }, + { name: 'col_y', inheritedid: 2 }, + ]; + const delta = cb({ columns: tmpCols }); + expect(delta.adding_inherit_cols).toBe(false); + expect(delta.columns).toEqual([{ name: 'col_x', inheritedid: 1 }]); + // New contract: tmpstate's columns array is NOT mutated. + expect(tmpCols).toHaveLength(2); + expect(tmpCols[1]).toEqual({ name: 'col_y', inheritedid: 2 }); + }); + + test('last one removed → drops columns belonging to the last table', async () => { + const state = { inherits: [] }; + const result = deferredDepChange(state, null, null, { + oldState: { inherits: [3] }, + }); + const cb = await result; + const tmpCols = [ + { name: 'col_z', inheritedid: 3 }, + { name: 'local', inheritedid: null }, + ]; + const delta = cb({ columns: tmpCols }); + expect(delta.columns).toEqual([{ name: 'local', inheritedid: null }]); + expect(tmpCols).toHaveLength(2); + }); + + // ---- NEW CONTRACT: opt-out paths --------------------------------------- + + test('no-op: both lists empty → returns undefined (was a hanging Promise)', () => { + const result = deferredDepChange( + { inherits: [] }, null, null, { oldState: { inherits: [] } }, + ); + expect(result).toBeUndefined(); + expect(getColumnsMock).not.toHaveBeenCalled(); + }); + + test('no-op: lists are deep-equal (re-fired without real change) → returns undefined', () => { + const result = deferredDepChange( + { inherits: [1, 2] }, null, null, { oldState: { inherits: [1, 2] } }, + ); + expect(result).toBeUndefined(); + expect(getColumnsMock).not.toHaveBeenCalled(); + }); + + // ---- Regression: stale inheritedTableList ------------------------------ + + test('remove of a table missing from inheritedTableList → returns undefined (no silent loss of local columns)', () => { + // Reproduces the data-loss bug found during aggressive review. + // If the removed table is not in `inheritedTableList`, getTableOid + // returns undefined, and the old callback would have filtered out + // every column with `inheritedid == undefined || null` — including + // user-added local columns. Contract: opt out instead. + const schemaWithStaleList = makeSchema(getColumnsMock); + schemaWithStaleList.inheritedTableList = []; // stale: removed table not here + const defChange = getDeferred(schemaWithStaleList); + const result = defChange( + { inherits: [] }, null, null, { oldState: { inherits: [99 /* unknown */] } }, + ); + expect(result).toBeUndefined(); + expect(getColumnsMock).not.toHaveBeenCalled(); + }); + + test('same length, swapped content (replace) → processes the remove so stale columns are cleared', async () => { + // A multi-select swap (e.g. user replaces parent table A with B + // while keeping C) emits a same-length array with different + // contents. Old behavior: opt out → stale columns from the + // removed parent stay forever. New behavior: detect the remove + // direction and apply it; the next selection of the new parent + // (when getColumns succeeds) will trigger a normal ADD for those. + const result = deferredDepChange( + { inherits: [1, 2] }, null, null, { oldState: { inherits: [2, 3] } }, + ); + expect(result).toBeInstanceOf(Promise); + const cb = await result; + const tmpCols = [ + { name: 'kept_local', inheritedid: null }, + { name: 'from_2', inheritedid: 2 }, + { name: 'from_3', inheritedid: 3 }, // 3 was removed + ]; + const delta = cb({ columns: tmpCols }); + expect(delta.columns).toEqual([ + { name: 'kept_local', inheritedid: null }, + { name: 'from_2', inheritedid: 2 }, + ]); + // No new fetch fired — adds are handled by the next user gesture. + expect(getColumnsMock).not.toHaveBeenCalled(); + }); +}); diff --git a/web/regression/javascript/schema_ui_files/index.ui.deferred.spec.js b/web/regression/javascript/schema_ui_files/index.ui.deferred.spec.js new file mode 100644 index 00000000000..18dbcf66e2d --- /dev/null +++ b/web/regression/javascript/schema_ui_files/index.ui.deferred.spec.js @@ -0,0 +1,110 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Characterization + contract tests for the `amname` deferredDepChange +// on IndexSchema. The four reachable paths are: +// 1) amname unchanged -> no-op +// 2) amname changed, columns empty -> no-op +// 3) amname changed, columns present, user confirms -> clear columns +// 4) amname changed, columns present, user cancels -> revert amname +// +// These tests assert the OBSERVABLE outputs (returned promise, callback +// return value, side-effects on the input state) so a refactor that +// preserves intent can be verified against them. + +import IndexSchema from + '../../../pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui'; +// IndexSchema's deferredDepChange calls `pgAdmin.Browser.notifier.confirm` +// via `import pgAdmin from 'sources/pgadmin'`. Jest's moduleNameMapper +// redirects `sources/pgadmin` to fake_pgadmin, so we import the SAME +// instance here and spy on its `confirm`. +import pgAdmin from '../fake_pgadmin'; + +const getAmnameDeferredDepChange = () => { + const schema = new IndexSchema( + { amname: () => Promise.resolve([]) }, + { table: {} }, + ); + const field = schema.baseFields.find((f) => f.id === 'amname'); + expect(field).toBeDefined(); + expect(typeof field.deferredDepChange).toBe('function'); + return field.deferredDepChange.bind(schema); +}; + +describe('IndexSchema.amname deferredDepChange — characterization', () => { + let deferredDepChange; + let confirmSpy; + beforeEach(() => { + deferredDepChange = getAmnameDeferredDepChange(); + confirmSpy = jest.spyOn(pgAdmin.Browser.notifier, 'confirm'); + confirmSpy.mockClear(); + }); + afterEach(() => { confirmSpy.mockRestore(); }); + + // ---- No-op paths --------------------------------------------------------- + + test('no-op: amname unchanged — returns undefined (opts out of queue)', () => { + const state = { amname: 'btree', columns: [{ name: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(result).toBeUndefined(); + expect(confirmSpy).not.toHaveBeenCalled(); + }); + + test('no-op: amname changed but columns empty — returns undefined', () => { + const state = { amname: 'hash', columns: [] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(result).toBeUndefined(); + expect(confirmSpy).not.toHaveBeenCalled(); + }); + + // ---- Confirm path -------------------------------------------------------- + + test('change + confirm: callback returns {columns: []} as a fresh empty array, without mutating input state', async () => { + const originalColumns = [{ name: 'c1' }, { name: 'c2' }]; + const state = { amname: 'hash', columns: originalColumns }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + const [/* title */, /* msg */, confirmCb /* cancelCb */] = + confirmSpy.mock.calls[0]; + + confirmCb(); + const cb = await result; + expect(typeof cb).toBe('function'); + const delta = cb(); + expect(delta).toEqual({ columns: [] }); + // New contract: no input-state mutation. + expect(state.columns).toBe(originalColumns); + expect(originalColumns).toHaveLength(2); + }); + + // ---- Cancel path --------------------------------------------------------- + + test('change + cancel: callback returns {amname: oldValue} without mutating input state', async () => { + const state = { amname: 'hash', columns: [{ name: 'c1' }] }; + const result = deferredDepChange(state, null, null, { + oldState: { amname: 'btree' }, + }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + const [, , , cancelCb] = confirmSpy.mock.calls[0]; + + cancelCb(); + const cb = await result; + expect(typeof cb).toBe('function'); + const delta = cb(); + expect(delta).toEqual({ amname: 'btree' }); + // New contract: input state's amname is NOT mutated. + expect(state.amname).toBe('hash'); + }); +}); diff --git a/web/regression/javascript/schema_ui_files/table.ui.deferred.spec.js b/web/regression/javascript/schema_ui_files/table.ui.deferred.spec.js new file mode 100644 index 00000000000..b5845b5b8a9 --- /dev/null +++ b/web/regression/javascript/schema_ui_files/table.ui.deferred.spec.js @@ -0,0 +1,175 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +// Opt-out + no-mutation contract tests for TableSchema's +// deferredDepChange on `typname` and `coll_inherits`. The happy paths +// for both fields are already covered by table.ui.spec.js; this file +// adds: +// +// * typname — no-change branch must return undefined, +// not a Promise wrapping a no-op callback +// * coll_inherits — same-length / both-empty / deep-equal must +// return undefined (previously hung) +// * coll_inherits — remove branch's callback must NOT mutate +// tmpstate.columns + +import _ from 'lodash'; +import { getNodeTableSchema } from + '../../../pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui'; +import * as nodeAjax from + '../../../pgadmin/browser/static/js/node_ajax'; + +const makeSchema = () => { + jest.spyOn(nodeAjax, 'getNodeAjaxOptions').mockReturnValue(Promise.resolve([])); + jest.spyOn(nodeAjax, 'getNodeListByName').mockReturnValue(Promise.resolve([])); + return getNodeTableSchema( + { server: { _id: 1 }, schema: { _label: 'public' } }, {}, + { + Nodes: { table: {} }, + serverInfo: { 1: { user: { name: 'Postgres' } } }, + }, + ); +}; + +const getDeferred = (schema, fieldId) => { + const field = _.find(schema.fields, (f) => f.id === fieldId); + expect(field).toBeDefined(); + expect(typeof field.deferredDepChange).toBe('function'); + return field.deferredDepChange.bind(schema); +}; + +describe('TableSchema.typname deferredDepChange — opt-out contract', () => { + let schema, deferredDepChange; + + beforeEach(() => { + schema = makeSchema(); + deferredDepChange = getDeferred(schema, 'typname'); + jest.spyOn(schema, 'changeColumnOptions').mockImplementation(() => {}); + }); + + test('no change → returns undefined (was Promise wrapping no-op callback)', () => { + const result = deferredDepChange( + { typname: null }, null, null, { oldState: { typname: null } }, + ); + expect(result).toBeUndefined(); + }); + + test('both equal non-empty → returns undefined', () => { + const result = deferredDepChange( + { typname: 'type1' }, null, null, { oldState: { typname: 'type1' } }, + ); + expect(result).toBeUndefined(); + }); + + test('stale ofTypeTables (state.typname not in options) → callback does not throw, columns=[]', async () => { + // Aggressive review caught: when state.typname is non-empty but + // doesn't match any loaded option (option list stale or empty), + // the original code did `typeTable.oftype_columns` on `undefined` + // and threw. The drain swallowed the throw into console.error + // with zero user feedback. + schema.ofTypeTables = []; // stale / empty + // Simulate the "later selection" path (typname changed, not from null). + const result = deferredDepChange( + { typname: 'no_such_type' }, null, null, + { oldState: { typname: 'old_type' } }, + ); + expect(result).toBeInstanceOf(Promise); + const cb = await result; + expect(() => cb()).not.toThrow(); + const delta = cb(); + expect(delta.columns).toEqual([]); + expect(delta.primary_key).toEqual([]); + }); +}); + +describe('TableSchema.coll_inherits deferredDepChange — opt-out + no-mutation', () => { + let schema, deferredDepChange; + + beforeEach(() => { + schema = makeSchema(); + deferredDepChange = getDeferred(schema, 'coll_inherits'); + jest.spyOn(schema, 'changeColumnOptions').mockImplementation(() => {}); + jest.spyOn(schema, 'getTableOid').mockReturnValue(140391); + jest.spyOn(schema, 'getColumns').mockResolvedValue([]); + }); + + test('both empty → returns undefined (was a hanging Promise)', () => { + const result = deferredDepChange( + { coll_inherits: [], columns: [] }, null, null, + { oldState: { coll_inherits: [] } }, + ); + expect(result).toBeUndefined(); + expect(schema.getColumns).not.toHaveBeenCalled(); + }); + + test('deep-equal repeat → returns undefined', () => { + const result = deferredDepChange( + { coll_inherits: ['t1', 't2'], columns: [] }, null, null, + { oldState: { coll_inherits: ['t1', 't2'] } }, + ); + expect(result).toBeUndefined(); + expect(schema.getColumns).not.toHaveBeenCalled(); + }); + + test('same-length swap → processes the remove so stale columns are cleared', async () => { + // Same shape as the foreign_table same-length swap fix. + schema.getTableOid.mockImplementation((name) => + ({ t1: 1, t2: 2, t3: 3 })[name]); + const result = deferredDepChange( + { coll_inherits: ['t1', 't3'], columns: [] }, null, null, + { oldState: { coll_inherits: ['t1', 't2'] } }, + ); + expect(result).toBeInstanceOf(Promise); + const cb = await result; + const tmpCols = [ + { name: 'kept_local', inheritedid: null }, + { name: 'from_t1', inheritedid: 1 }, + { name: 'from_t2', inheritedid: 2 }, // t2 was removed + ]; + const delta = cb({ columns: tmpCols }); + expect(delta.columns).toEqual([ + { name: 'kept_local', inheritedid: null }, + { name: 'from_t1', inheritedid: 1 }, + ]); + expect(schema.getColumns).not.toHaveBeenCalled(); + }); + + test('remove of a table missing from inheritedTableList → returns undefined (regression guard)', () => { + // Data-loss regression found during aggressive review. If the + // removed table is not in `inheritedTableList`, getTableOid returns + // undefined and the legacy refactored callback would have filtered + // out every column with `inheritedid` equal-by-coercion to undefined + // (i.e. null or missing) — silently dropping local user-added + // columns. Contract: opt out. + jest.spyOn(schema, 'getTableOid').mockReturnValue(undefined); + const result = deferredDepChange( + { coll_inherits: ['t1'], columns: [] }, null, null, + { oldState: { coll_inherits: ['t1', 't2'] } }, + ); + expect(result).toBeUndefined(); + expect(schema.getColumns).not.toHaveBeenCalled(); + }); + + test('remove → callback does NOT mutate tmpstate.columns', async () => { + const result = deferredDepChange( + { coll_inherits: [], columns: [] }, null, null, + { oldState: { coll_inherits: ['t1'] } }, + ); + const cb = await result; + const tmpCols = [ + { name: 'kept', inheritedid: null }, + { name: 'inherited', inheritedid: 140391 }, + ]; + const delta = cb({ columns: tmpCols }); + expect(delta.columns).toEqual([{ name: 'kept', inheritedid: null }]); + // tmpstate.columns must be intact (we used to splice it). + expect(tmpCols).toHaveLength(2); + expect(tmpCols[1]).toEqual({ name: 'inherited', inheritedid: 140391 }); + }); +}); diff --git a/web/regression/javascript/schema_ui_files/table.ui.spec.js b/web/regression/javascript/schema_ui_files/table.ui.spec.js index 51158783a4b..ad45c3f4992 100644 --- a/web/regression/javascript/schema_ui_files/table.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/table.ui.spec.js @@ -168,17 +168,16 @@ describe('TableSchema', () => { }); }); - it('empty', (done)=>{ + it('empty', ()=>{ + // typname unchanged is now an opt-out from the deferred queue. + // Previously it returned a Promise wrapping a no-op callback. let state = {typname: null}; - let deferredPromise = deferredDepChange(state, null, null, { + let result = deferredDepChange(state, null, null, { oldState: { typname: null, }, }); - deferredPromise.then((depChange)=>{ - expect(depChange()).toBeUndefined(); - done(); - }); + expect(result).toBeUndefined(); }); });