Skip to content

Commit

Permalink
fix: use full path for table lookups (#8097)
Browse files Browse the repository at this point in the history
The changes made as part of #7575 fixed some issues and introduced others. The problem became that the table name would be compared against the EntityMetadata tablePath. This would fail in some cases because while the EntityMetadata would specify database, it'd be the "default"/current database & would be omitted from the table name.

To work around that this PR introduces the database & schema on the table objects as well as a fully-qualified table path property which will always include all of them for comparing against EntityMetadata.
  • Loading branch information
imnotjames authored Aug 24, 2021
1 parent e9366b3 commit 22676a0
Show file tree
Hide file tree
Showing 5 changed files with 211 additions and 53 deletions.
57 changes: 39 additions & 18 deletions src/query-runner/BaseQueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ export abstract class BaseQueryRunner {
*/
protected mode: ReplicationMode;

private cachedTablePaths: Record<string, string> = {};

// -------------------------------------------------------------------------
// Public Abstract Methods
// -------------------------------------------------------------------------
Expand Down Expand Up @@ -223,13 +225,29 @@ export abstract class BaseQueryRunner {
* Gets table from previously loaded tables, otherwise loads it from database.
*/
protected async getCachedTable(tableName: string): Promise<Table> {
const table = this.loadedTables.find(table => table.name === tableName);
if (table) return table;
if (tableName in this.cachedTablePaths) {
const tablePath = this.cachedTablePaths[tableName];
const table = this.loadedTables.find(table => this.getTablePath(table) === tablePath);

if (table) {
return table;
}
}

const foundTables = await this.loadTables([tableName]);

if (foundTables.length > 0) {
this.loadedTables.push(foundTables[0]);
return foundTables[0];
const foundTablePath = this.getTablePath(foundTables[0]);

const cachedTable = this.loadedTables.find((table) => this.getTablePath(table) === foundTablePath);

if (!cachedTable) {
this.cachedTablePaths[tableName] = this.getTablePath(foundTables[0]);
this.loadedTables.push(foundTables[0]);
return foundTables[0];
} else {
return cachedTable;
}
} else {
throw new TypeORMError(`Table "${tableName}" does not exist.`);
}
Expand All @@ -239,7 +257,16 @@ export abstract class BaseQueryRunner {
* Replaces loaded table with given changed table.
*/
protected replaceCachedTable(table: Table, changedTable: Table): void {
const foundTable = this.loadedTables.find(loadedTable => loadedTable.name === table.name);
const oldTablePath = this.getTablePath(table);
const foundTable = this.loadedTables.find(loadedTable => this.getTablePath(loadedTable) === oldTablePath);

// Clean up the lookup cache..
for (const [key, cachedPath] of Object.entries(this.cachedTablePaths)) {
if (cachedPath === oldTablePath) {
this.cachedTablePaths[key] = this.getTablePath(changedTable);
}
}

if (foundTable) {
foundTable.database = changedTable.database;
foundTable.schema = changedTable.schema;
Expand All @@ -254,20 +281,14 @@ export abstract class BaseQueryRunner {
}
}

protected getTablePath(target: EntityMetadata | Table | TableForeignKey | string): string {
if (target instanceof Table) {
return target.name;
}

if (target instanceof TableForeignKey) {
return target.referencedTableName;
}

if (target instanceof EntityMetadata) {
return target.tablePath;
}
protected getTablePath(target: EntityMetadata | Table | View | TableForeignKey | string): string {
const parsed = this.connection.driver.parseTableName(target);

return target;
return this.connection.driver.buildTableName(
parsed.tableName,
parsed.schema,
parsed.database
);
}

protected getTypeormMetadataTableName(): string {
Expand Down
56 changes: 21 additions & 35 deletions src/schema-builder/RdbmsSchemaBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {SqlInMemory} from "../driver/SqlInMemory";
import {TableUtils} from "./util/TableUtils";
import {TableColumnOptions} from "./options/TableColumnOptions";
import {PostgresDriver} from "../driver/postgres/PostgresDriver";
import {SqlServerDriver} from "../driver/sqlserver/SqlServerDriver";
import {MysqlDriver} from "../driver/mysql/MysqlDriver";
import {TableUnique} from "./table/TableUnique";
import {TableCheck} from "./table/TableCheck";
Expand All @@ -35,16 +34,15 @@ import {AuroraDataApiDriver} from "../driver/aurora-data-api/AuroraDataApiDriver
* 9. create indices which are missing in db yet, and drops indices which exist in the db, but does not exist in the metadata anymore
*/
export class RdbmsSchemaBuilder implements SchemaBuilder {

// -------------------------------------------------------------------------
// Protected Properties
// -------------------------------------------------------------------------

/**
* Used to execute schema creation queries in a single connection.
*/
protected queryRunner: QueryRunner;

private currentDatabase?: string;

private currentSchema?: string;

// -------------------------------------------------------------------------
// Constructor
// -------------------------------------------------------------------------
Expand All @@ -61,6 +59,11 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
*/
async build(): Promise<void> {
this.queryRunner = this.connection.createQueryRunner();

// this.connection.driver.database || this.currentDatabase;
this.currentDatabase = this.connection.driver.database;
this.currentSchema = this.connection.driver.schema;

// CockroachDB implements asynchronous schema sync operations which can not been executed in transaction.
// E.g. if you try to DROP column and ADD it again in the same transaction, crdb throws error.
const isUsingTransactions = (
Expand Down Expand Up @@ -186,16 +189,14 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
await this.createViews();
}

private getTablePath(target: EntityMetadata | Table | TableForeignKey): string {
if (target instanceof Table) {
return target.name;
}

if (target instanceof TableForeignKey) {
return target.referencedTableName;
}
private getTablePath(target: EntityMetadata | Table | View | TableForeignKey | string): string {
const parsed = this.connection.driver.parseTableName(target);

return target.tablePath;
return this.connection.driver.buildTableName(
parsed.tableName,
parsed.schema || this.currentSchema,
parsed.database || this.currentDatabase
);
}

/**
Expand Down Expand Up @@ -389,18 +390,9 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
* Primary key only can be created in conclusion with auto generated column.
*/
protected async createNewTables(): Promise<void> {
const currentSchema = await this.queryRunner.getCurrentSchema();
for (const metadata of this.entityToSyncMetadatas) {
// check if table does not exist yet
const existTable = this.queryRunner.loadedTables.find(table => {
const database = metadata.database && metadata.database !== this.connection.driver.database ? metadata.database : undefined;
let schema = metadata.schema || (this.connection.driver.options as any).schema;
// if schema is default db schema (e.g. "public" in PostgreSQL), skip it.
schema = schema === currentSchema ? undefined : schema;
const fullTableName = this.connection.driver.buildTableName(metadata.tableName, schema, database);

return table.name === fullTableName;
});
const existTable = this.queryRunner.loadedTables.find(table => this.getTablePath(table) === this.getTablePath(metadata));
if (existTable)
continue;

Expand All @@ -417,12 +409,9 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
for (const metadata of this.viewEntityToSyncMetadatas) {
// check if view does not exist yet
const existView = this.queryRunner.loadedViews.find(view => {
const database = metadata.database && metadata.database !== this.connection.driver.database ? metadata.database : undefined;
const schema = metadata.schema || (<SqlServerDriver|PostgresDriver>this.connection.driver).options.schema;
const fullViewName = this.connection.driver.buildTableName(metadata.tableName, schema, database);
const viewExpression = typeof view.expression === "string" ? view.expression.trim() : view.expression(this.connection).getQuery();
const metadataExpression = typeof metadata.expression === "string" ? metadata.expression.trim() : metadata.expression!(this.connection).getQuery();
return view.name === fullViewName && viewExpression === metadataExpression;
return this.getTablePath(view) === this.getTablePath(metadata) && viewExpression === metadataExpression;
});
if (existView)
continue;
Expand All @@ -440,12 +429,9 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
const droppedViews: Set<View> = new Set();
for (const view of this.queryRunner.loadedViews) {
const existViewMetadata = this.viewEntityToSyncMetadatas.find(metadata => {
const database = metadata.database && metadata.database !== this.connection.driver.database ? metadata.database : undefined;
const schema = metadata.schema || (<SqlServerDriver|PostgresDriver>this.connection.driver).options.schema;
const fullViewName = this.connection.driver.buildTableName(metadata.tableName, schema, database);
const viewExpression = typeof view.expression === "string" ? view.expression.trim() : view.expression(this.connection).getQuery();
const metadataExpression = typeof metadata.expression === "string" ? metadata.expression.trim() : metadata.expression!(this.connection).getQuery();
return view.name === fullViewName && viewExpression === metadataExpression;
return this.getTablePath(view) === this.getTablePath(metadata) && viewExpression === metadataExpression;
});

if (existViewMetadata)
Expand Down Expand Up @@ -780,8 +766,8 @@ export class RdbmsSchemaBuilder implements SchemaBuilder {
* Creates typeorm service table for storing user defined Views.
*/
protected async createTypeormMetadataTable() {
const { schema } = this.connection.driver.options as any;
const database = this.connection.driver.database;
const schema = this.currentSchema;
const database = this.currentDatabase;
const typeormMetadataTable = this.connection.driver.buildTableName("typeorm_metadata", schema, database);

await this.queryRunner.createTable(new Table(
Expand Down
59 changes: 59 additions & 0 deletions test/functional/schema-builder/custom-db-and-schema-sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,65 @@ describe("schema builder > custom-db-and-schema-sync", () => {
await queryRunner.release();

})));

it("should correctly sync tables with `public` schema", () => Promise.all(connections.map(async connection => {
const queryRunner = connection.createQueryRunner();
const photoMetadata = connection.getMetadata("photo");
const albumMetadata = connection.getMetadata("album");

// create tables
photoMetadata.synchronize = true;
albumMetadata.synchronize = true;

photoMetadata.schema = "public";
photoMetadata.tablePath = "photo";

albumMetadata.schema = "public";
albumMetadata.tablePath = "album";

await queryRunner.createSchema(photoMetadata.schema, true);
await queryRunner.createSchema(albumMetadata.schema, true);

await connection.synchronize();

// create foreign key
let albumTable = await queryRunner.getTable(albumMetadata.tablePath);
let photoTable = await queryRunner.getTable(photoMetadata.tablePath);

albumTable!.should.be.exist;
photoTable!.should.be.exist;

photoTable!.foreignKeys.length.should.be.equal(0);

const columns = photoMetadata.columns.filter(column => column.propertyName === "albumId");
const referencedColumns = albumMetadata.columns.filter(column => column.propertyName === "id");
const fkMetadata = new ForeignKeyMetadata({
entityMetadata: photoMetadata,
referencedEntityMetadata: albumMetadata,
columns: columns,
referencedColumns: referencedColumns,
namingStrategy: connection.namingStrategy
});

photoMetadata.foreignKeys.push(fkMetadata);
await connection.synchronize();

photoTable = await queryRunner.getTable(photoMetadata.tablePath);
photoTable!.foreignKeys.length.should.be.equal(1);

// drop foreign key
photoMetadata.foreignKeys = [];
await connection.synchronize();

// drop tables manually, because they will not synchronize automatically
await queryRunner.dropTable(photoMetadata.tablePath, true, false);
await queryRunner.dropTable(albumMetadata.tablePath, true, false);

// drop created database
await queryRunner.dropDatabase("secondDB", true);

await queryRunner.release();
})));
})

describe("custom database and schema", () => {
Expand Down
10 changes: 10 additions & 0 deletions test/github-issues/7867/entity/Example.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Column, Entity, PrimaryGeneratedColumn } from "../../../../src";

@Entity()
export class Example {
@PrimaryGeneratedColumn()
id: number;

@Column()
name: string;
}
82 changes: 82 additions & 0 deletions test/github-issues/7867/issue-7867.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import "reflect-metadata";
import { Connection } from "../../../src";
import { closeTestingConnections, createTestingConnections, reloadTestingDatabases } from "../../utils/test-utils";
import { Example } from "./entity/Example";
import { expect } from "chai";

describe("github issues > #7867 Column not renamed when schema/database is set", () => {

describe('schema is set', () => {
let connections: Connection[];
before(async () => {
connections = await createTestingConnections({
entities: [ Example ],
schemaCreate: true,
dropSchema: true,
driverSpecific: {
schema: "public"
},
enabledDrivers: [ "postgres" ],
});
});
beforeEach(() => reloadTestingDatabases(connections));
after(() => closeTestingConnections(connections));

it("should correctly change column name", () => Promise.all(connections.map(async connection => {
const postMetadata = connection.getMetadata(Example);
const nameColumn = postMetadata.findColumnWithPropertyName("name")!;
nameColumn.propertyName = "title";
nameColumn.build(connection);

await connection.synchronize();

const queryRunner = connection.createQueryRunner();
const postTable = await queryRunner.getTable("example");
await queryRunner.release();

expect(postTable!.findColumnByName("name")).to.be.undefined;
postTable!.findColumnByName("title")!.should.be.exist;

// revert changes
nameColumn.propertyName = "name";
nameColumn.build(connection);
})));
});

describe('database is set', () => {
let connections: Connection[];
before(async () => {
connections = await createTestingConnections({
entities: [ Example ],
schemaCreate: true,
dropSchema: true,
driverSpecific: {
database: "test"
},
enabledDrivers: [ "mysql" ],
});
});
beforeEach(() => reloadTestingDatabases(connections));
after(() => closeTestingConnections(connections));

it("should correctly change column name", () => Promise.all(connections.map(async connection => {
const postMetadata = connection.getMetadata(Example);
const nameColumn = postMetadata.findColumnWithPropertyName("name")!;
nameColumn.propertyName = "title";
nameColumn.build(connection);

await connection.synchronize();

const queryRunner = connection.createQueryRunner();
const postTable = await queryRunner.getTable("example");
await queryRunner.release();

expect(postTable!.findColumnByName("name")).to.be.undefined;
postTable!.findColumnByName("title")!.should.be.exist;

// revert changes
nameColumn.propertyName = "name";
nameColumn.build(connection);
})));
});
});

0 comments on commit 22676a0

Please sign in to comment.