From 57213781a88cea5b8e33d888a0f71419368b820e Mon Sep 17 00:00:00 2001 From: fengmk2 Date: Mon, 17 Feb 2025 06:17:00 +0000 Subject: [PATCH] refactor(server): dont convert graphql bad request into internal server error (#10203) --- .../server/src/__tests__/app/graphql.e2e.ts | 11 ++++++--- .../server/src/__tests__/utils/testing-app.ts | 8 ++++++- packages/backend/server/src/base/error/def.ts | 6 +++++ .../server/src/base/error/errors.gen.ts | 14 ++++++++++- .../server/src/base/graphql/logger-plugin.ts | 4 +--- .../server/src/base/nestjs/exception.ts | 24 +++++++++++++++++++ packages/backend/server/src/schema.gql | 8 ++++++- 7 files changed, 66 insertions(+), 9 deletions(-) diff --git a/packages/backend/server/src/__tests__/app/graphql.e2e.ts b/packages/backend/server/src/__tests__/app/graphql.e2e.ts index 2b8fa8b1a1..49844892f2 100644 --- a/packages/backend/server/src/__tests__/app/graphql.e2e.ts +++ b/packages/backend/server/src/__tests__/app/graphql.e2e.ts @@ -1,15 +1,14 @@ -import type { INestApplication } from '@nestjs/common'; import type { TestFn } from 'ava'; import ava from 'ava'; import request from 'supertest'; import { buildAppModule } from '../../app.module'; -import { createTestingApp } from '../utils'; +import { createTestingApp, TestingApp } from '../utils'; const gql = '/graphql'; const test = ava as TestFn<{ - app: INestApplication; + app: TestingApp; }>; test.before('start app', async t => { @@ -83,3 +82,9 @@ test('should be able to call apis', async t => { // make sure the request id is set t.truthy(res.headers['x-request-id']); }); + +test('should not throw internal error when graphql call with invalid params', async t => { + await t.throwsAsync(t.context.app.gql(`query { workspace("1") }`), { + message: /Failed to execute gql: query { workspace\("1"\) \}, status: 400/, + }); +}); diff --git a/packages/backend/server/src/__tests__/utils/testing-app.ts b/packages/backend/server/src/__tests__/utils/testing-app.ts index 9ab9130b70..c0e22cf590 100644 --- a/packages/backend/server/src/__tests__/utils/testing-app.ts +++ b/packages/backend/server/src/__tests__/utils/testing-app.ts @@ -162,12 +162,18 @@ export class TestingApp extends ApplyType() { if (res.status !== 200) { throw new Error( `Failed to execute gql: ${query}, status: ${res.status}, body: ${JSON.stringify( - res.body + res.body, + null, + 2 )}` ); } if (res.body.errors?.length) { + if (TEST_LOG_LEVEL !== 'fatal') { + // print the error stack when log level is not fatal, for better debugging + console.error('%o', res.body); + } throw new Error(res.body.errors[0].message); } diff --git a/packages/backend/server/src/base/error/def.ts b/packages/backend/server/src/base/error/def.ts index 9e61ac5f2b..85e6b8f09b 100644 --- a/packages/backend/server/src/base/error/def.ts +++ b/packages/backend/server/src/base/error/def.ts @@ -251,6 +251,12 @@ export const USER_FRIENDLY_ERRORS = { type: 'bad_request', message: 'Bad request.', }, + graphql_bad_request: { + type: 'bad_request', + args: { code: 'string', message: 'string' }, + message: ({ code, message }) => + `GraphQL bad request, code: ${code}, ${message}`, + }, // Input errors query_too_long: { diff --git a/packages/backend/server/src/base/error/errors.gen.ts b/packages/backend/server/src/base/error/errors.gen.ts index e313a1aa6a..49d70022f8 100644 --- a/packages/backend/server/src/base/error/errors.gen.ts +++ b/packages/backend/server/src/base/error/errors.gen.ts @@ -28,6 +28,17 @@ export class BadRequest extends UserFriendlyError { } } @ObjectType() +class GraphqlBadRequestDataType { + @Field() code!: string + @Field() message!: string +} + +export class GraphqlBadRequest extends UserFriendlyError { + constructor(args: GraphqlBadRequestDataType, message?: string | ((args: GraphqlBadRequestDataType) => string)) { + super('bad_request', 'graphql_bad_request', message, args); + } +} +@ObjectType() class QueryTooLongDataType { @Field() max!: number } @@ -787,6 +798,7 @@ export enum ErrorNames { TOO_MANY_REQUEST, NOT_FOUND, BAD_REQUEST, + GRAPHQL_BAD_REQUEST, QUERY_TOO_LONG, USER_NOT_FOUND, USER_AVATAR_NOT_FOUND, @@ -891,5 +903,5 @@ registerEnumType(ErrorNames, { export const ErrorDataUnionType = createUnionType({ name: 'ErrorDataUnion', types: () => - [QueryTooLongDataType, WrongSignInCredentialsDataType, UnknownOauthProviderDataType, MissingOauthQueryParameterDataType, InvalidEmailDataType, InvalidPasswordLengthDataType, WorkspacePermissionNotFoundDataType, SpaceNotFoundDataType, MemberNotFoundInSpaceDataType, NotInSpaceDataType, AlreadyInSpaceDataType, SpaceAccessDeniedDataType, SpaceOwnerNotFoundDataType, SpaceShouldHaveOnlyOneOwnerDataType, DocNotFoundDataType, DocAccessDeniedDataType, VersionRejectedDataType, InvalidHistoryTimestampDataType, DocHistoryNotFoundDataType, BlobNotFoundDataType, ExpectToGrantDocUserRolesDataType, ExpectToRevokeDocUserRolesDataType, ExpectToUpdateDocUserRoleDataType, UnsupportedSubscriptionPlanDataType, SubscriptionAlreadyExistsDataType, SubscriptionNotExistsDataType, SameSubscriptionRecurringDataType, SubscriptionPlanNotFoundDataType, CopilotDocNotFoundDataType, CopilotMessageNotFoundDataType, CopilotPromptNotFoundDataType, CopilotProviderSideErrorDataType, CopilotInvalidContextDataType, CopilotContextFileNotSupportedDataType, CopilotFailedToModifyContextDataType, CopilotFailedToMatchContextDataType, RuntimeConfigNotFoundDataType, InvalidRuntimeConfigTypeDataType, InvalidLicenseUpdateParamsDataType, WorkspaceMembersExceedLimitToDowngradeDataType] as const, + [GraphqlBadRequestDataType, QueryTooLongDataType, WrongSignInCredentialsDataType, UnknownOauthProviderDataType, MissingOauthQueryParameterDataType, InvalidEmailDataType, InvalidPasswordLengthDataType, WorkspacePermissionNotFoundDataType, SpaceNotFoundDataType, MemberNotFoundInSpaceDataType, NotInSpaceDataType, AlreadyInSpaceDataType, SpaceAccessDeniedDataType, SpaceOwnerNotFoundDataType, SpaceShouldHaveOnlyOneOwnerDataType, DocNotFoundDataType, DocAccessDeniedDataType, VersionRejectedDataType, InvalidHistoryTimestampDataType, DocHistoryNotFoundDataType, BlobNotFoundDataType, ExpectToGrantDocUserRolesDataType, ExpectToRevokeDocUserRolesDataType, ExpectToUpdateDocUserRoleDataType, UnsupportedSubscriptionPlanDataType, SubscriptionAlreadyExistsDataType, SubscriptionNotExistsDataType, SameSubscriptionRecurringDataType, SubscriptionPlanNotFoundDataType, CopilotDocNotFoundDataType, CopilotMessageNotFoundDataType, CopilotPromptNotFoundDataType, CopilotProviderSideErrorDataType, CopilotInvalidContextDataType, CopilotContextFileNotSupportedDataType, CopilotFailedToModifyContextDataType, CopilotFailedToMatchContextDataType, RuntimeConfigNotFoundDataType, InvalidRuntimeConfigTypeDataType, InvalidLicenseUpdateParamsDataType, WorkspaceMembersExceedLimitToDowngradeDataType] as const, }); diff --git a/packages/backend/server/src/base/graphql/logger-plugin.ts b/packages/backend/server/src/base/graphql/logger-plugin.ts index 66e758d34c..15e2f1d37f 100644 --- a/packages/backend/server/src/base/graphql/logger-plugin.ts +++ b/packages/backend/server/src/base/graphql/logger-plugin.ts @@ -38,9 +38,7 @@ export class GQLLoggerPlugin implements ApolloServerPlugin { }, didEncounterErrors: ctx => { ctx.errors.forEach(gqlErr => { - const error = mapAnyError( - gqlErr.originalError ? gqlErr.originalError : gqlErr - ); + const error = mapAnyError(gqlErr); error.log('GraphQL'); metrics.gql.counter('query_error_counter').add(1, { diff --git a/packages/backend/server/src/base/nestjs/exception.ts b/packages/backend/server/src/base/nestjs/exception.ts index fa7675c73d..8baa67a35a 100644 --- a/packages/backend/server/src/base/nestjs/exception.ts +++ b/packages/backend/server/src/base/nestjs/exception.ts @@ -1,3 +1,4 @@ +import { ApolloServerErrorCode } from '@apollo/server/errors'; import { ArgumentsHost, Catch, @@ -9,10 +10,12 @@ import { GqlContextType } from '@nestjs/graphql'; import { ThrottlerException } from '@nestjs/throttler'; import { BaseWsExceptionFilter } from '@nestjs/websockets'; import { Response } from 'express'; +import { GraphQLError } from 'graphql'; import { of } from 'rxjs'; import { Socket } from 'socket.io'; import { + GraphqlBadRequest, InternalServerError, NotFound, TooManyRequest, @@ -21,7 +24,28 @@ import { import { metrics } from '../metrics'; import { getRequestIdFromHost } from '../utils'; +function isGraphQLBadRequest(error: any): error is GraphQLError { + // https://www.apollographql.com/docs/apollo-server/data/errors + const code = error.extensions?.code; + return ( + code === ApolloServerErrorCode.GRAPHQL_PARSE_FAILED || + code === ApolloServerErrorCode.GRAPHQL_VALIDATION_FAILED || + code === ApolloServerErrorCode.BAD_USER_INPUT || + code === ApolloServerErrorCode.BAD_REQUEST + ); +} + export function mapAnyError(error: any): UserFriendlyError { + if (error instanceof GraphQLError) { + const err = error; + if (isGraphQLBadRequest(error)) { + return new GraphqlBadRequest({ + code: err.extensions.code as string, + message: err.message, + }); + } + error = err.originalError ?? error; + } if (error instanceof UserFriendlyError) { return error; } else if (error instanceof ThrottlerException) { diff --git a/packages/backend/server/src/schema.gql b/packages/backend/server/src/schema.gql index 24a5f75421..f04f00de8b 100644 --- a/packages/backend/server/src/schema.gql +++ b/packages/backend/server/src/schema.gql @@ -315,7 +315,7 @@ type EditorType { name: String! } -union ErrorDataUnion = AlreadyInSpaceDataType | BlobNotFoundDataType | CopilotContextFileNotSupportedDataType | CopilotDocNotFoundDataType | CopilotFailedToMatchContextDataType | CopilotFailedToModifyContextDataType | CopilotInvalidContextDataType | CopilotMessageNotFoundDataType | CopilotPromptNotFoundDataType | CopilotProviderSideErrorDataType | DocAccessDeniedDataType | DocHistoryNotFoundDataType | DocNotFoundDataType | ExpectToGrantDocUserRolesDataType | ExpectToRevokeDocUserRolesDataType | ExpectToUpdateDocUserRoleDataType | InvalidEmailDataType | InvalidHistoryTimestampDataType | InvalidLicenseUpdateParamsDataType | InvalidPasswordLengthDataType | InvalidRuntimeConfigTypeDataType | MemberNotFoundInSpaceDataType | MissingOauthQueryParameterDataType | NotInSpaceDataType | QueryTooLongDataType | RuntimeConfigNotFoundDataType | SameSubscriptionRecurringDataType | SpaceAccessDeniedDataType | SpaceNotFoundDataType | SpaceOwnerNotFoundDataType | SpaceShouldHaveOnlyOneOwnerDataType | SubscriptionAlreadyExistsDataType | SubscriptionNotExistsDataType | SubscriptionPlanNotFoundDataType | UnknownOauthProviderDataType | UnsupportedSubscriptionPlanDataType | VersionRejectedDataType | WorkspaceMembersExceedLimitToDowngradeDataType | WorkspacePermissionNotFoundDataType | WrongSignInCredentialsDataType +union ErrorDataUnion = AlreadyInSpaceDataType | BlobNotFoundDataType | CopilotContextFileNotSupportedDataType | CopilotDocNotFoundDataType | CopilotFailedToMatchContextDataType | CopilotFailedToModifyContextDataType | CopilotInvalidContextDataType | CopilotMessageNotFoundDataType | CopilotPromptNotFoundDataType | CopilotProviderSideErrorDataType | DocAccessDeniedDataType | DocHistoryNotFoundDataType | DocNotFoundDataType | ExpectToGrantDocUserRolesDataType | ExpectToRevokeDocUserRolesDataType | ExpectToUpdateDocUserRoleDataType | GraphqlBadRequestDataType | InvalidEmailDataType | InvalidHistoryTimestampDataType | InvalidLicenseUpdateParamsDataType | InvalidPasswordLengthDataType | InvalidRuntimeConfigTypeDataType | MemberNotFoundInSpaceDataType | MissingOauthQueryParameterDataType | NotInSpaceDataType | QueryTooLongDataType | RuntimeConfigNotFoundDataType | SameSubscriptionRecurringDataType | SpaceAccessDeniedDataType | SpaceNotFoundDataType | SpaceOwnerNotFoundDataType | SpaceShouldHaveOnlyOneOwnerDataType | SubscriptionAlreadyExistsDataType | SubscriptionNotExistsDataType | SubscriptionPlanNotFoundDataType | UnknownOauthProviderDataType | UnsupportedSubscriptionPlanDataType | VersionRejectedDataType | WorkspaceMembersExceedLimitToDowngradeDataType | WorkspacePermissionNotFoundDataType | WrongSignInCredentialsDataType enum ErrorNames { ACCESS_DENIED @@ -364,6 +364,7 @@ enum ErrorNames { FAILED_TO_CHECKOUT FAILED_TO_SAVE_UPDATES FAILED_TO_UPSERT_SNAPSHOT + GRAPHQL_BAD_REQUEST INTERNAL_SERVER_ERROR INVALID_CHECKOUT_PARAMETERS INVALID_EMAIL @@ -475,6 +476,11 @@ type GrantedDocUserTypeEdge { node: GrantedDocUserType! } +type GraphqlBadRequestDataType { + code: String! + message: String! +} + type InvalidEmailDataType { email: String! }