Skip to content

Commit

Permalink
Add more details to mutation limit exception message and fix update m…
Browse files Browse the repository at this point in the history
…any query (#5460)

## Context
Since we rely on PgGraphql to query the DB, we have to map its errors to
more comprehensible errors before sending them back to the FE. This has
already been done for unicity constraint and mutation maximum records
but for the last one the message wasn't clear enough. This PR introduces
a new pgGraphqlConfig param to the util to pass down the 'atMost' config
that we are actually overwriting with an
'MUTATION_MAXIMUM_RECORD_AFFECTED' env variable. See how atMost works in
this doc (https://supabase.github.io/pg_graphql/api/#delete)

Also adding the same message for the update since this mutation is also
affected. Create is not though.

Lastly, this PR introduces a fix on the updateMany. Since the current FE
is not using updateMany, this was missed for a few weeks but a
regression has been introduced when we started checking if the id is a
valid UUID however for updateMany this was checking the data object
instead of the filter object. Actually, the data object should never
contain id because it wouldn't make sense to allow the update of the id
and even more for multiple records since the id should be unique.

## Test
locally with MUTATION_MAXIMUM_RECORD_AFFECTED=5

<img width="1408" alt="Screenshot 2024-05-18 at 02 11 59"
src="https://github.com/twentyhq/twenty/assets/1834158/06bf25ce-4a44-4851-8456-aed7689bb33e">
<img width="1250" alt="Screenshot 2024-05-18 at 02 12 10"
src="https://github.com/twentyhq/twenty/assets/1834158/06fc4329-147b-4bb4-9223-c3bce340a8d2">
<img width="1222" alt="Screenshot 2024-05-18 at 02 12 36"
src="https://github.com/twentyhq/twenty/assets/1834158/0674546e-73e2-4e5c-918f-9825f2ee5967">
<img width="1228" alt="Screenshot 2024-05-18 at 02 13 01"
src="https://github.com/twentyhq/twenty/assets/1834158/f50df435-1fd4-45df-a953-8fefa8f36e75">
<img width="1174" alt="Screenshot 2024-05-18 at 02 13 09"
src="https://github.com/twentyhq/twenty/assets/1834158/707b9300-2779-43df-8177-9658b8965b49">


<img width="1393" alt="Screenshot 2024-05-18 at 02 19 11"
src="https://github.com/twentyhq/twenty/assets/1834158/2cd167b6-1261-4914-a4db-36f792d810c0">
  • Loading branch information
Weiko committed May 18, 2024
1 parent 0e525ca commit 66637a3
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@ import {
InternalServerErrorException,
} from '@nestjs/common';

export type PgGraphQLConfig = {
atMost: number;
};

interface PgGraphQLErrorMapping {
[key: string]: (command: string, objectName: string) => HttpException;
[key: string]: (
command: string,
objectName: string,
pgGraphqlConfig: PgGraphQLConfig,
) => HttpException;
}

const pgGraphQLCommandMapping = {
Expand All @@ -15,13 +23,15 @@ const pgGraphQLCommandMapping = {
};

const pgGraphQLErrorMapping: PgGraphQLErrorMapping = {
'delete impacts too many records': (command, objectName) =>
'delete impacts too many records': (_, objectName, pgGraphqlConfig) =>
new BadRequestException(
`Cannot ${
pgGraphQLCommandMapping[command] ?? command
} ${objectName} because it impacts too many records.`,
`Cannot delete ${objectName} because it impacts too many records (more than ${pgGraphqlConfig?.atMost}).`,
),
'update impacts too many records': (_, objectName, pgGraphqlConfig) =>
new BadRequestException(
`Cannot update ${objectName} because it impacts too many records (more than ${pgGraphqlConfig?.atMost}).`,
),
'duplicate key value violates unique constraint': (command, objectName) =>
'duplicate key value violates unique constraint': (command, objectName, _) =>
new BadRequestException(
`Cannot ${
pgGraphQLCommandMapping[command] ?? command
Expand All @@ -33,6 +43,7 @@ export const computePgGraphQLError = (
command: string,
objectName: string,
errors: any[],
pgGraphqlConfig: PgGraphQLConfig,
) => {
const error = errors[0];
const errorMessage = error?.message;
Expand All @@ -46,7 +57,7 @@ export const computePgGraphQLError = (
: null;

if (mappedError) {
return mappedError(command, objectName);
return mappedError(command, objectName, pgGraphqlConfig);
}

return new InternalServerErrorException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ import {
PGGraphQLMutation,
PGGraphQLResult,
} from './interfaces/pg-graphql.interface';
import { computePgGraphQLError } from './utils/compute-pg-graphql-error.util';
import {
PgGraphQLConfig,
computePgGraphQLError,
} from './utils/compute-pg-graphql-error.util';

@Injectable()
export class WorkspaceQueryRunnerService {
Expand Down Expand Up @@ -374,7 +377,7 @@ export class WorkspaceQueryRunnerService {
const { userId, workspaceId, objectMetadataItem } = options;

assertMutationNotOnRemoteObject(objectMetadataItem);
assertIsValidUuid(args.data.id);
args.filter?.id?.in?.forEach((id) => assertIsValidUuid(id));

const maximumRecordAffected = this.environmentService.get(
'MUTATION_MAXIMUM_RECORD_AFFECTED',
Expand Down Expand Up @@ -613,6 +616,11 @@ export class WorkspaceQueryRunnerService {
command,
objectMetadataItem.nameSingular,
errors,
{
atMost: this.environmentService.get(
'MUTATION_MAXIMUM_RECORD_AFFECTED',
),
} satisfies PgGraphQLConfig,
);

throw error;
Expand Down

0 comments on commit 66637a3

Please sign in to comment.