"Expected SQL item" bug when using edge field with order by computed column on mutation payload

[benjie]

Summary

The edge field on mutation payloads throws an error when you attempt to use it with an orderBy that includes a computed column (with @sortable smart tag).

Steps to reproduce

See: https://github.com/pszafer/postgraphile_errors/tree/delete_error

Expected results

No error, correct edge/cursor returned.

Actual results

"Expected SQL item, instead received '({\n        queryBuilder\n      }) => sql.fragment`(${sql.identifier(proc.namespaceName, proc.name)}(${queryBuilder.getTableAlias()}))`'.",

Additional context

PostGraphile v4.11.0.

Possible Solution

The error lies here:

graphile-engine/packages/graphile-build-pg/src/plugins/PgMutationPayloadEdgePlugin.js

Lines 170 to 174 in b715f6d

	const expr = isString(col)
	? sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier(
	col
	)}`
	: col;

Specifically we've assumed if it's not a string then it's an SQL node, but that's not the case - it can also be a thunk.

Normally when ordering we call queryBuilder.orderBy with a value like the above:

graphile-engine/packages/graphile-build-pg/src/plugins/PgConnectionArgOrderBy.js

Line 165 in 1bc8cfe

queryBuilder.orderBy(expr, ascending, nullsFirst);

QueryBuilder automatically calls thunks when generating the compiled data:

graphile-engine/packages/graphile-build-pg/src/QueryBuilder.js

Lines 870 to 875 in 1bc8cfe

	} else if (type === "orderBy") {
	this.compiledData[type] = this.data[type].map(([a, b, c]) => [
	callIfNecessary(a, context),
	b,
	c,
	]);

which we've not done in the edge plugin. This is done because it passes a reference to the context:

graphile-engine/packages/graphile-build-pg/src/QueryBuilder.js

Lines 138 to 140 in 1bc8cfe

	lockContext: {
	queryBuilder: QueryBuilder,
	};

which includes the QueryBuilder so that orders may use things like the query builder's alias whilst building their fragments.

The PgMutationPayloadEdgePlugin does have access to a QueryBuilder so we should be able to fix this by changing the code above to something along the lines of:

const expr = isString(col)
  ? sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier(col)}`
  : typeof col === 'function'
  ? col({ queryBuilder })
  : col;

[benjie]

Oh, this was reported by @pszafer via Discord: https://discord.com/channels/489127045289476126/498852330754801666/842725956841766942

[benjie]

Fixed in #741