clone/nocodb
1
0
Fork 0
mirror of https://github.com/nocodb/nocodb.git synced 2026-04-25 04:26:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revert "fix: Group by - proper handling of blank values"

Browse Source
This commit is contained in:
Pranav C
2025-09-29 08:36:41 +00:00
parent 664b37ea8b
commit 016ef78b24
3 changed files with 71 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
packages/nocodb/src/db/BaseModelSqlv2.ts
View File

@@ -26,7 +26,6 @@ import {
NcApiVersion,
NcErrorType,
ncIsNull,
ncIsNullOrUndefined,
ncIsObject,
ncIsUndefined,
PermissionEntity,
@@ -6612,7 +6611,7 @@ class BaseModelSqlv2 implements IBaseModelSqlV2 {
column.uidt,
)
) {
if (!ncIsNullOrUndefined(data[column.column_name])) {
if (data[column.column_name]) {
const userIds = [];
if (

152
packages/nocodb/src/db/BaseModelSqlv2/group-by.ts
View File

@@ -25,33 +25,6 @@ import { BaseUser, Column, Filter, Sort } from '~/models';
import { getAliasGenerator, isOnPrem } from '~/utils';
import { replaceDelimitedWithKeyValueSqlite3 } from '~/db/aggregations/sqlite3';
// Returns a SQL expression that converts blank (null or '') values to NULL
const sqlNullIfBlank = ({
baseModel,
columnName,
isStringType = false,
}: {
baseModel: IBaseModelSqlV2;
columnName: string | Knex.QueryBuilder | Knex.Raw;
isStringType?: boolean;
}) => {
if (baseModel.isPg && !isStringType) {
return baseModel.dbDriver.raw(
`CASE
WHEN (pg_typeof(:column:) = 'text'::regtype
OR pg_typeof(:column:) = 'varchar'::regtype
OR pg_typeof(:column:) = 'char'::regtype)
AND (:column:)::text = ''
THEN NULL
ELSE :column:
END`,
{ column: columnName },
);
}
return baseModel.dbDriver.raw(`NULLIF(??, '')`, [columnName]);
};
export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
const list = async (args: {
where?: string;
@@ -140,11 +113,7 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
(column.colOptions as FormulaColumn).getParsedTree().dataType ===
FormulaDataTypes.STRING
) {
columnQuery = sqlNullIfBlank({
columnName: baseModel.dbDriver.raw(`??::text`, [columnQuery]),
baseModel,
isStringType: true,
});
columnQuery = baseModel.dbDriver.raw(`??::text`, [columnQuery]);
}
} catch (e) {
logger.log(e);
@@ -233,14 +202,10 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
column,
columns,
);
const defaultColumnNameQb = sqlNullIfBlank({
columnName: defaultColumnName,
baseModel,
});
columnQuery = baseModel.dbDriver.raw('??', [defaultColumnNameQb]);
columnQuery = baseModel.dbDriver.raw('??', [defaultColumnName]);
if (!isSubGroup) {
selectors.push(
baseModel.dbDriver.raw(`?? as ??`, [defaultColumnNameQb, alias]),
baseModel.dbDriver.raw(`?? as ??`, [defaultColumnName, alias]),
);
}
break;
@@ -262,11 +227,9 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
const subGroupQuery = await processColumn(subGroupColumnName, true);
qb.select(
baseModel.dbDriver.raw(
`COUNT(DISTINCT COALESCE(${sqlNullIfBlank({
columnName: baseModel.isPg ? '(??)::text' : '??',
baseModel,
isStringType: true,
})}, '__null__')) as ??`,
`COUNT(DISTINCT COALESCE(${
baseModel.isPg ? '(??)::text' : '??'
}, '__null__')) as ??`,
[baseModel.dbDriver.raw(subGroupQuery), '__sub_group_count__'],
),
);
@@ -332,26 +295,20 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
}
}
// group by using the column aliases
qb.groupBy(...groupBySelectors);
// Wrap in a CTE to allow referencing grouped/aliased columns in subqueries (esp. for Postgres)
// We'll use: WITH grouped AS (<qb>) SELECT ... FROM grouped g
const outerQb = baseModel.dbDriver
.with('grouped', qb.clone())
.select('*')
.from({ g: 'grouped' });
if (!isOnPrem) {
applyPaginate(outerQb, rest);
}
// Apply order by on the outer query, referencing g.<alias>
// if sort is provided filter out the group by columns sort and apply
// since we are grouping by the column and applying sort on any other column is not required
for (const sort of sorts || []) {
if (!groupByColumns[sort.fk_column_id]) {
continue;
}
const column = groupByColumns[sort.fk_column_id];
const columnName = await getColumnName(
baseModel.context,
column,
columns,
);
if (
[UITypes.User, UITypes.CreatedBy, UITypes.LastModifiedBy].includes(
column.uidt as UITypes,
@@ -361,17 +318,12 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
base_id: column.base_id,
include_internal_user: true,
});
const groupedCol = getAs(column);
const groupedColQb = sqlNullIfBlank({
columnName: baseModel.dbDriver.raw('??.??', ['g', groupedCol]),
baseModel,
isStringType: true,
});
let finalStatement = '';
if (baseModel.dbDriver.clientType() === 'pg') {
finalStatement = `(${replaceDelimitedWithKeyValuePg({
knex: baseModel.dbDriver,
needleColumn: groupedColQb as any,
needleColumn: columnName,
stack: baseUsers.map((user) => ({
key: user.id,
value: user.display_name || user.email,
@@ -380,34 +332,35 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
} else if (baseModel.dbDriver.clientType() === 'sqlite3') {
finalStatement = `(${replaceDelimitedWithKeyValueSqlite3({
knex: baseModel.dbDriver,
needleColumn: groupedColQb as any,
needleColumn: columnName,
stack: baseUsers.map((user) => ({
key: user.id,
value: user.display_name || user.email,
})),
})})`;
} else {
// use the original replace
finalStatement = baseUsers.reduce((acc, user) => {
const qbReplace = baseModel.dbDriver.raw(`REPLACE(${acc}, ?, ?)`, [
const qb = baseModel.dbDriver.raw(`REPLACE(${acc}, ?, ?)`, [
user.id,
user.display_name || user.email,
]);
return qbReplace.toQuery();
}, groupedColQb.toQuery());
return qb.toQuery();
}, baseModel.dbDriver.raw(`??`, [columnName]).toQuery());
}
if (!['asc', 'desc'].includes(sort.direction)) {
outerQb.orderBy(
'g.count',
qb.orderBy(
'count',
sort.direction === 'count-desc' ? 'desc' : 'asc',
sort.direction === 'count-desc' ? 'LAST' : 'FIRST',
);
outerQb.orderBy(
qb.orderBy(
sanitize(baseModel.dbDriver.raw(finalStatement)),
sort.direction,
'FIRST',
sort.direction === 'desc' ? 'LAST' : 'FIRST',
);
} else {
outerQb.orderBy(
qb.orderBy(
sanitize(baseModel.dbDriver.raw(finalStatement)),
sort.direction,
sort.direction === 'desc' ? 'LAST' : 'FIRST',
@@ -415,19 +368,19 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
}
} else {
if (!['asc', 'desc'].includes(sort.direction)) {
outerQb.orderBy(
'g.count',
qb.orderBy(
'count',
sort.direction === 'count-desc' ? 'desc' : 'asc',
sort.direction === 'count-desc' ? 'LAST' : 'FIRST',
);
outerQb.orderBy(
baseModel.dbDriver.raw('??.??', ['g', getAs(column)]) as any,
qb.orderBy(
getAs(column),
sort.direction,
'FIRST',
sort.direction === 'desc' ? 'LAST' : 'FIRST',
);
} else {
outerQb.orderBy(
baseModel.dbDriver.raw('??.??', ['g', getAs(column)]) as any,
qb.orderBy(
getAs(column),
sort.direction,
sort.direction === 'desc' ? 'LAST' : 'FIRST',
);
@@ -435,7 +388,14 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
}
}
return await baseModel.execAndParse(outerQb);
// group by using the column aliases
qb.groupBy(...groupBySelectors);
if (!isOnPrem) {
applyPaginate(qb, rest);
}
return await baseModel.execAndParse(qb);
};
const count = async (args: {
@@ -510,10 +470,7 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
);
selectQb = baseModel.dbDriver.raw(`?? as ??`, [
sqlNullIfBlank({
columnName: _selectQb.builder,
baseModel,
}),
_selectQb.builder,
getAs(column),
]);
} catch (e) {
@@ -631,10 +588,7 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
columns,
);
selectors.push(
baseModel.dbDriver.raw('?? as ??', [
sqlNullIfBlank({ columnName, baseModel }),
getAs(column),
]),
baseModel.dbDriver.raw('?? as ??', [columnName, getAs(column)]),
);
groupBySelectors.push(getAs(column));
}
@@ -643,7 +597,6 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
}),
);
// Build the group-by query
const qb = baseModel.dbDriver(baseModel.tnPath);
qb.count(`${baseModel.model.primaryKey?.column_name || '*'} as count`);
qb.select(...selectors);
@@ -688,15 +641,9 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
qb.groupBy(...groupBySelectors);
// Wrap in a CTE so that we can reference grouped columns safely in all engines
// SELECT COUNT(*) FROM (WITH grouped AS (<qb>) SELECT * FROM grouped g) sub
const groupedCte = baseModel.dbDriver
.with('grouped', qb.clone())
.select('*')
.from({ g: 'grouped' });
const qbP = baseModel.dbDriver
.count('*', { as: 'count' })
.from(groupedCte.as('sub'));
.from(qb.as('groupby'));
return (await baseModel.execAndParse(qbP, null, { raw: true, first: true }))
?.count;
@@ -1319,13 +1266,6 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
column,
columns,
);
const columnNameQb = sqlNullIfBlank({
columnName,
baseModel,
isStringType: true,
});
const baseUsers = await BaseUser.getUsersList(baseModel.context, {
base_id: column.base_id,
include_internal_user: true,
@@ -1338,7 +1278,7 @@ export const groupBy = (baseModel: IBaseModelSqlV2, logger: Logger) => {
user.display_name || user.email,
]);
return qb.toQuery();
}, columnNameQb.toQuery());
}, baseModel.dbDriver.raw(`??`, [columnName]).toQuery());
if (!['asc', 'desc'].includes(sort.direction)) {
tQb.orderBy(

48
tests/playwright/tests/db/general/groupCRUD.spec.ts
View File

@@ -49,10 +49,10 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.groupBy.add({ title: 'Category', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.openGroup({ indexMap: [3] });
await dashboard.grid.groupPage.openGroup({ indexMap: [2] });
await dashboard.grid.groupPage.addNewRow({
indexMap: [3],
indexMap: [2],
index: 10,
columnHeader: 'Sub_Group',
value: 'Aaaaaaaaaaaaaaaaaaaa',
@@ -65,13 +65,13 @@ test.describe('GroupBy CRUD Operations', () => {
//await toolbar.sort.add({title: 'Sub_Group', ascending: true, locallySaved: true});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3],
indexMap: [2],
rowIndex: 0,
columnHeader: 'Sub_Group',
value: 'Aaaaaaaaaaaaaaaaaaaa',
});
await dashboard.grid.groupPage.editRow({
indexMap: [3],
indexMap: [2],
rowIndex: 0,
columnHeader: 'Sub_Group',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -80,7 +80,7 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.sort.update({ index: 1, title: 'Sub_Group', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3],
indexMap: [2],
rowIndex: 0,
columnHeader: 'Sub_Group',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -88,12 +88,12 @@ test.describe('GroupBy CRUD Operations', () => {
await dashboard.grid.groupPage.deleteRow({
title: 'Sub_Group',
indexMap: [3],
indexMap: [2],
rowIndex: 0,
});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3],
indexMap: [2],
rowIndex: 0,
columnHeader: 'Sub_Group',
value: 'Angola',
@@ -102,7 +102,7 @@ test.describe('GroupBy CRUD Operations', () => {
await undo({ page, dashboard });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3],
indexMap: [2],
rowIndex: 0,
columnHeader: 'Sub_Group',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -117,10 +117,10 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.groupBy.add({ title: 'Category', ascending: false, locallySaved: false });
await toolbar.groupBy.add({ title: 'Sub_Group', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.openGroup({ indexMap: [3, 0] });
await dashboard.grid.groupPage.openGroup({ indexMap: [2, 0] });
await dashboard.grid.groupPage.addNewRow({
indexMap: [3, 0],
indexMap: [2, 0],
index: 10,
columnHeader: 'Sub_Category',
value: 'Aaaaaaaaaaaaaaaaaaaa',
@@ -133,13 +133,13 @@ test.describe('GroupBy CRUD Operations', () => {
//await toolbar.sort.add({title: 'Sub_Group', ascending: true, locallySaved: true});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
columnHeader: 'Sub_Category',
value: 'Aaaaaaaaaaaaaaaaaaaa',
});
await dashboard.grid.groupPage.editRow({
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
columnHeader: 'Sub_Category',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -148,7 +148,7 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.sort.update({ index: 1, title: 'Sub_Category', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
columnHeader: 'Sub_Category',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -156,12 +156,12 @@ test.describe('GroupBy CRUD Operations', () => {
await dashboard.grid.groupPage.deleteRow({
title: 'Sub_Category',
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
columnHeader: 'Sub_Category',
value: 'Afghanistan',
@@ -170,7 +170,7 @@ test.describe('GroupBy CRUD Operations', () => {
await undo({ page, dashboard });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0],
indexMap: [2, 0],
rowIndex: 0,
columnHeader: 'Sub_Category',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -186,10 +186,10 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.groupBy.add({ title: 'Sub_Group', ascending: false, locallySaved: false });
await toolbar.groupBy.add({ title: 'Sub_Category', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.openGroup({ indexMap: [3, 0, 0] });
await dashboard.grid.groupPage.openGroup({ indexMap: [2, 0, 0] });
await dashboard.grid.groupPage.addNewRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
index: 10,
columnHeader: 'Item',
value: 'Aaaaaaaaaaaaaaaaaaaa',
@@ -202,13 +202,13 @@ test.describe('GroupBy CRUD Operations', () => {
//await toolbar.sort.add({title: 'Sub_Group', ascending: true, locallySaved: true});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
columnHeader: 'Item',
value: 'Aaaaaaaaaaaaaaaaaaaa',
});
await dashboard.grid.groupPage.editRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
columnHeader: 'Item',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -217,7 +217,7 @@ test.describe('GroupBy CRUD Operations', () => {
await toolbar.sort.update({ index: 1, title: 'Item', ascending: false, locallySaved: false });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
columnHeader: 'Item',
value: 'Zzzzzzzzzzzzzzzzzzz',
@@ -225,12 +225,12 @@ test.describe('GroupBy CRUD Operations', () => {
await dashboard.grid.groupPage.deleteRow({
title: 'Item',
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
});
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
columnHeader: 'Item',
value: 'Argentina',
@@ -239,7 +239,7 @@ test.describe('GroupBy CRUD Operations', () => {
await undo({ page, dashboard });
await dashboard.grid.groupPage.validateFirstRow({
indexMap: [3, 0, 0],
indexMap: [2, 0, 0],
rowIndex: 0,
columnHeader: 'Item',
value: 'Zzzzzzzzzzzzzzzzzzz',