github/directus
1
1
Fork 0
mirror of https://github.com/directus/directus.git synced 2026-04-25 03:00:53 -04:00
Code Issues Packages Projects Releases Wiki Activity

Fix sorting for nested queries with permissions / filters (#23518)

Browse Source 
* 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 <licitdev@gmail.com>
This commit is contained in:
Hannes Küttner
2024-09-03 18:28:19 +02:00
committed by GitHub
parent 22f7d1080f
commit c88df15c19
9 changed files with 190 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/gorgeous-fans-rule.md Normal file
View File

@@ -0,0 +1,5 @@
---
'@directus/api': patch
---
Fixed SQL query generation for MSSQL queries that use field level permissions and sort

25
api/src/database/helpers/schema/dialects/cockroachdb.ts
View File

@@ -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));
}
}
}

28
api/src/database/helpers/schema/dialects/mssql.ts
View File

@@ -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));
}
}
}

41
api/src/database/helpers/schema/dialects/mysql.ts
View File

@@ -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));
}
}
}

30
api/src/database/helpers/schema/dialects/oracle.ts
View File

@@ -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));
}
}
}

32
api/src/database/helpers/schema/dialects/postgres.ts
View File

@@ -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));
}
}
}

4
api/src/database/helpers/schema/dialects/sqlite.ts
View File

@@ -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
}
}

13
api/src/database/helpers/schema/types.ts
View File

@@ -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
}
}

26
api/src/database/run-ast/lib/get-db-query.ts
View File

@@ -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);
}