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(aurora): pass formatOptions to Data API Client, fix extensions #6404

Merged
merged 4 commits into from Jul 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
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
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
79 changes: 78 additions & 1 deletion src/driver/postgres/PostgresDriver.ts
Expand Up @@ -1041,7 +1041,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 +1091,80 @@ 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 hasUuidColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.generatedColumns.filter(column => column.generationStrategy === "uuid").length > 0;
});
const hasCitextColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.columns.filter(column => column.type === "citext").length > 0;
});
const hasHstoreColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.columns.filter(column => column.type === "hstore").length > 0;
});
const hasCubeColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.columns.filter(column => column.type === "cube").length > 0;
});
const hasGeometryColumns = this.connection.entityMetadatas.some(metadata => {
return metadata.columns.filter(column => this.spatialTypes.indexOf(column.type) >= 0).length > 0;
});
const hasExclusionConstraints = this.connection.entityMetadatas.some(metadata => {
return metadata.exclusions.length > 0;
});
if (hasUuidColumns || hasCitextColumns || hasHstoreColumns || hasGeometryColumns || hasCubeColumns || hasExclusionConstraints) {
const {logger} = this.connection;

if (hasUuidColumns) {
try {
await this.executeQuery(this.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(this.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(this.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(this.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(this.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(this.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");
}
}
}
return Promise.resolve();
}
}