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

pg-connection-string ConnectionOptions interface is not compatible with PoolConfig #2280

Open
hjr3 opened this issue Jul 16, 2020 · 4 comments · May be fixed by #3128
Open

pg-connection-string ConnectionOptions interface is not compatible with PoolConfig #2280

hjr3 opened this issue Jul 16, 2020 · 4 comments · May be fixed by #3128

Comments

@hjr3
Copy link
Contributor

hjr3 commented Jul 16, 2020

There are a few issues:

  1. pg-connection-string ConnectionOptions has database: string | null | undefined vs PoolConfig has database?: string; (which effectively means database: string | undefined).
  2. pg-connection-string ConnectionOptions has ssl?: boolean | string vs PoolConfig has ssl?: boolean | ConnectionOptions
@hjr3
Copy link
Contributor Author

hjr3 commented Jul 16, 2020

I originally thought about making a short term fix to include tls.ConnectionOption as part of pg-connection-string. This may break some TypeScript code though if people were doing something like:

if (config.ssl === '1') {
   // some logic
} else {
   // implicitly thinking it is now a bool
}

My recommendation is:

  • Replace ConnectionOptions with PoolConfig
  • Release pg-connection-string to 3.0.0 with this breaking change

@ThisIsMissEm
Copy link

@brianc I just encountered this in the Mastodon codebase, and the only solution I had for working around it was this:

    const parsedUrl = dbUrlToConfig(env.DATABASE_URL);

    // The result of dbUrlToConfig from pg-connection-string is not type
    // compatible with pg.PoolConfig, since parts of the connection URL may be
    // `null` when pg.PoolConfig expects `undefined`, as such we have to
    // manually create the baseConfig object from the properties of the
    // parsedUrl.
    //
    // For more information see: https://github.com/brianc/node-postgres/issues/2280
    if (typeof parsedUrl.password === 'string') baseConfig.password = parsedUrl.password;
    if (typeof parsedUrl.host === 'string') baseConfig.host = parsedUrl.host;
    if (typeof parsedUrl.user === 'string') baseConfig.user = parsedUrl.user;
    if (typeof parsedUrl.port === 'string') baseConfig.port = parseInt(parsedUrl.port, 10);
    if (typeof parsedUrl.database === 'string') baseConfig.database = parsedUrl.database;
    if (typeof parsedUrl.options === 'string') baseConfig.options = parsedUrl.options;

    // The pg-connection-string type definition isn't correct, as parsedUrl.ssl
    // can absolutely be an Object, this mess is to work around these incorrect types:
    if (typeof parsedUrl.ssl === 'boolean') {
      baseConfig.ssl = parsedUrl.ssl;
    } else if (typeof parsedUrl.ssl === 'object') {
      /** @type {Record<string, any>} */
      const sslOptions = parsedUrl.ssl;
      baseConfig.ssl = {};

      baseConfig.ssl.cert = sslOptions.cert;
      baseConfig.ssl.key = sslOptions.key;
      baseConfig.ssl.ca = sslOptions.ca;
      baseConfig.ssl.rejectUnauthorized = sslOptions.rejectUnauthorized;
    }

Which is obviously quite a mess, but the only way to get the two to play nicely together.

@hjr3
Copy link
Contributor Author

hjr3 commented Jan 16, 2024

Maybe I can make a compat function in pg-connection-string to convert it into the correct type.

hjr3 added a commit to hjr3/node-postgres that referenced this issue Jan 17, 2024
Two new functions are introduced to make it easy for TypeScript
users to use a PostgresSQL connection string with pg Client.

Fixes brianc#2280
@hjr3
Copy link
Contributor Author

hjr3 commented Jan 17, 2024

@ThisIsMissEm i opened #3128 to resolve this issue.

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

Successfully merging a pull request may close this issue.

2 participants