-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix queryRunnerArgsFactory for updateMany resolver #10322
Conversation
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.
PR Summary
Fixed a critical bug in the UpdateMany resolver's data handling within the QueryRunnerArgsFactory, affecting bulk update operations.
- Modified
packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
to process a single data object instead of an array for UpdateMany operations - Fixed incorrect type usage from
UpdateOneResolverArgs.data
toUpdateManyResolverArgs.data
- Aligned UpdateMany data handling with UpdateOne resolver for consistency
- Impacts integration tests, particularly
all-people-resolvers.integration-spec.ts
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
data: await this.overrideDataByFieldMetadata( | ||
(args as UpdateOneResolverArgs).data, | ||
options, |
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.
logic: Incorrect type cast here - using UpdateOneResolverArgs.data when handling UpdateMany case. This could cause type safety issues.
data: await this.overrideDataByFieldMetadata( | |
(args as UpdateOneResolverArgs).data, | |
options, | |
(args as UpdateManyResolverArgs).data, | |
options, |
}, | ||
), | ||
) ?? [], | ||
data: await this.overrideDataByFieldMetadata( |
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.
unlike with CreateMany, data is not an array:
export interface CreateManyResolverArgs<
Data extends Partial<ObjectRecord> = Partial<ObjectRecord>,
> {
data: Data[];
upsert?: boolean;
}
...
export interface UpdateManyResolverArgs<
Data extends Partial<ObjectRecord> = Partial<ObjectRecord>,
Filter = any,
> {
filter: Filter;
data: Data;
}
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.
@eliasylonen FYI :)
This was updated a few weeks ago and went unnoticed since 1) integration tests were broken + 2) we don't have actionnable updateMany mutations in the product at the moment
It will fix some tests, at least all-people-resolvers.integration-spec.ts for instance