Skip to content

fix(deparser): keep schema qualification on GRANT ON TYPE/DOMAIN, COMMENT ON TRIGGER/RULE/OPERATOR/CAST and CREATE OPERATOR commutator #375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pyramation merged 4 commits into from
Nov 5, 2026
Merged

fix(deparser): keep schema qualification on GRANT ON TYPE/DOMAIN, COMMENT ON TRIGGER/RULE/OPERATOR/CAST and CREATE OPERATOR commutator #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
194e0fd
test(deparser): schema-qualified object references in GRANT ON TYPE/D…
pyramation Nov 5, 2026
b030b3a
fix(deparser): keep schema qualification on GRANT ON TYPE/DOMAIN, COM…
pyramation Nov 5, 2026
8951738
test(deparser): ALTER OPERATOR ... SET (COMMUTATOR = OPERATOR(schema.…
pyramation Nov 5, 2026
5aef5ad
fix(deparser): render multi-part COMMUTATOR/NEGATOR lists as OPERATOR…
pyramation Nov 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions __fixtures__/generated/generated.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21256,6 +21256,17 @@
"misc/quotes_etc-28.sql": "DO $$\nBEGIN\n RAISE NOTICE 'Line one\\nLine two';\nEND;\n$$ LANGUAGE plpgsql",
"misc/quotes_etc-29.sql": "CREATE USER MAPPING FOR local_user SERVER \"foreign_server\" OPTIONS (user 'remote_user', password 'secret123')",
"misc/quotes_etc-30.sql": "CREATE USER MAPPING FOR local_user SERVER foreign_server OPTIONS (user 'remote_user', password 'secret123')",
"misc/qualified-object-refs-1.sql": "GRANT USAGE ON TYPE fx.e TO r1",
"misc/qualified-object-refs-2.sql": "GRANT USAGE ON DOMAIN fx.d TO r1",
"misc/qualified-object-refs-3.sql": "REVOKE USAGE ON TYPE fx.e FROM r1",
"misc/qualified-object-refs-4.sql": "GRANT USAGE ON TYPE \"Fx\".\"Mixed Case\" TO r1",
"misc/qualified-object-refs-5.sql": "COMMENT ON TRIGGER t_tg ON fx.t IS 'trigger comment'",
"misc/qualified-object-refs-6.sql": "COMMENT ON RULE r ON fx.t IS 'rule comment'",
"misc/qualified-object-refs-7.sql": "COMMENT ON OPERATOR fx.### (int, int) IS 'operator comment'",
"misc/qualified-object-refs-8.sql": "COMMENT ON OPERATOR fx.### (NONE, int) IS 'prefix operator comment'",
"misc/qualified-object-refs-9.sql": "COMMENT ON CAST (int AS text) IS 'cast comment'",
"misc/qualified-object-refs-10.sql": "CREATE OPERATOR fx.### (LEFTARG = int, RIGHTARG = int, FUNCTION = fx.add3, COMMUTATOR = OPERATOR(fx.###), NEGATOR = OPERATOR(fx.!!!))",
"misc/qualified-object-refs-11.sql": "ALTER OPERATOR fx.### (int, int) SET (COMMUTATOR = OPERATOR(fx.!!!), NEGATOR = OPERATOR(fx.!!!))",
"misc/policy-quoted-names-1.sql": "CREATE POLICY \"Users can view own data\" ON public.todos FOR SELECT TO authenticated USING (auth.uid() = user_id)",
"misc/policy-quoted-names-2.sql": "DROP POLICY \"Users can view own data\" ON public.todos",
"misc/policy-quoted-names-3.sql": "DROP POLICY IF EXISTS \"Users can view own data\" ON todos",
Expand Down
13 changes: 13 additions & 0 deletions __fixtures__/kitchen-sink/misc/qualified-object-refs.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
-- Schema-qualified object references in GRANT/REVOKE, COMMENT and CREATE OPERATOR
-- (found while round-tripping pgpm diff migrations through the deparser)
GRANT USAGE ON TYPE fx.e TO r1;
GRANT USAGE ON DOMAIN fx.d TO r1;
REVOKE USAGE ON TYPE fx.e FROM r1;
GRANT USAGE ON TYPE "Fx"."Mixed Case" TO r1;
COMMENT ON TRIGGER t_tg ON fx.t IS 'trigger comment';
COMMENT ON RULE r ON fx.t IS 'rule comment';
COMMENT ON OPERATOR fx.### (int, int) IS 'operator comment';
COMMENT ON OPERATOR fx.### (NONE, int) IS 'prefix operator comment';
COMMENT ON CAST (int AS text) IS 'cast comment';
CREATE OPERATOR fx.### (LEFTARG = int, RIGHTARG = int, FUNCTION = fx.add3, COMMUTATOR = OPERATOR(fx.###), NEGATOR = OPERATOR(fx.!!!));
ALTER OPERATOR fx.### (int, int) SET (COMMUTATOR = OPERATOR(fx.!!!), NEGATOR = OPERATOR(fx.!!!));
19 changes: 19 additions & 0 deletions packages/deparser/__tests__/kitchen-sink/misc-qualified-object-refs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@

import { FixtureTestUtils } from '../../test-utils';
const fixtures = new FixtureTestUtils();

it('misc-qualified-object-refs', async () => {
await fixtures.runFixtureTests([
"misc/qualified-object-refs-1.sql",
"misc/qualified-object-refs-2.sql",
"misc/qualified-object-refs-3.sql",
"misc/qualified-object-refs-4.sql",
"misc/qualified-object-refs-5.sql",
"misc/qualified-object-refs-6.sql",
"misc/qualified-object-refs-7.sql",
"misc/qualified-object-refs-8.sql",
"misc/qualified-object-refs-9.sql",
"misc/qualified-object-refs-10.sql",
"misc/qualified-object-refs-11.sql"
]);
});
50 changes: 35 additions & 15 deletions packages/deparser/src/deparser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5944,10 +5944,13 @@ export class Deparser implements DeparserVisitor {
if (node.arg && this.getNodeType(node.arg) === 'List') {
const listData = this.getNodeData(node.arg);
const listItems = ListUtils.unwrapList(listData.items);
const preservedName = this.preserveOperatorDefElemCase(node.defname);
if (listItems.length === 1 && listItems[0].String) {
const preservedName = this.preserveOperatorDefElemCase(node.defname);
return `${preservedName} = ${listItems[0].String.sval}`;
}
if (listItems.length > 1 && listItems.every((item: any) => item.String)) {
return `${preservedName} = OPERATOR(${this.qualifiedOperatorName(listItems)})`;
}
}
}

Expand Down Expand Up @@ -6059,6 +6062,9 @@ export class Deparser implements DeparserVisitor {
if (listItems.length === 1 && listItems[0].String) {
return `${node.defname} = ${listItems[0].String.sval}`;
}
if (listItems.length > 1 && listItems.every((item: any) => item.String)) {
return `${node.defname} = OPERATOR(${this.qualifiedOperatorName(listItems)})`;
}
}
}

Expand Down Expand Up @@ -6980,6 +6986,13 @@ export class Deparser implements DeparserVisitor {
}
}

