Skip to content
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

Add distinctOn to QueryBuilder #805

Closed
wants to merge 14 commits into from
Closed
1 change: 1 addition & 0 deletions packages/graphile-build-pg/src/QueryBuilder.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export default class QueryBuilder {
public offset(offsetGen: NumberGen): void;
public first(first: number): void;
public last(last: number): void;
public distinctOn(exprGen: SQLGen): void;

// ----------------------------------------

Expand Down
38 changes: 32 additions & 6 deletions packages/graphile-build-pg/src/QueryBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as sql from "pg-sql2";
import type { SQL } from "pg-sql2";
import isSafeInteger from "lodash/isSafeInteger";
import chunk from "lodash/chunk";
import isEqual from "lodash/isEqual";
import type { PgClass, PgType } from "./plugins/PgIntrospectionPlugin";

// eslint-disable-next-line flowtype/no-weak-types
Expand Down Expand Up @@ -106,6 +107,7 @@ class QueryBuilder {
offset: ?NumberGen,
first: ?number,
last: ?number,
distinctOn: Array<SQLGen>,
beforeLock: {
[string]: Array<() => void> | null,
},
Expand Down Expand Up @@ -133,6 +135,7 @@ class QueryBuilder {
offset: ?number,
first: ?number,
last: ?number,
distinctOn: Array<SQLGen>,
cursorComparator: ?CursorComparator,
};
lockContext: {
Expand Down Expand Up @@ -170,6 +173,7 @@ class QueryBuilder {
last: false,
limit: false,
offset: false,
distinctOn: false,
};
this.finalized = false;
this.selectedIdentifiers = false;
Expand All @@ -192,6 +196,7 @@ class QueryBuilder {
offset: null,
first: null,
last: null,
distinctOn: [],
benjie marked this conversation as resolved.
Show resolved Hide resolved
beforeLock: {
// As a performance optimisation, we're going to list a number of lock
// types so that V8 doesn't need to mutate the object too much
Expand Down Expand Up @@ -231,6 +236,7 @@ class QueryBuilder {
offset: null,
first: null,
last: null,
distinctOn: [],
cursorComparator: null,
};
this._children = new Map();
Expand Down Expand Up @@ -511,6 +517,10 @@ ${sql.join(
}
this.data.last = last;
}
distinctOn(exprGen: SQLGen) {
this.checkLock("distinctOn");
this.data.distinctOn.push(exprGen);
}

// ----------------------------------------

Expand Down Expand Up @@ -744,7 +754,15 @@ ${sql.join(
: this.buildSelectFields();

let fragment = sql.fragment`\
select ${useAsterisk ? sql.fragment`${this.getTableAlias()}.*` : fields}
select ${
this.compiledData.distinctOn.length
? sql.fragment`distinct on (${sql.join(
this.compiledData.distinctOn,
", "
)})`
: sql.blank
}
${useAsterisk ? sql.fragment`${this.getTableAlias()}.*` : fields}
${
this.compiledData.from &&
sql.fragment`from ${this.compiledData.from[0]} as ${this.getTableAlias()}`
Expand Down Expand Up @@ -868,11 +886,16 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor
this.locks[type] = isDev ? new Error("Initally locked here").stack : true;
this.compiledData[type] = data;
} else if (type === "orderBy") {
this.compiledData[type] = this.data[type].map(([a, b, c]) => [
callIfNecessary(a, context),
b,
c,
]);
// Add distinct to existing order by to avoid `SELECT DISTINCT ON expressions must match initial ORDER BY expressions`
// Convert distinct fields to order by applying ascending and using existing order by for matching fields
const distinctAsOrderBy = this.data["distinctOn"].map(
d => this.data[type].find(([a]) => isEqual(a, d)) ?? [d, true, null]
stwiname marked this conversation as resolved.
Show resolved Hide resolved
);

// Create a union of the distinct and order by fields
this.compiledData[type] = [
...new Set([...distinctAsOrderBy, ...this.data[type]]),
].map(([a, b, c]) => [callIfNecessary(a, context), b, c]);
stwiname marked this conversation as resolved.
Show resolved Hide resolved
} else if (type === "from") {
if (this.data.from) {
const f = this.data.from;
Expand All @@ -894,6 +917,8 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor
this.compiledData[type] = this.data[type];
} else if (type === "last") {
this.compiledData[type] = this.data[type];
} else if (type === "distinctOn") {
this.compiledData[type] = callIfNecessaryArray(this.data[type], context);
} else {
throw new Error(`Wasn't expecting to lock '${type}'`);
}
Expand Down Expand Up @@ -925,6 +950,7 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor
this.lock("limit");
this.lock("first");
this.lock("last");
this.lock("distinctOn");
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that because distinctOn changes the order, it may break connections (specifically cursors) in some cases. This is one of the reasons that we lock orderBy when we do. E.g. if you order by [USERNAME_ASC, NAME_DESC, ID_ASC] and then distinctOn: [NAME] then the order would be changed to NAME_DESC, USERNAME_ASC, ID_ASC and pagination would work differently.

If you don't want to dig into this, maybe the best bet is to forbid combining distinctOn with connections - e.g. before setting distinctOn you lock cursorComparator/selectCursor/etc and then you enforce that they aren't used (otherwise you throw).

Copy link
Author

Choose a reason for hiding this comment

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

I have added the appropriate locks as I don't have the time to dig into it. I hope I got the locks correct.

From the little research I did cursors should work correctly with DISTINCT ON

// We must execute select after orderBy otherwise we cannot generate a cursor
this.lock("fixedSelectExpression");
this.lock("selectCursor");
Expand Down
2 changes: 2 additions & 0 deletions packages/postgraphile-core/__tests__/helpers-v5.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { withPgClient, getServerVersionNum } from "./helpers";
import jsonwebtoken from "jsonwebtoken";
import { createPostGraphileSchema } from "..";
import { makeExtendSchemaPlugin, gql } from "graphile-utils";
import DistinctOnPlugin from "./integration/DistinctOnPlugin";
import ToyCategoriesPlugin from "./integration/ToyCategoriesPlugin";

/**
Expand Down Expand Up @@ -468,6 +469,7 @@ const makeSchema = config => {
appendPlugins: [
ExtendedPlugin,
config.ToyCategoriesPlugin ? ToyCategoriesPlugin : null,
config.DistinctOnPlugin ? DistinctOnPlugin : null,
].filter(isNotNullish),
}
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
module.exports = builder => {
builder.hook(
"GraphQLObjectType:fields:field:args",
(args, build, context) => {
const {
graphql: { GraphQLString, GraphQLList },
pgSql: sql,
} = build;

const {
Self,
scope: { fieldName },
addArgDataGenerator,
} = context;

if (!(Self.name === "Query" && fieldName === "allToys")) {
return args;
}

addArgDataGenerator(({ distinct }) => {
return {
pgQuery: queryBuilder => {
distinct?.map(field => {
const id = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier(
field
)}`;
stwiname marked this conversation as resolved.
Show resolved Hide resolved

queryBuilder.distinctOn(id);
});
},
};
});

return build.extend(
args,
{
distinct: {
type: new GraphQLList(GraphQLString),
},
},
"test"
);
}
);
};
7 changes: 7 additions & 0 deletions packages/postgraphile-core/__tests__/kitchen-sink-data.sql
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,13 @@ insert into named_query_builder.toy_categories(toy_id, category_id, approved) va
(4, 2, true),
(1, 3, false);


insert into distinct_query_builder.toys (id, name, color) values
(1, 'Rex', 'green'),
(2, 'Toy Soldiers', 'green'),
(3, 'Dino-Rocket Launcher', 'red'),
(4, 'History of Dinosaurs book', 'red');

--------------------------------------------------------------------------------
alter sequence enum_tables.letter_descriptions_id_seq restart with 101;
insert into enum_tables.letter_descriptions(letter, letter_via_view, description) values
Expand Down
11 changes: 11 additions & 0 deletions packages/postgraphile-core/__tests__/kitchen-sink-schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ drop schema if exists
large_bigint,
network_types,
named_query_builder,
distinct_query_builder,
enum_tables,
geometry
cascade;
Expand Down Expand Up @@ -1140,6 +1141,16 @@ create table named_query_builder.toy_categories (

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

create schema distinct_query_builder;

create table distinct_query_builder.toys (
id serial primary key,
name text not null,
color text not null
);

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

create schema enum_tables;
create table enum_tables.abcd (letter text primary key, description text);
comment on column enum_tables.abcd.description is E'@enumDescription';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
t1: {
nodes: [
{
name: "Rex",
color: "green",
},
{
name: "Dino-Rocket Launcher",
color: "red",
},
],
},
t2: {
nodes: [
{
name: "Rex",
},
{
name: "Toy Soldiers",
},
{
name: "Dino-Rocket Launcher",
},
{
name: "History of Dinosaurs book",
},
],
},
t3: {
nodes: [
{
name: "Rex",
},
{
name: "Toy Soldiers",
},
{
name: "Dino-Rocket Launcher",
},
{
name: "History of Dinosaurs book",
},
],
},
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
with __local_0__ as (
select to_json(
(
json_build_object(
'__identifiers'::text,
json_build_array(__local_1__."id"),
'name'::text,
(__local_1__."name"),
'color'::text,
(__local_1__."color")
)
)
) as "@nodes"
from (
select distinct on (__local_1__."color") __local_1__.*
from "distinct_query_builder"."toys" as __local_1__
where (TRUE) and (TRUE)
order by __local_1__."color" ASC,
__local_1__."id" ASC
) __local_1__
),
__local_2__ as (
select json_agg(
to_json(__local_0__)
) as data
from __local_0__
)
select coalesce(
(
select __local_2__.data
from __local_2__
),
'[]'::json
) as "data"

with __local_0__ as (
select to_json(
(
json_build_object(
'__identifiers'::text,
json_build_array(__local_1__."id"),
'name'::text,
(__local_1__."name")
)
)
) as "@nodes"
from (
select distinct on (
__local_1__."id",
__local_1__."name"
) __local_1__.*
from "distinct_query_builder"."toys" as __local_1__
where (TRUE) and (TRUE)
order by __local_1__."id" ASC,
__local_1__."name" ASC
) __local_1__
),
__local_2__ as (
select json_agg(
to_json(__local_0__)
) as data
from __local_0__
)
select coalesce(
(
select __local_2__.data
from __local_2__
),
'[]'::json
) as "data"

with __local_0__ as (
select to_json(
(
json_build_object(
'__identifiers'::text,
json_build_array(__local_1__."id"),
'name'::text,
(__local_1__."name")
)
)
) as "@nodes"
from (
select __local_1__.*
from "distinct_query_builder"."toys" as __local_1__
where (TRUE) and (TRUE)
order by __local_1__."id" ASC
) __local_1__
),
__local_2__ as (
select json_agg(
to_json(__local_0__)
) as data
from __local_0__
)
select coalesce(
(
select __local_2__.data
from __local_2__
),
'[]'::json
) as "data"
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
## expect(errors).toBeFalsy();
#> schema: ["distinct_query_builder"]
#> subscriptions: true
#> DistinctOnPlugin: true
{
t1: allToys(distinct: ["color"]) {
nodes {
name
color
stwiname marked this conversation as resolved.
Show resolved Hide resolved
}
}
t2: allToys(distinct: ["id", "name"]) {
nodes {
name
stwiname marked this conversation as resolved.
Show resolved Hide resolved
}
}
t3: allToys {
nodes {
name
stwiname marked this conversation as resolved.
Show resolved Hide resolved
}
}
}