feat: always log unexpected errors (#215)

* feat: always log unexpected errors

* feat: better handling for syntax errors on CLI and API

* chore: add changesets

* Update integration-tests/tests/cli/schema.spec.ts

* chore: ignore test fixture from prettier formatting

* run prettier

Co-authored-by: Kamil Kisiela <kamil.kisiela@gmail.com>

.changeset/little-dingos-exercise.md

@@ -0,0 +1,5 @@
+---
+'@hive/api': patch
+---
+
+Stop masking GraphQL syntax errors within `Mutation.schemaPublish`.

.changeset/quiet-dogs-flow.md

@@ -0,0 +1,5 @@
+---
+'@graphql-hive/cli': patch
+---
+
+Better error messages for SDL syntax errors.

.prettierignore

@@ -12,3 +12,6 @@ __generated__/
 /packages/web/app/src/gql/gql.d.ts
 /packages/web/app/src/gql/graphql.ts
 /packages/web/app/src/graphql/index.ts
+
+# test fixtures
+integration-tests/fixtures/init-invalid-schema.graphql

integration-tests/dockest.ts

@@ -9,6 +9,7 @@ async function main() {
     logLevel: logLevel.DEBUG,
     jestOpts: {
       runInBand: true,
+      testMatch: process.env.TEST_FILTER ? [`**/${process.env.TEST_FILTER}?(*.)+(spec|test).[jt]s?(x)`] : undefined,
       config: JSON.stringify({
         roots: ['<rootDir>/tests'],
         transform: {

integration-tests/fixtures/init-invalid-schema.graphql

@@ -0,0 +1 @@
+iliketurtles.mp4

integration-tests/tests/cli/schema.spec.ts

@@ -70,6 +70,70 @@ test('can publish and check a schema with target:registry:read access', async ()
   );
 });
 
+test('publishing invalid schema SDL provides meaningful feedback for the user.', async () => {
+  const { access_token: owner_access_token } = await authenticate('main');
+  const orgResult = await createOrganization(
+    {
+      name: 'foo',
+    },
+    owner_access_token
+  );
+  const org = orgResult.body.data!.createOrganization.ok!.createdOrganizationPayload.organization;
+  const code = org.inviteCode;
+
+  // Join
+  const { access_token: member_access_token } = await authenticate('extra');
+  await joinOrganization(code, member_access_token);
+
+  const projectResult = await createProject(
+    {
+      organization: org.cleanId,
+      type: ProjectType.Single,
+      name: 'foo',
+    },
+    owner_access_token
+  );
+
+  const project = projectResult.body.data!.createProject.ok!.createdProject;
+  const target = projectResult.body.data!.createProject.ok!.createdTargets[0];
+
+  // Create a token with write rights
+  const writeTokenResult = await createToken(
+    {
+      name: 'test',
+      organization: org.cleanId,
+      project: project.cleanId,
+      target: target.cleanId,
+      organizationScopes: [],
+      projectScopes: [],
+      targetScopes: [TargetAccessScope.RegistryRead, TargetAccessScope.RegistryWrite],
+    },
+    owner_access_token
+  );
+  expect(writeTokenResult.body.errors).not.toBeDefined();
+  const writeToken = writeTokenResult.body.data!.createToken.ok!.secret;
+
+  const allocatedError = new Error('Should have thrown.');
+  try {
+    await schemaPublish([
+      '--token',
+      writeToken,
+      '--author',
+      'Kamil',
+      '--commit',
+      'abc123',
+      'fixtures/init-invalid-schema.graphql',
+    ]);
+    throw allocatedError;
+  } catch (err) {
+    if (err === allocatedError) {
+      throw err;
+    }
+    expect(String(err)).toMatch(`The SDL is not valid at line 1, column 1:`);
+    expect(String(err)).toMatch(`Syntax Error: Unexpected Name "iliketurtles"`);
+  }
+});
+
 test('service url should be available in supergraph', async () => {
   const { access_token: owner_access_token } = await authenticate('main');
   const orgResult = await createOrganization(

packages/libraries/cli/src/commands/schema/publish.ts

@@ -1,6 +1,6 @@
 import { transformCommentsToDescriptions } from '@graphql-tools/utils';
 import { Flags, Errors } from '@oclif/core';
-import { print } from 'graphql';
+import { GraphQLError, print } from 'graphql';
 import Command from '../../base-command';
 import { gitInfo } from '../../helpers/git';
 import { invariant } from '../../helpers/validation';
@@ -138,12 +138,20 @@ export default class SchemaPublish extends Command {
         throw new Errors.CLIError(`Missing "commit"`);
       }
 
-      const sdl = await loadSchema(file);
-
-      invariant(typeof sdl === 'string' && sdl.length > 0, 'Schema seems empty');
-
-      const transformedSDL = print(transformCommentsToDescriptions(sdl));
-      const minifiedSDL = minifySchema(transformedSDL);
+      let sdl: string;
+      try {
+        const rawSdl = await loadSchema(file);
+        invariant(typeof rawSdl === 'string' && rawSdl.length > 0, 'Schema seems empty');
+        const transformedSDL = print(transformCommentsToDescriptions(rawSdl));
+        sdl = minifySchema(transformedSDL);
+      } catch (err) {
+        if (err instanceof GraphQLError) {
+          const location = err.locations?.[0];
+          const locationString = location ? ` at line ${location.line}, column ${location.column}` : '';
+          throw new Error(`The SDL is not valid${locationString}:\n ${err.message}`);
+        }
+        throw err;
+      }
 
       const result = await this.registryApi(registry, token).schemaPublish({
         input: {
@@ -151,7 +159,7 @@ export default class SchemaPublish extends Command {
           url,
           author,
           commit,
-          sdl: minifiedSDL,
+          sdl,
           force,
           metadata,
           github: usesGitHubApp,

packages/services/api/src/modules/schema/providers/schema-publisher.ts

@@ -1,13 +1,21 @@
 import { Injectable, Inject, Scope } from 'graphql-modules';
 import lodash from 'lodash';
 import type { Span } from '@sentry/types';
-import { Schema, Target, Project, ProjectType, createSchemaObject, Orchestrator } from '../../../shared/entities';
+import {
+  Schema,
+  Target,
+  Project,
+  ProjectType,
+  createSchemaObject,
+  Orchestrator,
+  GraphQLDocumentStringInvalidError,
+} from '../../../shared/entities';
 import * as Types from '../../../__generated__/types';
 import { ProjectManager } from '../../project/providers/project-manager';
 import { Logger } from '../../shared/providers/logger';
 import { updateSchemas } from '../../../shared/schema';
 import { SchemaManager } from './schema-manager';
-import { SchemaValidator } from './schema-validator';
+import { SchemaValidator, ValidationResult } from './schema-validator';
 import { sentry } from '../../../shared/sentry';
 import type { TargetSelector } from '../../shared/providers/storage';
 import { IdempotentRunner } from '../../shared/providers/idempotent-runner';
@@ -22,6 +30,7 @@ import { TargetAccessScope } from '../../auth/providers/target-access';
 import { GitHubIntegrationManager } from '../../integrations/providers/github-integration-manager';
 import type { SchemaModuleConfig } from './config';
 import { SCHEMA_MODULE_CONFIG } from './config';
+import { HiveError } from '../../../shared/errors';
 
 type CheckInput = Omit<Types.SchemaCheckInput, 'project' | 'organization' | 'target'> & TargetSelector;
 
@@ -450,18 +459,29 @@ export class SchemaPublisher {
 
     const isInitialSchema = schemas.length === 0;
 
-    const { errors, changes, valid } = await this.schemaValidator.validate({
-      orchestrator,
-      incoming: incomingSchema,
-      before: schemas,
-      after: newSchemas,
-      selector: {
-        organization: organizationId,
-        project: projectId,
-        target: targetId,
-      },
-      baseSchema: baseSchema,
-    });
+    let result: ValidationResult;
+
+    try {
+      result = await this.schemaValidator.validate({
+        orchestrator,
+        incoming: incomingSchema,
+        before: schemas,
+        after: newSchemas,
+        selector: {
+          organization: organizationId,
+          project: projectId,
+          target: targetId,
+        },
+        baseSchema: baseSchema,
+      });
+    } catch (err) {
+      if (err instanceof GraphQLDocumentStringInvalidError) {
+        throw new HiveError(err.message);
+      }
+      throw err;
+    }
+
+    const { changes, errors, valid } = result;
 
     const hasNewUrl =
       !!latest.version && !!previousSchema && (previousSchema.url ?? null) !== (incomingSchema.url ?? null);

packages/services/api/src/modules/schema/providers/schema-validator.ts

@@ -6,6 +6,11 @@ import { Logger } from '../../shared/providers/logger';
 import { sentry } from '../../../shared/sentry';
 import { Inspector } from './inspector';
 
+export type ValidationResult = {
+  valid: boolean;
+  errors: Array<Types.SchemaError>;
+  changes: Array<Types.SchemaChange>;
+};
 @Injectable({
   scope: Scope.Operation,
 })
@@ -31,7 +36,7 @@ export class SchemaValidator {
     after: readonly Schema[];
     selector: Types.TargetSelector;
     baseSchema: string | null;
-  }) {
+  }): Promise<ValidationResult> {
     this.logger.debug('Validating Schema');
     const existing = findSchema(before, incoming);
     const afterWithBase = after.map((schema, index) => {

packages/services/api/src/shared/entities.ts

@@ -1,4 +1,4 @@
-import type { DocumentNode } from 'graphql';
+import { DocumentNode, GraphQLError, SourceLocation } from 'graphql';
 import type {
   SchemaError,
   AlertChannelType,
@@ -54,9 +54,27 @@ export interface PersistedOperation {
 
 export const emptySource = '*';
 
+export class GraphQLDocumentStringInvalidError extends Error {
+  constructor(message: string, location?: SourceLocation) {
+    const locationString = location ? ` at line ${location.line}, column ${location.column}` : '';
+    super(`The provided SDL is not valid${locationString}\n: ${message}`);
+  }
+}
+
 export function createSchemaObject(schema: Schema): SchemaObject {
+  let document: DocumentNode;
+
+  try {
+    document = parse(schema.source);
+  } catch (err) {
+    if (err instanceof GraphQLError) {
+      throw new GraphQLDocumentStringInvalidError(err.message, err.locations?.[0]);
+    }
+    throw err;
+  }
+
   return {
-    document: parse(schema.source),
+    document,
     raw: schema.source,
     source: schema.service ?? emptySource,
     url: schema.url ?? null,

packages/services/server/src/graphql-handler.ts

@@ -1,7 +1,7 @@
 import type { RouteHandlerMethod, FastifyRequest, FastifyReply } from 'fastify';
 import { Registry } from '@hive/api';
 import { cleanRequestId } from '@hive/service-common';
-import { createServer } from '@graphql-yoga/node';
+import { createServer, GraphQLYogaError } from '@graphql-yoga/node';
 import { GraphQLError, ValidationContext, ValidationRule, Kind, OperationDefinitionNode, print } from 'graphql';
 import { useGraphQLModules } from '@envelop/graphql-modules';
 import { useAuth0 } from '@envelop/auth0';
@@ -83,10 +83,7 @@ export const graphqlHandler = (options: GraphQLHandlerOptions): RouteHandlerMeth
           return args.operationName === 'readiness';
         },
       }),
-      useSentryUser(),
-      useErrorHandler(errors => {
-        errors?.map(e => server.logger.error(e));
-      })
+      useSentryUser()
     );
   }
 
@@ -98,6 +95,15 @@ export const graphqlHandler = (options: GraphQLHandlerOptions): RouteHandlerMeth
   }>({
     plugins: [
       ...additionalPlugins,
+      useErrorHandler(errors => {
+        for (const error of errors) {
+          // Only log unexpected errors.
+          if (error.originalError instanceof GraphQLYogaError) {
+            continue;
+          }
+          server.logger.error(error);
+        }
+      }),
       useAuth0({
         onError() {},
         domain: process.env.AUTH0_DOMAIN!,