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

Support for Postgres Pooling #363

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"importMap": "./import_map.json"
}
3 changes: 2 additions & 1 deletion deps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ export {
} from "https://deno.land/x/mysql@v2.10.1/mod.ts";
export type { LoggerConfig } from "https://deno.land/x/mysql@v2.10.1/mod.ts";

export { Client as PostgresClient } from "https://deno.land/x/postgres@v0.14.2/mod.ts";
export { Client as PostgresClient,Pool as PostgresPool } from "https://deno.land/x/postgres@v0.16.1/mod.ts";
export type { ClientOptions as PostgresClientOptions, ConnectionString as PostgresConnectionString } from "https://deno.land/x/postgres@v0.16.1/mod.ts";

export { DB as SQLiteClient } from "https://deno.land/x/sqlite@v3.1.3/mod.ts";

Expand Down
7 changes: 7 additions & 0 deletions import_map.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"imports": {
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/timers.js": "https://deno.land/std@0.159.0/node/timers.ts",
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/url.js": "https://deno.land/std@0.159.0/node/url.ts",
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/events.js": "https://deno.land/std@0.159.0/node/events.ts"
}
}
Comment on lines +1 to +7
Copy link
Owner

Choose a reason for hiding this comment

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

I am sure this is a larger problem, but do you mind sharing what was causing this issue?

What parent lib?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, i saw that some of these libraries relies on "dev.jspm.io" and are no longer available. But anyway, the parent lib is the main postgres lib. The error that gives is the following:

Module not found :
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/url.js" at https://dev.jspm.io/pg-connection-string@2.5.0:3:8

Copy link
Contributor

@Aiko-Suzuki Aiko-Suzuki Oct 16, 2022

Choose a reason for hiding this comment

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

one of the borken import for me is

export { default as SQLQueryBuilder } from "https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/mod-dyn.ts";
we can fix it by pointing to the branch instead of a commit.

https://github.com/Zhomart/dex/blob/update-deps/mod-dyn.ts

Copy link
Author

Choose a reason for hiding this comment

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

Yep sure, thanks!

20 changes: 15 additions & 5 deletions lib/connectors/connector.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
import { PostgresPool } from "../../deps.ts";
import type { QueryDescription } from "../query-builder.ts";
import { Translator } from "../translators/translator.ts";

/** Default connector options. */
export interface ConnectorOptions {}
export interface ConnectorOptions { }

/** Default pool options. */
export interface ConnectorPoolOptions { }

/** Default connector client. */
export interface ConnectorClient {}
export interface ConnectorClient { }

/** Default connector pool. */
export interface ConnectionPool extends PostgresPool{ }
starrematte marked this conversation as resolved.
Show resolved Hide resolved

