-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aggregated queries #1 #8345
Aggregated queries #1 #8345
Changes from 4 commits
4b51b58
3f929c9
49bc25a
10fdfd5
6b3e533
3fb0fe0
7adb8ce
ad441bd
fe53fb8
cd4465f
d26a1bd
870fc61
dc72de9
d3dbf46
52e8a88
55ce64d
f8d4c2a
5af68f8
db02867
2c6549a
b347f39
6ca4628
7c7ee44
22d0fb9
b182252
2d7ff9c
fce5c81
c1df0e5
765d9a6
e5a9e3d
b44ceb9
1aee63c
853126a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,5 +18,5 @@ | |
"src/**/*.d.ts", | ||
"src/**/*.ts", | ||
"src/**/*.tsx" | ||
] | ||
, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Server-side files should not be included in front-end build config. Remove these files from the include array. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Test files (*.spec.ts) should be excluded as per line 8, but are being included here |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,5 +9,5 @@ | |
"src/**/*.d.ts", | ||
"src/**/*.ts", | ||
"src/**/*.tsx" | ||
] | ||
, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Server files should not be included in frontend tsconfig. Remove '../twenty-server/src/utils/is-uuid.ts' and '../twenty-server/src/utils/is-uuid.utils.spec.ts' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove also, no? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,5 +15,5 @@ | |
"tsup.config.ts", | ||
"tsup.ui.index.tsx", | ||
"vite.config.ts" | ||
] | ||
, "../twenty-server/src/utils/is-uuid.utils.spec.ts" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Remove this line - server test files should not be included in front-end test config There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove also, no? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,12 +33,18 @@ export class GraphqlQuerySelectedFieldsParser { | |
relations: {}, | ||
}; | ||
|
||
const hasEdges = Object.keys(graphqlSelectedFields).includes('edges'); | ||
Weiko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for (const [fieldKey, fieldValue] of Object.entries( | ||
graphqlSelectedFields, | ||
)) { | ||
if (hasEdges && fieldKey !== 'edges') { | ||
continue; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: skipping non-edges fields when edges exist could prevent aggregation fields from being processed at the root level |
||
if (this.shouldNotParseField(fieldKey)) { | ||
continue; | ||
} | ||
|
||
if (this.isConnectionField(fieldKey, fieldValue)) { | ||
const subResult = this.parse(fieldValue, fieldMetadataMap); | ||
|
||
|
@@ -83,9 +89,7 @@ export class GraphqlQuerySelectedFieldsParser { | |
} | ||
|
||
private shouldNotParseField(fieldKey: string): boolean { | ||
return ['__typename', 'totalCount', 'pageInfo', 'cursor'].includes( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we replace totalCount everywhere in the app by countUniqueId? We'll need to give a heads up for deprecation but we can start replacing it in our frontend at least. We plan to email people about API Key deprecation so we can let them know about this at the same time. |
||
fieldKey, | ||
); | ||
return ['__typename', 'cursor'].includes(fieldKey); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: removing 'totalCount' and 'pageInfo' from shouldNotParseField may cause these fields to be incorrectly included in the select statement |
||
|
||
private parseCompositeField( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
|
||
import { isDefined } from 'class-validator'; | ||
import graphqlFields from 'graphql-fields'; | ||
import { SelectQueryBuilder } from 'typeorm'; | ||
|
||
import { ResolverService } from 'src/engine/api/graphql/graphql-query-runner/interfaces/resolver-service.interface'; | ||
import { | ||
|
@@ -13,6 +13,7 @@ import { | |
import { IConnection } from 'src/engine/api/graphql/workspace-query-runner/interfaces/connection.interface'; | ||
import { WorkspaceQueryRunnerOptions } from 'src/engine/api/graphql/workspace-query-runner/interfaces/query-runner-option.interface'; | ||
import { FindManyResolverArgs } from 'src/engine/api/graphql/workspace-resolver-builder/interfaces/workspace-resolvers-builder.interface'; | ||
import { FieldMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata.interface'; | ||
|
||
import { QUERY_MAX_RECORDS } from 'src/engine/api/graphql/graphql-query-runner/constants/query-max-records.constant'; | ||
import { | ||
|
@@ -27,8 +28,14 @@ import { | |
getCursor, | ||
getPaginationInfo, | ||
} from 'src/engine/api/graphql/graphql-query-runner/utils/cursors.util'; | ||
import { | ||
AggregationField, | ||
getAvailableAggregationsFromObjectFields, | ||
} from 'src/engine/api/graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util'; | ||
import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager'; | ||
import { formatResult } from 'src/engine/twenty-orm/utils/format-result.util'; | ||
import { isDefined } from 'src/utils/is-defined'; | ||
import { isUuid } from 'src/utils/is-uuid'; | ||
|
||
@Injectable() | ||
export class GraphqlQueryFindManyResolverService | ||
|
@@ -85,8 +92,6 @@ export class GraphqlQueryFindManyResolverService | |
); | ||
const isForwardPagination = !isDefined(args.before); | ||
|
||
const limit = args.first ?? args.last ?? QUERY_MAX_RECORDS; | ||
|
||
const withDeletedCountQueryBuilder = | ||
graphqlQueryParser.applyDeletedAtToBuilder( | ||
withFilterCountQueryBuilder, | ||
|
@@ -139,12 +144,30 @@ export class GraphqlQueryFindManyResolverService | |
args.filter ?? ({} as Filter), | ||
); | ||
|
||
const selectedAggregatedFields = this.getSelectedAggregatedFields({ | ||
objectFields: Object.values( | ||
Object.fromEntries( | ||
Object.entries(objectMetadataMapItem.fields).filter( | ||
([key, _value]) => !isUuid(key), // remove objectMetadataMapItem fields duplicates | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit ugly, did you consider solving the root cause in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I say 2 objects it's 2 different map for object level but also have 2 properties instead of one (for the field map by id + field map by name) within the ObjectMetadataMapItem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure how this map with ids as key are used today, I just know/think its for perf improvement reasons. @charlesBochet has more context and probably knows better if a refactor is desirable i think ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I understand! Thank you. But we should introduce two maps on byName and one byId. It doesn't make sense to mix both and lose information, to then re-create it later |
||
), | ||
), | ||
), | ||
selectedFields, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Filtering out UUID fields here prevents aggregations on valid UUID columns that may need aggregation. Consider moving this filter to the aggregation utility. |
||
|
||
this.addSelectedAggregatedFieldsQueriesToQueryBuilder({ | ||
selectedAggregatedFields, | ||
queryBuilder: withDeletedQueryBuilder, | ||
}); | ||
|
||
const limit = args.first ?? args.last ?? QUERY_MAX_RECORDS; | ||
|
||
const nonFormattedObjectRecords = await withDeletedQueryBuilder | ||
.take(limit + 1) | ||
.getMany(); | ||
.getRawAndEntities(); | ||
|
||
const objectRecords = formatResult( | ||
nonFormattedObjectRecords, | ||
nonFormattedObjectRecords.entities, | ||
objectMetadataMapItem, | ||
objectMetadataMap, | ||
); | ||
|
@@ -186,7 +209,12 @@ export class GraphqlQueryFindManyResolverService | |
hasPreviousPage, | ||
}); | ||
|
||
return result; | ||
const aggregatedFieldsResults = this.extractAggregatedFieldsResults({ | ||
selectedAggregatedFields, | ||
rawObjectRecords: nonFormattedObjectRecords.raw, | ||
}); | ||
|
||
return { ...result, ...aggregatedFieldsResults }; | ||
} | ||
|
||
async validate<Filter extends RecordFilter>( | ||
|
@@ -230,4 +258,64 @@ export class GraphqlQueryFindManyResolverService | |
); | ||
} | ||
} | ||
|
||
private addSelectedAggregatedFieldsQueriesToQueryBuilder = ({ | ||
selectedAggregatedFields, | ||
queryBuilder, | ||
}: { | ||
selectedAggregatedFields: AggregationField[]; | ||
queryBuilder: SelectQueryBuilder<any>; | ||
}) => { | ||
selectedAggregatedFields.forEach((aggregatedField) => { | ||
const [[aggregatedFieldName, aggregatedFieldDetails]] = | ||
Object.entries(aggregatedField); | ||
const operation = aggregatedFieldDetails.aggregationOperation; | ||
const fieldName = aggregatedFieldDetails.fromField; | ||
|
||
queryBuilder.addSelect( | ||
`${operation}("${fieldName}") OVER()`, | ||
`${aggregatedFieldName}`, | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: SQL injection vulnerability - fieldName and operation are directly interpolated into the query string. Use query parameters instead. |
||
}); | ||
}; | ||
|
||
private getSelectedAggregatedFields = ({ | ||
objectFields, | ||
selectedFields, | ||
}: { | ||
objectFields: FieldMetadataInterface[]; | ||
selectedFields: any[]; | ||
}) => { | ||
const allAggregatedFields = | ||
getAvailableAggregationsFromObjectFields(objectFields); | ||
|
||
return allAggregatedFields.reduce( | ||
(acc, aggregatedField) => { | ||
const aggregatedFieldName = Object.keys(aggregatedField)[0]; | ||
|
||
if (!Object.keys(selectedFields).includes(aggregatedFieldName)) | ||
return acc; | ||
|
||
return [...acc, aggregatedField]; | ||
}, | ||
[] as typeof allAggregatedFields, | ||
); | ||
}; | ||
|
||
private extractAggregatedFieldsResults = ({ | ||
selectedAggregatedFields, | ||
rawObjectRecords, | ||
}: { | ||
selectedAggregatedFields: AggregationField[]; | ||
rawObjectRecords: any[]; | ||
}) => { | ||
return selectedAggregatedFields.reduce((acc, aggregatedField) => { | ||
const aggregatedFieldName = Object.keys(aggregatedField)[0]; | ||
|
||
return { | ||
...acc, | ||
[aggregatedFieldName]: rawObjectRecords[0][aggregatedFieldName], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Potential null reference if rawObjectRecords[0] is undefined. Add null check before accessing. |
||
}; | ||
}, {}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,14 @@ import { GraphQLFieldConfigMap, GraphQLInt, GraphQLObjectType } from 'graphql'; | |
import { WorkspaceBuildSchemaOptions } from 'src/engine/api/graphql/workspace-schema-builder/interfaces/workspace-build-schema-optionts.interface'; | ||
import { ObjectMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/object-metadata.interface'; | ||
|
||
import { getAvailableAggregationsFromObjectFields } from 'src/engine/api/graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util'; | ||
import { pascalCase } from 'src/utils/pascal-case'; | ||
|
||
import { ConnectionTypeFactory } from './connection-type.factory'; | ||
import { | ||
ObjectTypeDefinition, | ||
ObjectTypeDefinitionKind, | ||
} from './object-type-definition.factory'; | ||
import { ConnectionTypeFactory } from './connection-type.factory'; | ||
|
||
export enum ConnectionTypeDefinitionKind { | ||
Edge = 'Edge', | ||
|
@@ -43,7 +44,25 @@ export class ConnectionTypeDefinitionFactory { | |
objectMetadata: ObjectMetadataInterface, | ||
options: WorkspaceBuildSchemaOptions, | ||
): GraphQLFieldConfigMap<any, any> { | ||
const fields: GraphQLFieldConfigMap<any, any> = {}; | ||
const fields: GraphQLFieldConfigMap<any, any> = Object.assign( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using Array.reduce instead of Object.assign + map for better readability and performance |
||
{}, | ||
...getAvailableAggregationsFromObjectFields(objectMetadata.fields).map( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should follow the existing factory pattern here: |
||
(agg) => { | ||
const [ | ||
[ | ||
key, | ||
{ | ||
aggregationOperation: _aggregationOperation, | ||
fromField: _fromField, | ||
...rest | ||
}, | ||
], | ||
] = Object.entries(agg); | ||
|
||
return { [key]: rest }; | ||
}, | ||
), | ||
); | ||
|
||
fields.edges = { | ||
type: this.connectionTypeFactory.create( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import { GraphQLISODateTime } from '@nestjs/graphql'; | ||
|
||
import { GraphQLFloat, GraphQLString } from 'graphql'; | ||
|
||
import { FieldMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata.interface'; | ||
|
||
import { FieldMetadataType } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; | ||
import { capitalize } from 'src/utils/capitalize'; | ||
|
||
enum AGGREGATION_OPERATIONS { | ||
min = 'MIN', | ||
max = 'MAX', | ||
avg = 'AVG', | ||
sum = 'SUM', | ||
} | ||
|
||
type AggregationValue = { | ||
type: typeof GraphQLString; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a big fan of typeOf! why not directly putting the right one? |
||
description: string; | ||
fromField: string; | ||
aggregationOperation: AGGREGATION_OPERATIONS; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: AggregationValue.type is incorrectly typed as GraphQLString when it needs to handle both GraphQLISODateTime and GraphQLFloat |
||
|
||
export type AggregationField = { | ||
[key: string]: AggregationValue; | ||
}; | ||
|
||
export const getAvailableAggregationsFromObjectFields = ( | ||
fields: FieldMetadataInterface[], | ||
): AggregationField[] => { | ||
return fields.reduce<Array<Record<string, any>>>((acc, field) => { | ||
Weiko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (field.type === FieldMetadataType.DATE_TIME) { | ||
return [ | ||
...acc, | ||
{ | ||
[`min${capitalize(field.name)}`]: { | ||
type: GraphQLISODateTime, | ||
Weiko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description: `Oldest date contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.min, | ||
}, | ||
}, | ||
{ | ||
[`max${capitalize(field.name)}`]: { | ||
type: GraphQLISODateTime, | ||
description: `Most recent date contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.max, | ||
}, | ||
}, | ||
]; | ||
} | ||
|
||
if (field.type === FieldMetadataType.NUMBER) { | ||
Weiko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return [ | ||
...acc, | ||
{ | ||
[`min${capitalize(field.name)}`]: { | ||
type: GraphQLFloat, | ||
description: `Minimum value contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.min, | ||
}, | ||
}, | ||
{ | ||
[`max${capitalize(field.name)}`]: { | ||
type: GraphQLFloat, | ||
description: `Maximum value contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.max, | ||
}, | ||
}, | ||
{ | ||
[`avg${capitalize(field.name)}`]: { | ||
type: GraphQLFloat, | ||
description: `Average value contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.avg, | ||
}, | ||
}, | ||
{ | ||
[`sum${capitalize(field.name)}`]: { | ||
type: GraphQLFloat, | ||
description: `Sum of amounts contained in the field ${field.name}`, | ||
fromField: field.name, | ||
aggregationOperation: AGGREGATION_OPERATIONS.sum, | ||
}, | ||
}, | ||
]; | ||
} | ||
|
||
return acc; | ||
}, []); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
export const isUuid = (value: unknown): boolean => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Function lacks JSDoc documentation explaining its purpose and expected input/output |
||
if (typeof value !== 'string') { | ||
return false; | ||
} | ||
|
||
if (value.length !== 36) { | ||
return false; | ||
} | ||
|
||
const uuidRegex = | ||
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider caching this regex pattern outside the function since it's constant and regex compilation is expensive |
||
|
||
return uuidRegex.test(value); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commited this although I suppose it should not be this way. I suppose something is wrong with my test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this line seems strange, why would you add this? Sorry I don't understand the goal