From c4d07fba751e312eedd39bd8d32126acd5e6894e Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Fri, 26 Aug 2022 14:51:51 +1200 Subject: [PATCH 01/13] Add distinctOn to QueryBuilder --- .../graphile-build-pg/src/QueryBuilder.d.ts | 1 + .../graphile-build-pg/src/QueryBuilder.js | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.d.ts b/packages/graphile-build-pg/src/QueryBuilder.d.ts index 2baa3ecbc..7f53f9fa7 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.d.ts +++ b/packages/graphile-build-pg/src/QueryBuilder.d.ts @@ -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; // ---------------------------------------- diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 9ba9e587b..eaf2fde90 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -106,6 +106,7 @@ class QueryBuilder { offset: ?NumberGen, first: ?number, last: ?number, + distinctOn: Array, beforeLock: { [string]: Array<() => void> | null, }, @@ -133,6 +134,7 @@ class QueryBuilder { offset: ?number, first: ?number, last: ?number, + distinctOn: Array, cursorComparator: ?CursorComparator, }; lockContext: { @@ -170,6 +172,7 @@ class QueryBuilder { last: false, limit: false, offset: false, + distinctOn: false, }; this.finalized = false; this.selectedIdentifiers = false; @@ -192,6 +195,7 @@ class QueryBuilder { offset: null, first: null, last: null, + distinctOn: [], 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 @@ -231,6 +235,7 @@ class QueryBuilder { offset: null, first: null, last: null, + distinctOn: [], cursorComparator: null, }; this._children = new Map(); @@ -511,6 +516,10 @@ ${sql.join( } this.data.last = last; } + distinctOn(exprGen: SQLGen) { + this.checkLock("distinctOn"); + this.data.distinctOn.push(exprGen); + } // ---------------------------------------- @@ -744,7 +753,14 @@ ${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, + ", " + )})` + } +${useAsterisk ? sql.fragment`${this.getTableAlias()}.*` : fields} ${ this.compiledData.from && sql.fragment`from ${this.compiledData.from[0]} as ${this.getTableAlias()}` @@ -868,6 +884,14 @@ 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") { + // Add distinct to existing order by to avoid `SELECT DISTINCT ON expressions must match initial ORDER BY expressions` + if (this.data[type].length) { + this.data["distinctOn"].forEach(d => { + if (!this.data[type].find(([a]) => a === d)) { + this.data[type].unshift([d, undefined, undefined]); + } + }); + } this.compiledData[type] = this.data[type].map(([a, b, c]) => [ callIfNecessary(a, context), b, @@ -894,6 +918,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}'`); } @@ -925,6 +951,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"); // We must execute select after orderBy otherwise we cannot generate a cursor this.lock("fixedSelectExpression"); this.lock("selectCursor"); From b47ddc6e4bf1d749ab3daecc561a5710cbbb3d4a Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Mon, 29 Aug 2022 09:27:42 +1200 Subject: [PATCH 02/13] Use ternary to avoid inserting `0` into sql Co-authored-by: Benjie --- packages/graphile-build-pg/src/QueryBuilder.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index eaf2fde90..132c1ff52 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -754,11 +754,12 @@ ${sql.join( let fragment = sql.fragment`\ select ${ - this.compiledData.distinctOn.length && - sql.fragment`distinct on (${sql.join( - this.compiledData.distinctOn, - ", " - )})` + this.compiledData.distinctOn.length + ? sql.fragment`distinct on (${sql.join( + this.compiledData.distinctOn, + ", " + )})` + : sql.blank } ${useAsterisk ? sql.fragment`${this.getTableAlias()}.*` : fields} ${ From 341f5b99ed426cea31ef10d7d8d7c593e9c97770 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Mon, 29 Aug 2022 13:53:47 +1200 Subject: [PATCH 03/13] Add test for distinctOn --- .../postgraphile-core/__tests__/helpers-v5.js | 2 + .../__tests__/integration/DistinctOnPlugin.js | 46 +++++++++ .../__tests__/kitchen-sink-data.sql | 7 ++ .../__tests__/kitchen-sink-schema.sql | 11 +++ .../base/distinct_query_builder.toys.json5 | 46 +++++++++ .../base/distinct_query_builder.toys.sql | 99 +++++++++++++++++++ .../distinct_query_builder.toys.test.graphql | 22 +++++ 7 files changed, 233 insertions(+) create mode 100644 packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js create mode 100644 packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 create mode 100644 packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql create mode 100644 packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql diff --git a/packages/postgraphile-core/__tests__/helpers-v5.js b/packages/postgraphile-core/__tests__/helpers-v5.js index 40f7fd493..ddd5e0c46 100644 --- a/packages/postgraphile-core/__tests__/helpers-v5.js +++ b/packages/postgraphile-core/__tests__/helpers-v5.js @@ -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"; /** @@ -468,6 +469,7 @@ const makeSchema = config => { appendPlugins: [ ExtendedPlugin, config.ToyCategoriesPlugin ? ToyCategoriesPlugin : null, + config.DistinctOnPlugin ? DistinctOnPlugin : null, ].filter(isNotNullish), } ); diff --git a/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js new file mode 100644 index 000000000..630c5b04a --- /dev/null +++ b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js @@ -0,0 +1,46 @@ +module.exports = builder => { + builder.hook( + "GraphQLObjectType:fields:field:args", + (args, build, context) => { + const { + graphql: { GraphQLString }, + pgSql: sql, + } = build; + + const { + Self, + scope: { fieldName }, + addArgDataGenerator, + } = context; + + if (!(Self.name === "Query" && fieldName === "allToys")) { + return args; + } + + addArgDataGenerator(({ distinct }) => { + return { + pgQuery: queryBuilder => { + if (!distinct) { + return; + } + const id = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier( + distinct + )}`; + + queryBuilder.distinctOn(id); + }, + }; + }); + + return build.extend( + args, + { + distinct: { + type: GraphQLString, + }, + }, + "test" + ); + } + ); +}; diff --git a/packages/postgraphile-core/__tests__/kitchen-sink-data.sql b/packages/postgraphile-core/__tests__/kitchen-sink-data.sql index 2070c9671..076fc77f5 100644 --- a/packages/postgraphile-core/__tests__/kitchen-sink-data.sql +++ b/packages/postgraphile-core/__tests__/kitchen-sink-data.sql @@ -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 diff --git a/packages/postgraphile-core/__tests__/kitchen-sink-schema.sql b/packages/postgraphile-core/__tests__/kitchen-sink-schema.sql index f26de5d5f..b0d551653 100644 --- a/packages/postgraphile-core/__tests__/kitchen-sink-schema.sql +++ b/packages/postgraphile-core/__tests__/kitchen-sink-schema.sql @@ -13,6 +13,7 @@ drop schema if exists large_bigint, network_types, named_query_builder, + distinct_query_builder, enum_tables, geometry cascade; @@ -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'; diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 new file mode 100644 index 000000000..2f69f1c4e --- /dev/null +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -0,0 +1,46 @@ +{ + t1: { + nodes: [ + { + name: "Dino-Rocket Launcher", + color: "red", + }, + { + name: "Rex", + color: "green", + }, + ], + }, + t2: { + nodes: [ + { + name: "Toy Soldiers", + }, + { + name: "Rex", + }, + { + name: "History of Dinosaurs book", + }, + { + name: "Dino-Rocket Launcher", + }, + ], + }, + t3: { + nodes: [ + { + name: "Rex", + }, + { + name: "Toy Soldiers", + }, + { + name: "Dino-Rocket Launcher", + }, + { + name: "History of Dinosaurs book", + }, + ], + }, +} diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql new file mode 100644 index 000000000..18e5d58e3 --- /dev/null +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -0,0 +1,99 @@ +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" DESC, + __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__."name") __local_1__.* + from "distinct_query_builder"."toys" as __local_1__ + where (TRUE) and (TRUE) + order by __local_1__."name" DESC, + __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 __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" \ No newline at end of file diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql new file mode 100644 index 000000000..817c4ad17 --- /dev/null +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -0,0 +1,22 @@ +## expect(errors).toBeFalsy(); +#> schema: ["distinct_query_builder"] +#> subscriptions: true +#> DistinctOnPlugin: true +{ + t1: allToys(distinct: "color") { + nodes { + name + color + } + } + t2: allToys(distinct: "name") { + nodes { + name + } + } + t3: allToys { + nodes { + name + } + } +} From 372c8c200df07af20bb266cd732693003112472d Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Mon, 29 Aug 2022 14:44:43 +1200 Subject: [PATCH 04/13] Specify ASC for order by distinct columns --- packages/graphile-build-pg/src/QueryBuilder.js | 2 +- .../base/distinct_query_builder.toys.json5 | 16 ++++++++-------- .../queries/base/distinct_query_builder.toys.sql | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 132c1ff52..7fdc104c2 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -889,7 +889,7 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor if (this.data[type].length) { this.data["distinctOn"].forEach(d => { if (!this.data[type].find(([a]) => a === d)) { - this.data[type].unshift([d, undefined, undefined]); + this.data[type].unshift([d, true, undefined]); } }); } diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index 2f69f1c4e..78cd37028 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -1,29 +1,29 @@ { t1: { nodes: [ - { - name: "Dino-Rocket Launcher", - color: "red", - }, { name: "Rex", color: "green", }, + { + name: "Dino-Rocket Launcher", + color: "red", + }, ], }, t2: { nodes: [ { - name: "Toy Soldiers", + name: "Dino-Rocket Launcher", }, { - name: "Rex", + name: "History of Dinosaurs book", }, { - name: "History of Dinosaurs book", + name: "Rex", }, { - name: "Dino-Rocket Launcher", + name: "Toy Soldiers", }, ], }, diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index 18e5d58e3..645c0a7ca 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -15,7 +15,7 @@ with __local_0__ as ( 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" DESC, + order by __local_1__."color" ASC, __local_1__."id" ASC ) __local_1__ ), @@ -48,7 +48,7 @@ with __local_0__ as ( select distinct on (__local_1__."name") __local_1__.* from "distinct_query_builder"."toys" as __local_1__ where (TRUE) and (TRUE) - order by __local_1__."name" DESC, + order by __local_1__."name" ASC, __local_1__."id" ASC ) __local_1__ ), From 939d66f65bd1a53d0b37df9a4e5f6af4cf010371 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Tue, 30 Aug 2022 13:42:51 +1200 Subject: [PATCH 05/13] Fix merging distinct and order by --- .../graphile-build-pg/src/QueryBuilder.js | 22 +++++++++---------- .../__tests__/integration/DistinctOnPlugin.js | 17 +++++++------- .../base/distinct_query_builder.toys.json5 | 8 +++---- .../base/distinct_query_builder.toys.sql | 9 +++++--- .../distinct_query_builder.toys.test.graphql | 4 ++-- 5 files changed, 30 insertions(+), 30 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 7fdc104c2..78c6d8a26 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -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 @@ -886,18 +887,15 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor this.compiledData[type] = data; } else if (type === "orderBy") { // Add distinct to existing order by to avoid `SELECT DISTINCT ON expressions must match initial ORDER BY expressions` - if (this.data[type].length) { - this.data["distinctOn"].forEach(d => { - if (!this.data[type].find(([a]) => a === d)) { - this.data[type].unshift([d, true, undefined]); - } - }); - } - this.compiledData[type] = this.data[type].map(([a, b, c]) => [ - callIfNecessary(a, context), - b, - c, - ]); + // 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] + ); + + // 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]); } else if (type === "from") { if (this.data.from) { const f = this.data.from; diff --git a/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js index 630c5b04a..05efbc6a1 100644 --- a/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js +++ b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js @@ -3,7 +3,7 @@ module.exports = builder => { "GraphQLObjectType:fields:field:args", (args, build, context) => { const { - graphql: { GraphQLString }, + graphql: { GraphQLString, GraphQLList }, pgSql: sql, } = build; @@ -20,14 +20,13 @@ module.exports = builder => { addArgDataGenerator(({ distinct }) => { return { pgQuery: queryBuilder => { - if (!distinct) { - return; - } - const id = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier( - distinct - )}`; + distinct?.map(field => { + const id = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier( + field + )}`; - queryBuilder.distinctOn(id); + queryBuilder.distinctOn(id); + }); }, }; }); @@ -36,7 +35,7 @@ module.exports = builder => { args, { distinct: { - type: GraphQLString, + type: new GraphQLList(GraphQLString), }, }, "test" diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index 78cd37028..3f1ed47e9 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -14,16 +14,16 @@ t2: { nodes: [ { - name: "Dino-Rocket Launcher", + name: "Rex", }, { - name: "History of Dinosaurs book", + name: "Toy Soldiers", }, { - name: "Rex", + name: "Dino-Rocket Launcher", }, { - name: "Toy Soldiers", + name: "History of Dinosaurs book", }, ], }, diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index 645c0a7ca..8817aeeaa 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -45,11 +45,14 @@ with __local_0__ as ( ) ) as "@nodes" from ( - select distinct on (__local_1__."name") __local_1__.* + 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__."name" ASC, - __local_1__."id" ASC + order by __local_1__."id" ASC, + __local_1__."name" ASC ) __local_1__ ), __local_2__ as ( diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql index 817c4ad17..a08461a8a 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -3,13 +3,13 @@ #> subscriptions: true #> DistinctOnPlugin: true { - t1: allToys(distinct: "color") { + t1: allToys(distinct: ["color"]) { nodes { name color } } - t2: allToys(distinct: "name") { + t2: allToys(distinct: ["id", "name"]) { nodes { name } From 28c64e0b039f514aa27655bf5d7b58b886d9c408 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Fri, 2 Sep 2022 10:45:07 +1200 Subject: [PATCH 06/13] Apply suggestions from code review Co-authored-by: Benjie --- .../__tests__/integration/DistinctOnPlugin.js | 3 +++ .../queries/base/distinct_query_builder.toys.test.graphql | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js index 05efbc6a1..35574cf44 100644 --- a/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js +++ b/packages/postgraphile-core/__tests__/integration/DistinctOnPlugin.js @@ -21,6 +21,9 @@ module.exports = builder => { return { pgQuery: queryBuilder => { distinct?.map(field => { + // THIS IS AN EXAMPLE FOR THE TESTS. DO NOT USE THIS IN REAL PRODUCTION CODE! + // Instead you should use an enum to indicate the allowed identifiers; otherwise + // you risk interacting with the system columns and maybe worse. const id = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier( field )}`; diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql index a08461a8a..a1cb5aa47 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -5,18 +5,23 @@ { t1: allToys(distinct: ["color"]) { nodes { + id name color } } t2: allToys(distinct: ["id", "name"]) { nodes { + id name + color } } t3: allToys { nodes { + id name + color } } } From c6c0d54bf12d4c50f0f0d346aa29b5eed3284ab6 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Fri, 2 Sep 2022 12:43:54 +1200 Subject: [PATCH 07/13] Address code related comments --- .../graphile-build-pg/src/QueryBuilder.js | 44 ++++++++++++------- packages/graphile-build-pg/src/utils.js | 18 ++++++++ .../base/distinct_query_builder.toys.json5 | 18 ++++++++ .../base/distinct_query_builder.toys.sql | 17 +++++-- 4 files changed, 79 insertions(+), 18 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 78c6d8a26..5bd990c81 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -3,8 +3,8 @@ 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"; +import { arraysMatch } from "./utils"; // eslint-disable-next-line flowtype/no-weak-types type GraphQLContext = any; @@ -214,6 +214,7 @@ class QueryBuilder { last: [], limit: [], offset: [], + distinctOn: [], }, cursorComparator: null, liveConditions: [], @@ -756,11 +757,11 @@ ${sql.join( let fragment = sql.fragment`\ select ${ this.compiledData.distinctOn.length - ? sql.fragment`distinct on (${sql.join( - this.compiledData.distinctOn, - ", " - )})` - : sql.blank + ? sql.fragment`distinct on (${sql.join( + this.compiledData.distinctOn, + ", " + )})` + : sql.blank } ${useAsterisk ? sql.fragment`${this.getTableAlias()}.*` : fields} ${ @@ -886,16 +887,29 @@ 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") { - // 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] - ); + if (this.data["distinctOn"].length) { + // 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]) => + a === d || + (Array.isArray(a) && Array.isArray(d) && arraysMatch(a, d)) + ) ?? [d, true, null] + ); - // 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]); + // 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]); + } else { + this.compiledData[type] = this.data[type].map(([a, b, c]) => [ + callIfNecessary(a, context), + b, + c, + ]); + } } else if (type === "from") { if (this.data.from) { const f = this.data.from; diff --git a/packages/graphile-build-pg/src/utils.js b/packages/graphile-build-pg/src/utils.js index fec346b24..2b328e2cf 100644 --- a/packages/graphile-build-pg/src/utils.js +++ b/packages/graphile-build-pg/src/utils.js @@ -29,3 +29,21 @@ export const parseTags = (str: string) => { } ); }; + +export function arraysMatch( + array1: ReadonlyArray, + array2: ReadonlyArray, + comparator: (val1: T, val2: T) => boolean = (v1, v2) => v1 === v2 +): boolean { + const l = array1.length; + if (l !== array2.length) { + return false; + } + + for (let i = 0; i < l; i++) { + if (!comparator(array1[i], array2[i])) { + return false; + } + } + return true; +} diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index 3f1ed47e9..e8d03523e 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -2,10 +2,12 @@ t1: { nodes: [ { + id: 1, name: "Rex", color: "green", }, { + id: 3, name: "Dino-Rocket Launcher", color: "red", }, @@ -14,32 +16,48 @@ t2: { nodes: [ { + id: 1, name: "Rex", + color: "green", }, { + id: 2, name: "Toy Soldiers", + color: "green", }, { + id: 3, name: "Dino-Rocket Launcher", + color: "red", }, { + id: 4, name: "History of Dinosaurs book", + color: "red", }, ], }, t3: { nodes: [ { + id: 1, name: "Rex", + color: "green", }, { + id: 2, name: "Toy Soldiers", + color: "green", }, { + id: 3, name: "Dino-Rocket Launcher", + color: "red", }, { + id: 4, name: "History of Dinosaurs book", + color: "red", }, ], }, diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index 8817aeeaa..7f4b33ed5 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -4,6 +4,8 @@ with __local_0__ as ( json_build_object( '__identifiers'::text, json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), 'name'::text, (__local_1__."name"), 'color'::text, @@ -39,8 +41,12 @@ with __local_0__ as ( json_build_object( '__identifiers'::text, json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), 'name'::text, - (__local_1__."name") + (__local_1__."name"), + 'color'::text, + (__local_1__."color") ) ) ) as "@nodes" @@ -52,7 +58,8 @@ with __local_0__ as ( from "distinct_query_builder"."toys" as __local_1__ where (TRUE) and (TRUE) order by __local_1__."id" ASC, - __local_1__."name" ASC + __local_1__."name" ASC, + __local_1__."id" ASC ) __local_1__ ), __local_2__ as ( @@ -75,8 +82,12 @@ with __local_0__ as ( json_build_object( '__identifiers'::text, json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), 'name'::text, - (__local_1__."name") + (__local_1__."name"), + 'color'::text, + (__local_1__."color") ) ) ) as "@nodes" From 617f8c655b20da479f4311920708cb8231f947c5 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Tue, 6 Sep 2022 13:19:25 +1200 Subject: [PATCH 08/13] Lock cursor when distinctOn is used and vice versa --- .../graphile-build-pg/src/QueryBuilder.js | 10 ++++ .../__tests__/kitchen-sink-data.sql | 1 - .../base/distinct_query_builder.toys.json5 | 15 ++++++ .../base/distinct_query_builder.toys.sql | 47 ++++++++++++++++++- .../distinct_query_builder.toys.test.graphql | 8 ++++ 5 files changed, 79 insertions(+), 2 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 5bd990c81..5e5051eb4 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -265,6 +265,16 @@ class QueryBuilder { this.lock("limit"); this.lock("offset"); }); + this.beforeLock("distinctOn", () => { + this.lock("selectCursor"); + this.lock("cursorComparator"); + }); + this.beforeLock("selectCursor", () => { + this.lock("distinctOn"); + }); + this.beforeLock("cursorComparator", () => { + this.lock("distinctOn"); + }); this.lockContext = Object.freeze({ queryBuilder: this, }); diff --git a/packages/postgraphile-core/__tests__/kitchen-sink-data.sql b/packages/postgraphile-core/__tests__/kitchen-sink-data.sql index 076fc77f5..672fb1785 100644 --- a/packages/postgraphile-core/__tests__/kitchen-sink-data.sql +++ b/packages/postgraphile-core/__tests__/kitchen-sink-data.sql @@ -276,7 +276,6 @@ 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'), diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index e8d03523e..c0db957b8 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -61,4 +61,19 @@ }, ], }, + t4: { + totalCount: 4, + nodes: [ + { + id: 1, + name: "Rex", + color: "green", + }, + { + id: 3, + name: "Dino-Rocket Launcher", + color: "red", + }, + ], + }, } diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index 7f4b33ed5..001119161 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -110,4 +110,49 @@ select coalesce( from __local_2__ ), '[]'::json -) as "data" \ No newline at end of file +) as "data" + +with __local_0__ as ( + select to_json( + ( + json_build_object( + '__identifiers'::text, + json_build_array(__local_1__."id"), + 'id'::text, + (__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", +( + select json_build_object( + 'totalCount'::text, + count(1) + ) + from "distinct_query_builder"."toys" as __local_1__ + where 1 = 1 +) as "aggregates" \ No newline at end of file diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql index a1cb5aa47..b416b00fa 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -24,4 +24,12 @@ color } } + t4: allToys(distinct: ["color"]) { + totalCount + nodes { + id + name + color + } + } } From f32bd5950d692f6c047f1061d779d9a366c89f04 Mon Sep 17 00:00:00 2001 From: Scott Twiname Date: Thu, 8 Sep 2022 13:47:10 +1200 Subject: [PATCH 09/13] Fix equality check add additional test --- .../graphile-build-pg/src/QueryBuilder.js | 8 +++- .../base/distinct_query_builder.toys.json5 | 14 +++++++ .../base/distinct_query_builder.toys.sql | 42 +++++++++++++++++-- .../distinct_query_builder.toys.test.graphql | 7 ++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 5e5051eb4..91ff1b067 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -905,7 +905,13 @@ order by (row_number() over (partition by 1)) desc`; /* We don't need to factor this.data[type].find( ([a]) => a === d || - (Array.isArray(a) && Array.isArray(d) && arraysMatch(a, d)) + (Array.isArray(a) && + Array.isArray(d) && + arraysMatch( + a, + d, + (v1, v2) => JSON.stringify(v1) === JSON.stringify(v2) + )) ) ?? [d, true, null] ); diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index c0db957b8..ba566018a 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -76,4 +76,18 @@ }, ], }, + t5: { + nodes: [ + { + id: 3, + name: "Dino-Rocket Launcher", + color: "red", + }, + { + id: 1, + name: "Rex", + color: "green", + }, + ], + }, } diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index 001119161..e4ff10b4f 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -58,8 +58,7 @@ with __local_0__ as ( from "distinct_query_builder"."toys" as __local_1__ where (TRUE) and (TRUE) order by __local_1__."id" ASC, - __local_1__."name" ASC, - __local_1__."id" ASC + __local_1__."name" ASC ) __local_1__ ), __local_2__ as ( @@ -155,4 +154,41 @@ select coalesce( ) from "distinct_query_builder"."toys" as __local_1__ where 1 = 1 -) as "aggregates" \ No newline at end of file +) as "aggregates" + +with __local_0__ as ( + select to_json( + ( + json_build_object( + '__identifiers'::text, + json_build_array(__local_1__."id"), + 'id'::text, + (__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" DESC, + __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" \ No newline at end of file diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql index b416b00fa..657ce9707 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -32,4 +32,11 @@ color } } + t5: allToys(distinct: ["color"], orderBy: COLOR_DESC) { + nodes { + id + name + color + } + } } From e012e1fdbf45a47493788a5bdeb85496032d4823 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 5 Oct 2023 12:39:59 +0100 Subject: [PATCH 10/13] Backport sql.isEquivalent from pg-sql2@5 --- packages/graphile-build-pg/src/utils.js | 18 +------ packages/pg-sql2/src/index.ts | 66 +++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 17 deletions(-) diff --git a/packages/graphile-build-pg/src/utils.js b/packages/graphile-build-pg/src/utils.js index 2b328e2cf..ef17ba982 100644 --- a/packages/graphile-build-pg/src/utils.js +++ b/packages/graphile-build-pg/src/utils.js @@ -30,20 +30,4 @@ export const parseTags = (str: string) => { ); }; -export function arraysMatch( - array1: ReadonlyArray, - array2: ReadonlyArray, - comparator: (val1: T, val2: T) => boolean = (v1, v2) => v1 === v2 -): boolean { - const l = array1.length; - if (l !== array2.length) { - return false; - } - - for (let i = 0; i < l; i++) { - if (!comparator(array1[i], array2[i])) { - return false; - } - } - return true; -} +export { arraysMatch } from "pg-sql2"; diff --git a/packages/pg-sql2/src/index.ts b/packages/pg-sql2/src/index.ts index 9add48c74..f2529bf2a 100644 --- a/packages/pg-sql2/src/index.ts +++ b/packages/pg-sql2/src/index.ts @@ -1,6 +1,7 @@ import * as debugFactory from "debug"; import { QueryConfig } from "pg"; import LRU from "@graphile/lru"; +import { inspect } from "util"; const debug = debugFactory("pg-sql2"); @@ -326,6 +327,71 @@ export function join(items: Array, rawSeparator = ""): SQLQuery { return currentItems; } +export function arraysMatch( + array1: ReadonlyArray, + array2: ReadonlyArray, + comparator?: (val1: T, val2: T) => boolean +): boolean { + if (array1 === array2) return true; + const l = array1.length; + if (l !== array2.length) { + return false; + } + for (let i = 0; i < l; i++) { + if ( + comparator ? !comparator(array1[i]!, array2[i]!) : array1[i] !== array2[i] + ) { + return false; + } + } + return true; +} + +export function isEquivalent( + sql1: SQL, + sql2: SQL, + options?: { + symbolSubstitutes?: Map; + } +): boolean { + if (sql1 === sql2) { + return true; + } else if (Array.isArray(sql1)) { + if (!Array.isArray(sql2)) { + return false; + } + return arraysMatch(sql1, sql2, (a, b) => isEquivalent(a, b, options)); + } else if (Array.isArray(sql2)) { + return false; + } else { + switch (sql1.type) { + case "RAW": { + if (sql2.type !== sql1.type) { + return false; + } + return sql1.text === sql2.text; + } + case "VALUE": { + if (sql2.type !== sql1.type) { + return false; + } + return sql1.value === sql2.value; + } + case "IDENTIFIER": { + if (sql2.type !== sql1.type) { + return false; + } + return arraysMatch(sql1.names, sql2.names); + } + default: { + const never: never = sql1; + console.error(`Unhandled node type: ${inspect(never)}`); + return false; + } + } + } +} + // Copied from https://github.com/brianc/node-postgres/blob/860cccd53105f7bc32fed8b1de69805f0ecd12eb/lib/client.js#L285-L302 // Ported from PostgreSQL 9.2.4 source code in src/interfaces/libpq/fe-exec.c // Trivial performance optimisations by Benjie. From b81a04db248d332dcb4846abca9a48c53f7fadd6 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 5 Oct 2023 12:40:33 +0100 Subject: [PATCH 11/13] Change how the 'ORDER BY' clause is built --- .../graphile-build-pg/src/QueryBuilder.js | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 91ff1b067..1b9cb89ea 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -4,7 +4,6 @@ import type { SQL } from "pg-sql2"; import isSafeInteger from "lodash/isSafeInteger"; import chunk from "lodash/chunk"; import type { PgClass, PgType } from "./plugins/PgIntrospectionPlugin"; -import { arraysMatch } from "./utils"; // eslint-disable-next-line flowtype/no-weak-types type GraphQLContext = any; @@ -269,6 +268,9 @@ class QueryBuilder { this.lock("selectCursor"); this.lock("cursorComparator"); }); + this.beforeLock("orderBy", () => { + this.lock("distinctOn"); + }); this.beforeLock("selectCursor", () => { this.lock("distinctOn"); }); @@ -764,6 +766,23 @@ ${sql.join( })} as object` : this.buildSelectFields(); + const orderBy: Array<[sql.SQL, boolean, boolean | null]> = []; + const remainingOrderBy = [...this.compiledData.orderBy]; + for (const distinctExpression of this.compiledData.distinctOn) { + const relevantOrderByIndex = remainingOrderBy.findIndex(o => + sql.isEquivalent(o[0], distinctExpression) + ); + if (relevantOrderByIndex >= 0) { + orderBy.push(remainingOrderBy[relevantOrderByIndex]); + remainingOrderBy.splice(relevantOrderByIndex, 1); + } else { + orderBy.push([distinctExpression, true, null]); + } + } + for (const orderSpec of remainingOrderBy) { + orderBy.push(orderSpec); + } + let fragment = sql.fragment`\ select ${ this.compiledData.distinctOn.length @@ -781,9 +800,9 @@ ${ ${this.compiledData.join.length && sql.join(this.compiledData.join, " ")} where ${this.buildWhereClause(true, true, options)} ${ - this.compiledData.orderBy.length + orderBy.length ? sql.fragment`order by ${sql.join( - this.compiledData.orderBy.map( + orderBy.map( ([expr, ascending, nullsFirst]) => sql.fragment`${expr} ${ Number(ascending) ^ Number(flip) @@ -897,35 +916,11 @@ 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") { - if (this.data["distinctOn"].length) { - // 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]) => - a === d || - (Array.isArray(a) && - Array.isArray(d) && - arraysMatch( - a, - d, - (v1, v2) => JSON.stringify(v1) === JSON.stringify(v2) - )) - ) ?? [d, true, null] - ); - - // 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]); - } else { - this.compiledData[type] = this.data[type].map(([a, b, c]) => [ - callIfNecessary(a, context), - b, - c, - ]); - } + this.compiledData[type] = this.data[type].map(([a, b, c]) => [ + callIfNecessary(a, context), + b, + c, + ]); } else if (type === "from") { if (this.data.from) { const f = this.data.from; From 915cffa9cb29aa1c84e752026284db3e9ec964cb Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 5 Oct 2023 14:20:13 +0100 Subject: [PATCH 12/13] Move orderBy logic again, and forbid distinctOn with last --- .../graphile-build-pg/src/QueryBuilder.js | 45 ++++++++++++------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/packages/graphile-build-pg/src/QueryBuilder.js b/packages/graphile-build-pg/src/QueryBuilder.js index 1b9cb89ea..7296ff51a 100644 --- a/packages/graphile-build-pg/src/QueryBuilder.js +++ b/packages/graphile-build-pg/src/QueryBuilder.js @@ -601,6 +601,10 @@ ${sql.join( throw new Error("Cannot combine 'last' and 'offset'"); } else { if (this.compiledData.orderBy.length > 0) { + if (this.compiledData.distinctOn.length > 0) { + // TODO: improve this + throw new Error(`'distinctOn' cannot be combined with 'last'`); + } flip = true; limit = this.compiledData.last; } else { @@ -620,9 +624,31 @@ ${sql.join( getFinalLimit() { return this.getFinalLimitAndOffset().limit; } + _orderByExpressionsAndDirections: Array< + [sql.SQL, boolean, boolean | null] + > | null = null; getOrderByExpressionsAndDirections() { this.lock("orderBy"); - return this.compiledData.orderBy; + if (!this._orderByExpressionsAndDirections) { + const orderBy: Array<[sql.SQL, boolean, boolean | null]> = []; + const remainingOrderBy = [...this.compiledData.orderBy]; + for (const distinctExpression of this.compiledData.distinctOn) { + const relevantOrderByIndex = remainingOrderBy.findIndex(o => + sql.isEquivalent(o[0], distinctExpression) + ); + if (relevantOrderByIndex >= 0) { + orderBy.push(remainingOrderBy[relevantOrderByIndex]); + remainingOrderBy.splice(relevantOrderByIndex, 1); + } else { + orderBy.push([distinctExpression, true, null]); + } + } + for (const orderSpec of remainingOrderBy) { + orderBy.push(orderSpec); + } + this._orderByExpressionsAndDirections = orderBy; + } + return this._orderByExpressionsAndDirections; } getSelectFieldsCount() { this.lockEverything(); @@ -766,22 +792,7 @@ ${sql.join( })} as object` : this.buildSelectFields(); - const orderBy: Array<[sql.SQL, boolean, boolean | null]> = []; - const remainingOrderBy = [...this.compiledData.orderBy]; - for (const distinctExpression of this.compiledData.distinctOn) { - const relevantOrderByIndex = remainingOrderBy.findIndex(o => - sql.isEquivalent(o[0], distinctExpression) - ); - if (relevantOrderByIndex >= 0) { - orderBy.push(remainingOrderBy[relevantOrderByIndex]); - remainingOrderBy.splice(relevantOrderByIndex, 1); - } else { - orderBy.push([distinctExpression, true, null]); - } - } - for (const orderSpec of remainingOrderBy) { - orderBy.push(orderSpec); - } + const orderBy = this.getOrderByExpressionsAndDirections(); let fragment = sql.fragment`\ select ${ From d7be967605738c993f7bbc2d49ab418e482dbf38 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 5 Oct 2023 14:30:01 +0100 Subject: [PATCH 13/13] Add more tests --- .../base/distinct_query_builder.toys.json5 | 39 +++ .../base/distinct_query_builder.toys.sql | 252 +++++++++++++++++- .../distinct_query_builder.toys.test.graphql | 43 +++ 3 files changed, 333 insertions(+), 1 deletion(-) diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 index ba566018a..403985e11 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.json5 @@ -90,4 +90,43 @@ }, ], }, + t6: { + totalCount: 4, + nodes: [ + { + id: 3, + name: "Dino-Rocket Launcher", + color: "red", + }, + ], + pageInfo: { + endCursor: "WyJuYW1lX2FzYyIsImNvbG9yX2Rlc2MiLFsicmVkIiwiRGluby1Sb2NrZXQgTGF1bmNoZXIiLDNdXQ==", + }, + }, + t7: { + totalCount: 4, + nodes: [ + { + id: 4, + name: "History of Dinosaurs book", + color: "red", + }, + ], + pageInfo: { + endCursor: "WyJuYW1lX2FzYyIsImNvbG9yX2Rlc2MiLFsicmVkIiwiSGlzdG9yeSBvZiBEaW5vc2F1cnMgYm9vayIsNF1d", + }, + }, + t8: { + totalCount: 4, + nodes: [ + { + id: 3, + name: "Dino-Rocket Launcher", + color: "red", + }, + ], + pageInfo: { + endCursor: "WyJuYW1lX2FzYyIsImNvbG9yX2Rlc2MiLFsicmVkIiwiRGluby1Sb2NrZXQgTGF1bmNoZXIiLDNdXQ==", + }, + }, } diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql index e4ff10b4f..7debc8d6d 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.sql @@ -191,4 +191,254 @@ select coalesce( from __local_2__ ), '[]'::json -) as "data" \ No newline at end of file +) as "data" + +with __local_0__ as ( + select to_json( + ( + json_build_object( + '__identifiers'::text, + json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), + 'name'::text, + (__local_1__."name"), + 'color'::text, + (__local_1__."color") + ) + ) + ) as "@nodes", + to_json( + json_build_array( + 'name_asc', + 'color_desc', + json_build_array( + __local_1__."color", + __local_1__."name", + __local_1__."id" + ) + ) + ) as "__cursor" + 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" DESC, + __local_1__."name" ASC, + __local_1__."id" ASC + limit 1 + ) __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", +( + select json_build_object( + 'totalCount'::text, + count(1) + ) + from "distinct_query_builder"."toys" as __local_1__ + where 1 = 1 +) as "aggregates" + +with __local_0__ as ( + select to_json( + ( + json_build_object( + '__identifiers'::text, + json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), + 'name'::text, + (__local_1__."name"), + 'color'::text, + (__local_1__."color") + ) + ) + ) as "@nodes", + to_json( + json_build_array( + 'name_asc', + 'color_desc', + json_build_array( + __local_1__."color", + __local_1__."name", + __local_1__."id" + ) + ) + ) as "__cursor" + from ( + select distinct on (__local_1__."color") __local_1__.* + from "distinct_query_builder"."toys" as __local_1__ + where ( + ( + ( + ( + ( + 'name_asc', + 'color_desc' + ) = ( + $1, + $2 + ) + ) AND ( + ( + ( + __local_1__."color" < $3 + ) OR ( + __local_1__."color" = $3 AND ( + ( + __local_1__."name" > $4 + ) OR ( + __local_1__."name" = $4 AND ( + ( + __local_1__."id" > $5 + ) OR ( + __local_1__."id" = $5 AND false + ) + ) + ) + ) + ) + ) + ) + ) + ) + ) and (TRUE) + order by __local_1__."color" DESC, + __local_1__."name" ASC, + __local_1__."id" ASC + limit 1 + ) __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", +( + select json_build_object( + 'totalCount'::text, + count(1) + ) + from "distinct_query_builder"."toys" as __local_1__ + where 1 = 1 +) as "aggregates" + +with __local_0__ as ( + select to_json( + ( + json_build_object( + '__identifiers'::text, + json_build_array(__local_1__."id"), + 'id'::text, + (__local_1__."id"), + 'name'::text, + (__local_1__."name"), + 'color'::text, + (__local_1__."color") + ) + ) + ) as "@nodes", + to_json( + json_build_array( + 'name_asc', + 'color_desc', + json_build_array( + __local_1__."color", + __local_1__."name", + __local_1__."id" + ) + ) + ) as "__cursor" + from ( + with __local_2__ as ( + select distinct on (__local_1__."color") __local_1__.* + from "distinct_query_builder"."toys" as __local_1__ + where (TRUE) + and ( + ( + ( + ( + ( + 'name_asc', + 'color_desc' + ) = ( + $1, + $2 + ) + ) AND ( + ( + ( + __local_1__."color" > $3 + ) OR ( + __local_1__."color" = $3 AND ( + ( + __local_1__."name" < $4 + ) OR ( + __local_1__."name" = $4 AND ( + ( + __local_1__."id" < $5 + ) OR ( + __local_1__."id" = $5 AND false + ) + ) + ) + ) + ) + ) + ) + ) + ) + ) + order by __local_1__."color" ASC, + __local_1__."name" DESC, + __local_1__."id" DESC + limit 1 + ) + select * + from __local_2__ + order by ( + row_number( ) over (partition by 1) + ) desc + ) __local_1__ +), +__local_3__ as ( + select json_agg( + to_json(__local_0__) + ) as data + from __local_0__ +) +select coalesce( + ( + select __local_3__.data + from __local_3__ + ), + '[]'::json +) as "data", +( + select json_build_object( + 'totalCount'::text, + count(1) + ) + from "distinct_query_builder"."toys" as __local_1__ + where 1 = 1 +) as "aggregates" \ No newline at end of file diff --git a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql index 657ce9707..383fdf3d2 100644 --- a/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql +++ b/packages/postgraphile-core/__tests__/queries/base/distinct_query_builder.toys.test.graphql @@ -39,4 +39,47 @@ color } } + t6: allToys(distinct: ["color"], orderBy: [NAME_ASC, COLOR_DESC], first: 1) { + totalCount + nodes { + id + name + color + } + pageInfo { + endCursor + } + } + t7: allToys( + distinct: ["color"] + orderBy: [NAME_ASC, COLOR_DESC] + first: 1 + after: "WyJuYW1lX2FzYyIsImNvbG9yX2Rlc2MiLFsicmVkIiwiRGluby1Sb2NrZXQgTGF1bmNoZXIiLDNdXQ==" # cursor from t6 + ) { + totalCount + nodes { + id + name + color + } + pageInfo { + endCursor + } + } + t8: allToys( + distinct: ["color"] + orderBy: [NAME_ASC, COLOR_DESC] + last: 1 + before: "WyJuYW1lX2FzYyIsImNvbG9yX2Rlc2MiLFsicmVkIiwiSGlzdG9yeSBvZiBEaW5vc2F1cnMgYm9vayIsNF1d" # cursor from t7 + ) { + totalCount + nodes { + id + name + color + } + pageInfo { + endCursor + } + } }