chore: separate database dialect and driver dialect #235
Conversation
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
from
0ccd6ea to
69dbd46
Compare
Its the name of the target drivers we are using, node-postgres and mysql2 |
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
5 times, most recently
from
f44af69 to
4de2262
Compare
| getDialectName(): string; | ||
| abort(targetClient: ClientWrapper): Promise<void>; | ||
| rollback(targetClient: ClientWrapper): Promise<any>; | ||
| connect(targetClient: any): Promise<any>; |
There was a problem hiding this comment.
With this latest update the function signature should be:
connect(props: Map<string, any>): Promise<any>;
| } | ||
| } | ||
|
|
||
| connect(props: Map<string, any>): Promise<any> { |
There was a problem hiding this comment.
With the latest update connect should be returning the connected 'raw' targetClient of type any.
It probably needs to be async connect(...) {}
Please verify, but I think this function should look like this:
async connect(props: Map<string, any>): Promise<any> {
const targetClient: any = createConnection(WrapperProperties.removeWrapperProperties(props));
await targetClient
.promise()
.connect()
.catch((error: any) => {
throw error;
});
return targetClient;
}
There was a problem hiding this comment.
We don't need to call connect here
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
from
4de2262 to
a6f20c3
Compare
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
from
a6f20c3 to
c7c630f
Compare
common/lib/plugin_service.ts
Outdated
| @@ -404,7 +404,7 @@ export class PluginService implements ErrorHandler, HostListProviderService { | |||
| } | |||
| } | |||
|
|
|||
| async abortTargetClient(targetClient: ClientWrapper | undefined): Promise<void> { | |||
| async abortTargetClient(targetClient: ClientWrapper | undefined | null): Promise<void> { | |||
| if (targetClient) { | |||
| await this.getDriverDialect().abort(targetClient); | |||
There was a problem hiding this comment.
Why not to implement abort() in ClientWrapper classes (similar to rollback and destroy)?
There was a problem hiding this comment.
targetClient might be a PoolClientWrapper. Can driver dialect abort such connection?
There was a problem hiding this comment.
I forgot about abort()
common/lib/pool_client_wrapper.ts
Outdated
| } | ||
|
|
||
| end(): Promise<void> { | ||
| return this.client.release(); |
There was a problem hiding this comment.
return this.pool.release(this.cllient);
?
There was a problem hiding this comment.
Should be calling release on the target client not the pool.
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
from
cf3ba4e to
93d6109
Compare
karenc-bq
force-pushed
the
refactor/driver-dialect
branch
from
93d6109 to
b4a951c
Compare
Summary
The database dialect class is a little convoluted right now. It contains methods that should are not relevant to the database and are more relevant to the target driver instead.
This PR separates the dialect classes into database and driver dialect.
This PR also implements the
ClientWrapperinterface viaPgClientWrapper,MySQLClientWrapperandPoolClientWrapper. Prior to this change, the wrapper was passingClientWrapperaround, which contains no information about the underlying target client, the wrapper had to rely on under classes such as driver dialects and connection providers to determine which database method to call on the received ClientWrapper object. With these specific implementations, the wrapper is able to simply callClientWrapper#endwithout having to worry about the driver specific implementation.Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.