Skip to content

Commit

Permalink
fix(connection): don't query identifiers unless necessary (#429)
Browse files Browse the repository at this point in the history 
Connections can be used for aggregates such as `totalCount`. If no nodes/cursors are requested then pulling down identifiers is unnecessary and leads to slower `totalCount` and unnecessary live fields.

This PR stops connection planners from pulling down identifiers and connection resolvers from registering live records, instead leaving that to the parts of the schema (`nodes` / `edges[].node` / `edges[].cursor`) that do require that functionality.

Fixes #428
  • Loading branch information
@benjie
benjie committed Mar 25, 2019
1 parent 306defc commit 16785c4
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 53 deletions.
9 changes: 7 additions & 2 deletions packages/graphile-build-pg/src/plugins/PgAllRows.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export default (async function PgAllRows(
);
}
if (primaryKeys) {
if (subscriptions) {
if (subscriptions && !isConnection) {
queryBuilder.selectIdentifiers(table);
}
queryBuilder.beforeLock("orderBy", () => {
Expand Down Expand Up @@ -179,7 +179,12 @@ export default (async function PgAllRows(
} = result;
return addStartEndCursor(row);
} else {
if (primaryKeys && resolveContext.liveRecord) {
if (
subscriptions &&
!isConnection &&
primaryKeys &&
resolveContext.liveRecord
) {
result.rows.forEach(
row =>
row &&
Expand Down
3 changes: 2 additions & 1 deletion packages/graphile-build-pg/src/plugins/PgBackwardRelationPlugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ export default (function PgBackwardRelationPlugin(
if (primaryKeys) {
if (
subscriptions &&
!isConnection &&
table.primaryKeyConstraint
) {
innerQueryBuilder.selectIdentifiers(
Expand Down Expand Up @@ -388,7 +389,7 @@ export default (function PgBackwardRelationPlugin(
return addStartEndCursor(data[safeAlias]);
} else {
const records = data[safeAlias];
if (resolveContext.liveRecord) {
if (subscriptions && resolveContext.liveRecord) {
records.forEach(
r =>
r &&
Expand Down
20 changes: 17 additions & 3 deletions packages/graphile-build-pg/src/plugins/PgTablesPlugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const hasNonNullKey = row => {

export default (function PgTablesPlugin(
builder,
{ pgForbidSetofFunctionsToReturnNull = false }
{ pgForbidSetofFunctionsToReturnNull = false, subscriptions = false }
) {
const handleNullRow = pgForbidSetofFunctionsToReturnNull
? (row, _identifiers) => row
Expand Down Expand Up @@ -378,7 +378,14 @@ export default (function PgTablesPlugin(
},
},
{},
false
false,
{
withQueryBuilder: queryBuilder => {
if (subscriptions) {
queryBuilder.selectIdentifiers(table);
}
},
}
),
};
},
Expand Down Expand Up @@ -450,7 +457,14 @@ export default (function PgTablesPlugin(
},
},
{},
false
false,
{
withQueryBuilder: queryBuilder => {
if (subscriptions) {
queryBuilder.selectIdentifiers(table);
}
},
}
),
edges: pgField(
build,
Expand Down
111 changes: 65 additions & 46 deletions packages/graphile-build-pg/src/plugins/makeProcField.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ export default function makeProcField(
parsedResolveInfoFragment,
ReturnType
);
const isConnection = !forceList && !isMutation && proc.returnsSet;
const query = queryFromResolveData(
sqlMutationQuery,
functionAlias,
Expand All @@ -363,9 +364,8 @@ export default function makeProcField(
(isTableLike || isRecordLike) &&
(forceList || proc.returnsSet || rawReturnType.isPgArray) && // only bother with lists
proc.language !== "sql", // sql functions can be inlined, so GRANTs still apply
withPagination: !forceList && !isMutation && proc.returnsSet,
withPaginationAsFields:
!forceList && !isMutation && proc.returnsSet && !computed,
withPagination: isConnection,
withPaginationAsFields: isConnection && !computed,
asJson:
computed &&
(forceList || (!proc.returnsSet && !returnFirstValueAsValue)),
Expand Down Expand Up @@ -404,6 +404,7 @@ export default function makeProcField(
} else if (
subscriptions &&
returnTypeTable &&
!isConnection &&
returnTypeTable.primaryKeyConstraint
) {
innerQueryBuilder.selectIdentifiers(returnTypeTable);
Expand Down Expand Up @@ -576,7 +577,8 @@ export default function makeProcField(
type: nullableIf(GraphQLNonNull, !proc.tags.notNull, ReturnType),
args: args,
resolve: computed
? (data, _args, _context, resolveInfo) => {
? (data, _args, resolveContext, resolveInfo) => {
const { liveRecord } = resolveContext;
const safeAlias = getSafeAliasFromResolveInfo(resolveInfo);
const value = data[safeAlias];
if (returnFirstValueAsValue) {
Expand All @@ -591,14 +593,36 @@ export default function makeProcField(
return pg2gql(value, returnType);
}
} else {
const makeRecordLive =
subscriptions && isTableLike && returnTypeTable && liveRecord
? record => {
if (record) {
liveRecord(
"pg",
returnTypeTable,
record.__identifiers
);
}
}
: _record => {};
if (proc.returnsSet && !isMutation && !forceList) {
// Connection - do not make live (the connection will handle this)
return addStartEndCursor({
...value,
data: value.data ? value.data.map(scalarAwarePg2gql) : null,
});
} else if (proc.returnsSet || rawReturnType.isPgArray) {
return value.map(v => pg2gql(v, returnType));
// List
const records = value.map(v => {
makeRecordLive(v);
return pg2gql(v, returnType);
});
return records;
} else {
// Object
if (value) {
makeRecordLive(value);
}
return pg2gql(value, returnType);
}
}
Expand Down Expand Up @@ -681,64 +705,59 @@ export default function makeProcField(
const rows = queryResultRows;
const [row] = rows;
const result = (() => {
const makeRecordLive =
subscriptions && isTableLike && returnTypeTable && liveRecord
? record => {
if (record) {
liveRecord(
"pg",
returnTypeTable,
record.__identifiers
);
}
}
: _record => {};
if (returnFirstValueAsValue) {
// `returnFirstValueAsValue` implies either `isMutation` is
// true, or `ConnectionType` does not exist - either way,
// we're not returning a connection.
if (proc.returnsSet && !isMutation && !forceList) {
// EITHER `isMutation` is true, or `ConnectionType` does
// not exist - either way, we're not returning a
// connection.
return row.data.map(v => pg2gql(firstValue(v), returnType));
return row.data.map(v => {
const fv = firstValue(v);
makeRecordLive(fv);
return pg2gql(fv, returnType);
});
} else if (proc.returnsSet || rawReturnType.isPgArray) {
return rows.map(v => pg2gql(firstValue(v), returnType));
return rows.map(v => {
const fv = firstValue(v);
makeRecordLive(fv);
return pg2gql(fv, returnType);
});
} else {
return pg2gql(firstValue(row), returnType);
const fv = firstValue(row);
makeRecordLive(fv);
const record = pg2gql(fv, returnType);
return record;
}
} else {
if (proc.returnsSet && !isMutation && !forceList) {
// Connection
const data = row.data
? row.data.map(scalarAwarePg2gql)
: null;
if (
subscriptions &&
isTableLike &&
data &&
returnTypeTable &&
liveRecord
) {
data.forEach(
row =>
row &&
liveRecord("pg", returnTypeTable, row.__identifiers)
);
}
return addStartEndCursor({
...row,
data,
});
} else if (proc.returnsSet || rawReturnType.isPgArray) {
if (
subscriptions &&
isTableLike &&
returnTypeTable &&
liveRecord
) {
rows.forEach(
row =>
row &&
liveRecord("pg", returnTypeTable, row.__identifiers)
);
}
return rows.map(row => pg2gql(row, returnType));
// List
return rows.map(row => {
makeRecordLive(row);
return pg2gql(row, returnType);
});
} else {
if (
subscriptions &&
isTableLike &&
row &&
returnTypeTable &&
liveRecord
) {
liveRecord("pg", returnTypeTable, row.__identifiers);
}
// Object
makeRecordLive(row);
return pg2gql(row, returnType);
}
}
Expand Down
18 changes: 17 additions & 1 deletion packages/graphile-utils/src/fieldHelpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@ export function makeFieldHelpers<TSource>(
withPaginationAsFields: isConnection,
},
(sqlBuilder: QueryBuilder) => {
if (primaryKeys && build.options.subscriptions && table) {
if (
!isConnection &&
primaryKeys &&
build.options.subscriptions &&
table
) {
sqlBuilder.selectIdentifiers(table);
}

Expand All @@ -68,6 +73,17 @@ export function makeFieldHelpers<TSource>(
if (isConnection) {
return build.pgAddStartEndCursor(rows[0]);
} else {
if (
build.options.subscriptions &&
!isConnection &&
primaryKeys &&
context.liveRecord
) {
rows.forEach(
(row: any) =>
row && context.liveRecord("pg", table, row.__identifiers)
);
}
return rows;
}
};
Expand Down

0 comments on commit 16785c4

Please sign in to comment.