From f0952fdef82101cc2d4bf39925f267c18510c0c5 Mon Sep 17 00:00:00 2001 From: Jordi Enric <37541088+jordienr@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:49:01 +0200 Subject: [PATCH] fix(studio): delete only the selected foreign server FE-4462 (#50785) ## Problem The dashboard lists one row per foreign server, but deleting a row dropped its foreign data wrapper with CASCADE. When multiple servers shared a wrapper, deleting one removed all of them. ## Fix Drop the selected server and its foreign tables. Remove the underlying wrapper and Vault secret only when no servers still use it. Edits to a shared wrapper now stop before making changes because the existing edit flow recreates the underlying wrapper. ## How to test 1. Configure two BigQuery foreign servers that use the same foreign data wrapper. Delete one from the dashboard. 2. Confirm the other server and its foreign tables still exist and work. 3. Delete the remaining server. Confirm the foreign data wrapper and its Vault secret are removed. 4. Attempt to edit one of two servers sharing a wrapper. Confirm the edit fails without removing either server. Focused pg-meta tests and typecheck pass. ## Summary by CodeRabbit * **New Features** * Shared connections are identified in the integrations list, with guidance for editing them in the SQL Editor. Editing is disabled when a wrapper is shared, with an explanation shown. * Deleting a connection removes its foreign tables and removes the wrapper and Vault secret only when no other connection uses them. * **Bug Fixes** * Connection deletion verifies that the selected server still belongs to the wrapper and reports failures using connection-focused wording. * Attempts to edit a wrapper used by another connection are blocked with a clear explanation. * Connection deletion and confirmation messages now consistently refer to deleting a connection. --- .../Wrappers/DeleteWrapperModal.tsx | 10 +- .../Integrations/Wrappers/WrapperRow.tsx | 45 ++++++- .../Integrations/Wrappers/WrapperTable.tsx | 40 ++++-- apps/studio/data/fdw/fdw-delete-mutation.ts | 6 +- .../pg-meta/src/sql/studio/database/fdw.ts | 123 +++++++++++++----- packages/pg-meta/test/sql/studio/fdw.test.ts | 74 ++++++++++- 6 files changed, 237 insertions(+), 61 deletions(-) diff --git a/apps/studio/components/interfaces/Integrations/Wrappers/DeleteWrapperModal.tsx b/apps/studio/components/interfaces/Integrations/Wrappers/DeleteWrapperModal.tsx index ca7418dcfd1..d6c35b2906d 100644 --- a/apps/studio/components/interfaces/Integrations/Wrappers/DeleteWrapperModal.tsx +++ b/apps/studio/components/interfaces/Integrations/Wrappers/DeleteWrapperModal.tsx @@ -45,7 +45,7 @@ export const DeleteWrapperModal = () => { const { mutateAsync: deleteFDW, isSuccess: isSuccessDelete } = useFDWDeleteMutation({ onSuccess: () => { - toast.success(`Successfully disabled ${selectedWrapper?.name} foreign data wrapper`) + toast.success('Connection deleted') setSelectedWrapperToDelete(null) }, }) @@ -84,16 +84,16 @@ export const DeleteWrapperModal = () => { > - {`Confirm to disable ${selectedWrapper?.name}`} + {`Delete connection ${selectedWrapper?.server_name}?`} - Are you sure you want to disable {selectedWrapper?.name}? This will also remove all - tables created with this wrapper. + This deletes this connection and its foreign tables. If this is the last connection + using the wrapper, its Vault secret and wrapper are also removed. Cancel - Confirm + Delete connection diff --git a/apps/studio/components/interfaces/Integrations/Wrappers/WrapperRow.tsx b/apps/studio/components/interfaces/Integrations/Wrappers/WrapperRow.tsx index 5ef2f2a5c7e..2fbcbdef192 100644 --- a/apps/studio/components/interfaces/Integrations/Wrappers/WrapperRow.tsx +++ b/apps/studio/components/interfaces/Integrations/Wrappers/WrapperRow.tsx @@ -14,9 +14,10 @@ import { useAsyncCheckPermissions } from '@/hooks/misc/useCheckPermissions' interface WrapperRowProps { wrapper: FDW + isShared: boolean } -export const WrapperRow = ({ wrapper }: WrapperRowProps) => { +export const WrapperRow = ({ wrapper, isShared }: WrapperRowProps) => { const { ref, id } = useParams() const { can: canManageWrappers } = useAsyncCheckPermissions( PermissionAction.TENANT_SQL_ADMIN_WRITE, @@ -39,11 +40,45 @@ export const WrapperRow = ({ wrapper }: WrapperRowProps) => { ) const _tables = formatWrapperTables(wrapper, integration?.meta) + const canEdit = canManageWrappers && !isShared + let editTooltip = 'Edit wrapper' + if (!canManageWrappers) editTooltip = 'You need additional permissions to edit wrappers' + else if (isShared) editTooltip = 'Shared wrappers cannot be edited in the dashboard' return ( {wrapper.name} +

+ Connection: {wrapper.server_name} +

+ {isShared && ( +

+ This wrapper is shared. To edit this connection, use ALTER SERVER on{' '} + {wrapper.server_name} or{' '} + ALTER FOREIGN TABLE in the{' '} + + SQL Editor + + . + {encryptedMetadata.length > 0 && ( + <> + {' '} + Edit this server's credentials in{' '} + + Vault + + . Changes to a secret used by other connections affect them too. + + )} +

+ )} {visibleMetadata.map((metadata) => (
{
} className="px-1.5" onClick={() => setSelectedWrapperToEdit(wrapper.id.toString())} tooltip={{ content: { side: 'bottom', - text: !canManageWrappers - ? 'You need additional permissions to edit wrappers' - : 'Edit wrapper', + text: editTooltip, }, }} /> @@ -145,7 +178,7 @@ export const WrapperRow = ({ wrapper }: WrapperRowProps) => { side: 'bottom', text: !canManageWrappers ? 'You need additional permissions to delete wrappers' - : 'Delete wrapper', + : 'Delete connection', }, }} /> diff --git a/apps/studio/components/interfaces/Integrations/Wrappers/WrapperTable.tsx b/apps/studio/components/interfaces/Integrations/Wrappers/WrapperTable.tsx index c458bd96eb9..83e840e1f32 100644 --- a/apps/studio/components/interfaces/Integrations/Wrappers/WrapperTable.tsx +++ b/apps/studio/components/interfaces/Integrations/Wrappers/WrapperTable.tsx @@ -1,6 +1,6 @@ import { useParams } from 'common' import { parseAsString, useQueryState } from 'nuqs' -import { useEffect, useMemo, useState } from 'react' +import { useEffect, useMemo, useRef, useState } from 'react' import { toast } from 'sonner' import { Card, @@ -35,7 +35,7 @@ export const WrapperTable = ({ isLatest = false }: WrapperTableProps) => { const [isClosingEditWrapper, setIsClosingEditWrapper] = useState(false) - const { data, isError } = useFDWsQuery({ + const { data, isError, isSuccess } = useFDWsQuery({ projectRef: ref, connectionString: project?.connectionString, }) @@ -49,14 +49,36 @@ export const WrapperTable = ({ isLatest = false }: WrapperTableProps) => { ) const [selectedWrapperIdToEdit, setSelectedWrapperToEdit] = useQueryState('edit', parseAsString) - const selectedWrapperToEdit = wrappers.find((w) => w.id.toString() === selectedWrapperIdToEdit) + const isSharedWrapper = (wrapper: (typeof wrappers)[number]) => + data?.some((other) => other.id !== wrapper.id && other.name === wrapper.name) ?? false + const selectedWrapper = wrappers.find((w) => w.id.toString() === selectedWrapperIdToEdit) + const isSelectedWrapperShared = selectedWrapper !== undefined && isSharedWrapper(selectedWrapper) + const selectedWrapperToEdit = isSelectedWrapperShared ? undefined : selectedWrapper + const openedWrapperId = useRef(null) useEffect(() => { - if (isError && !!selectedWrapperIdToEdit && !selectedWrapperToEdit) { - toast('Wrapper not found') + if (!selectedWrapperIdToEdit) { + openedWrapperId.current = null + } else if (selectedWrapperToEdit) { + openedWrapperId.current = selectedWrapperIdToEdit + } else if (isSuccess || isError) { + if (openedWrapperId.current !== selectedWrapperIdToEdit) { + toast( + isSelectedWrapperShared + ? 'Shared wrappers cannot be edited in the dashboard. Use the SQL Editor to edit this connection.' + : 'Wrapper not found' + ) + } setSelectedWrapperToEdit(null) } - }, [isError, selectedWrapperIdToEdit, selectedWrapperToEdit, setSelectedWrapperToEdit]) + }, [ + isError, + isSelectedWrapperShared, + isSuccess, + selectedWrapperIdToEdit, + selectedWrapperToEdit, + setSelectedWrapperToEdit, + ]) if (!integration || integration.type !== 'wrapper') { return ( @@ -81,9 +103,9 @@ export const WrapperTable = ({ isLatest = false }: WrapperTableProps) => { - {(isLatest ? wrappers.slice(0, 3) : wrappers).map((x) => { - return - })} + {(isLatest ? wrappers.slice(0, 3) : wrappers).map((x) => ( + + ))} key !== 'table_name' && key !== 'schema_name' && + key !== 'schema' && + key !== 'id' && key !== 'columns' && key !== 'index' && key !== 'is_new_schema' && @@ -298,7 +300,7 @@ export const getDeleteFDWSql = ({ wrapper, wrapperMeta, }: { - wrapper: { name: string } + wrapper: { id: number; name: string; server_name: string } wrapperMeta: SimplifiedWrapperMeta }): SafeSqlFragment => { const encryptedOptions = wrapperMeta.server.options.filter((option) => option.encrypted) @@ -309,41 +311,45 @@ export const getDeleteFDWSql = ({ return safeSql` do $$ begin - -- Old wrappers has an implicit dependency on pgsodium. For new wrappers - -- we use Vault directly. - if (select extversion from pg_extension where extname = 'wrappers') in ( - '0.1.0', - '0.1.1', - '0.1.4', - '0.1.5', - '0.1.6', - '0.1.7', - '0.1.8', - '0.1.9', - '0.1.10', - '0.1.11', - '0.1.12', - '0.1.14', - '0.1.15', - '0.1.16', - '0.1.17', - '0.1.18', - '0.1.19', - '0.2.0', - '0.3.0', - '0.3.1', - '0.4.0', - '0.4.1', - '0.4.2', - '0.4.3', - '0.4.4', - '0.4.5' + if not exists ( + select 1 from pg_catalog.pg_foreign_data_wrapper where fdwname = ${literal(wrapper.name)} ) then - delete from vault.secrets where key_id = (select id from pgsodium.valid_key where name = ${literal(key)}); + -- Old wrappers has an implicit dependency on pgsodium. For new wrappers + -- we use Vault directly. + if (select extversion from pg_extension where extname = 'wrappers') in ( + '0.1.0', + '0.1.1', + '0.1.4', + '0.1.5', + '0.1.6', + '0.1.7', + '0.1.8', + '0.1.9', + '0.1.10', + '0.1.11', + '0.1.12', + '0.1.14', + '0.1.15', + '0.1.16', + '0.1.17', + '0.1.18', + '0.1.19', + '0.2.0', + '0.3.0', + '0.3.1', + '0.4.0', + '0.4.1', + '0.4.2', + '0.4.3', + '0.4.4', + '0.4.5' + ) then + delete from vault.secrets where key_id = (select id from pgsodium.valid_key where name = ${literal(key)}); - delete from pgsodium.key where name = ${literal(key)}; - else - delete from vault.secrets where name = ${literal(key)}; + delete from pgsodium.key where name = ${literal(key)}; + else + delete from vault.secrets where name = ${literal(key)}; + end if; end if; end $$; ` @@ -351,8 +357,37 @@ export const getDeleteFDWSql = ({ const deleteEncryptedSecretsSql = joinSqlFragments(deleteEncryptedSecretsSqlArray, '\n') + const ensureSelectedServerSql = safeSql` + begin + if not exists ( + select 1 + from pg_catalog.pg_foreign_server s + join pg_catalog.pg_foreign_data_wrapper w on w.oid = s.srvfdw + where s.oid = ${literal(wrapper.id)} + and s.srvname = ${literal(wrapper.server_name)} + and w.fdwname = ${literal(wrapper.name)} + ) then + raise exception 'The selected foreign server no longer belongs to this wrapper.'; + end if; + end + ` + const sql = safeSql` - drop foreign data wrapper if exists ${ident(wrapper.name)} cascade; + do ${literal(ensureSelectedServerSql)}; + + drop server if exists ${ident(wrapper.server_name)} cascade; + + do $$ + begin + if not exists ( + select 1 + from pg_catalog.pg_foreign_server s + join pg_catalog.pg_foreign_data_wrapper w on w.oid = s.srvfdw + where w.fdwname = ${literal(wrapper.name)} + ) then + execute format('drop foreign data wrapper if exists %I cascade', ${literal(wrapper.name)}); + end if; + end $$; ${deleteEncryptedSecretsSql} ` @@ -366,11 +401,25 @@ export const getUpdateFDWSql = ({ formState, tables, }: { - wrapper: { name: string } + wrapper: { id: number; name: string; server_name: string } wrapperMeta: SimplifiedWrapperMeta formState: { [k: string]: string } tables: any[] }): SafeSqlFragment => { + const ensureWrapperIsNotSharedSql = safeSql` + do $$ + begin + if exists ( + select 1 + from pg_catalog.pg_foreign_server s + join pg_catalog.pg_foreign_data_wrapper w on w.oid = s.srvfdw + where w.fdwname = ${literal(wrapper.name)} + and s.srvname <> ${literal(wrapper.server_name)} + ) then + raise exception 'This wrapper is used by another server and cannot be edited here.'; + end if; + end $$; + ` const deleteWrapperSql = getDeleteFDWSql({ wrapper, wrapperMeta }) const createWrapperSql = getCreateFDWSql({ wrapperMeta, @@ -382,6 +431,8 @@ export const getUpdateFDWSql = ({ }) const sql = safeSql` + ${ensureWrapperIsNotSharedSql} + ${deleteWrapperSql} ${createWrapperSql} diff --git a/packages/pg-meta/test/sql/studio/fdw.test.ts b/packages/pg-meta/test/sql/studio/fdw.test.ts index bf8613b4c4a..741c6f9c3de 100644 --- a/packages/pg-meta/test/sql/studio/fdw.test.ts +++ b/packages/pg-meta/test/sql/studio/fdw.test.ts @@ -1,6 +1,6 @@ import { expect, test } from 'vitest' -import { getCreateFDWSql } from '../../../src' +import { getCreateFDWSql, getDeleteFDWSql, getUpdateFDWSql } from '../../../src' import { literal } from '../../../src/pg-format' const baseArgs = { @@ -54,3 +54,75 @@ test('encrypted server options still resolve their value through Vault unchanged expect(sql).toContain("api_secret ''%s''") expect(sql).toContain('vault.create_secret') }) + +test('deleting a wrapper row drops its server and preserves shared wrapper dependencies', () => { + const sql = getDeleteFDWSql({ + wrapper: { id: 42, name: 'bigquery_fdw', server_name: 'selected_bigquery_server' }, + wrapperMeta: { + handlerName: 'big_query_fdw_handler', + validatorName: 'big_query_fdw_validator', + server: { options: [{ name: 'sa_key_id', encrypted: true }] }, + }, + }) + + expect(sql).toContain('where s.oid = 42') + expect(sql).toContain("and s.srvname = ''selected_bigquery_server''") + expect(sql).toContain("and w.fdwname = ''bigquery_fdw''") + expect(sql.indexOf('raise exception')).toBeLessThan(sql.indexOf('drop server')) + expect(sql).toContain('drop server if exists selected_bigquery_server cascade') + expect(sql).toContain("where w.fdwname = 'bigquery_fdw'") + expect(sql).toContain("execute format('drop foreign data wrapper if exists %I cascade'") + expect(sql).not.toContain('drop foreign data wrapper if exists "bigquery_fdw" cascade') + expect(sql).toContain("where fdwname = 'bigquery_fdw'") +}) + +test('editing a shared wrapper fails before any server is dropped', () => { + const sql = getUpdateFDWSql({ + wrapper: { id: 42, name: 'bigquery_fdw', server_name: 'selected_bigquery_server' }, + wrapperMeta: { + handlerName: 'big_query_fdw_handler', + validatorName: 'big_query_fdw_validator', + server: { options: [] }, + }, + formState: { wrapper_name: 'bigquery_fdw', server_name: 'selected_bigquery_server' }, + tables: [], + }) + + expect(sql).toContain("s.srvname <> 'selected_bigquery_server'") + expect(sql.indexOf('raise exception')).toBeLessThan(sql.indexOf('drop server')) +}) + +test('editing an existing foreign table excludes catalog fields from its options', () => { + const sql = getUpdateFDWSql({ + wrapper: { id: 42, name: 'bigquery_fdw', server_name: 'bigquery_server' }, + wrapperMeta: { + handlerName: 'big_query_fdw_handler', + validatorName: 'big_query_fdw_validator', + server: { options: [{ name: 'project_id', encrypted: false }] }, + }, + formState: { + wrapper_name: 'bigquery_fdw', + server_name: 'bigquery_server', + project_id: 'example-project', + }, + tables: [ + { + id: 171639, + schema: 'qa_bq', + schema_name: 'qa_bq', + table_name: 'orders', + columns: [{ name: 'id', type: 'text' }], + is_new_schema: false, + index: 0, + table: 'orders', + location: 'US', + }, + ], + }) + + expect(sql).toContain('create foreign table qa_bq.orders') + expect(sql).toContain('"table" \'orders\'') + expect(sql).toContain("location 'US'") + expect(sql).not.toContain('id 171639') + expect(sql).not.toContain("schema 'qa_bq'") +})