Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
154 changes: 140 additions & 14 deletions graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,32 @@ import {
EXPORTABLE_OBJECT_CLONE,
gatherConfig,
} from "graphile-build";
import type { PgProc, PgProcArgument } from "pg-introspection";
import type { PgProc, PgProcArgument, PgType } from "pg-introspection";

import { exportNameHint, forbidRequired } from "../utils.ts";
import { version } from "../version.ts";

/**
* Given a type OID, walk through domain chains and return the type if
* it resolves to a composite type (typtype 'c' with a typrelid), or
* null otherwise.
*/
function resolveToCompositeType(
typeOid: string,
types: ReadonlyArray<PgType>,
): PgType | null {
let type = types.find((t) => t._id === typeOid);
// Follow domain chains (typtype 'd') to reach the underlying type
while (type && type.typtype === "d" && type.typbasetype) {
type = types.find((t) => t._id === type!.typbasetype!);
}
// Return only if it resolved to a composite (table) type
if (type && type.typtype === "c" && type.typrelid) {
return type;
}
return null;
}

declare global {
namespace GraphileBuild {
interface BehaviorStrings {
Expand Down Expand Up @@ -214,10 +235,68 @@ export const PgProceduresPlugin: GraphileConfig.Plugin = {
const executor =
info.helpers.pgIntrospection.getExecutorForService(serviceName);

const name = info.inflection.functionResourceName({
let name = info.inflection.functionResourceName({
serviceName,
pgProc,
});

// When overloaded functions target distinct composite types, each
// is allowed through the overload check in pgIntrospection_proc.
// However, the resource registry keys by name (see datasource.ts),
// so we must ensure each resource has a unique name. We suffix with
// the target table's schema and name, for example `code(a.pets)`
// becomes `code_a_pets`.
// GraphQL field names are unaffected since computedAttributeField
// inflection uses resource.extensions.pg.name (the raw SQL name).
{
const introspection = (
await info.helpers.pgIntrospection.getIntrospection()
).find((n) => n.pgService.name === serviceName)!.introspection;
// Check if another proc in the same namespace shares this name
let hasOverload = false;
for (const p of introspection.procs) {
if (
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id
) {
hasOverload = true;
break;
}
}
if (hasOverload) {
// Resolve the first argument to its target table type
const firstArgType = pgProc.proargtypes?.[0];
if (firstArgType) {
const composite = resolveToCompositeType(
firstArgType,
introspection.types,
);
if (composite) {
// Look up the pg_class and pg_namespace for the target
// table so we can build the suffix
let className: string | undefined;
let schemaName: string | undefined;
for (const c of introspection.classes) {
if (c._id === composite.typrelid) {
className = c.relname;
for (const n of introspection.namespaces) {
if (n._id === c.relnamespace) {
schemaName = n.nspname;
break;
}
}
break;
}
}
if (className && schemaName) {
name = `${name}_${schemaName}_${className}`;
}
}
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This logic should be moved to the functionResourceName inflector; otherwise other parts of the code that use the inflector to determine the name of the function (e.g. so they can read it from the pgRegistry) will not get the same name and might extract the wrong function.

Further, renaming a function when it gets an overload makes me uncomfortable... But I think we'll allow it because the alternatives seem worse.

Though having said this... the inflector shouldn't need to look through the rest of the registry to determine what something is called... so... Yeah this is a non-trivial issue.

If we're doing opt-in for this feature, I think just renaming all procedures so they're named by their signature (${fn.name}_${params.map(param => param.type.name).join('_')}) might be the best bet. Or at least all functions that look like computed columns.

Actually we already have a naming convention that encourages ${tableName}_${fieldName} as a name - so I think we can fix this by just enforcing that:

  • If this is a "computed column function" (stable/immutable, first argument is composite, belongs to same schema as composite type), then:
    • If the function name starts with the composite type name followed by underscore, use current logic;
    • Otherwise, pretend the function was named such that it started with the composite type name followed by an underscore, and the follow current logic.

If we follow this, the vast majority of PostGraphile users won't be impacted at all, and only functions that don't follow our established naming conventions will be renamed, and they'll be renamed whether or not they are overloads which means adding an overload later won't break/rename them.

Yes, this feels much better.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

belongs to same schema as composite type

Although for our use-case we actually place all computed columns in a schema specifically for that purpose. This creates nice explicit SQL, e.g.:

SELECT id, computed.code(orders.*) FROM sales.orders;

This works fine already, except for computed columns that are overloaded. e.g. if we would add a computed.code(orderlines.*), then suddenly the computed.code(orders.*) computed column disappears.

So I would prefer to have cross-schema computed columns remain possible with this approach as well, I'm working on a diff for this PR right now :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see this would mean existing functions that take a compound argument will change name.. That's not very nice as it's a breaking change. On the other hand the current behavior where computed.code(orders.*) works fine as long as there is no second computed.code(?) is not very nice either..

Should we accept the breaking change in favor of consistent behavior? Or perhaps be looking at a different solution/direction?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We cannot accept a breaking change at this point; but it can be handled through a plugin/preset.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah ofcourse, good idea!

I have reverted the existing tests to their original state so we don't have breaking changes any more.
Ive extracted a small part of the functionResourceName into an inflector which can be overridden by plugin/preset. Does it make sense to do it like this? Or should we move the whole functionResourceName to be plugin-overridable?

Do you want me to add a global plugin/preset that's available to the user or just leave that implementation to the user.

I did add a warning log when a computed column is thrown out when it is overloaded. I think it is warranted since, from the user's perspective, its confusing that a field on resource A suddenly disappears when an overload for resource B is added.


const identifier = `${serviceName}.${namespace.nspname}.${
pgProc.proname
}(${pgProc.getArguments().map(argTypeName).join(",")})`;
Expand Down Expand Up @@ -648,18 +727,65 @@ 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.
const overload = introspection.procs.find(
(p) =>
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id,
);
if (overload) {
return;
// We don’t want procedures that have been defined in our namespace
// twice, as this leads to duplicate fields in the API. However, we
// allow overloads where each targets a distinct composite type as its
// first argument (i.e. computed columns on different tables), since
// each will be exposed on a different GraphQL type.
{
let hasOverload = false;
for (const p of introspection.procs) {
if (
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue we're really trying to avoid here is when the multiple (overloaded) functions end up with the same name in inflection. Maybe we should encode that specifically as the rule and skip the remaining complexity?

Suggested change
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id &&
// Check if name conflicts:
info.inflection.functionResourceName({
serviceName,
pgProc: p,
}) === name

) {
hasOverload = true;
break;
}
}
if (hasOverload) {
// This proc has overloads — check if it qualifies as a computed
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems unnecessary to have rewritten this? Does it need a naked block? In general I'm not a fan of let either, const should be preferred where possible, and in this case the performance cost of find vs for isn't a major concern.

// column on a unique composite type.
const thisFirstArgType = pgProc.proargtypes?.[0];
if (!thisFirstArgType) {
// No first arg — not a computed column candidate
return;
}
const thisComposite = resolveToCompositeType(
thisFirstArgType,
introspection.types,
);
if (!thisComposite) {
// First arg doesn’t resolve to a composite type
return;
}
// Check that no other overload targets the same composite type;
// if two overloads both target, they'd produce duplicate fields on
// the same GraphQL type so we must skip.
for (const p of introspection.procs) {
if (
p.pronamespace === pgProc.pronamespace &&
p.proname === pgProc.proname &&
p._id !== pgProc._id
) {
const otherFirstArgType = p.proargtypes?.[0];
if (!otherFirstArgType) continue;
const otherComposite = resolveToCompositeType(
otherFirstArgType,
introspection.types,
);
if (
otherComposite &&
otherComposite._id === thisComposite._id
) {
// Another overload targets the same composite type — skip
return;
}
}
}
// This proc targets a unique composite type — allow it through
}
}

await helpers.pgProcedures.getResourceOptions(serviceName, pgProc);
Expand Down
28 changes: 19 additions & 9 deletions postgraphile/postgraphile/__tests__/kitchen-sink-schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -2474,7 +2474,7 @@ create function function_returning_enum.applicants_next_stage(
a function_returning_enum.applicants
) returns function_returning_enum.stage_options_enum_domain
as $$
select (case when a.stage = 'round 2' then 'hired'
select (case when a.stage = 'round 2' then 'hired'
else 'rejected' end)::function_returning_enum.stage_options_enum_domain;
$$ language sql stable;
comment on function function_returning_enum.applicants_next_stage is E'@filterable';
Expand All @@ -2494,7 +2494,7 @@ create function function_returning_enum.text_length(
min_length int
) returns function_returning_enum.length
as $$
select (case when length(text) < min_length then 'too_short'
select (case when length(text) < min_length then 'too_short'
else 'ok' end)::function_returning_enum.length;
$$ language sql stable;

Expand All @@ -2509,23 +2509,23 @@ comment on function function_returning_enum.applicants_name_length is E'@filtera
create function function_returning_enum.applicants_by_stage(
wanted_stage function_returning_enum.stage_options_enum_domain
) returns setof function_returning_enum.applicants
as $$
as $$
select * from function_returning_enum.applicants a where a.stage = wanted_stage;
$$ language sql stable;

create function function_returning_enum.applicants_by_favorite_pet(
pet function_returning_enum.animal_type
) returns setof function_returning_enum.applicants
as $$
as $$
select * from function_returning_enum.applicants a where a.favorite_pet = pet;
$$ language sql stable;

create function function_returning_enum.applicants_pet_food(
a function_returning_enum.applicants
) returns function_returning_enum.animal_type
as $$
select (case
when a.favorite_pet = 'FISH' then null
select (case
when a.favorite_pet = 'FISH' then null
when a.favorite_pet = 'CAT' then 'FISH'
when a.favorite_pet = 'DOG' then 'CAT'
else null
Expand All @@ -2539,16 +2539,16 @@ comment on domain function_returning_enum.transportation is E'@enum enum_table_t
create function function_returning_enum.applicants_by_transportation(
transportation function_returning_enum.transportation
) returns setof function_returning_enum.applicants
as $$
as $$
select * from function_returning_enum.applicants a where a.transportation = applicants_by_transportation.transportation;
$$ language sql stable;

create function function_returning_enum.applicants_favorite_pet_transportation(
a function_returning_enum.applicants
) returns function_returning_enum.transportation
as $$
select (case
when a.favorite_pet = 'FISH' then 'SUBWAY'
select (case
when a.favorite_pet = 'FISH' then 'SUBWAY'
when a.favorite_pet = 'CAT' then 'CAR'
when a.favorite_pet = 'DOG' then 'BIKE'
else null
Expand All @@ -2574,6 +2574,16 @@ create table cjk."期间" (

--------------------------------------------------------------------------------

-- Test overloaded computed column functions targeting different tables.
create table a.pets (id serial primary key, name text);
create table a.buildings (id serial primary key, address text);
create function a.code(a.pets) returns text as $$ select 'P' || $1.id::text; $$ language sql stable;
create function a.code(a.buildings) returns text as $$ select 'B' || $1.id::text; $$ language sql stable;
comment on function a.code(a.pets) is E'@behavior +typeField';
comment on function a.code(a.buildings) is E'@behavior +typeField';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The reason the snapshots have changed so much is you've added stuff to one of our most used schemas (a); please instead create a new schema ("function_overloads") and create these elements in there.

Also, you may need to change the behavior to +typeField -queryField - I can't remember. Check in the generated schema for unexpected changes (e.g. this function showing up twice or causing a clash).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The __tests__/schema/... is where you'd add a schema test for this. You can use the defaultOptions test as a template, just change the schema name.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed, thanks for the help!


--------------------------------------------------------------------------------

-- SCIFI: Snake Case Is For Initiates.
-- All the cool kids use UpperCamelCase
create schema scifi;
Expand Down
Loading