diff --git a/packages/cubejs-schema-compiler/src/compiler/CompileError.ts b/packages/cubejs-schema-compiler/src/compiler/CompileError.ts index a8693bcc0fbbf..4bc07deab9165 100644 --- a/packages/cubejs-schema-compiler/src/compiler/CompileError.ts +++ b/packages/cubejs-schema-compiler/src/compiler/CompileError.ts @@ -5,4 +5,8 @@ export class CompileError extends Error { ) { super(`Compile errors:\n${messages}`); } + + public get plainMessage(): string | undefined { + return this.plainMessages; + } } diff --git a/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts b/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts index 586b8151f95de..4171514dafd93 100644 --- a/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts +++ b/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts @@ -1001,7 +1001,7 @@ export class CubeValidator { const result = cube.isView ? viewSchema.validate(cube, options) : cubeSchema.validate(cube, options); if (result.error != null) { - errorReporter.error(formatErrorMessage(result.error), result.error); + errorReporter.error(formatErrorMessage(result.error)); } else { this.validCubes.set(cube.name, true); } diff --git a/packages/cubejs-schema-compiler/src/compiler/DataSchemaCompiler.ts b/packages/cubejs-schema-compiler/src/compiler/DataSchemaCompiler.ts index 4bc551ec42098..17a75f78e66f3 100644 --- a/packages/cubejs-schema-compiler/src/compiler/DataSchemaCompiler.ts +++ b/packages/cubejs-schema-compiler/src/compiler/DataSchemaCompiler.ts @@ -631,7 +631,7 @@ export class DataSchemaCompiler { return files.map((file, index) => { errorsReport.inFile(file); if (!res[index]) { // This should not happen in theory but just to be safe - errorsReport.error(`No transpilation result received for the file ${file.fileName}.`); + errorsReport.error('No transpilation result received for the file.'); return undefined; } errorsReport.addErrors(res[index].errors, file.fileName); @@ -658,7 +658,7 @@ export class DataSchemaCompiler { return files.map((file, index) => { errorsReport.inFile(file); if (!res[index]) { // This should not happen in theory but just to be safe - errorsReport.error(`No transpilation result received for the file ${file.fileName}.`); + errorsReport.error('No transpilation result received for the file.'); return undefined; } errorsReport.addErrors(res[index].errors, file.fileName); @@ -737,7 +737,7 @@ export class DataSchemaCompiler { const err = e as SyntaxErrorInterface; const line = file.content.split('\n')[(err.loc?.start?.line || 1) - 1]; const spaces = Array(err.loc?.start.column).fill(' ').join(''); - errorsReport.error(`Syntax error during '${file.fileName}' parsing: ${err.message}:\n${line}\n${spaces}^`); + errorsReport.error(`Syntax error during parsing: ${err.message}:\n${line}\n${spaces}^`, file.fileName); } else { errorsReport.error(e); } diff --git a/packages/cubejs-schema-compiler/src/compiler/ErrorReporter.ts b/packages/cubejs-schema-compiler/src/compiler/ErrorReporter.ts index 94b96e278372f..103bc8a9987ad 100644 --- a/packages/cubejs-schema-compiler/src/compiler/ErrorReporter.ts +++ b/packages/cubejs-schema-compiler/src/compiler/ErrorReporter.ts @@ -20,7 +20,8 @@ export interface CompilerErrorInterface { export interface SyntaxErrorInterface { message: string; plainMessage?: string - loc: SourceLocation | null | undefined, + loc?: SourceLocation | null | undefined, + fileName?: string; } interface File { @@ -32,6 +33,8 @@ export interface ErrorReporterOptions { logger: (msg: string) => void } +const NO_FILE_SPECIFIED = '_No-file-specified'; + export class ErrorReporter { protected warnings: SyntaxErrorInterface[] = []; @@ -56,13 +59,15 @@ export class ErrorReporter { this.file = file; } - public warning(e: SyntaxErrorInterface) { + public warning(e: SyntaxErrorInterface, fileName?: string) { + const targetFileName = fileName || e.fileName || this.file?.fileName; + if (this.file && e.loc) { const codeFrame = codeFrameColumns(this.file.content, e.loc, { message: e.message, highlightCode: true, }); - const plainMessage = `Warning: ${e.message} in ${this.file.fileName}`; + const plainMessage = `Warning: ${e.message}`; const message = `${plainMessage}\n${codeFrame}`; if (this.rootReporter().warnings.find(m => (m.message || m) === message)) { @@ -73,27 +78,41 @@ export class ErrorReporter { message, plainMessage, loc: e.loc, + fileName: targetFileName, }); - this.options.logger(message); + if (targetFileName) { + this.options.logger(`${targetFileName}:\n${message}`); + } else { + this.options.logger(message); + } } else { if (this.rootReporter().warnings.find(m => (m.message || m) === e.message)) { return; } - this.rootReporter().warnings.push(e); + this.rootReporter().warnings.push({ + ...e, + fileName: targetFileName, + }); - this.options.logger(e.message); + if (targetFileName) { + this.options.logger(`${targetFileName}:\n${e.message}`); + } else { + this.options.logger(e.message); + } } } - public syntaxError(e: SyntaxErrorInterface) { + public syntaxError(e: SyntaxErrorInterface, fileName?: string) { + const targetFileName = fileName || e.fileName || this.file?.fileName; + if (this.file && e.loc) { const codeFrame = codeFrameColumns(this.file.content, e.loc, { message: e.message, highlightCode: true, }); - const plainMessage = `Syntax Error: ${e.message} in ${this.file.fileName}`; + const plainMessage = `Syntax Error: ${e.message}`; const message = `${plainMessage}\n${codeFrame}`; if (this.rootReporter().errors.find(m => (m.message || m) === message)) { @@ -103,44 +122,98 @@ export class ErrorReporter { this.rootReporter().errors.push({ message, plainMessage, + fileName: targetFileName, }); } else { if (this.rootReporter().errors.find(m => (m.message || m) === e.message)) { return; } - this.rootReporter().errors.push(e); + this.rootReporter().errors.push({ + ...e, + fileName: targetFileName, + }); } } public error(e: any, fileName?: any, lineNumber?: any, position?: any) { + const targetFileName = fileName || this.file?.fileName; const message = `${this.context.length ? `${this.context.join(' -> ')}: ` : ''}${e.message ? e.message : (e.stack || e)}`; if (this.rootReporter().errors.find(m => (m.message || m) === message)) { return; } - if (fileName) { - this.rootReporter().errors.push({ - message, fileName, lineNumber, position - }); - } else { - this.rootReporter().errors.push({ - message, - }); - } + this.rootReporter().errors.push({ + message, + fileName: targetFileName, + lineNumber, + position + }); } public inContext(context: string) { return new ErrorReporter(this, this.context.concat(context)); } + private groupErrors(): Map { + const { errors } = this.rootReporter(); + + const errorsByFile = new Map(); + + for (const error of errors) { + const key = error.fileName || NO_FILE_SPECIFIED; + if (!errorsByFile.has(key)) { + errorsByFile.set(key, []); + } + errorsByFile.get(key)!.push(error); + } + + return errorsByFile; + } + public throwIfAny() { - if (this.rootReporter().errors.length) { - throw new CompileError( - this.rootReporter().errors.map((e) => e.message).join('\n'), - this.rootReporter().errors.map((e) => e.plainMessage).join('\n') - ); + const { errors } = this.rootReporter(); + + if (errors.length === 0) { + return; } + + const errorsByFile = this.groupErrors(); + + // Build formatted report + const messageParts: string[] = []; + const plainMessageParts: string[] = []; + + const sortedFiles = Array.from(errorsByFile.keys()).sort(); + + for (const fileName of sortedFiles) { + const fileErrors = errorsByFile.get(fileName)!; + const reportFileName = fileName === NO_FILE_SPECIFIED ? '' : `${fileName} `; + + messageParts.push(`${reportFileName}Errors:`); + + const plainMessagesForFile: string[] = []; + for (const error of fileErrors) { + messageParts.push(error.message); + if (error.plainMessage) { + plainMessagesForFile.push(error.plainMessage); + } + } + + if (plainMessagesForFile.length > 0) { + plainMessageParts.push(`${reportFileName}Errors:`); + plainMessageParts.push(...plainMessagesForFile); + plainMessageParts.push(''); + } + + // Add blank line between file groups + messageParts.push(''); + } + + throw new CompileError( + messageParts.join('\n'), + plainMessageParts.join('\n') + ); } public getErrors() { diff --git a/packages/cubejs-schema-compiler/test/unit/__snapshots__/error-reporter.test.ts.snap b/packages/cubejs-schema-compiler/test/unit/__snapshots__/error-reporter.test.ts.snap new file mode 100644 index 0000000000000..3f4aa0aa99e85 --- /dev/null +++ b/packages/cubejs-schema-compiler/test/unit/__snapshots__/error-reporter.test.ts.snap @@ -0,0 +1,132 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`ErrorReporter should deduplicate identical errors and warnings 1`] = ` +Object { + "errors": Array [ + Object { + "fileName": "test.js", + "message": "Syntax Error: Duplicate error +> 1 | test content +  | ^^^ Duplicate error", + "plainMessage": "Syntax Error: Duplicate error", + }, + ], + "warnings": Array [ + Object { + "fileName": "test.js", + "message": "Duplicate warning", + }, + ], +} +`; + +exports[`ErrorReporter should group and format errors and warnings from different files: grouped-errors-message 1`] = ` +"Compile errors: +Errors: +Generic error + +config/database.js Errors: +Connection failed + +schema/custom.js Errors: +Parse error + +schema/orders.js Errors: +Missing required field + +schema/products.js Errors: +Validation error + +schema/users.js Errors: +Syntax Error: Invalid measure definition +  2 | sql: \`SELECT * FROM users\`, +  3 | measures: { +> 4 | count: { +  | ^^^^^ Invalid measure definition +  5 | type: 'count' +  6 | } +  7 | } +" +`; + +exports[`ErrorReporter should group and format errors and warnings from different files: grouped-errors-plain-message 1`] = ` +"schema/users.js Errors: +Syntax Error: Invalid measure definition +" +`; + +exports[`ErrorReporter should group and format errors and warnings from different files: warning-logs 1`] = ` +Array [ + "schema/users.js: +Warning: Deprecated syntax +  1 | cube('Users', { +> 2 | sql: \`SELECT * FROM users\`, +  | ^^^ Deprecated syntax +  3 | measures: { +  4 | count: { +  5 | type: 'count'", + "schema/orders.js: +Consider adding indexes", + "schema/analytics.js: +Performance warning", +] +`; + +exports[`ErrorReporter should handle addErrors and addWarnings 1`] = ` +Object { + "errors": Array [ + Object { + "fileName": "batch.js", + "lineNumber": undefined, + "message": "Error 1", + "position": undefined, + }, + Object { + "fileName": "batch.js", + "lineNumber": undefined, + "message": "Error 2", + "position": undefined, + }, + Object { + "fileName": "batch.js", + "lineNumber": undefined, + "message": "Error 3", + "position": undefined, + }, + ], + "warnings": Array [ + Object { + "fileName": undefined, + "message": "Warning 1", + }, + Object { + "fileName": undefined, + "message": "Warning 2", + }, + ], +} +`; + +exports[`ErrorReporter should handle errors without fileName at the end 1`] = ` +"Compile errors: +Errors: +Generic error without file + +fileA.js Errors: +Error in file A + +fileB.js Errors: +Error in file B +" +`; + +exports[`ErrorReporter should handle inContext correctly 1`] = ` +Array [ + Object { + "fileName": undefined, + "lineNumber": undefined, + "message": "Processing Users cube: Test error", + "position": undefined, + }, +] +`; diff --git a/packages/cubejs-schema-compiler/test/unit/error-reporter.test.ts b/packages/cubejs-schema-compiler/test/unit/error-reporter.test.ts new file mode 100644 index 0000000000000..889d34727d0ba --- /dev/null +++ b/packages/cubejs-schema-compiler/test/unit/error-reporter.test.ts @@ -0,0 +1,209 @@ +import { ErrorReporter } from '../../src/compiler/ErrorReporter'; +import { CompileError } from '../../src/compiler/CompileError'; + +describe('ErrorReporter', () => { + it('should group and format errors and warnings from different files', () => { + const logs: string[] = []; + const reporter = new ErrorReporter(null, [], { + logger: (msg) => logs.push(msg) + }); + + // Test inFile and exitFile + reporter.inFile({ + fileName: 'schema/users.js', + content: `cube('Users', {\n sql: \`SELECT * FROM users\`,\n measures: {\n count: {\n type: 'count'\n }\n }\n});` + }); + + // Test syntaxError with location + reporter.syntaxError({ + message: 'Invalid measure definition', + loc: { + start: { line: 4, column: 4 }, + end: { line: 4, column: 9 } + } + }); + + // Test warning with location + reporter.warning({ + message: 'Deprecated syntax', + loc: { + start: { line: 2, column: 2 }, + end: { line: 2, column: 5 } + } + }); + + reporter.exitFile(); + + // Test error without file context but with explicit fileName + reporter.error( + new Error('Connection failed'), + 'config/database.js', + 10, + 5 + ); + + // Test inFile for another file + reporter.inFile({ + fileName: 'schema/orders.js', + content: `cube('Orders', {\n sql: \`SELECT * FROM orders\`\n});` + }); + + // Test syntaxError without location but with file context + reporter.syntaxError({ + message: 'Missing required field' + }); + + // Test warning without location + reporter.warning({ + message: 'Consider adding indexes' + }); + + // Test error with explicit fileName (overrides current file) + reporter.error( + { message: 'Validation error' }, + 'schema/products.js' + ); + + reporter.exitFile(); + + // Test error without any file context + reporter.error(new Error('Generic error')); + + // Test syntaxError with explicit fileName + reporter.syntaxError( + { + message: 'Parse error' + }, + 'schema/custom.js' + ); + + // Test warning with explicit fileName + reporter.warning( + { + message: 'Performance warning' + }, + 'schema/analytics.js' + ); + + // Note: warnings with same message are deduplicated + expect(reporter.getErrors().length).toBe(6); + expect(reporter.getWarnings().length).toBeGreaterThanOrEqual(3); + + // Test throwIfAny - should format errors grouped by file + expect(() => reporter.throwIfAny()).toThrow(CompileError); + + try { + reporter.throwIfAny(); + } catch (e: any) { + // Snapshot the error message to verify formatting + expect(e.message).toMatchSnapshot('grouped-errors-message'); + expect(e.plainMessage).toMatchSnapshot('grouped-errors-plain-message'); + } + + // Snapshot the collected logs + expect(logs).toMatchSnapshot('warning-logs'); + }); + + it('should handle inContext correctly', () => { + const reporter = new ErrorReporter(null, [], { + logger: () => { /* empty */ } + }); + + const contextReporter = reporter.inContext('Processing Users cube'); + contextReporter.error(new Error('Test error')); + + expect(reporter.getErrors()).toMatchSnapshot(); + }); + + it('should deduplicate identical errors and warnings', () => { + const reporter = new ErrorReporter(null, [], { + logger: () => { /* empty */ } + }); + + reporter.inFile({ + fileName: 'test.js', + content: 'test content' + }); + + // Add same syntax error twice + reporter.syntaxError({ + message: 'Duplicate error', + loc: { + start: { line: 1, column: 1 }, + end: { line: 1, column: 4 } + } + }); + + reporter.syntaxError({ + message: 'Duplicate error', + loc: { + start: { line: 1, column: 1 }, + end: { line: 1, column: 4 } + } + }); + + // Add same warning twice + reporter.warning({ + message: 'Duplicate warning' + }); + + reporter.warning({ + message: 'Duplicate warning' + }); + + expect({ + errors: reporter.getErrors(), + warnings: reporter.getWarnings() + }).toMatchSnapshot(); + }); + + it('should handle addErrors and addWarnings', () => { + const reporter = new ErrorReporter(null, [], { + logger: () => { /* empty */ } + }); + + // Test addErrors with fileName + reporter.addErrors([ + new Error('Error 1'), + 'Error 2', + { message: 'Error 3' } + ], 'batch.js'); + + // Test addWarnings + reporter.addWarnings([ + { message: 'Warning 1' }, + { message: 'Warning 2' } + ]); + + expect({ + errors: reporter.getErrors(), + warnings: reporter.getWarnings() + }).toMatchSnapshot(); + }); + + it('should not throw if no errors', () => { + const reporter = new ErrorReporter(null, [], { + logger: () => { /* empty */ } + }); + + reporter.warning({ message: 'Just a warning' }); + + expect(() => reporter.throwIfAny()).not.toThrow(); + }); + + it('should handle errors without fileName at the end', () => { + const reporter = new ErrorReporter(null, [], { + logger: () => { /* empty */ } + }); + + reporter.error({ message: 'Error in file A' }, 'fileA.js'); + reporter.error({ message: 'Error in file B' }, 'fileB.js'); + reporter.error({ message: 'Generic error without file' }); + + try { + reporter.throwIfAny(); + } catch (e: any) { + expect(e.message).toMatchSnapshot(); + } + }); +});