Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip mssql tables with dots in their names instead of crashing the agent #869

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 52 additions & 31 deletions packages/datasource-sql/src/introspection/introspector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default class Introspector {
static async introspect(sequelize: Sequelize, logger?: Logger): Promise<Table[]> {
const tableNames = await this.getTableNames(sequelize as SequelizeWithOptions);
const promises = tableNames.map(name => this.getTable(sequelize, logger, name));
const tables = await Promise.all(promises);
const tables = (await Promise.all(promises)).filter(table => Boolean(table));

this.sanitizeInPlace(tables);

Expand Down Expand Up @@ -95,41 +95,62 @@ export default class Introspector {
// the table identifier is correct on our side
const tableIdentifierForQuery = Introspector.getTableIdentifier(tableIdentifier, sequelize);

const [columnDescriptions, tableIndexes, tableReferences] = await Promise.all([
queryInterface.describeTable(tableIdentifierForQuery),
queryInterface.showIndex(tableIdentifierForQuery),
queryInterface.getForeignKeyReferencesForTable(tableIdentifierForQuery),
]);
try {
const [columnDescriptions, tableIndexes, tableReferences] = await Promise.all([
queryInterface.describeTable(tableIdentifierForQuery),
queryInterface.showIndex(tableIdentifierForQuery),
queryInterface.getForeignKeyReferencesForTable(tableIdentifierForQuery),
]);

await this.detectBrokenRelationship(
tableIdentifierForQuery,
sequelize,
tableReferences,
logger,
);
await this.detectBrokenRelationship(
tableIdentifierForQuery,
sequelize,
tableReferences,
logger,
);

const columns = await Promise.all(
Object.entries(columnDescriptions).map(async ([name, description]) => {
const references = tableReferences.filter(
// There is a bug right now with sequelize on postgresql: returned association
// are not filtered on the schema. So we have to filter them manually.
// Should be fixed with Sequelize v7
r => r.columnName === name && r.tableSchema === tableIdentifier.schema,
);
const options = { name, description, references };

const columns = await Promise.all(
Object.entries(columnDescriptions).map(async ([name, description]) => {
const references = tableReferences.filter(
// There is a bug right now with sequelize on postgresql: returned association
// are not filtered on the schema. So we have to filter them manually.
// Should be fixed with Sequelize v7
r => r.columnName === name && r.tableSchema === tableIdentifier.schema,
return this.getColumn(sequelize, logger, tableIdentifier, options);
}),
);

return {
name: tableIdentifierForQuery.tableName,
schema: tableIdentifierForQuery.schema,
columns: columns.filter(Boolean),
unique: tableIndexes
.filter(i => i.unique || i.primary)
.map(i => i.fields.map(f => f.attribute)),
};
} catch (e) {
if (
(e as Error).message.includes(
`No description found for "${tableIdentifierForQuery.tableName}" table`,
)
) {
logger?.(
'Warn',
`Skipping table '${tableIdentifier.tableName}'. ${
sequelize.getDialect() === 'mssql' && tableIdentifier.tableName.includes('.')
? 'MSSQL tables with dots in their names are not supported'
: e.message
}`,
);
const options = { name, description, references };

return this.getColumn(sequelize, logger, tableIdentifier, options);
}),
);
return null;
}

return {
name: tableIdentifierForQuery.tableName,
schema: tableIdentifierForQuery.schema,
columns: columns.filter(Boolean),
unique: tableIndexes
.filter(i => i.unique || i.primary)
.map(i => i.fields.map(f => f.attribute)),
};
throw e;
}
}

private static async getColumn(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,5 +137,44 @@ describe('Introspector', () => {
"Failed to load constraints on relation 'fk_unknown_column' on table 'table1'. The relation will be ignored.",
);
});
it('should return null and log errors when failing to get table description', async () => {
const mockDescribeTable = jest
.fn()
.mockRejectedValue(new Error(`No description found for "table1" table`));
mockQueryInterface.describeTable = mockDescribeTable;
mockSequelize.getDialect = jest.fn().mockReturnValue('sqlite');
mockSequelize.options = { schema: 'public' };

const result = await Introspector.introspect(mockSequelize, logger);
expect(result).toEqual([]);
expect(logger).toHaveBeenCalledWith(
'Warn',
'Skipping table \'table1\'. No description found for "table1" table',
);
});
it('should log a specific errors for mssql tables with dots in their names', async () => {
const mockDescribeTable = jest
.fn()
.mockRejectedValue(new Error(`No description found for "table.1" table`));
mockQueryInterface.describeTable = mockDescribeTable;
mockQueryInterface.showAllTables = jest.fn().mockResolvedValue([{ tableName: 'table.1' }]);
mockSequelize.getDialect = jest.fn().mockReturnValue('mssql');

const result = await Introspector.introspect(mockSequelize, logger);
expect(result).toEqual([]);
expect(logger).toHaveBeenCalledWith(
'Warn',
"Skipping table 'table.1'. MSSQL tables with dots in their names are not supported",
);
});
it('should throw other errors', async () => {
mockQueryInterface.describeTable = jest.fn().mockRejectedValue(new Error(`A random error`));
mockSequelize.getDialect = jest.fn().mockReturnValue('sqlite');

await expect(Introspector.introspect(mockSequelize, logger)).rejects.toStrictEqual(
new Error(`A random error`),
);
expect(logger).not.toHaveBeenCalled();
});
});
});
Loading