/** Connector interface for a database provider connection. */
export interface Connector {
Expand All @@ -16,19 +23,22 @@ export interface Connector {
_translator: Translator;

/** Client that maintains an external database connection. */
_client: ConnectorClient;
_client?: ConnectorClient | undefined;
starrematte marked this conversation as resolved.
Show resolved Hide resolved

/** Options to connect to an external instance. */
_options: ConnectorOptions;

/** Is the client connected to an external instance. */
_connected: boolean;
_connected?: boolean | undefined;

/** Is the optional pool for making connections to an external instance. */
_pool?: ConnectionPool | undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing here, | undefined should be omitted :)


/** Test connection. */
ping(): Promise<boolean>;

/** Connect to an external database instance. */
_makeConnection(): void;
_makeConnection(): void | ConnectorClient | Promise<void> | Promise<ConnectorClient>;
Copy link
Owner

Choose a reason for hiding this comment

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

We should keep this a void method

It makes a connection and shouldn't return anything

Copy link
Author

@starrematte starrematte Oct 14, 2022

Choose a reason for hiding this comment

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

For the pool, we need it because of the underlying implementation. Should we keep it so?


/** Execute a query on the external database instance. */
query(queryDescription: QueryDescription): Promise<any | any[]>;
Expand Down
98 changes: 74 additions & 24 deletions lib/connectors/postgres-connector.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { PostgresClient } from "../../deps.ts";
import type { Connector, ConnectorOptions } from "./connector.ts";
import { PostgresClient, PostgresPool } from "../../deps.ts";
import type { Connector, ConnectorOptions, ConnectorPoolOptions } from "./connector.ts";
import { SQLTranslator } from "../translators/sql-translator.ts";
import type { SupportedSQLDatabaseDialect } from "../translators/sql-translator.ts";
import type { QueryDescription } from "../query-builder.ts";
Expand All @@ -17,24 +17,53 @@ interface PostgresOptionsWithURI extends ConnectorOptions {
uri: string;
}

interface PostgresPoolOptions extends ConnectorPoolOptions {
connection_params: PostgresOptionsWithConfig | PostgresOptionsWithURI,
size: number,
lazy: boolean,
}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep everything on the same level and avoid connection_params here

type PostgresClientOptions = PostgresOptionsWithConfig | PostgresOptionsWithURI;

interface PostgresPoolOptions extends ConnectorPoolOptions, PostgresClientOptions {
  size: number;
  lazy: boolean;
}

export type PostgresOptions = PostgresClientOptions | PostgresPoolOptions;

Copy link
Author

@starrematte starrematte Oct 14, 2022

Choose a reason for hiding this comment

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

It should be like this, right? :

type PostgresClientOptions = PostgresOptionsWithConfig & PostgresOptionsWithURI;

Edit: no, of course it should be as you said, but the problem here is that we cannot intersect types like that.
Ideally is correct as you mentioned, but for the moment we stick with this (or other suggestion if you something in mind):

interface PostgresPoolOptions extends ConnectorPoolOptions, PostgresOptionsWithConfig, PostgresOptionsWithURI {
  size: number;
  lazy: boolean;
}


export type PostgresOptions =
PostgresPoolOptions
| PostgresOptionsWithConfig
| PostgresOptionsWithURI;



export class PostgresConnector implements Connector {
_dialect: SupportedSQLDatabaseDialect = "postgres";

_client: PostgresClient;
_pool: PostgresPool | undefined;
_client: PostgresClient | undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
_pool: PostgresPool | undefined;
_client: PostgresClient | undefined;
_pool?: PostgresPool;
_client?: PostgresClient;

_options: PostgresOptions;
_translator: SQLTranslator;
_connected = false;

/** Create a PostgreSQL connection. */
_connected: boolean | undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this as the previous code:

Suggested change
_connected: boolean | undefined;
_connected = false;


/** Create a PostgreSQL connection
* @example
* //Direct connection usage:
* const connection = new PostgresConnector({
* host: '...',
* username: 'user',
* password: 'password',
* database: 'airlines',
* });
* //Pool connection usage:
* const connection = new PostgresConnector({
* connection_params: {
* host: '...',
* username: 'user',
* password: 'password',
* database: 'airlines',
* },
* size: 5,
* lazy: false
* });
Copy link
Owner

Choose a reason for hiding this comment

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

I know this comes from a good intention but I believe types are good hints to use the connector

Types are always be up-to-date, we don't need to manually edit them each time

So let's keep your life easy here haha :D

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you are right!

*/
constructor(options: PostgresOptions) {
this._options = options;
if ("uri" in options) {
this._client = new PostgresClient(options.uri);
} else {
} else if ("database" in options) {
Copy link
Owner

Choose a reason for hiding this comment

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

Condition should be reversed with the other one below

  1. uri in options
  2. size in options
  3. else

For 2), we can also use the new util this._isPoolConnector() mentioned in a comment below

this._client = new PostgresClient({
hostname: options.host,
user: options.username,
Expand All @@ -43,25 +72,43 @@ export class PostgresConnector implements Connector {
port: options.port ?? 5432,
});
}
else {
this._pool = new PostgresPool("uri" in options.connection_params ? options.connection_params.uri : {
hostname: options.connection_params.host,
user: options.connection_params.username,
...options.connection_params,
},
options.size,
options.lazy
);
}
this._translator = new SQLTranslator(this._dialect);
}

async _makeConnection() {
if (this._connected) {
return;
if (this._client) {
if (this._connected) {
return;
}

await this._client.connect();
this._connected = true;
return this._client;
} else {
return await this._pool!.connect();
Copy link
Owner

Choose a reason for hiding this comment

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

Let's create two new methods:

_isPoolConnector() {
  return "size" in this._options;
}

_getClientOrPool() {
  return this._isPoolConnector() ? this.getPool() : this.getClient();
}

_makeConnection() {
  if (this._connected) {
    return;
  }

  await this._getClientOrPool().connect();
  this._connected = true;
}

}

await this._client.connect();
this._connected = true;
}

async ping() {
await this._makeConnection();
return await this.tryConnection(await this._makeConnection());
}

async tryConnection(client?: PostgresClient) {
try {
const [result] = (
await this._client.queryObject("SELECT 1 + 1 as result")
).rows;
await client!.queryArray("SELECT 1 + 1 as result")
).rows[0];
return result === 2;
} catch {
return false;
Copy link
Owner

Choose a reason for hiding this comment

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

Could we keep all the code inside ping()?

Now we'll need to call this._getClientOrPool() every time we need this._client or ._pool

Copy link
Author

@starrematte starrematte Oct 14, 2022

Choose a reason for hiding this comment

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

Yes, as we stick to the this._getClientOrPool() we can keep everything inside ping()

Expand All @@ -70,32 +117,35 @@ export class PostgresConnector implements Connector {

// deno-lint-ignore no-explicit-any
async query(queryDescription: QueryDescription): Promise<any | any[]> {
await this._makeConnection();

const client = await this._makeConnection()
const query = this._translator.translateToQuery(queryDescription);
const response = await this._client.queryObject(query);
const response = await client!.queryObject(query);
Copy link
Owner

Choose a reason for hiding this comment

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

I assume that every call to client should now call this._getClientOrPool()

const results = response.rows as Values[];

if (queryDescription.type === "insert") {
return results.length === 1 ? results[0] : results;
}

return results;
}

async transaction(queries: () => Promise<void>) {
const transaction = this._client.createTransaction("transaction");
const client = await this._makeConnection()
const transaction = client!.createTransaction("transaction");
await transaction.begin();
await queries();
return transaction.commit();
}

async close() {
if (!this._connected) {
return;
if (this._client) {
if (!this._connected) {
return;
}
await this._client.end();
} else {
await this._pool?.end()!
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing here, using this._getClientOrPool().end() would be great :)


await this._client.end();
this._connected = false;
}

}
5 changes: 5 additions & 0 deletions lib/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ export class Database {
return this.getConnector()._client;
}

/* Get the database pool if existent. */
getPool?() {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we drop the ? here?

Suggested change
getPool?() {
getPool() {

Copy link
Author

Choose a reason for hiding this comment

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

For this, actually is up to the implementation to give a "pooling" experience on the connector. Afaik, not every connectors have a pool implementation out of the box, but prove me wrong

Copy link
Owner

Choose a reason for hiding this comment

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

You're right, no problem!

return this.getConnector()._pool;
}

/** Create the given models in the current database.
*
* await db.sync({ drop: true });
Expand Down
38 changes: 37 additions & 1 deletion tests/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ const defaultSQLiteOptions = {
filepath: "test.sqlite",
};

const defaultPostgreSQLPoolOptions = {
connection_params: {
uri: "postgres://postgres:user@localhost:5432/test"
},
size: 2,
lazy: false
};

const defaultPostgreSQLOptions = {
uri: "postgres://postgres:user@localhost:5432/test"
};

const getMySQLConnection = (options = {}, debug = true): Database => {
const connection: Database = new Database(
{ dialect: "mysql", debug },
Expand All @@ -39,4 +51,28 @@ const getSQLiteConnection = (options = {}, debug = true): Database => {
return connection;
};

export { getMySQLConnection, getSQLiteConnection };
const getPostgreSQLPoolConnection = (options = {}, debug = true): Database => {
const connection: Database = new Database(
{ dialect: "postgres", debug },
{
...defaultPostgreSQLPoolOptions,
...options,
},
);

return connection;
};

const getPostgreSQLConnection = (options = {}, debug = true): Database => {
const connection: Database = new Database(
{ dialect: "postgres", debug },
{
...defaultPostgreSQLOptions,
...options,
},
);

return connection;
};

export { getMySQLConnection, getSQLiteConnection, getPostgreSQLPoolConnection, getPostgreSQLConnection };
18 changes: 18 additions & 0 deletions tests/units/connectors/postgres/connection.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { getPostgreSQLPoolConnection, getPostgreSQLConnection } from "../../../connection.ts";
import { assertEquals } from "../../../deps.ts";

Deno.test({ name: "PostgreSQL: Connection", sanitizeResources: false}, async function () {
const connection = getPostgreSQLPoolConnection();
const ping = await connection.ping();
await connection.close();

assertEquals(ping, true);
});

Deno.test({ name: "PostgreSQL: Pool Connection", sanitizeResources: false}, async function () {
const connection = getPostgreSQLConnection();
const ping = await connection.ping();
await connection.close();

assertEquals(ping, true);
});