mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 01:15:03 +03:00
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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
1 parent
12894ddd2a
commit
f0952fdef8
6 files changed
+237
-61
No files matched your search
@@ -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 = () => {
|
||||
>
|
||||
<AlertDialogContent size="medium">
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>{`Confirm to disable ${selectedWrapper?.name}`}</AlertDialogTitle>
|
||||
<AlertDialogTitle>{`Delete connection ${selectedWrapper?.server_name}?`}</AlertDialogTitle>
|
||||
<AlertDialogDescription>
|
||||
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.
|
||||
</AlertDialogDescription>
|
||||
</AlertDialogHeader>
|
||||
<AlertDialogFooter>
|
||||
<AlertDialogCancel>Cancel</AlertDialogCancel>
|
||||
<AlertDialogAction variant="danger" onClick={onConfirmDelete}>
|
||||
Confirm
|
||||
Delete connection
|
||||
</AlertDialogAction>
|
||||
</AlertDialogFooter>
|
||||
</AlertDialogContent>
|
||||
|
||||
@@ -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 (
|
||||
<TableRow>
|
||||
<TableCell className="gap-2 align-top py-3! min-w-80">
|
||||
{wrapper.name}
|
||||
<p className="text-sm text-foreground-light">
|
||||
Connection: <code className="text-code-inline">{wrapper.server_name}</code>
|
||||
</p>
|
||||
{isShared && (
|
||||
<p className="text-sm text-foreground-light">
|
||||
This wrapper is shared. To edit this connection, use <code>ALTER SERVER</code> on{' '}
|
||||
<code className="text-code-inline">{wrapper.server_name}</code> or{' '}
|
||||
<code>ALTER FOREIGN TABLE</code> in the{' '}
|
||||
<Link
|
||||
href={`/project/${ref}/sql/new?skip=true`}
|
||||
className="underline underline-offset-2"
|
||||
>
|
||||
SQL Editor
|
||||
</Link>
|
||||
.
|
||||
{encryptedMetadata.length > 0 && (
|
||||
<>
|
||||
{' '}
|
||||
Edit this server's credentials in{' '}
|
||||
<Link
|
||||
href={`/project/${ref}/settings/vault/secrets`}
|
||||
className="underline underline-offset-2"
|
||||
>
|
||||
Vault
|
||||
</Link>
|
||||
. Changes to a secret used by other connections affect them too.
|
||||
</>
|
||||
)}
|
||||
</p>
|
||||
)}
|
||||
|
||||
{visibleMetadata.map((metadata) => (
|
||||
<div
|
||||
@@ -122,16 +157,14 @@ export const WrapperRow = ({ wrapper }: WrapperRowProps) => {
|
||||
<TableCell className="flex-nowrap">
|
||||
<div className="flex items-center gap-x-2">
|
||||
<ButtonTooltip
|
||||
disabled={!canManageWrappers}
|
||||
disabled={!canEdit}
|
||||
icon={<Edit strokeWidth={1.5} />}
|
||||
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',
|
||||
},
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -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<string | null>(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) => {
|
||||
</TableRow>
|
||||
</TableHeader>
|
||||
<TableBody>
|
||||
{(isLatest ? wrappers.slice(0, 3) : wrappers).map((x) => {
|
||||
return <WrapperRow key={x.id} wrapper={x} />
|
||||
})}
|
||||
{(isLatest ? wrappers.slice(0, 3) : wrappers).map((x) => (
|
||||
<WrapperRow key={x.id} wrapper={x} isShared={isSharedWrapper(x)} />
|
||||
))}
|
||||
</TableBody>
|
||||
<TableFooter
|
||||
className={cn(
|
||||
|
||||
@@ -14,7 +14,7 @@ import type { ResponseError, UseCustomMutationOptions } from '@/types'
|
||||
export type FDWDeleteVariables = {
|
||||
projectRef?: string
|
||||
connectionString?: string | null
|
||||
wrapper: { name: string }
|
||||
wrapper: { id: number; name: string; server_name: string }
|
||||
wrapperMeta: WrapperMeta
|
||||
}
|
||||
|
||||
@@ -57,9 +57,7 @@ export const useFDWDeleteMutation = ({
|
||||
},
|
||||
async onError(data, variables, context) {
|
||||
if (onError === undefined) {
|
||||
toast.error(
|
||||
`Failed to disable ${variables.wrapper.name} foreign data wrapper: ${data.message}`
|
||||
)
|
||||
toast.error(`Failed to delete ${variables.wrapper.name} connection: ${data.message}`)
|
||||
} else {
|
||||
onError(data, variables, context)
|
||||
}
|
||||
|
||||
@@ -255,6 +255,8 @@ export function getCreateFDWSql({
|
||||
([key, value]) =>
|
||||
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}
|
||||
|
||||
@@ -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'")
|
||||
})
|
||||
Reference in new issue
Block a user