Skip to content

Commit b3fa8ed

Browse files
nichenqinnichenqin
authored
fix: fix rollup expression regression (#2023)
* fix: fix rollup expression regression * fix: fix date filter with field reference component
1 parent e9a39a3 commit b3fa8ed

9 files changed

Lines changed: 459 additions & 82 deletions

File tree

‎apps/nestjs-backend/src/features/record/query-builder/field-cte-visitor.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1080,7 +1080,6 @@ export class FieldCteVisitor implements IFieldVisitor<ICteResult> {
10801080
switch (fn) {
10811081
case 'array_join':
10821082
case 'array_compact':
1083-
case 'array_unique':
10841083
case 'concatenate':
10851084
return true;
10861085
default:

‎apps/nestjs-backend/src/features/record/query-builder/providers/pg-record-query-dialect.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -237,15 +237,12 @@ export class PgRecordQueryDialect implements IRecordQueryDialectProvider {
237237
? `STRING_AGG(${fieldExpression}::text, ', ' ORDER BY ${orderByField})`
238238
: `STRING_AGG(${fieldExpression}::text, ', ')`;
239239
case 'array_unique':
240-
if (orderByField) {
241-
return `(SELECT json_agg(val) FROM (SELECT DISTINCT ${fieldExpression} AS val ORDER BY ${orderByField}) __teable_rollup_unique)`;
242-
}
243240
return `json_agg(DISTINCT ${fieldExpression})`;
244241
case 'array_compact': {
245242
const buildAggregate = (expr: string) =>
246243
orderByField
247-
? `jsonb_agg(${expr} ORDER BY ${orderByField}) FILTER (WHERE ${expr} IS NOT NULL)`
248-
: `jsonb_agg(${expr}) FILTER (WHERE ${expr} IS NOT NULL)`;
244+
? `jsonb_agg(${expr} ORDER BY ${orderByField}) FILTER (WHERE (${expr}) IS NOT NULL AND (${expr})::text <> '')`
245+
: `jsonb_agg(${expr}) FILTER (WHERE (${expr}) IS NOT NULL AND (${expr})::text <> '')`;
249246
const baseAggregate = buildAggregate(fieldExpression);
250247
if (flattenNestedArray) {
251248
return `(WITH RECURSIVE flattened(val) AS (
@@ -256,7 +253,11 @@ export class PgRecordQueryDialect implements IRecordQueryDialectProvider {
256253
CROSS JOIN LATERAL jsonb_array_elements(flattened.val) AS elem
257254
WHERE jsonb_typeof(flattened.val) = 'array'
258255
)
259-
SELECT jsonb_agg(val) FILTER (WHERE jsonb_typeof(val) <> 'array') FROM flattened)`;
256+
SELECT jsonb_agg(val) FILTER (
257+
WHERE jsonb_typeof(val) <> 'array'
258+
AND jsonb_typeof(val) <> 'null'
259+
AND val <> '""'::jsonb
260+
) FROM flattened)`;
260261
}
261262
return baseAggregate;
262263
}

‎apps/nestjs-backend/src/features/record/query-builder/providers/sqlite-record-query-dialect.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class SqliteRecordQueryDialect implements IRecordQueryDialectProvider {
167167
case 'array_unique':
168168
return `json_group_array(DISTINCT ${fieldExpression})`;
169169
case 'array_compact':
170-
return `json_group_array(CASE WHEN ${fieldExpression} IS NOT NULL THEN ${fieldExpression} END)`;
170+
return `json_group_array(CASE WHEN ${fieldExpression} IS NOT NULL AND CAST(${fieldExpression} AS TEXT) <> '' THEN ${fieldExpression} END)`;
171171
default:
172172
throw new Error(`Unsupported rollup function: ${fn}`);
173173
}

‎apps/nestjs-backend/test/conditional-rollup.e2e-spec.ts‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,145 @@ describe('OpenAPI Conditional Rollup field (e2e)', () => {
5454
await app.close();
5555
});
5656

57+
describe('expression coverage', () => {
58+
const setupConditionalRollupFixtures = async () => {
59+
const foreign = await createTable(baseId, {
60+
name: 'ConditionalRollupExpr_Foreign',
61+
fields: [
62+
{ name: 'Label', type: FieldType.SingleLineText } as IFieldRo,
63+
{ name: 'Amount', type: FieldType.Number } as IFieldRo,
64+
{ name: 'Flag', type: FieldType.Checkbox } as IFieldRo,
65+
],
66+
records: [
67+
{ fields: { Label: 'Alpha', Amount: 10, Flag: true } },
68+
{ fields: { Label: 'Alpha', Amount: null, Flag: true } },
69+
{ fields: { Label: 'Beta', Amount: 20, Flag: true } },
70+
],
71+
});
72+
73+
const host = await createTable(baseId, {
74+
name: 'ConditionalRollupExpr_Host',
75+
fields: [{ name: 'Name', type: FieldType.SingleLineText } as IFieldRo],
76+
records: [{ fields: { Name: 'Host Row' } }],
77+
});
78+
79+
const linkField = await createField(host.id, {
80+
name: 'Links',
81+
type: FieldType.Link,
82+
options: {
83+
relationship: Relationship.OneMany,
84+
foreignTableId: foreign.id,
85+
},
86+
} as IFieldRo);
87+
88+
const hostRecordId = host.records[0].id;
89+
await updateRecordByApi(host.id, hostRecordId, linkField.id, [
90+
{ id: foreign.records[0].id },
91+
{ id: foreign.records[1].id },
92+
{ id: foreign.records[2].id },
93+
]);
94+
95+
const labelId = foreign.fields.find((field) => field.name === 'Label')!.id;
96+
const amountId = foreign.fields.find((field) => field.name === 'Amount')!.id;
97+
const flagId = foreign.fields.find((field) => field.name === 'Flag')!.id;
98+
return { foreign, host, linkField, hostRecordId, labelId, amountId, flagId };
99+
};
100+
101+
const conditionalRollupCases: Array<{
102+
expression: string;
103+
lookupFieldKey: 'labelId' | 'amountId' | 'flagId';
104+
expected: unknown;
105+
}> = [
106+
{ expression: 'countall({values})', lookupFieldKey: 'amountId', expected: 3 },
107+
{ expression: 'counta({values})', lookupFieldKey: 'labelId', expected: 3 },
108+
{ expression: 'count({values})', lookupFieldKey: 'amountId', expected: 2 },
109+
{ expression: 'sum({values})', lookupFieldKey: 'amountId', expected: 30 },
110+
{ expression: 'average({values})', lookupFieldKey: 'amountId', expected: 15 },
111+
{ expression: 'max({values})', lookupFieldKey: 'amountId', expected: 20 },
112+
{ expression: 'min({values})', lookupFieldKey: 'amountId', expected: 10 },
113+
{ expression: 'and({values})', lookupFieldKey: 'flagId', expected: true },
114+
{ expression: 'or({values})', lookupFieldKey: 'flagId', expected: true },
115+
{ expression: 'xor({values})', lookupFieldKey: 'flagId', expected: true },
116+
{
117+
expression: 'array_join({values})',
118+
lookupFieldKey: 'labelId',
119+
expected: 'Alpha, Alpha, Beta',
120+
},
121+
{
122+
expression: 'array_unique({values})',
123+
lookupFieldKey: 'labelId',
124+
expected: ['Alpha', 'Beta'],
125+
},
126+
{
127+
expression: 'array_compact({values})',
128+
lookupFieldKey: 'labelId',
129+
expected: ['Alpha', 'Alpha', 'Beta'],
130+
},
131+
{
132+
expression: 'concatenate({values})',
133+
lookupFieldKey: 'labelId',
134+
expected: 'Alpha, Alpha, Beta',
135+
},
136+
];
137+
138+
it.each(conditionalRollupCases)(
139+
'should support conditional rollup expression %s without filters',
140+
async ({ expression, lookupFieldKey, expected }) => {
141+
let fixtures: Awaited<ReturnType<typeof setupConditionalRollupFixtures>> | undefined;
142+
try {
143+
fixtures = await setupConditionalRollupFixtures();
144+
const { foreign, host, linkField, hostRecordId } = fixtures;
145+
const lookupFieldId = fixtures[lookupFieldKey];
146+
147+
const field = await createField(host.id, {
148+
name: `conditional rollup ${expression}`,
149+
type: FieldType.ConditionalRollup,
150+
options: {
151+
foreignTableId: foreign.id,
152+
lookupFieldId,
153+
expression,
154+
filter: {
155+
conjunction: 'and',
156+
filterSet: [
157+
{
158+
fieldId: fixtures.labelId,
159+
operator: 'isNotEmpty',
160+
value: null,
161+
},
162+
],
163+
},
164+
},
165+
} as IFieldRo);
166+
167+
const record = await getRecord(host.id, hostRecordId);
168+
const value = record.fields[field.id];
169+
170+
if (Array.isArray(expected)) {
171+
expect(Array.isArray(value)).toBe(true);
172+
const sortedExpected = [...expected].sort();
173+
const sortedValue = [...(value as unknown[])].sort();
174+
expect(sortedValue).toEqual(sortedExpected);
175+
} else if (typeof expected === 'string') {
176+
if (expected.includes(', ')) {
177+
expect((value as string).split(', ').sort()).toEqual(expected.split(', ').sort());
178+
} else {
179+
expect(value).toEqual(expected);
180+
}
181+
} else {
182+
expect(value).toEqual(expected);
183+
}
184+
} finally {
185+
if (fixtures?.host) {
186+
await permanentDeleteTable(baseId, fixtures.host.id);
187+
}
188+
if (fixtures?.foreign) {
189+
await permanentDeleteTable(baseId, fixtures.foreign.id);
190+
}
191+
}
192+
}
193+
);
194+
});
195+
57196
describe('table and field retrieval', () => {
58197
let foreign: ITableFullVo;
59198
let host: ITableFullVo;

‎apps/nestjs-backend/test/rollup.e2e-spec.ts‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,166 @@ describe('OpenAPI Rollup field (e2e)', () => {
446446
expect(recordAfter1.fields[rollupFieldVo.id]).toEqual('123, 456');
447447
});
448448

449+
describe('rollup expression coverage', () => {
450+
const baseId = globalThis.testConfig.baseId;
451+
452+
const setupRollupFixtures = async () => {
453+
const foreign = await createTable(baseId, {
454+
name: 'RollupExpr_Foreign',
455+
fields: [
456+
{ name: 'Label', type: FieldType.SingleLineText } as IFieldRo,
457+
{ name: 'Amount', type: FieldType.Number } as IFieldRo,
458+
{ name: 'Flag', type: FieldType.Checkbox } as IFieldRo,
459+
],
460+
records: [
461+
{ fields: { Label: 'Alpha', Amount: 10, Flag: true } },
462+
{ fields: { Label: 'Beta', Amount: 20, Flag: false } },
463+
],
464+
});
465+
466+
const host = await createTable(baseId, {
467+
name: 'RollupExpr_Host',
468+
fields: [{ name: 'Name', type: FieldType.SingleLineText } as IFieldRo],
469+
records: [{ fields: { Name: 'Rollup Holder' } }],
470+
});
471+
472+
const linkField = await createField(host.id, {
473+
name: 'Links',
474+
type: FieldType.Link,
475+
options: {
476+
relationship: Relationship.OneMany,
477+
foreignTableId: foreign.id,
478+
},
479+
} as IFieldRo);
480+
481+
const hostRecordId = host.records[0].id;
482+
await updateRecordField(
483+
host.id,
484+
hostRecordId,
485+
linkField.id,
486+
foreign.records.map((record) => ({ id: record.id }))
487+
);
488+
489+
const amountId = foreign.fields.find((field) => field.name === 'Amount')!.id;
490+
const labelId = foreign.fields.find((field) => field.name === 'Label')!.id;
491+
const flagId = foreign.fields.find((field) => field.name === 'Flag')!.id;
492+
493+
return { foreign, host, linkField, hostRecordId, amountId, labelId, flagId };
494+
};
495+
496+
const rollupCases: Array<{
497+
expression: string;
498+
lookupFieldKey: 'amountId' | 'labelId' | 'flagId';
499+
expected: unknown;
500+
}> = [
501+
{ expression: 'countall({values})', lookupFieldKey: 'amountId', expected: 2 },
502+
{ expression: 'counta({values})', lookupFieldKey: 'labelId', expected: 2 },
503+
{ expression: 'count({values})', lookupFieldKey: 'amountId', expected: 2 },
504+
{ expression: 'sum({values})', lookupFieldKey: 'amountId', expected: 30 },
505+
{ expression: 'average({values})', lookupFieldKey: 'amountId', expected: 15 },
506+
{ expression: 'max({values})', lookupFieldKey: 'amountId', expected: 20 },
507+
{ expression: 'min({values})', lookupFieldKey: 'amountId', expected: 10 },
508+
{ expression: 'and({values})', lookupFieldKey: 'flagId', expected: true },
509+
{ expression: 'or({values})', lookupFieldKey: 'flagId', expected: true },
510+
{ expression: 'xor({values})', lookupFieldKey: 'flagId', expected: true },
511+
{ expression: 'array_join({values})', lookupFieldKey: 'labelId', expected: 'Alpha, Beta' },
512+
{
513+
expression: 'array_unique({values})',
514+
lookupFieldKey: 'labelId',
515+
expected: ['Alpha', 'Beta'],
516+
},
517+
{
518+
expression: 'array_compact({values})',
519+
lookupFieldKey: 'labelId',
520+
expected: ['Alpha', 'Beta'],
521+
},
522+
{ expression: 'concatenate({values})', lookupFieldKey: 'labelId', expected: 'Alpha, Beta' },
523+
];
524+
525+
it.each(rollupCases)(
526+
'should compute rollup using %s',
527+
async ({ expression, lookupFieldKey, expected }) => {
528+
let fixtures: Awaited<ReturnType<typeof setupRollupFixtures>> | undefined;
529+
try {
530+
fixtures = await setupRollupFixtures();
531+
const { foreign, host, linkField, hostRecordId } = fixtures;
532+
const lookupFieldId = fixtures[lookupFieldKey];
533+
534+
const field = await createField(host.id, {
535+
name: `rollup ${expression}`,
536+
type: FieldType.Rollup,
537+
options: { expression },
538+
lookupOptions: {
539+
foreignTableId: foreign.id,
540+
linkFieldId: linkField.id,
541+
lookupFieldId,
542+
} as ILookupOptionsRo,
543+
} as IFieldRo);
544+
545+
const record = await getRecord(host.id, hostRecordId);
546+
const value = record.fields[field.id];
547+
548+
if (Array.isArray(expected)) {
549+
expect(Array.isArray(value)).toBe(true);
550+
const sortedExpected = [...expected].sort();
551+
const sortedValue = [...(value as unknown[])].sort();
552+
expect(sortedValue).toEqual(sortedExpected);
553+
} else if (typeof expected === 'string') {
554+
if (expected.includes(', ')) {
555+
expect((value as string).split(', ').sort()).toEqual(expected.split(', ').sort());
556+
} else {
557+
expect(value).toEqual(expected);
558+
}
559+
} else {
560+
expect(value).toEqual(expected);
561+
}
562+
} finally {
563+
if (fixtures?.host) {
564+
await permanentDeleteTable(baseId, fixtures.host.id);
565+
}
566+
if (fixtures?.foreign) {
567+
await permanentDeleteTable(baseId, fixtures.foreign.id);
568+
}
569+
}
570+
}
571+
);
572+
});
573+
574+
it('should create rollup fields with array join, unique, and compact expressions', async () => {
575+
const textField = getFieldByType(table2.fields, FieldType.SingleLineText);
576+
const linkFieldId = getFieldByType(table1.fields, FieldType.Link).id;
577+
578+
// Link all foreign records to a host record for evaluation
579+
await updateRecordField(table1.id, table1.records[1].id, linkFieldId, [
580+
{ id: table2.records[0].id },
581+
{ id: table2.records[1].id },
582+
{ id: table2.records[2].id },
583+
]);
584+
585+
// Populate duplicate values to verify join & unique behaviours
586+
await updateRecordField(table2.id, table2.records[0].id, textField.id, 'Alpha');
587+
await updateRecordField(table2.id, table2.records[1].id, textField.id, 'Alpha');
588+
await updateRecordField(table2.id, table2.records[2].id, textField.id, 'Beta');
589+
590+
const arrayJoinRollup = await rollupFrom(table1, textField.id, 'array_join({values})');
591+
const arrayUniqueRollup = await rollupFrom(table1, textField.id, 'array_unique({values})');
592+
593+
let record = await getRecord(table1.id, table1.records[1].id);
594+
const joinedValues = (record.fields[arrayJoinRollup.id] as string).split(', ').sort();
595+
expect(joinedValues).toEqual(['Alpha', 'Alpha', 'Beta'].sort());
596+
const uniqueValues = [...(record.fields[arrayUniqueRollup.id] as string[])].sort();
597+
expect(uniqueValues).toEqual(['Alpha', 'Beta']);
598+
599+
// Update values to include blanks and verify compact removes empty entries
600+
await updateRecordField(table2.id, table2.records[0].id, textField.id, 'Gamma');
601+
await updateRecordField(table2.id, table2.records[1].id, textField.id, '');
602+
await updateRecordField(table2.id, table2.records[2].id, textField.id, null);
603+
604+
const arrayCompactRollup = await rollupFrom(table1, textField.id, 'array_compact({values})');
605+
record = await getRecord(table1.id, table1.records[1].id);
606+
expect(record.fields[arrayCompactRollup.id]).toEqual(['Gamma']);
607+
});
608+
449609
it('should roll up a flat array multiple select field -> one - many rollup field', async () => {
450610
const lookedUpToField = getFieldByType(table2.fields, FieldType.MultipleSelect);
451611
const rollupFieldVo = await rollupFrom(table1, lookedUpToField.id, 'countall({values})');

‎apps/nestjs-backend/test/utils/init-app.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,16 @@ const ensureConditionalRollupOptions = (fieldRo: IFieldRo): IFieldRo => {
421421
return fieldRo;
422422
}
423423

424+
if (options.filter === null) {
425+
return {
426+
...fieldRo,
427+
options: {
428+
...options,
429+
filter: undefined,
430+
} as IConditionalRollupFieldOptions,
431+
};
432+
}
433+
424434
const hasFilterConditions =
425435
options.filter?.filterSet != null && options.filter.filterSet.length > 0;
426436

0 commit comments

Comments
 (0)