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.

Sign up for GitHub

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

Jump to bottom

fix(aurora): pass formatOptions to Data API Client, fix extensions #6404

Merged
merged 4 commits into from Jul 21, 2020
Merged

fix(aurora): pass formatOptions to Data API Client, fix extensions #6404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44dc875
fix(aurora): pass formatOptions to Data API Client, fix UUID type sup…
ArsenyYankovsky Jul 14, 2020
ca61985
fix(aurora): refactored the code to avoid duplication (#1)
ArsenyYankovsky Jul 18, 2020
a431511
Merge branch 'master' into master
ArsenyYankovsky Jul 18, 2020
19edb49
fix(aurora): refactored the code to avoid duplication
ArsenyYankovsky Jul 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/driver/DriverFactory.ts
View file
Open in desktop
Expand Up @@ -9,13 +9,12 @@ import {ReactNativeDriver} from "./react-native/ReactNativeDriver";
import {NativescriptDriver} from "./nativescript/NativescriptDriver";
import {SqljsDriver} from "./sqljs/SqljsDriver";
import {MysqlDriver} from "./mysql/MysqlDriver";
import {PostgresDriver} from "./postgres/PostgresDriver";
import {PostgresDriver, AuroraDataApiPostgresDriver} from "./postgres/PostgresDriver";
import {ExpoDriver} from "./expo/ExpoDriver";
import {AuroraDataApiDriver} from "./aurora-data-api/AuroraDataApiDriver";
import {Driver} from "./Driver";
import {Connection} from "../connection/Connection";
import {SapDriver} from "./sap/SapDriver";
import {AuroraDataApiPostgresDriver} from "./postgres/PostgresDriver";
import {BetterSqlite3Driver} from "./better-sqlite3/BetterSqlite3Driver";

/**
Expand Down
2 changes: 2 additions & 0 deletions src/driver/aurora-data-api-pg/AuroraDataApiPostgresConnectionOptions.ts
View file
Open in desktop
Expand Up @@ -33,4 +33,6 @@ export interface AuroraDataApiPostgresConnectionOptions extends BaseConnectionOp
readonly poolErrorHandler?: (err: any) => any;

readonly serviceConfigOptions?: { [key: string]: any };

readonly formatOptions?: { [key: string]: any };
}
2 changes: 2 additions & 0 deletions src/driver/aurora-data-api/AuroraDataApiConnectionOptions.ts
View file
Open in desktop
Expand Up @@ -23,6 +23,8 @@ export interface AuroraDataApiConnectionOptions extends BaseConnectionOptions, A

readonly serviceConfigOptions?: { [key: string]: any }; // pass optional AWS.ConfigurationOptions here

readonly formatOptions?: { [key: string]: any };

/**
* Use spatial functions like GeomFromText and AsText which are removed in MySQL 8.
* (Default: true)
Expand Down
3 changes: 2 additions & 1 deletion src/driver/aurora-data-api/AuroraDataApiDriver.ts
View file
Open in desktop
Expand Up @@ -306,7 +306,8 @@ export class AuroraDataApiDriver implements Driver {
this.options.resourceArn,
this.options.database,
(query: string, parameters?: any[]) => this.connection.logger.logQuery(query, parameters),
this.options.serviceConfigOptions
this.options.serviceConfigOptions,
this.options.formatOptions,
);

// validate options to make sure everything is set
Expand Down
149 changes: 94 additions & 55 deletions src/driver/postgres/PostgresDriver.ts
View file
Open in desktop
Expand Up @@ -301,6 +301,76 @@ export class PostgresDriver implements Driver {
* Makes any action after connection (e.g. create extensions in Postgres driver).
*/
async afterConnect(): Promise<void> {
const extensionsMetadata = await this.checkMetadataForExtensions();

if (extensionsMetadata.hasExtensions) {
await Promise.all([this.master, ...this.slaves].map(pool => {
return new Promise((ok, fail) => {
pool.connect(async (err: any, connection: any, release: Function) => {
await this.enableExtensions(extensionsMetadata, connection);
if (err) return fail(err);
release();
ok();
});
});
}));
}

return Promise.resolve();
}

protected async enableExtensions(extensionsMetadata: any, connection: any) {
const { logger } = this.connection;

const {
hasUuidColumns,
hasCitextColumns,
hasHstoreColumns,
hasCubeColumns,
hasGeometryColumns,
hasExclusionConstraints,
} = extensionsMetadata;

if (hasUuidColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "${this.options.uuidExtension || "uuid-ossp"}"`);
} catch (_) {
logger.log("warn", `At least one of the entities has uuid column, but the '${this.options.uuidExtension || "uuid-ossp"}' extension cannot be installed automatically. Please install it manually using superuser rights, or select another uuid extension.`);
}
if (hasCitextColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "citext"`);
} catch (_) {
logger.log("warn", "At least one of the entities has citext column, but the 'citext' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasHstoreColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "hstore"`);
} catch (_) {
logger.log("warn", "At least one of the entities has hstore column, but the 'hstore' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasGeometryColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "postgis"`);
} catch (_) {
logger.log("warn", "At least one of the entities has a geometry column, but the 'postgis' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasCubeColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "cube"`);
} catch (_) {
logger.log("warn", "At least one of the entities has a cube column, but the 'cube' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasExclusionConstraints)
try {
// The btree_gist extension provides operator support in PostgreSQL exclusion constraints
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "btree_gist"`);
} catch (_) {
logger.log("warn", "At least one of the entities has an exclusion constraint, but the 'btree_gist' extension cannot be installed automatically. Please install it manually using superuser rights");
}
}

protected async checkMetadataForExtensions() {
const hasUuidColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.generatedColumns.filter(column => column.generationStrategy === "uuid").length > 0;
});
Expand All @@ -319,57 +389,16 @@ export class PostgresDriver implements Driver {
const hasExclusionConstraints = this.connection.entityMetadatas.some(metadata => {
return metadata.exclusions.length > 0;
});
if (hasUuidColumns || hasCitextColumns || hasHstoreColumns || hasGeometryColumns || hasCubeColumns || hasExclusionConstraints) {
await Promise.all([this.master, ...this.slaves].map(pool => {
return new Promise((ok, fail) => {
pool.connect(async (err: any, connection: any, release: Function) => {
const { logger } = this.connection;
if (err) return fail(err);
if (hasUuidColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "${this.options.uuidExtension || "uuid-ossp"}"`);
} catch (_) {
logger.log("warn", `At least one of the entities has uuid column, but the '${this.options.uuidExtension || "uuid-ossp"}' extension cannot be installed automatically. Please install it manually using superuser rights, or select another uuid extension.`);
}
if (hasCitextColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "citext"`);
} catch (_) {
logger.log("warn", "At least one of the entities has citext column, but the 'citext' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasHstoreColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "hstore"`);
} catch (_) {
logger.log("warn", "At least one of the entities has hstore column, but the 'hstore' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasGeometryColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "postgis"`);
} catch (_) {
logger.log("warn", "At least one of the entities has a geometry column, but the 'postgis' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasCubeColumns)
try {
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "cube"`);
} catch (_) {
logger.log("warn", "At least one of the entities has a cube column, but the 'cube' extension cannot be installed automatically. Please install it manually using superuser rights");
}
if (hasExclusionConstraints)
try {
// The btree_gist extension provides operator support in PostgreSQL exclusion constraints
await this.executeQuery(connection, `CREATE EXTENSION IF NOT EXISTS "btree_gist"`);
} catch (_) {
logger.log("warn", "At least one of the entities has an exclusion constraint, but the 'btree_gist' extension cannot be installed automatically. Please install it manually using superuser rights");
}
release();
ok();
});
});
}));
}

return Promise.resolve();
return {
hasUuidColumns,
hasCitextColumns,
hasHstoreColumns,
hasCubeColumns,
hasGeometryColumns,
hasExclusionConstraints,
hasExtensions: hasUuidColumns || hasCitextColumns || hasHstoreColumns || hasGeometryColumns || hasCubeColumns || hasExclusionConstraints,
};
}

/**
Expand All @@ -395,7 +424,7 @@ export class PostgresDriver implements Driver {
/**
* Creates a query runner used to execute database queries.
*/
createQueryRunner(mode: "master"|"slave" = "master") {
createQueryRunner(mode: "master"|"slave" = "master"): QueryRunner {
return new PostgresQueryRunner(this, mode);
}

Expand Down Expand Up @@ -987,9 +1016,6 @@ abstract class PostgresWrapper extends PostgresDriver {
abstract createQueryRunner(mode: "master"|"slave"): any;
}

/**
* Organizes communication with PostgreSQL DBMS.
*/
export class AuroraDataApiPostgresDriver extends PostgresWrapper {

// -------------------------------------------------------------------------
Expand Down Expand Up @@ -1041,7 +1067,8 @@ export class AuroraDataApiPostgresDriver extends PostgresWrapper {
this.options.resourceArn,
this.options.database,
(query: string, parameters?: any[]) => this.connection.logger.logQuery(query, parameters),
this.options.serviceConfigOptions
this.options.serviceConfigOptions,
this.options.formatOptions,
);
}

Expand Down Expand Up @@ -1090,4 +1117,16 @@ export class AuroraDataApiPostgresDriver extends PostgresWrapper {
return this.client.query(query);
}

/**
* Makes any action after connection (e.g. create extensions in Postgres driver).
*/
async afterConnect(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code confused me a lot. And I just found that AuroraDataApiPostgresDriver in the same file as PostgresDriver. Can we have it in a separate file?

Also found there is a weird PostgresWrapper... Can we avoid it?

Also why do we need this duplication with afterConnect? Even hard-coded to a specific implementation if check inside PostgresDriver would be better than this kind-of duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main issue why they're in the same file is the circular dependency. I remember trying to break it out but ended up having them like that.
As for the PostgresWrapper I guess we can get rid of it if we define the options as PostgresConnectionOptions | AuroraDataApiPostgresConnectionOptions and get rid of it.
The afterConnect I will refactor, you're right there's no need for the duplication there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostgresConnectionOptions | AuroraDataApiPostgresConnectionOptions option I don't think will work because it means we need to cast options everywhere we use them in the PostgresDriver class.
I also tried moving AuroraDataApiPostgresDriver out in a separate file but I found no way around the circular dependency issue.

const extensionsMetadata = await this.checkMetadataForExtensions();

if (extensionsMetadata.hasExtensions) {
await this.enableExtensions(extensionsMetadata, this.connection);
}

return Promise.resolve();
}
}