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

Database driver adaptations #32

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

tksilicon
Copy link
Collaborator

Following issue #4 , the Postgres referenced in #2 was adapted. Deno-sqlite was selected. DSO can now run on Postgres and Sqlite as well as MySQL.

Changes have been made to readme file. It seems there is need to bump up Deno version to 1.1.
Github workflow for automated testing needs to be updated to have Postgres and Sqlite Databases.

@afinch7
Copy link
Contributor

afinch7 commented Aug 4, 2020

Here's a patch for #33

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index e950cfa..001c3d0 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -18,8 +18,18 @@ jobs:
         env:
           MYSQL_ALLOW_EMPTY_PASSWORD: "true"
           MYSQL_ROOT_PASSWORD: ""
-        options: --health-cmd="mysqladmin ping" --health-interval=5s --health-timeout=2s --health-retries=3 
+        options: --health-cmd="mysqladmin ping" --health-interval=5s --health-timeout=2s --health-retries=3
 
+      postgres:
+        image: postgres:10.8
+        ports:
+          - 5432:5432
+        env:
+          POSTGRES_USER: "thankgodukachukwu"
+          POSTGRES_PASSWORD: "test_orm" 
+          POSTGRES_DB: "test_orm"
+        options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
+        
     steps:
       - uses: actions/checkout@v2
       - name: Setup Deno
diff --git a/docker-compose.yml b/docker-compose.yml
index 682dddf..33420df 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -7,4 +7,12 @@ services:
       - 3306:3306
     environment:
       MYSQL_ALLOW_EMPTY_PASSWORD: "true"
-      MYSQL_ROOT_PASSWORD: ""
\ No newline at end of file
+      MYSQL_ROOT_PASSWORD: ""
+  postgres:
+    image: postgres:10.8
+    ports:
+      - 5432:5432
+    environment:
+      POSTGRES_USER: "thankgodukachukwu"
+      POSTGRES_PASSWORD: "test_orm" 
+      POSTGRES_DB: "test_orm"
\ No newline at end of file
diff --git a/test.ts b/test.ts
index 8be1365..8c810d1 100644
--- a/test.ts
+++ b/test.ts
@@ -46,7 +46,7 @@ const config2 = {
   user: "thankgodukachukwu",
   database: "test_orm",
   hostname: "127.0.0.1",
-  password: "",
+  password: "test_orm",
   port: 5432,
 };

@tksilicon
Copy link
Collaborator Author

Here's a patch for #33

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index e950cfa..001c3d0 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -18,8 +18,18 @@ jobs:
         env:
           MYSQL_ALLOW_EMPTY_PASSWORD: "true"
           MYSQL_ROOT_PASSWORD: ""
-        options: --health-cmd="mysqladmin ping" --health-interval=5s --health-timeout=2s --health-retries=3 
+        options: --health-cmd="mysqladmin ping" --health-interval=5s --health-timeout=2s --health-retries=3
 
+      postgres:
+        image: postgres:10.8
+        ports:
+          - 5432:5432
+        env:
+          POSTGRES_USER: "thankgodukachukwu"
+          POSTGRES_PASSWORD: "test_orm" 
+          POSTGRES_DB: "test_orm"
+        options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
+        
     steps:
       - uses: actions/checkout@v2
       - name: Setup Deno
diff --git a/docker-compose.yml b/docker-compose.yml
index 682dddf..33420df 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -7,4 +7,12 @@ services:
       - 3306:3306
     environment:
       MYSQL_ALLOW_EMPTY_PASSWORD: "true"
-      MYSQL_ROOT_PASSWORD: ""
\ No newline at end of file
+      MYSQL_ROOT_PASSWORD: ""
+  postgres:
+    image: postgres:10.8
+    ports:
+      - 5432:5432
+    environment:
+      POSTGRES_USER: "thankgodukachukwu"
+      POSTGRES_PASSWORD: "test_orm" 
+      POSTGRES_DB: "test_orm"
\ No newline at end of file
diff --git a/test.ts b/test.ts
index 8be1365..8c810d1 100644
--- a/test.ts
+++ b/test.ts
@@ -46,7 +46,7 @@ const config2 = {
   user: "thankgodukachukwu",
   database: "test_orm",
   hostname: "127.0.0.1",
-  password: "",
+  password: "test_orm",
   port: 5432,
 };

Thanks, two more things:

  1. Sqlite in ci-yml and docker-compose.yml
  2. In ci.yml, steps. Replace:

