-
-
Notifications
You must be signed in to change notification settings - Fork 620
Support overloaded computed column functions #2997
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
8e3ef40
4a8e6b7
090c885
e8197a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| "graphile-build-pg": patch | ||
| "postgraphile": patch | ||
| --- | ||
|
|
||
| Support overloaded computed column functions targeting different composite | ||
| types. Computed column functions that don't follow the | ||
| `${tableName}_${fieldName}` naming convention now have their resource name | ||
| prefixed with the composite type name (e.g. `code(pets)` and `code(buildings)` | ||
| become `pets_code` and `buildings_code`), so overloads no longer clash. The new | ||
| `functionResourceNameCompositeTypePrefix` inflector controls this prefix; by | ||
| default it only applies to functions in the same schema as their composite type; | ||
| override it to support cross-schema computed columns. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,23 @@ declare global { | |||||
| pgProc: PgProc; | ||||||
| }, | ||||||
| ): string; | ||||||
| /** | ||||||
| * The prefix (if any) to add to the resource name of a "computed | ||||||
| * column function" (a stable/immutable function whose first argument | ||||||
| * is a composite type) such that it follows the | ||||||
| * `${tableName}_${fieldName}` naming convention. Returns the empty | ||||||
| * string when no prefix should be added, e.g. when the function name | ||||||
| * already starts with the composite type's name. By default only | ||||||
| * functions in the same schema as their composite type are prefixed; | ||||||
| * override this inflector to support cross-schema computed columns. | ||||||
| */ | ||||||
| functionResourceNameCompositeTypePrefix( | ||||||
| this: Inflection, | ||||||
| details: { | ||||||
| serviceName: string; | ||||||
| pgProc: PgProc; | ||||||
| }, | ||||||
| ): string; | ||||||
| functionRecordReturnCodecName( | ||||||
| this: Inflection, | ||||||
| details: { | ||||||
|
|
@@ -123,14 +140,38 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = { | |||||
|
|
||||||
| inflection: { | ||||||
| add: { | ||||||
| functionResourceName(options, { serviceName, pgProc }) { | ||||||
| functionResourceName(options, details) { | ||||||
| const { serviceName, pgProc } = details; | ||||||
| const { tags } = pgProc.getTagsAndDescription(); | ||||||
| if (typeof tags.name === "string") { | ||||||
| return tags.name; | ||||||
| } | ||||||
| const pgNamespace = pgProc.getNamespace()!; | ||||||
| const schemaPrefix = this._schemaPrefix({ serviceName, pgNamespace }); | ||||||
| return `${schemaPrefix}${pgProc.proname}`; | ||||||
| const computedPrefix = | ||||||
| this.functionResourceNameCompositeTypePrefix(details); | ||||||
| return `${schemaPrefix}${computedPrefix}${pgProc.proname}`; | ||||||
| }, | ||||||
| functionResourceNameCompositeTypePrefix(_options, { pgProc }) { | ||||||
| // Computed column functions are conventionally named | ||||||
| // `${tableName}_${fieldName}`; enforcing this convention ensures | ||||||
| // overloaded functions like code(a.pets) and code(a.buildings) | ||||||
| // produce distinct resource names (pets_code, buildings_code). | ||||||
| if (pgProc.provolatile === "v") { | ||||||
| return ""; | ||||||
| } | ||||||
| const firstArg = pgProc.getArguments().find((a) => a.isIn); | ||||||
| if (!firstArg || firstArg.type.typtype !== "c") { | ||||||
| return ""; | ||||||
| } | ||||||
| // By default, only consider a function as a computed column if it | ||||||
| // belongs to the same schema as the composite type. Override this | ||||||
| // inflector to support cross-schema computed columns. | ||||||
| if (firstArg.type.typnamespace !== pgProc.pronamespace) { | ||||||
| return ""; | ||||||
| } | ||||||
| const prefix = firstArg.type.typname + "_"; | ||||||
| return pgProc.proname.startsWith(prefix) ? "" : prefix; | ||||||
| }, | ||||||
| functionRecordReturnCodecName(options, details) { | ||||||
| return this.upperCamelCase( | ||||||
|
|
@@ -606,7 +647,10 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = { | |||||
| resourceOptionsByPgProcByService: new Map(), | ||||||
| }), | ||||||
| hooks: { | ||||||
| async pgIntrospection_proc({ helpers, resolvedPreset }, event) { | ||||||
| async pgIntrospection_proc( | ||||||
| { helpers, resolvedPreset, inflection }, | ||||||
| event, | ||||||
| ) { | ||||||
| const { entity: pgProc, serviceName } = event; | ||||||
|
|
||||||
| const pgService = resolvedPreset.pgServices?.find( | ||||||
|
|
@@ -648,17 +692,39 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = { | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| // We also don’t want procedures that have been defined in our namespace | ||||||
| // twice. This leads to duplicate fields in the API which throws an | ||||||
| // error. In the future we may support this case. For now though, it is | ||||||
| // too complex. | ||||||
| // We don’t want procedures whose inflected resource name clashes | ||||||
| // with another overload — this would produce duplicate fields. | ||||||
| // Overloads targeting distinct composite types get unique names | ||||||
| // from the inflector (e.g. pets_code vs buildings_code). | ||||||
| const overload = introspection.procs.find( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably rename this to |
||||||
| (p) => | ||||||
| p.pronamespace === pgProc.pronamespace && | ||||||
| p.proname === pgProc.proname && | ||||||
| p._id !== pgProc._id, | ||||||
| p._id !== pgProc._id && | ||||||
| inflection.functionResourceName({ | ||||||
| serviceName, | ||||||
| pgProc: p, | ||||||
| }) === | ||||||
| inflection.functionResourceName({ | ||||||
| serviceName, | ||||||
| pgProc, | ||||||
| }), | ||||||
|
Comment on lines
+708
to
+711
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need to be recomputed each time, let's hoist it up into a |
||||||
| ); | ||||||
| if (overload) { | ||||||
| // Warn if both functions target composite types — the user likely | ||||||
| // intended these as computed columns on different tables, but the | ||||||
| // inflector produced the same name (e.g. cross-schema overloads). | ||||||
| const thisFirstArg = pgProc.getArguments().find((a) => a.isIn); | ||||||
| const otherFirstArg = overload.getArguments().find((a) => a.isIn); | ||||||
| if ( | ||||||
| thisFirstArg?.type.typtype === "c" && | ||||||
| otherFirstArg?.type.typtype === "c" && | ||||||
| thisFirstArg.type._id !== otherFirstArg.type._id | ||||||
| ) { | ||||||
| console.warn( | ||||||
| `Skipping function '${namespace!.nspname}.${pgProc.proname}' because its resource name clashes with an overload. Consider overriding the 'functionResourceNameCompositeTypePrefix' inflector to support cross-schema computed columns.`, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| -- Test overloaded computed column functions targeting different tables | ||
| drop schema if exists function_overloads, function_overloads_other_schema cascade; | ||
|
|
||
| create schema function_overloads; | ||
| create schema function_overloads_other_schema; | ||
| create table function_overloads.pets (id serial primary key, name text); | ||
| create table function_overloads.buildings (id serial primary key, address text); | ||
|
|
||
| -- Two overloaded functions in the SAME schema as their target tables | ||
| create function function_overloads.code(function_overloads.pets) returns text | ||
| as $$ select 'P' || $1.id::text; $$ language sql stable; | ||
| create function function_overloads.code(function_overloads.buildings) returns text | ||
| as $$ select 'B' || $1.id::text; $$ language sql stable; | ||
| comment on function function_overloads.code(function_overloads.pets) | ||
| is E'@behavior +typeField -queryField'; | ||
| comment on function function_overloads.code(function_overloads.buildings) | ||
| is E'@behavior +typeField -queryField'; | ||
|
|
||
| -- Cross-schema computed column functions (different schema from target tables) | ||
| create function function_overloads_other_schema.age(function_overloads.pets) returns int | ||
| as $$ select 42; $$ language sql stable; | ||
| create function function_overloads_other_schema.age(function_overloads.buildings) returns int | ||
| as $$ select 99; $$ language sql stable; | ||
| comment on function function_overloads_other_schema.age(function_overloads.pets) | ||
| is E'@behavior +typeField -queryField'; | ||
| comment on function function_overloads_other_schema.age(function_overloads.buildings) | ||
| is E'@behavior +typeField -queryField'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not keen that we mix "will this be a computed column" logic into the inflector for a resource. Whether it's a computed column or not is the result of behaviors, and inflection doesn't have access to that.
Might a simpler solution be to offer an extra preset that overrides
functionResourceNameand appends the types of the input arguments, unilaterally, enabling overloads throughout? This'll then have knock-on consequences that'll likely have to be dealt with, but pushing conflicts from the pgRegistry down into schema build time seems more sensible - the registry should be able to represent everything in the DB, even if the schema can't make use of it.