From 0167be7754ab49024f7be2a18ad02807f21221dc Mon Sep 17 00:00:00 2001 From: David Thyresson Date: Tue, 8 Aug 2023 15:08:07 -0400 Subject: [PATCH] fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (#9005) See: https://github.com/redwoodjs/redwood/issues/8988 1. In GraphQL there are many reserved names - `Subscription` being one of them. And Redwood's schema validation checks that a validator directive is applied to queries, mutations -- and subscriptions. However, we have cases where existing RW apps have used `Subscription` as a model and a type. This PR will only check for the directive on Subscriptions if Redwood Realtime is enabled. Note: a workaround is to keep your Prisma model named Subscription, but just rename the type to something like "CustomerSubscription" or "ProductSubscription" and rework your services to have resolver types that match your GraphQL schema. 2. This PR also will raise errors when reserved names are uses as types ,inputs or interfaces -- like if for example you had a Prisma model and a generated type like "Float" because maybe you had a fishing or sailing store and needed to have a collection of floats :) Note to @jtoar - this is a potentially breaking changes because some projects might be using reserved GraphQL names as types but really shouldn't to mitigate issues downstream. --------- Co-authored-by: Dominic Saadi --- ...alidateSchemaForReservedNames.test.ts.snap | 19 ++ .../validateSchemaForAuthDirectives.test.ts | 4 +- .../validateSchemaForReservedNames.test.ts | 177 ++++++++++++++++++ packages/internal/src/project.ts | 26 +++ packages/internal/src/validateSchema.ts | 69 ++++++- 5 files changed, 290 insertions(+), 5 deletions(-) create mode 100644 packages/internal/src/__tests__/__snapshots__/validateSchemaForReservedNames.test.ts.snap create mode 100644 packages/internal/src/__tests__/validateSchemaForReservedNames.test.ts diff --git a/packages/internal/src/__tests__/__snapshots__/validateSchemaForReservedNames.test.ts.snap b/packages/internal/src/__tests__/__snapshots__/validateSchemaForReservedNames.test.ts.snap new file mode 100644 index 000000000000..3f30dbda5375 --- /dev/null +++ b/packages/internal/src/__tests__/__snapshots__/validateSchemaForReservedNames.test.ts.snap @@ -0,0 +1,19 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`SDL with no reserved names used SDL is invalid because uses a reserved name as a type 1`] = ` +"The type named 'Float' is a reserved GraphQL name. +Please rename it to something more specific, like: ApplicationFloat. +" +`; + +exports[`SDL with no reserved names used because uses a reserved name as an input 1`] = ` +"The input type named 'Float' is a reserved GraphQL name. +Please rename it to something more specific, like: ApplicationFloat. +" +`; + +exports[`SDL with no reserved names used because uses a reserved name as an interface 1`] = ` +"The interface named 'Float' is a reserved GraphQL name. +Please rename it to something more specific, like: ApplicationFloat. +" +`; diff --git a/packages/internal/src/__tests__/validateSchemaForAuthDirectives.test.ts b/packages/internal/src/__tests__/validateSchemaForAuthDirectives.test.ts index 977e41b3c3b5..3d745d9e1a82 100644 --- a/packages/internal/src/__tests__/validateSchemaForAuthDirectives.test.ts +++ b/packages/internal/src/__tests__/validateSchemaForAuthDirectives.test.ts @@ -8,7 +8,7 @@ import { DocumentNode } from 'graphql' import { getPaths } from '@redwoodjs/project-config' import { - validateSchemaForDirectives, + validateSchema, DIRECTIVE_REQUIRED_ERROR_MESSAGE, DIRECTIVE_INVALID_ROLE_TYPES_ERROR_MESSAGE, } from '../validateSchema' @@ -43,7 +43,7 @@ const validateSdlFile = async (sdlFile: string) => { // Merge in the rootSchema with JSON scalars, etc. const mergedDocumentNode = mergeTypeDefs([projectDocumentNodes]) - validateSchemaForDirectives(mergedDocumentNode) + validateSchema(mergedDocumentNode) } describe('SDL uses auth directives', () => { diff --git a/packages/internal/src/__tests__/validateSchemaForReservedNames.test.ts b/packages/internal/src/__tests__/validateSchemaForReservedNames.test.ts new file mode 100644 index 000000000000..695337441c6a --- /dev/null +++ b/packages/internal/src/__tests__/validateSchemaForReservedNames.test.ts @@ -0,0 +1,177 @@ +import path from 'path' + +import { DocumentNode } from 'graphql' +import gql from 'graphql-tag' + +import { validateSchema } from '../validateSchema' + +const FIXTURE_PATH = path.resolve( + __dirname, + '../../../../__fixtures__/example-todo-main' +) + +beforeAll(() => { + process.env.RWJS_CWD = FIXTURE_PATH +}) +afterAll(() => { + delete process.env.RWJS_CWD +}) + +const validateSdlFile = async (document: DocumentNode) => { + validateSchema(document) +} + +describe('SDL with no reserved names used', () => { + describe('SDL is valid', () => { + test('with proper type definition names', async () => { + const document = gql` + type Message { + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + + input SendMessageInput { + roomId: ID! + from: String! + body: String! + } + + type Mutation { + sendMessage(input: SendMessageInput!): Message! @skipAuth + } + ` + + await expect(validateSdlFile(document)).resolves.not.toThrowError() + }) + test('with proper interface interface definition names', async () => { + const document = gql` + interface Node { + id: ID! + } + + type Message implements Node { + id: ID! + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + ` + await expect(validateSdlFile(document)).resolves.not.toThrowError() + }) + test('with proper interface input type definition names', async () => { + const document = gql` + type Message { + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + + input SendMessageInput { + roomId: ID! + from: String! + body: String! + } + + type Mutation { + sendMessage(input: SendMessageInput!): Message! @skipAuth + } + ` + await expect(validateSdlFile(document)).resolves.not.toThrowError() + }) + }) + + describe('SDL is invalid', () => { + test('because uses a reserved name as a type', async () => { + const document = gql` + type Float { + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + + input SendMessageInput { + roomId: ID! + from: String! + body: String! + } + + type Mutation { + sendMessage(input: SendMessageInput!): Message! @skipAuth + } + ` + await expect( + validateSdlFile(document) + ).rejects.toThrowErrorMatchingSnapshot() + }) + }) + + test('because uses a reserved name as an input', async () => { + const document = gql` + type Message { + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + + input Float { + roomId: ID! + from: String! + body: String! + } + + type Mutation { + sendMessage(input: SendMessageInput!): Message! @skipAuth + } + ` + await expect( + validateSdlFile(document) + ).rejects.toThrowErrorMatchingSnapshot() + }) + + test('because uses a reserved name as an interface', async () => { + const document = gql` + interface Float { + id: ID! + } + + type Message implements Float { + id: ID! + from: String + body: String + } + + type Query { + room(id: ID!): [Message!]! @skipAuth + } + + input SendMessageInput { + roomId: ID! + from: String! + body: String! + } + + type Mutation { + sendMessage(input: SendMessageInput!): Message! @skipAuth + } + ` + await expect( + validateSdlFile(document) + ).rejects.toThrowErrorMatchingSnapshot() + }) +}) diff --git a/packages/internal/src/project.ts b/packages/internal/src/project.ts index c1607d7341cd..d16a458e536c 100644 --- a/packages/internal/src/project.ts +++ b/packages/internal/src/project.ts @@ -29,3 +29,29 @@ export const getTsConfigs = () => { web: webTsConfig?.config ?? null, } } + +export const isTypeScriptProject = () => { + const paths = getPaths() + return ( + fs.existsSync(path.join(paths.web.base, 'tsconfig.json')) || + fs.existsSync(path.join(paths.api.base, 'tsconfig.json')) + ) +} + +export const isServerFileSetup = () => { + const serverFilePath = path.join( + getPaths().api.src, + `server.${isTypeScriptProject() ? 'ts' : 'js'}` + ) + + return fs.existsSync(serverFilePath) +} + +export const isRealtimeSetup = () => { + const realtimePath = path.join( + getPaths().api.lib, + `realtime.${isTypeScriptProject() ? 'ts' : 'js'}` + ) + + return fs.existsSync(realtimePath) +} diff --git a/packages/internal/src/validateSchema.ts b/packages/internal/src/validateSchema.ts index 613ccf2ee437..83c525005b1f 100644 --- a/packages/internal/src/validateSchema.ts +++ b/packages/internal/src/validateSchema.ts @@ -6,20 +6,74 @@ import { DocumentNode, Kind, ObjectTypeDefinitionNode, visit } from 'graphql' import { rootSchema } from '@redwoodjs/graphql-server' import { getPaths } from '@redwoodjs/project-config' +import { isServerFileSetup, isRealtimeSetup } from './project' + export const DIRECTIVE_REQUIRED_ERROR_MESSAGE = 'You must specify one of @requireAuth, @skipAuth or a custom directive' export const DIRECTIVE_INVALID_ROLE_TYPES_ERROR_MESSAGE = 'Please check that the requireAuth roles is a string or an array of strings.' -export function validateSchemaForDirectives( + +/** + * These are names that are commonly used in GraphQL schemas as scalars + * and would cause a conflict if used as a type name. + * + * Note: Query, Mutation, and Subscription are not included here because + * they are checked for separately. + */ +export const RESERVED_TYPES = [ + 'Int', + 'Float', + 'Boolean', + 'String', + 'DateTime', + 'ID', + 'uid', + 'as', +] + +export function validateSchema( schemaDocumentNode: DocumentNode, - typesToCheck: string[] = ['Query', 'Mutation', 'Subscription'] + typesToCheck: string[] = ['Query', 'Mutation'] ) { const validationOutput: string[] = [] + const reservedNameValidationOutput: Record = [] const directiveRoleValidationOutput: Record = [] + // Is Subscriptions are enabled with Redwood Realtime, then enforce a rule + // that a Subscription type needs to have a authentication directive applied, + // just as Query and Mutation requires + if (isServerFileSetup() && isRealtimeSetup()) { + typesToCheck.push('Subscription') + } + visit(schemaDocumentNode, { + InterfaceTypeDefinition(typeNode) { + // Warn that an interface definition in the SDL is using a reserved GraphQL type + if (RESERVED_TYPES.includes(typeNode.name.value)) { + reservedNameValidationOutput.push({ + objectType: 'interface', + name: typeNode.name.value, + }) + } + }, + InputObjectTypeDefinition(typeNode) { + // Warn that an input definition in the SDL is using a reserved GraphQL type + if (RESERVED_TYPES.includes(typeNode.name.value)) { + reservedNameValidationOutput.push({ + objectType: 'input type', + name: typeNode.name.value, + }) + } + }, ObjectTypeDefinition(typeNode) { + // Warn that a type definition in the SDL is using a reserved GraphQL type + if (RESERVED_TYPES.includes(typeNode.name.value)) { + reservedNameValidationOutput.push({ + objectType: 'type', + name: typeNode.name.value, + }) + } if (typesToCheck.includes(typeNode.name.value)) { for (const field of typeNode.fields || ([] as ObjectTypeDefinitionNode[])) { @@ -102,6 +156,15 @@ export function validateSchemaForDirectives( )} \n\nFor example: @requireAuth(roles: "admin") or @requireAuth(roles: ["admin", "editor"])` ) } + + if (reservedNameValidationOutput.length > 0) { + const reservedNameMsg = reservedNameValidationOutput.map( + (output: Record) => { + return `The ${output.objectType} named '${output.name}' is a reserved GraphQL name.\nPlease rename it to something more specific, like: Application${output.name}.\n` + } + ) + throw new TypeError(reservedNameMsg.join('\n')) + } } export const loadAndValidateSdls = async () => { @@ -137,5 +200,5 @@ export const loadAndValidateSdls = async () => { projectDocumentNodes, ]) - validateSchemaForDirectives(mergedDocumentNode) + validateSchema(mergedDocumentNode) }