/** `schema.op` for an operator name list: schema parts quoted, the operator symbol as is. */
private qualifiedOperatorName(objname: any): string {
const names = ListUtils.unwrapList(objname ?? []).map((n: any) => (n.String ? n.String.sval : 'unknown'));
if (names.length === 0) return 'unknown';
return [...names.slice(0, -1).map((n: string) => QuoteUtils.quoteIdentifier(n)), names[names.length - 1]].join('.');
}

CommentStmt(node: t.CommentStmt, context: DeparserContext): string {
const output: string[] = ['COMMENT ON'];

Expand Down Expand Up @@ -7098,20 +7111,18 @@ export class Deparser implements DeparserVisitor {
output.push(objectParts.join('.'));
}
} else if (node.objtype === 'OBJECT_TRIGGER') {
if (objectParts.length === 2) {
const [table, trigger] = objectParts;
output.push(trigger);
if (objectParts.length >= 2) {
output.push(objectParts[objectParts.length - 1]);
output.push('ON');
output.push(table);
output.push(objectParts.slice(0, -1).join('.'));
} else {
output.push(objectParts.join('.'));
}
} else if (node.objtype === 'OBJECT_RULE') {
if (objectParts.length === 2) {
const [table, rule] = objectParts;
output.push(rule);
if (objectParts.length >= 2) {
output.push(objectParts[objectParts.length - 1]);
output.push('ON');
output.push(table);
output.push(objectParts.slice(0, -1).join('.'));
} else {
output.push(objectParts.join('.'));
}
Expand All @@ -7120,8 +7131,7 @@ export class Deparser implements DeparserVisitor {
// For operators, we need to handle ObjectWithArgs structure
if (node.object && (node.object as any).ObjectWithArgs) {
const objWithArgs = (node.object as any).ObjectWithArgs;
let operatorName = objWithArgs.objname && objWithArgs.objname[0] && objWithArgs.objname[0].String
? objWithArgs.objname[0].String.sval : 'unknown';
let operatorName = this.qualifiedOperatorName(objWithArgs.objname);

if (operatorName.startsWith('"') && operatorName.endsWith('"')) {
operatorName = operatorName.slice(1, -1);
Expand Down Expand Up @@ -7180,15 +7190,16 @@ export class Deparser implements DeparserVisitor {
} else {
output.push(objectParts.join('.'));
}
} else if (node.objtype === 'OBJECT_CAST' && objectParts.length === 2) {
output.push(`(${objectParts[0]} AS ${objectParts[1]})`);
} else {
output.push(objectParts.join('.'));
}
}
} else if (node.objtype === 'OBJECT_OPERATOR' && node.object && (node.object as any).ObjectWithArgs) {
// Handle direct ObjectWithArgs for OPERATOR syntax: COMMENT ON OPERATOR -(NONE, integer) IS 'comment'
const objWithArgs = (node.object as any).ObjectWithArgs;
let operatorName = objWithArgs.objname && objWithArgs.objname[0] && objWithArgs.objname[0].String
? objWithArgs.objname[0].String.sval : 'unknown';
let operatorName = this.qualifiedOperatorName(objWithArgs.objname);

// Remove quotes from operator name if present
if (operatorName.startsWith('"') && operatorName.endsWith('"')) {
Expand Down Expand Up @@ -8722,8 +8733,11 @@ export class Deparser implements DeparserVisitor {
output.push('DATABASE');
}
if (node.objects && node.objects.length > 0) {
const qualifiedKinds = ['OBJECT_TYPE', 'OBJECT_DOMAIN'];
const objects = ListUtils.unwrapList(node.objects)
.map(obj => this.visit(obj, context))
.map(obj => qualifiedKinds.includes(node.objtype) && (obj as any).List
? ListUtils.unwrapList((obj as any).List.items).map((item: any) => this.visit(item, context)).join('.')
: this.visit(obj, context))
.join(', ');
output.push(objects);
}
Expand Down Expand Up @@ -8762,8 +8776,11 @@ export class Deparser implements DeparserVisitor {
break;
default:
if (node.objects && node.objects.length > 0) {
const qualifiedKinds = ['OBJECT_TYPE', 'OBJECT_DOMAIN'];
const objects = ListUtils.unwrapList(node.objects)
.map(obj => this.visit(obj, context))
.map(obj => qualifiedKinds.includes(node.objtype) && (obj as any).List
? ListUtils.unwrapList((obj as any).List.items).map((item: any) => this.visit(item, context)).join('.')
: this.visit(obj, context))
.join(', ');
output.push(objects);
}
Expand Down Expand Up @@ -10128,6 +10145,9 @@ export class Deparser implements DeparserVisitor {
if (listItems.length === 1 && listItems[0].String) {
return `${preservedDefName} = ${listItems[0].String.sval}`;
}
if (listItems.length > 1 && listItems.every(item => item.String)) {
return `${preservedDefName} = OPERATOR(${this.qualifiedOperatorName(listItems)})`;
}
Comment on lines +10148 to +10150

@tenki-reviewer tenki-reviewer Bot Nov 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 bug · medium

Qualified OPERATOR() commutator support is incomplete

The new multi-item OPERATOR(schema.op) emission for COMMUTATOR/NEGATOR was added only to the DefineStmt OBJECT_OPERATOR inline handler (packages/deparser/src/deparser.ts:10142). Sibling handlers were not extended, so ALTER OPERATOR fx.### (int, int) SET (COMMUTATOR = OPERATOR(fx.!!!)) round-trips to the invalid SQL commutator = 'fx, !!!' (verified by running the deparser). Users deparsing qualified commutator changes get non-reparseable output.

📋 Prompt for AI Agents

In packages/deparser/src/deparser.ts, the AlterOperatorStmt branch of the DefElem handler (around line 6045) and the DefineStmt-context branches in DefElem (~line 5941) and formatOptionValue (~line 6421) only handle single-item commutator/negator lists; a schema-qualified list such as OPERATOR(fx.!!!) falls through and renders as the invalid SQL commutator = 'fx, !!!'. Extend those branches to emit OPERATOR(<qualified name>) for multi-item all-String lists, mirroring the handling added in the DefineStmt OBJECT_OPERATOR inline path at lines 10142-10144. Verified reproduction: ALTER OPERATOR fx.### (int, int) SET (COMMUTATOR = OPERATOR(fx.!!!));.

}
// For commutator/negator, we already handled them above
if ((defName.toLowerCase() === 'commutator' || defName.toLowerCase() === 'negator')) {
Expand Down
Loading