steps:
- uses: actions/checkout@v2
- name: Setup Deno
uses: denolib/setup-deno@v2
with:
deno-version: ${{ matrix.deno }}
- name: Check format
run: |
deno fmt --check
- name: Test
run: |
deno test -c tsconfig.json --allow-net

with

steps:
- uses: actions/checkout@v2
- name: Setup Deno
uses: denolib/setup-deno@v2
with:
deno-version: ${{ matrix.deno }}
- name: Check format
run: |
deno fmt --check
- name: Test
run: |
deno test --allow-net --allow-read --allow-write -c tsconfig.json

@tksilicon tksilicon requested a review from manyuanrong August 4, 2020 18:39
@tksilicon tksilicon self-assigned this Aug 4, 2020
@tksilicon tksilicon added the enhancement New feature or request label Aug 4, 2020
Comment on lines +1 to +3
export interface Base {
type: string; // MySQl|Postgres|Sqlite
}
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer the base file to define a unified abstraction of different database clients, similar to:

export enum ClientType {
  MYSQL = "MYSQL",
  POSTGRESS = "POSTGRESS",
  SQLITE = "SQLITE",
}

export interface DsoConnection {
  query<T = any>(sql: string, params?: any[]): Promise<T>;
  execute<T = any>(sql: string, params?: any[]): Promise<T>;
}

/** Transaction processor */
export interface TransactionProcessor<T> {
  (connection: DsoConnection): Promise<T>;
}

export interface PoolInfo {
  size: number;
  maxSize: number;
  available: number;
}

/**
 * DSO client
 */
export abstract class DsoClient {
  /** get pool info */
  abstract get pool(): PoolInfo;

  /**
   * connect to database
   * @param config config for client
   * @returns Clinet instance
   */
  abstract connect<T>(config: T): Promise<DsoClient>;

  /**
   * excute query sql
   * @param sql query sql string
   * @param params query params
   */
  abstract async query<T = any>(sql: string, params?: any[]): Promise<T>;

  /**
   * excute sql
   * @param sql sql string
   * @param params query params
   */
  abstract async execute<T = any>(sql: string, params?: any[]): Promise<T>;

  /**
   * Execute a transaction process, and the transaction successfully
   * returns the return value of the transaction process
   * @param processor transation processor
   */
  abstract async transaction<T = any>(
    processor: TransactionProcessor<T>
  ): Promise<T>;

  /**
   * close connection
   */
  abstract async close(): Promise<void>;
}

// import { Client } from "../../deps.ts";

// export class MysqlClient extends DsoClient {
//   #client: Client = new Client();

//   async close(): Promise<void> {
//     this.#client.close();
//   }

//   async connect(config: MysqlConfig): Promise<DsoClient> {
//     await this.#client.connect(config);
//     return this;
//   }

//   ...
// }

This can avoid using too much template code when dso implements core functions, which is not conducive to maintenance and understanding

get configClientReturn(): _ClientType {
return _configClientReturn;
},

/**
* all models
Copy link
Owner

Choose a reason for hiding this comment

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

Once we establish the correct client abstraction, the client used or exported by dso at this time will be DsoClient

Copy link
Owner

@manyuanrong manyuanrong left a comment

Choose a reason for hiding this comment

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

This is a lot of work. Thank you for your effort. I suggest to separate some of the changes and merge them first. For example, the abstraction of DsoClient and the implementation of MySQL.

Comment on lines +115 to +142
async connect<T extends PostgresConfig | MysqlConfig | SqliteConfig>(
config: T,
): Promise<_ClientType | undefined> {
if (config["type"].toUpperCase() === "POSTGRES") {
_clientPostgres = new PostgresClient(config["clientConfig"]);
await _clientPostgres.connect();

return _configClientReturn = {
postgres: _clientPostgres,
};
} else if (config["type"].toUpperCase() === "MYSQL") {
if (config["client"] && config["client"] instanceof Client) {
_client = config["client"];
} else {
_client = new Client();
await _client.connect(config["clientConfig"]);
}
return _configClientReturn = {
mysql: _client,
};
} else if (config["type"].toUpperCase() === "SQLITE") {
const configgy: any = config["clientConfig"];

_clientSqlite = new SqliteClient(configgy["database"]);

return _configClientReturn = {
sqlite: _clientSqlite,
};
Copy link
Owner

Choose a reason for hiding this comment

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

Since different databases will inherit and implement a unified DsoClient abstract class, I think such a large amount of template code will be avoided

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update with the changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants