From c88df15c19df2a25f507bbe6b5a9f9a16adbe62a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=BCttner?= Date: Tue, 3 Sep 2024 18:28:19 +0200 Subject: [PATCH] Fix sorting for nested queries with permissions / filters (#23518) * Always include inner query sort fields in group by expression of inner query * Add changeset * Move DB vendor specific code into helper * Include MariaDB in comments * Prefix unused vars with underscore --------- Co-authored-by: ian --- .changeset/gorgeous-fans-rule.md | 5 +++ .../helpers/schema/dialects/cockroachdb.ts | 25 ++++++++++- .../database/helpers/schema/dialects/mssql.ts | 28 ++++++++++++- .../database/helpers/schema/dialects/mysql.ts | 41 ++++++++++++++++++- .../helpers/schema/dialects/oracle.ts | 30 +++++++++++++- .../helpers/schema/dialects/postgres.ts | 32 ++++++++++++++- .../helpers/schema/dialects/sqlite.ts | 4 ++ api/src/database/helpers/schema/types.ts | 13 ++++++ api/src/database/run-ast/lib/get-db-query.ts | 26 ++++++++---- 9 files changed, 190 insertions(+), 14 deletions(-) create mode 100644 .changeset/gorgeous-fans-rule.md diff --git a/.changeset/gorgeous-fans-rule.md b/.changeset/gorgeous-fans-rule.md new file mode 100644 index 0000000000..92e0645f57 --- /dev/null +++ b/.changeset/gorgeous-fans-rule.md @@ -0,0 +1,5 @@ +--- +'@directus/api': patch +--- + +Fixed SQL query generation for MSSQL queries that use field level permissions and sort diff --git a/api/src/database/helpers/schema/dialects/cockroachdb.ts b/api/src/database/helpers/schema/dialects/cockroachdb.ts index 91ed8a0bfb..0bcdbb27a8 100644 --- a/api/src/database/helpers/schema/dialects/cockroachdb.ts +++ b/api/src/database/helpers/schema/dialects/cockroachdb.ts @@ -1,5 +1,6 @@ import type { KNEX_TYPES } from '@directus/constants'; -import type { Options, Sql } from '../types.js'; +import { type Knex } from 'knex'; +import type { Options, SortRecord, Sql } from '../types.js'; import { SchemaHelper } from '../types.js'; import { useEnv } from '@directus/env'; import { preprocessBindings } from '../utils/preprocess-bindings.js'; @@ -43,4 +44,26 @@ export class SchemaHelperCockroachDb extends SchemaHelper { override preprocessBindings(queryParams: Sql): Sql { return preprocessBindings(queryParams, { format: (index) => `$${index + 1}` }); } + + override addInnerSortFieldsToGroupBy( + groupByFields: (string | Knex.Raw)[], + sortRecords: SortRecord[], + hasMultiRelationalSort: boolean, + ) { + if (hasMultiRelationalSort) { + /* + Cockroach allows aliases to be used in the GROUP BY clause and only needs columns in the GROUP BY clause that + are not functionally dependent on the primary key. + + > You can group columns by an alias (i.e., a label assigned to the column with an AS clause) rather than the column name. + + > If aggregate groups are created on a full primary key, any column in the table can be selected as a target_elem, + or specified in a HAVING clause. + + https://www.cockroachlabs.com/docs/stable/select-clause#parameters + */ + + groupByFields.push(...sortRecords.map(({ alias }) => alias)); + } + } } diff --git a/api/src/database/helpers/schema/dialects/mssql.ts b/api/src/database/helpers/schema/dialects/mssql.ts index c6ed9e8433..729b390a85 100644 --- a/api/src/database/helpers/schema/dialects/mssql.ts +++ b/api/src/database/helpers/schema/dialects/mssql.ts @@ -1,5 +1,5 @@ import type { Knex } from 'knex'; -import { SchemaHelper, type Sql } from '../types.js'; +import { SchemaHelper, type SortRecord, type Sql } from '../types.js'; import { preprocessBindings } from '../utils/preprocess-bindings.js'; export class SchemaHelperMSSQL extends SchemaHelper { @@ -35,4 +35,30 @@ export class SchemaHelperMSSQL extends SchemaHelper { override preprocessBindings(queryParams: Sql): Sql { return preprocessBindings(queryParams, { format: (index) => `@p${index}` }); } + + override addInnerSortFieldsToGroupBy( + groupByFields: (string | Knex.Raw)[], + sortRecords: SortRecord[], + _hasMultiRelationalSort: boolean, + ) { + /* + MSSQL requires all selected columns that are not aggregated over are to be present in the GROUP BY clause + + > When the select list has no aggregations, each column in the select list must be included in the GROUP BY list. + + https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16#g-syntax-variations-for-group-by + + MSSQL does not support aliases in the GROUP BY clause + + > The column expression cannot contain: + A column alias that is defined in the SELECT list. It can use a column alias for a derived table that is defined + in the FROM clause. + + https://learn.microsoft.com/en-us/sql/t-sql/queries/select-group-by-transact-sql?view=sql-server-ver16 + */ + + if (sortRecords.length > 0) { + groupByFields.push(...sortRecords.map(({ column }) => column)); + } + } } diff --git a/api/src/database/helpers/schema/dialects/mysql.ts b/api/src/database/helpers/schema/dialects/mysql.ts index 707d6905bf..d6f04c1e4b 100644 --- a/api/src/database/helpers/schema/dialects/mysql.ts +++ b/api/src/database/helpers/schema/dialects/mysql.ts @@ -1,7 +1,7 @@ import { useEnv } from '@directus/env'; import type { Knex } from 'knex'; import { getDatabaseVersion } from '../../../index.js'; -import { SchemaHelper } from '../types.js'; +import { SchemaHelper, type SortRecord } from '../types.js'; const env = useEnv(); @@ -49,4 +49,43 @@ export class SchemaHelperMySQL extends SchemaHelper { return null; } } + + override addInnerSortFieldsToGroupBy( + groupByFields: (string | Knex.Raw)[], + sortRecords: SortRecord[], + hasMultiRelationalSort: boolean, + ) { + if (hasMultiRelationalSort) { + /* + ** MySQL ** + + MySQL only requires all selected sort columns that are not functionally dependent on the primary key to be included. + + > If the ONLY_FULL_GROUP_BY SQL mode is enabled (which it is by default), + MySQL rejects queries for which the select list, HAVING condition, or ORDER BY list refer to + nonaggregated columns that are neither named in the GROUP BY clause nor are functionally dependent on them. + + https://dev.mysql.com/doc/refman/8.4/en/group-by-handling.html + + MySQL allows aliases to be used in the GROUP BY clause + + > You can use the alias in GROUP BY, ORDER BY, or HAVING clauses to refer to the column: + + https://dev.mysql.com/doc/refman/8.4/en/problems-with-alias.html + + ** MariaDB ** + + MariaDB does not document how it supports functional dependent columns in GROUP BY clauses. + But testing shows that it does support the same features as MySQL in this area. + + MariaDB allows aliases to be used in the GROUP BY clause + + > The GROUP BY expression can be a computed value, and can refer back to an identifer specified with AS. + + https://mariadb.com/kb/en/group-by/#group-by-examples + */ + + groupByFields.push(...sortRecords.map(({ alias }) => alias)); + } + } } diff --git a/api/src/database/helpers/schema/dialects/oracle.ts b/api/src/database/helpers/schema/dialects/oracle.ts index 52ee76162c..c005682898 100644 --- a/api/src/database/helpers/schema/dialects/oracle.ts +++ b/api/src/database/helpers/schema/dialects/oracle.ts @@ -1,6 +1,7 @@ import type { KNEX_TYPES } from '@directus/constants'; import type { Field, Relation, Type } from '@directus/types'; -import type { Options, Sql } from '../types.js'; +import type { Knex } from 'knex'; +import type { Options, SortRecord, Sql } from '../types.js'; import { SchemaHelper } from '../types.js'; import { preprocessBindings } from '../utils/preprocess-bindings.js'; @@ -55,4 +56,31 @@ export class SchemaHelperOracle extends SchemaHelper { override preprocessBindings(queryParams: Sql): Sql { return preprocessBindings(queryParams, { format: (index) => `:${index + 1}` }); } + + override addInnerSortFieldsToGroupBy( + groupByFields: (string | Knex.Raw)[], + sortRecords: SortRecord[], + _hasMultiRelationalSort: boolean, + ) { + /* + Oracle requires all selected columns that are not aggregated over to be present in the GROUP BY clause + aliases can not be used before version 23c. + + > If you also specify a group_by_clause in this statement, then this select list can contain only the following + types of expressions: + * Constants + * Aggregate functions and the functions USER, UID, and SYSDATE + * Expressions identical to those in the group_by_clause. If the group_by_clause is in a subquery, + then all columns in the select list of the subquery must match the GROUP BY columns in the subquery. + If the select list and GROUP BY columns of a top-level query or of a subquery do not match, + then the statement results in ORA-00979. + * Expressions involving the preceding expressions that evaluate to the same value for all rows in a group + + https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/SELECT.html + */ + + if (sortRecords.length > 0) { + groupByFields.push(...sortRecords.map(({ column }) => column)); + } + } } diff --git a/api/src/database/helpers/schema/dialects/postgres.ts b/api/src/database/helpers/schema/dialects/postgres.ts index 993bcd8f5f..4340a45616 100644 --- a/api/src/database/helpers/schema/dialects/postgres.ts +++ b/api/src/database/helpers/schema/dialects/postgres.ts @@ -1,5 +1,6 @@ import { useEnv } from '@directus/env'; -import { SchemaHelper, type Sql } from '../types.js'; +import type { Knex } from 'knex'; +import { SchemaHelper, type SortRecord, type Sql } from '../types.js'; import { preprocessBindings } from '../utils/preprocess-bindings.js'; const env = useEnv(); @@ -18,4 +19,33 @@ export class SchemaHelperPostgres extends SchemaHelper { override preprocessBindings(queryParams: Sql): Sql { return preprocessBindings(queryParams, { format: (index) => `$${index + 1}` }); } + + override addInnerSortFieldsToGroupBy( + groupByFields: (string | Knex.Raw)[], + sortRecords: SortRecord[], + hasMultiRelationalSort: boolean, + ) { + if (hasMultiRelationalSort) { + /* + Postgres only requires selected columns that are not functionally dependent on the primary key to be + included in the GROUP BY clause. Since the results are already grouped by the primary key, we don't need to + always include the sort columns in the GROUP BY but only if there is a multi relational sort involved, eg. + a sort column that comes from a related M2O relation. + + > When GROUP BY is present, or any aggregate functions are present, it is not valid for the SELECT list + expressions to refer to ungrouped columns except within aggregate functions or when the ungrouped column is + functionally dependent on the grouped columns, since there would otherwise be more than one possible value to + return for an ungrouped column. + https://www.postgresql.org/docs/current/sql-select.html + + Postgres allows aliases to be used in the GROUP BY clause + + > In strict SQL, GROUP BY can only group by columns of the source table but PostgreSQL extends this to also allow + GROUP BY to group by columns in the select list. + https://www.postgresql.org/docs/16/queries-table-expressions.html#QUERIES-GROUP + */ + + groupByFields.push(...sortRecords.map(({ alias }) => alias)); + } + } } diff --git a/api/src/database/helpers/schema/dialects/sqlite.ts b/api/src/database/helpers/schema/dialects/sqlite.ts index d8908d14f4..41fd744d68 100644 --- a/api/src/database/helpers/schema/dialects/sqlite.ts +++ b/api/src/database/helpers/schema/dialects/sqlite.ts @@ -26,4 +26,8 @@ export class SchemaHelperSQLite extends SchemaHelper { return null; } } + + override addInnerSortFieldsToGroupBy() { + // SQLite does not need any special handling for inner query sort columns + } } diff --git a/api/src/database/helpers/schema/types.ts b/api/src/database/helpers/schema/types.ts index 1d9136ab13..606c05b8dc 100644 --- a/api/src/database/helpers/schema/types.ts +++ b/api/src/database/helpers/schema/types.ts @@ -12,6 +12,11 @@ export type Sql = { bindings: readonly Knex.Value[]; }; +export type SortRecord = { + alias: string; + column: Knex.Raw; +}; + export abstract class SchemaHelper extends DatabaseHelper { isOneOfClients(clients: DatabaseClient[]): boolean { return clients.includes(getDatabaseClient(this.knex)); @@ -155,4 +160,12 @@ export abstract class SchemaHelper extends DatabaseHelper { preprocessBindings(queryParams: Sql): Sql { return queryParams; } + + addInnerSortFieldsToGroupBy( + _groupByFields: (string | Knex.Raw)[], + _sortRecords: SortRecord[], + _hasMultiRelationalSort: boolean, + ): void { + // no-op by default + } } diff --git a/api/src/database/run-ast/lib/get-db-query.ts b/api/src/database/run-ast/lib/get-db-query.ts index 08a80f7d11..389756e311 100644 --- a/api/src/database/run-ast/lib/get-db-query.ts +++ b/api/src/database/run-ast/lib/get-db-query.ts @@ -61,7 +61,7 @@ export function getDBQuery( const primaryKey = schema.collections[table]!.primary; let dbQuery = knex.from(table); let sortRecords: ColumnSortRecord[] | undefined; - const innerQuerySortRecords: { alias: string; order: 'asc' | 'desc' }[] = []; + const innerQuerySortRecords: { alias: string; order: 'asc' | 'desc'; column: Knex.Raw }[] = []; let hasMultiRelationalSort: boolean | undefined; if (queryCopy.sort) { @@ -126,21 +126,24 @@ export function getDBQuery( const sortAlias = `sort_${generateAlias()}`; + let orderByColumn: Knex.Raw; + if (sortRecord.column.includes('.')) { const [alias, field] = sortRecord.column.split('.'); const originalCollectionName = getCollectionFromAlias(alias!, aliasMap); dbQuery.select(getColumn(knex, alias!, field!, sortAlias, schema, { originalCollectionName })); orderByString += `?? ${sortRecord.order}`; - orderByFields.push(getColumn(knex, alias!, field!, false, schema, { originalCollectionName })); + orderByColumn = getColumn(knex, alias!, field!, false, schema, { originalCollectionName }); } else { dbQuery.select(getColumn(knex, table, sortRecord.column, sortAlias, schema)); orderByString += `?? ${sortRecord.order}`; - orderByFields.push(getColumn(knex, table, sortRecord.column, false, schema)); + orderByColumn = getColumn(knex, table, sortRecord.column, false, schema); } - innerQuerySortRecords.push({ alias: sortAlias, order: sortRecord.order }); + orderByFields.push(orderByColumn); + innerQuerySortRecords.push({ alias: sortAlias, order: sortRecord.order, column: orderByColumn }); }); if (hasMultiRelationalSort) { @@ -238,11 +241,16 @@ export function getDBQuery( const groupByFields = [knex.raw('??.??', [table, primaryKey])]; - if (hasMultiRelationalSort) { - // Sort fields that are not directly in the table the primary key is from need to be included in the group - // by clause, otherwise this causes problems on some DBs - groupByFields.push(...innerQuerySortRecords.map(({ alias }) => knex.raw('??', alias))); - } + // For some DB vendors sort fields need to be included in the group by clause, otherwise this causes problems those DBs + // since sort fields are selected in the inner query, and they expect all selected columns to be in + // the group by clause or aggregated over. + // For some DBs the field needs to be the actual raw column expression, since aliases are not available in the + // group by clause. + // Since the fields are expected to be the same for a single primary key it is safe to include them in the + // group by without influencing the result. + + // This inclusion depends on the DB vendor, as such it is handled in a dialect specific helper. + helpers.schema.addInnerSortFieldsToGroupBy(groupByFields, innerQuerySortRecords, hasMultiRelationalSort ?? false); dbQuery.groupBy(groupByFields); }